Patch Set 2:

Patch Set 2:

(3 comments)

This change is ready for review.

Fat finger - needs changes first.

Eugene,
Thank you for the review. I've uploaded patch set 3 with your feedback. I also updated the SMRAM layout map (described on top of smm_load_module). STM stays as is. Moved stacks to the start of SMRAM since stacks grow downward and they can't corrupt any SMM code. SMM GDT (global descriptor table) is set for flat code and flat data, so there are no protections. By having stacks on top, in theory they could grow downward enough to corrupt code and data, though we have a canary in place, it could still cause problems. That is my reasoning for moving stacks to the bottom for better protection (unless we change the descriptor for stack segment and make it bound, then it can be anywhere in SMRAM).

View Change

18 comments:

• Commit Message:

  • Patch Set #1, Line 44: exiting SMM properly.i

    Trailing i.

    Done

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

  • Patch Set #1, Line 546: mdelay(1000);

    I guess, that is one of the reasons this is marked as WIP. […]

    Correct. I would like others to comment if possible.

  • Patch Set #1, Line 776: Thereforefore

    Therefore

    Done

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

  • Patch Set #1, Line 59: /**

    It’s common to start with exactly `/*` in this file.

    Done

  • Patch Set #1, Line 97: * be created from 0 to num_cpus.

    Please re-flow for 96 character line length.

    Done

  • Patch Set #1, Line 218: */

    Please move that on the line above. […]

    Done

  • Patch Set #1, Line 220: int size;

    Please use unsigned types were possible.

    Done

  • Patch Set #1, Line 225: */

    https://doc.coreboot.org/coding_style. […]

    Done

  • Patch Set #1, Line 227: -

    Please add spaces around the operators.

    Done

  • Patch Set #1, Line 553: if (size > SMM_CODE_SEGMENT_SIZE*2) {

    'size' needs to be reduced here to account for the MSEG and the BIOS RESOURCE LIST when the STM is […]

    Done

  • Patch Set #1, Line 554: base = (char *)(params->smram_end - SMM_CODE_SEGMENT_SIZE);

    base gets used outside of this block.

    Done

  • Patch Set #1, Line 567: /* Stacks start at the base of SMRAM. */

    Important issue is the location of the BIOS Resource List. […]

    Done

• File src/soc/intel/xeon_sp/Makefile.inc:

  • Patch Set #1, Line 11: +=smmrelocate.c

    Missing space.

    Done

• File src/soc/intel/xeon_sp/smmrelocate.c:

  • Patch Set #1, Line 13: */

    Please use the current file header.

    Done

  • Patch Set #1, Line 15:

    #include <types.h>
    #include <string.h>
    #include <device/device.h>
    #include <device/pci.h>
    #include <device/pci_ops.h>
    #include <cpu/x86/cache.h>
    #include <cpu/x86/lapic.h>
    #include <cpu/x86/mp.h>
    #include <cpu/x86/msr.h>
    #include <cpu/x86/mtrr.h>
    #include <cpu/x86/smm.h>
    #include <cpu/intel/em64t101_save_state.h>
    #include <cpu/intel/smm_reloc.h>
    #include <console/console.h>
    #include <smp/node.h>
    #include <soc/cpu.h>
    #include <soc/msr.h>
    #include <soc/pci_devs.h>
    #include "chip.h"
    

    Please include only what you use.

    Done

• File src/southbridge/intel/common/pmbase.c:

  • Patch Set #1, Line 87: #if CONFIG(ACPI_INTEL_HARDWARE_SLEEP_VALUES)

    Please use C if statement, so all code gets build tested. […]

    I'm retracting my changes. I have enabled ACPI_INTEL_HARDWARE_SLEEP_VALUES in my board config for now to make this work. Though we should not have ACPI code in a common file that provides access routines of read/write pmbase.

• File src/southbridge/intel/common/smihandler.c:

  • Patch Set #1, Line 56: static void busmaster_disable_on_bus(int bus)

    Use the unused annotation instead?

    I'm retracting my changes. I have enabled ACPI_INTEL_HARDWARE_SLEEP_VALUES to make this work for now.

  • Patch Set #1, Line 113: static void southbridge_smi_sleep(void)

    Use the unused annotation?

    I'm retracting my changes. I have enabled ACPI_INTEL_HARDWARE_SLEEP_VALUES to make this work for now.

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