UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 10 +++++ UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c | 48 +++++++++++----------- 2 files changed, 34 insertions(+), 24 deletions(-)
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.
Cc: Ray Ni <ray.ni@intel.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Zhiguang Liu <zhiguang.liu@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;
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;
--
2.31.1.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111188): https://edk2.groups.io/g/devel/message/111188
Mute This Topic: https://groups.io/mt/102576442/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-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.ni@intel.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Signed-off-by: Zhiguang Liu <zhiguang.liu@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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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 <lersek@redhat.com>
> Sent: Wednesday, November 15, 2023 12:54 AM
> To: devel@edk2.groups.io; Liu, Zhiguang <zhiguang.liu@intel.com>
> Cc: Ni, Ray <ray.ni@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>;
> Gerd Hoffmann <kraxel@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.ni@intel.com>
> > Cc: Rahul Kumar <rahul1.kumar@intel.com>
> > Cc: Gerd Hoffmann <kraxel@redhat.com>
> > Signed-off-by: Zhiguang Liu <zhiguang.liu@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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 11/14/23 17:53, Laszlo Ersek wrote: > Second, "SMM_DISPATCHER_MP_SYNC_DATA.BspIndex" has type "(volatile) > UINT32", but WhoAmI() writes an UINTN. On IA32, you're going to corrupt > memory. sorry, it's on X64 where you are going to corrupt memory (the UINTN write is a UINT64 write, but the field to accept it is only UINT32) Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#111211): https://edk2.groups.io/g/devel/message/111211 Mute This Topic: https://groups.io/mt/102576442/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2024 Red Hat, Inc.