Patch Set 4:

(1 comment)

Angel,
I incorporated your changes from the other patch. I've added one more change in mp_init.c. There are 2 SMM regions:
1. Default Region at 30000h. During this relocation phase only so many CPUs can relocate at the same time otherwise the state save will corrupt the entry code. Therefore, you will notice I added code to handle this case which means CPUs will relocate one at a time. This will add a slight delay to the boot time (hopefully its negligible). Unfortunately there is nothing we can do because of the SMRAM HW limitations.

IMHO, looks good. I've bumped all comments from older patchsets, most of which are not done yet.

Another thing: to make sure this code gets build-tested, I would add `CONFIG_X86_SMM_LOADER_VERSION2=y` to `configs/config.asrock_b85m_pro4.debug_smmstore_hotplug_gcov_ubsan_em100`. This is a config file I've added to build-test several options that were previously not build-tested.

Patch set 4:Code-Review +1

View Change

15 comments:

• Commit Message:

  • Patch Set #2, Line 9: Xeon-SP a Skylake Scalable Processor can have 36 CPU threads(18 cores). Current

    nit: Please wrap these lines at 72 characters

    *poke*

• File src/cpu/x86/Kconfig:

  • Patch Set #2, Line 125: bool

    Could we please add a prompt here so that one can easily enable it?

    No need

  • Patch Set #2, Line 129: server

    nit: maybe deemphasize the "server" mentions a bit? There are few non-server designs out there with […]

    Agreed, this works even on non-server stuff

• File src/cpu/x86/mp_init.c:

  • Patch Set #4, Line 734: __func__, cpu);

    Fits on the previous line (line length limit is 96 characters)

• File src/cpu/x86/smm/smm_module_loaderv2.c:

  • Patch Set #2, Line 15: uint8_t

    #include <stdint. […]

    *poke*

    Or, if you use `bool` and `size_t`, just #include <types.h>

  • Patch Set #2, Line 51: unsigned int

    Are these memory addresses? If so, I'd use uintptr_t

    *poke*

  • Patch Set #2, Line 108: printk(BIOS_ERR, "%s: unable to get SMM module size\n", __func__);

    Can we continue after this error?

    *poke*

  • Patch Set #2, Line 112: 0xFFFF

    What does this magic number mean?

    *poke*

  • Patch Set #2, Line 123: not enough space in SMM to setup all CPUs

    So how would a firmware developer start to solve this, or is SMM just not available on designs with […]

    *poke*

  • Patch Set #2, Line 176: int

    Shouldn't this be an uintptr_t as well?

    Done

  • Patch Set #2, Line 213: unsigned int

    uintptr_t

    *poke*

  • Patch Set #2, Line 226: params->smm_main_entry_offset < stack_top) {

    I'd indent this part differently so that it is clear it's a continuation (e.g. […]

    *poke*

  • Patch Set #2, Line 231: __func__, cpus[num_cpus].smbase, stack_top);

    one more tab

    *poke*

  • Patch Set #2, Line 604: "%s: need more SMRAM\n", __func__);

    add another tab

    *poke*

    Or reflow, it fits in a single line

• File src/include/cpu/x86/smm.h:

  • Patch Set #2, Line 152: #if CONFIG(X86_SMM_LOADER_VERSION2)

    nit: I would not indent preprocessor

    *poke*

To view, visit change 43684. To unsubscribe, or for help writing mail filters, visit settings.