Rocky Phagura has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41829 )
Change subject: cpu/x86/smm: Enable SMM support for Xeon-SP ......................................................................
Patch Set 8:
(3 comments)
Patch Set 8: Code-Review+1
Patch Set 8:
Patch Set 8:
(7 comments)
Patch Set 8:
Just wondering if it wouldn't be easier to use some linker script to generate the correct SMRAM layout and then put everything into the SMM rmodule.
Benefits:
- You would get a build error if you try to use more space than available
- No magic calculations that need to be done, as everything is known at compile time
- Nothin to set up as everything is already in place
Downside: Bigger SMM rmodule (hopefully compression catches that)
For long term a dynamic is method is better. A linker script would solve the problem temporarily but it will be more work. A lot of effort has already been placed into fixing the existing SMM loader. There are certain places where it is necessary to take action based on the CPUID or different flags in MSRs (such as state save flags) and this cannot be done at compile time. Also, the handlers are installed based on the number of CPU threads detected in the system.
In addition, the SMM handler will need to go through more changes later on. Currently the entry into SMM is flat 32 real mode. Intel now supports protected mode entries and 64 bit support in SMM. This is beneficial for server platforms mostly.
Another approach is that we could call this SMM module loader version 2. Only the server platforms will use version 2 and client/desktop/etc can continue to use the old version. It will make this patch integrate much better into the existing model.
I don't see where runtime decisions need to be made. You know the savestate size and the maximum CPU count at compile-time. Having less CPU cores active than supported should be easy to catch. I don't see x86_64 support in SMM anytime soon and it's unrelated to this code. As mobile and desktops already have a lot of cores tofay, we'll run into the same problem in the near future. Splitting this into two models doesn't seem useful either.
FYI - There has been x86_64 support in the Intel processors since Sandy Bridge. For EFI based platforms the STM starts the SMI handler 64-bit mode. For coreboot, the SMI handler is started in 32-bit mode.
There doesn't seem to be any run time decisions that affect the memory layout in SMM. If you have a different CPUID, you're building a new SMI handler anyway. The save state locations are fixed relative to the SMBASE, so only the contents need to be evaluated.
The main decision made at runtime is do we have enough space for all the CPUs? The TSEG base and LIMIT are programmed by the FSP. At compile time, we do not know the size of SMM. The rest of the calculations can be done ahead of time assuming in the future HW doesn't require any runtime checks based on some capability flag. If so, then we are doomed as architecture can't handle it. Having a SMM architecture that can adapt dynamically is preferred by me. However, it would be great to get other's input on this. One thing is clear, the existing SMM loader architecture does not scale. Having two SMM loaders, one legacy and one new is a good way to transition into the new one as products need to do so.
https://review.coreboot.org/c/coreboot/+/41829/8/src/cpu/x86/mp_init.c File src/cpu/x86/mp_init.c:
https://review.coreboot.org/c/coreboot/+/41829/8/src/cpu/x86/mp_init.c@799 PS8, Line 799: permanaent
permanent
Done. Good catch.
https://review.coreboot.org/c/coreboot/+/41829/6/src/cpu/x86/smm/smm_module_... File src/cpu/x86/smm/smm_module_loader.c:
https://review.coreboot.org/c/coreboot/+/41829/6/src/cpu/x86/smm/smm_module_... PS6, Line 510: +-----------------+ <-
If it's wrong, I'd either fix it or delete it
Ascii art is correct. Both the art and description help clarify the difficult layout of SMM entry which is tied to x86/Intel architecture. If I can help clarify more, please let me know.
https://review.coreboot.org/c/coreboot/+/41829/8/src/cpu/x86/smm/smm_module_... File src/cpu/x86/smm/smm_module_loader.c:
https://review.coreboot.org/c/coreboot/+/41829/8/src/cpu/x86/smm/smm_module_... PS8, Line 68: * +-----------------+ : * +-----------------+
What goes here?
Done
-
Rocky Phagura (Code Review)