Rocky Phagura has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41829 )
Change subject: [WIP] cpu/x86/smm: Enable SMM support for Xeon-SP ......................................................................
Patch Set 3:
(18 comments)
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).
https://review.coreboot.org/c/coreboot/+/41829/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41829/1//COMMIT_MSG@44 PS1, Line 44: exiting SMM properly.i
Trailing i.
Done
https://review.coreboot.org/c/coreboot/+/41829/1/src/cpu/x86/mp_init.c File src/cpu/x86/mp_init.c:
https://review.coreboot.org/c/coreboot/+/41829/1/src/cpu/x86/mp_init.c@546 PS1, 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.
https://review.coreboot.org/c/coreboot/+/41829/1/src/cpu/x86/mp_init.c@776 PS1, Line 776: Thereforefore
Therefore
Done
https://review.coreboot.org/c/coreboot/+/41829/1/src/cpu/x86/smm/smm_module_... File src/cpu/x86/smm/smm_module_loader.c:
https://review.coreboot.org/c/coreboot/+/41829/1/src/cpu/x86/smm/smm_module_... PS1, Line 59: /**
It’s common to start with exactly `/*` in this file.
Done
https://review.coreboot.org/c/coreboot/+/41829/1/src/cpu/x86/smm/smm_module_... PS1, Line 97: * be created from 0 to num_cpus.
Please re-flow for 96 character line length.
Done
https://review.coreboot.org/c/coreboot/+/41829/1/src/cpu/x86/smm/smm_module_... PS1, Line 218: */
Please move that on the line above. […]
Done
https://review.coreboot.org/c/coreboot/+/41829/1/src/cpu/x86/smm/smm_module_... PS1, Line 220: int size;
Please use unsigned types were possible.
Done
https://review.coreboot.org/c/coreboot/+/41829/1/src/cpu/x86/smm/smm_module_... PS1, Line 225: */
Done
https://review.coreboot.org/c/coreboot/+/41829/1/src/cpu/x86/smm/smm_module_... PS1, Line 227: -
Please add spaces around the operators.
Done
https://review.coreboot.org/c/coreboot/+/41829/1/src/cpu/x86/smm/smm_module_... PS1, 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
https://review.coreboot.org/c/coreboot/+/41829/1/src/cpu/x86/smm/smm_module_... PS1, Line 554: base = (char *)(params->smram_end - SMM_CODE_SEGMENT_SIZE);
base gets used outside of this block.
Done
https://review.coreboot.org/c/coreboot/+/41829/1/src/cpu/x86/smm/smm_module_... PS1, Line 567: /* Stacks start at the base of SMRAM. */
Important issue is the location of the BIOS Resource List. […]
Done
https://review.coreboot.org/c/coreboot/+/41829/1/src/soc/intel/xeon_sp/Makef... File src/soc/intel/xeon_sp/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/41829/1/src/soc/intel/xeon_sp/Makef... PS1, Line 11: +=smmrelocate.c
Missing space.
Done
https://review.coreboot.org/c/coreboot/+/41829/1/src/soc/intel/xeon_sp/smmre... File src/soc/intel/xeon_sp/smmrelocate.c:
https://review.coreboot.org/c/coreboot/+/41829/1/src/soc/intel/xeon_sp/smmre... PS1, Line 13: */
Please use the current file header.
Done
https://review.coreboot.org/c/coreboot/+/41829/1/src/soc/intel/xeon_sp/smmre... PS1, 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
https://review.coreboot.org/c/coreboot/+/41829/1/src/southbridge/intel/commo... File src/southbridge/intel/common/pmbase.c:
https://review.coreboot.org/c/coreboot/+/41829/1/src/southbridge/intel/commo... PS1, 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.
https://review.coreboot.org/c/coreboot/+/41829/1/src/southbridge/intel/commo... File src/southbridge/intel/common/smihandler.c:
https://review.coreboot.org/c/coreboot/+/41829/1/src/southbridge/intel/commo... PS1, 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.
https://review.coreboot.org/c/coreboot/+/41829/1/src/southbridge/intel/commo... PS1, 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.
-
Rocky Phagura (Code Review)