Attention is currently required from: Sean Rhodes, Paul Menzel, Arthur Heymans, Patrick Rudolph. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/62990 )
Change subject: drivers/smmstore: Add option to set SMMSTORE block size at runtime ......................................................................
Patch Set 19:
(1 comment)
Patchset:
PS5:
It's later collected and then checked here: https://github.com/StarLabsLtd/edk2/blob/fd2feb5121ebfd34000dbffe9c55113366c...
Interesting. I don't see how your edk2 patches make a difference for this check, though.
I read the code around that check now (just another day where a coreboot dev reviews broken UEFI code to prevent workarounds in coreboot). The code seems only wrong in one spot: UefiPayloadPkg/SmmStoreFvb/SmmStoreFvbRuntime.c:209 ``` NvStorageBase = MmioAddress; ``` All the `base` PCDs set can't be better aligned than this MMIO address and neither does it have to nor is there any guarantee that the MMIO address is aligned in any way. Now these PCDs are consumed by the FTW DXE which seems wrong (not your fault, but I wouldn't base my firmware on such code). It does additional alignment checks; looks like a layering violation to me. At least I don't see why this shouldn't be up to the FVB provider. To make things worse, I think I've seen code that casts these `base` addresses to pointers (why have FVB and then bypass it??).
To make it right, somebody needs to have a good look at the whole edk2 code involved and fix/re-write it (it looks like it works by coincidence on x86 and probably nothing else TBH). Dropping the MMIO assumption and absolute addresses everywhere, would fix all the problems, AFAICS. A specific FVB implementation can use MMIO of course if it wants to. But that should be encapsulated (and definitely not spread everywhere via global variables).
But to get things working, it would also do to remove the spurious checks. At least I haven't seen any code that actually relies on the alignment, only offsets have to be but not the absolute addresses. For the alignment in flash the FVB provider should be responsible anyway.
The whole point of smmstore is to abstract both read and writes in smm. I find it weird that it needs an alignment check & an mmio address at all.
Alas, it seems that not only the UefiPayloadPkg/smmstore code is doing wrong checks. And UefiPayloadPkg authors are not the only ones who miss "the whole point" of certain things.