Small patch, but I have several comments :)
On 11/14/23 03:08, Zhiguang Liu wrote:
> Currently, if BSP election is not enabled, will use Core0 as SMM BSP,
> however, Core0 does not always have the highest performance core.
> So, we can used NonSmm BSP as default BSP.
(1) Please consider mentioning in the commit message that the change
from this patch will take effect in SmiRendezvous().
(2) Please put "UefiCpuPkg/PiSmmCpuDxeSmm: ..." in the subject.
>
> Cc: Ray Ni <ray...@intel.com>
> Cc: Rahul Kumar <rahul1.ku...@intel.com>
> Cc: Gerd Hoffmann <kra...@redhat.com>
> Signed-off-by: Zhiguang Liu <zhiguang....@intel.com>
> ---
> UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 10 +++++
> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c | 48 +++++++++++-----------
> 2 files changed, 34 insertions(+), 24 deletions(-)
>
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> index 25d058c5b9..a4f83bb122 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> @@ -29,6 +29,8 @@ MM_COMPLETION mSmmStartupThisApToken;
> //
> UINT32 *mPackageFirstThreadIndex = NULL;
>
> +extern EFI_MP_SERVICES_PROTOCOL *mMpServices;
> +
> /**
> Performs an atomic compare exchange operation to get semaphore.
> The compare exchange operation must be performed using
> @@ -1953,6 +1955,14 @@ InitializeMpSyncData (
> // Enable BSP election by setting BspIndex to -1
> //
> mSmmMpSyncData->BspIndex = (UINT32)-1;
> + } else {
> + //
> + // Use NonSMM BSP as SMM BSP
> + //
> + ASSERT (mMpServices != NULL);
> + if (mMpServices != NULL) {
> + mMpServices->WhoAmI (mMpServices, (UINTN
> *)&mSmmMpSyncData->BspIndex);
> + }
> }
>
> mSmmMpSyncData->EffectiveSyncMode = mCpuSmmSyncMode;
(3) This branch cannot be tested on OVMF, due to commit 43df61878d94
("OvmfPkg: enable SMM Monarch Election in PiSmmCpuDxeSmm", 2020-03-04).
That's not a problem with the patch of course, just saying that I can't
offer regression testing.
(4) Not checking the return value of WhoAmI() is actually valid here.
Per PI spec, WhoAmI() can only fail if we pass a null pointer for
"ProcessorNumber" (and we don't do that here).
Not sure about static analysis tools though. If they complain, we might
want to cast the return value to (VOID) explicitly:
(VOID)mMpServices->WhoAmI (...);
Not needed unless those tools complain, of course.
(5) Passing the following argument for the "ProcessorNumber" parameter
(UINTN *)&mSmmMpSyncData->BspIndex
is undefined behavior, for two reasons.
First, "SMM_DISPATCHER_MP_SYNC_DATA.BspIndex" is volatile-qualified, but
the access here (or more precisely, inside WhoAmI()) happens through an
lvalue that doesn't necessarily have volatile-qualified type. That's UB.
Second, "SMM_DISPATCHER_MP_SYNC_DATA.BspIndex" has type "(volatile)
UINT32", but WhoAmI() writes an UINTN. On IA32, you're going to corrupt
memory.
Therefore you should to do this:
UINTN DxeBspNumber;
MpServices->WhoAmI (mMpServices, &DxeBspNumber);
if (DxeBspNumber <= MAX_UINT32) {
mSmmMpSyncData->BspIndex = (UINT32)DxeBspNumber;
}
In other words, use a local variable of the right type, and range check
it. If the range check fails, then just fall back to the current
behavior (leave BspIndex at zero). Otherwise, employ an explicit
assignment, including casting. This will in particular keep "volatile"
happy (and there can't be any overflow either, after the range check).
One more comment below:
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
> index 1d022a7051..18c77c59e6 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
> @@ -128,7 +128,8 @@ SPIN_LOCK *mConfigSmmCodeAccessCheckLock = NULL;
> EFI_SMRAM_DESCRIPTOR *mSmmCpuSmramRanges;
> UINTN mSmmCpuSmramRangeCount;
>
> -UINT8 mPhysicalAddressBits;
> +UINT8 mPhysicalAddressBits;
> +EFI_MP_SERVICES_PROTOCOL *mMpServices;
>
> //
> // Control register contents saved for SMM S3 resume state initialization.
> @@ -603,26 +604,25 @@ PiCpuSmmEntry (
> IN EFI_SYSTEM_TABLE *SystemTable
> )
> {
> - EFI_STATUS Status;
> - EFI_MP_SERVICES_PROTOCOL *MpServices;
> - UINTN NumberOfEnabledProcessors;
> - UINTN Index;
> - VOID *Buffer;
> - UINTN BufferPages;
> - UINTN TileCodeSize;
> - UINTN TileDataSize;
> - UINTN TileSize;
> - UINT8 *Stacks;
> - VOID *Registration;
> - UINT32 RegEax;
> - UINT32 RegEbx;
> - UINT32 RegEcx;
> - UINT32 RegEdx;
> - UINTN FamilyId;
> - UINTN ModelId;
> - UINT32 Cr3;
> - EFI_HOB_GUID_TYPE *GuidHob;
> - SMM_BASE_HOB_DATA *SmmBaseHobData;
> + EFI_STATUS Status;
> + UINTN NumberOfEnabledProcessors;
> + UINTN Index;
> + VOID *Buffer;
> + UINTN BufferPages;
> + UINTN TileCodeSize;
> + UINTN TileDataSize;
> + UINTN TileSize;
> + UINT8 *Stacks;
> + VOID *Registration;
> + UINT32 RegEax;
> + UINT32 RegEbx;
> + UINT32 RegEcx;
> + UINT32 RegEdx;
> + UINTN FamilyId;
> + UINTN ModelId;
> + UINT32 Cr3;
> + EFI_HOB_GUID_TYPE *GuidHob;
> + SMM_BASE_HOB_DATA *SmmBaseHobData;
>
> GuidHob = NULL;
> SmmBaseHobData = NULL;
> @@ -656,13 +656,13 @@ PiCpuSmmEntry (
> //
> // Get MP Services Protocol
> //
> - Status = SystemTable->BootServices->LocateProtocol
> (&gEfiMpServiceProtocolGuid, NULL, (VOID **)&MpServices);
> + Status = SystemTable->BootServices->LocateProtocol
> (&gEfiMpServiceProtocolGuid, NULL, (VOID **)&mMpServices);
> ASSERT_EFI_ERROR (Status);
>
> //
> // Use MP Services Protocol to retrieve the number of processors and
> number of enabled processors
> //
> - Status = MpServices->GetNumberOfProcessors (MpServices, &mNumberOfCpus,
> &NumberOfEnabledProcessors);
> + Status = mMpServices->GetNumberOfProcessors (mMpServices, &mNumberOfCpus,
> &NumberOfEnabledProcessors);
> ASSERT_EFI_ERROR (Status);
> ASSERT (mNumberOfCpus <= PcdGet32 (PcdCpuMaxLogicalProcessorNumber));
>
> @@ -945,7 +945,7 @@ PiCpuSmmEntry (
> gSmmCpuPrivate->Operation[Index] = SmmCpuNone;
>
> if (Index < mNumberOfCpus) {
> - Status = MpServices->GetProcessorInfo (MpServices, Index |
> CPU_V2_EXTENDED_TOPOLOGY, &gSmmCpuPrivate->ProcessorInfo[Index]);
> + Status = mMpServices->GetProcessorInfo (mMpServices, Index |
> CPU_V2_EXTENDED_TOPOLOGY, &gSmmCpuPrivate->ProcessorInfo[Index]);
> ASSERT_EFI_ERROR (Status);
> mCpuHotPlugData.ApicId[Index] =
> gSmmCpuPrivate->ProcessorInfo[Index].ProcessorId;
>
(6) Turning the "MpServices" local variable of PiCpuSmmEntry() into a
global variable is a bad idea.
The global variable will live in SMRAM, so it is a waste of SMRAM, for
starters.
Second, it is a security risk. Any pointer that exists in SMRAM and
points *outside* of SMRAM is a security risk. If, additionally, the
pointer points at a DXE protocol (i.e., it is used for executing DXE
code), then the risk increases.
You don't actually *need* this pointer (MpServices) to outlive
PiCpuSmmEntry(). PiCpuSmmEntry(), and functions invoked from
PiCpuSmmEntry(), are permitted to consume DXE protocols, but once the
entry point function returns, PiSmmCpuDxeSmm is not permitted to access
DXE artifacts. Therefore allowing a DXE protocol's address to persist in
PiSmmCpuDxeSmm after PiCpuSmmEntry() returns is just asking for trouble.
(
Side comment: note that PiSmmCpuDxeSmm does not use
UefiBootServicesTableLib! In other words, it does not use the "gBS"
global variable. That is not random. It is intentional. It prevents
you from accidentally invoking a UEFI Boot Service where you
shouldn't.
Accordingly, the entry point function looks up the MP Services
Protocol like this:
Status = SystemTable->BootServices->LocateProtocol
(&gEfiMpServiceProtocolGuid, NULL, (VOID **)&MpServices);
Explicit "SystemTable->BootServices" dereference, rather than "gBS"
dereference.
Importantly, SystemTable is a *parameter* of PiCpuSmmEntry(), so no
other function in PiSmmCpuDxeSmm will have access to it, unless
PiCpuSmmEntry() passes it on.
)
So, this is our call chain:
PiCpuSmmEntry() [UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c]
InitializeMpServiceData() [UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c]
InitializeMpSyncData() [UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c]
I suggest extending InitializeMpServiceData() and InitializeMpSyncData()
to take MpServices.
Thread MpServices from PiCpuSmmEntry() through InitializeMpServiceData()
to InitializeMpSyncData().
That will clarify that InitializeMpSyncData() consumes MpServices only
because it runs on the stack of PiCpuSmmEntry().
Thanks
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111210): https://edk2.groups.io/g/devel/message/111210
Mute This Topic: https://groups.io/mt/102576442/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe:
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
