Thanks Laszlo for the comments.
To avoid using MpService Protocol, and also for S3 boot flow, the newer version
of patch chooses to use global variable gSmmCpuPrivate instead of WhoAmI.
Please help review [PATCH v2] UefiCpuPkg/PiSmmCpuDxeSmm: Use NonSmm BSP as
default SMM BSP.
Thanks
Zhiguang
> -----Original Message-----
> From: Laszlo Ersek <ler...@redhat.com>
> Sent: Wednesday, November 15, 2023 12:54 AM
> To: devel@edk2.groups.io; Liu, Zhiguang <zhiguang....@intel.com>
> Cc: Ni, Ray <ray...@intel.com>; Kumar, Rahul R <rahul.r.ku...@intel.com>;
> Gerd Hoffmann <kra...@redhat.com>
> Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg: Use NonSmm BSP as BSP if
> BSP election is not enabled.
>
> 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 (#111440): https://edk2.groups.io/g/devel/message/111440
Mute This Topic: https://groups.io/mt/102576442/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
