Hi Gerd,
>
> Ok, so TileSize is what the firmware needs to store code and state.
> Where does the SIZE_8KB come from? I assume this is the amount of
> per-cpu memory allocated by the PEI module? Shouldn't this be passed
> in the HOB instead of being hard-coded?
>
Yes, TileSize is for firmware store code and data, including 3 parts:
1. CPU SMRAM Save State Map starts at SMBASE +
SMRAM_SAVE_STATE_MAP_OFFSET(0xfc00),
2. extra CPU specific context start starts at SMBASE + SMM_PSD_OFFSET
(PROCESSOR SMM DESCRIPTO, 0xfb00),
3. SMI entry point starts at SMBASE + SMM_HANDLER_OFFSET (0x8000).
This size is rounded up to nearest power of 2. So, you can refer the below
existing algorithm:
TileCodeSize = GetSmiHandlerSize ();
TileCodeSize = ALIGN_VALUE (TileCodeSize, SIZE_4KB);
TileDataSize = (SMRAM_SAVE_STATE_MAP_OFFSET - SMM_PSD_OFFSET) + sizeof
(SMRAM_SAVE_STATE_MAP);
TileDataSize = ALIGN_VALUE (TileDataSize, SIZE_4KB);
TileSize = TileDataSize + TileCodeSize - 1;
TileSize = 2 * GetPowerOfTwo32 ((UINT32)TileSize);
DEBUG ((DEBUG_INFO, "SMRAM TileSize = 0x%08x (0x%08x, 0x%08x)\n", TileSize,
TileCodeSize, TileDataSize));
Based on above, we hardcode the size to 8k, because It's almost impossible to
exceed 8k for total code & data size. So, there is the hard requirement that
SMI Entry Size <= 0x1000, data Size < 0x1000 in pi smm cpu driver. To simplify
the usage, we add the size check as below to catch this very little possibility
case instead of passing or defining the new interface for that, which means we
add such rigorous processed assumption to avoid define the new interface that
may not be changed and used.
In PEI module, it also has such assumption, so we don't pass in the HOB for the
resolved smbase mem size, because we have avoided the possibility of error in
the reference pi smm cpu driver.
if (TileSize > SIZE_8KB) {
DEBUG ((DEBUG_ERROR, "The Range of Smbase in SMRAM is not enough --
Required TileSize = 0x%08x, Actual TileSize = 0x%08x\n", TileSize, SIZE_8KB));
CpuDeadLoop ();
return RETURN_BUFFER_TOO_SMALL;
}
> > + // Allocate buffer for all of the tiles.
> > + //
> > + // Intel(R) 64 and IA-32 Architectures Software Developer's Manual
> > + // Volume 3C, Section 34.11 SMBASE Relocation
> > + // For Pentium and Intel486 processors, the SMBASE values must be
> > + // aligned on a 32-KByte boundary or the processor will enter
> shutdown
> > + // state during the execution of a RSM instruction.
> > + //
> > + // Intel486 processors: FamilyId is 4
> > + // Pentium processors : FamilyId is 5
> > + //
> > + BufferPages = EFI_SIZE_TO_PAGES (SIZE_32KB + TileSize *
> (mMaxNumberOfCpus - 1));
>
> I think correct is:
> BufferPages = EFI_SIZE_TO_PAGES(TileSize * mMaxNumberOfCpus);
>
This is the existing code logic & it's correct, not wrong, I don't change it.
To understand that, we need understand the algorithm of smbase:
The SIZE_32KB covers the *several* SMI Entry and Save State of CPU 0, while
TileSize * (mMaxNumberOfCpus - 1) to cover Save State of CPU 1+, not include
the cpu0, so, it's the mMaxNumberOfCpus - 1.
> > + if ((FamilyId == 4) || (FamilyId == 5)) {
> > + Buffer = AllocateAlignedCodePages (BufferPages, SIZE_32KB);
>
> Does that actually matter still? I'm pretty sure we can safely use
> "ASSERT(FamilyId > 5)" here. Pentium processors have been built in
> the last century, predating x64.
>
This is the existing code logic. I don't change it. If you think we don't need
it, please file Bugzilla for change.
> Beside that the code is broken for SMP, only cpu0 will get a properly
> aligned smbase. Not sure penium processors support SMP in the first
> place though ...
>
I don't understand why "only cpu0 will get a properly aligned smbase"
> > for (Index = 0; Index < mMaxNumberOfCpus; Index++) {
> > - mCpuHotPlugData.SmBase[Index] = (UINTN)Buffer + Index *
> TileSize - SMM_HANDLER_OFFSET;
> > + mCpuHotPlugData.SmBase[Index] = mSmmRelocated ?
> (UINTN)SmmBaseHobData->SmBase[Index] : (UINTN)Buffer + Index *
> TileSize - SMM_HANDLER_OFFSET;
>
> For Index = 0 this evaluates to "Buffer - SMM_HANDLER_OFFSET", which
> looks
> wrong to me.
>
No, it's correct, we don't allocate the buffer for [smbase 0, smbase + smi
handler), we just record the address of smbase 0, there is no need for the
[smbase 0, smbase + smi handler) usage.
Thanks,
Jiaxin
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#100151): https://edk2.groups.io/g/devel/message/100151
Mute This Topic: https://groups.io/mt/96932003/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
