[edk2-devel] [PATCH v2 05/10] MdePkg: add MmRegisterShutdownInterface()
Laszlo Ersek
lersek at redhat.com
Thu Jan 7 21:00:23 UTC 2021
On 01/07/21 21:48, Laszlo Ersek wrote:
> On 01/07/21 20:55, Ankur Arora wrote:
>> Add MmRegisterShutdownInterface(), which is used to register a callback,
>> that gets used to do the final ejection as part of CPU hot-unplug.
>>
>> Cc: Jian J Wang <jian.j.wang at intel.com>
>> Cc: Michael D Kinney <michael.d.kinney at intel.com>
>> Cc: Liming Gao <gaoliming at byosoft.com.cn>
>> Cc: Zhiguang Liu <zhiguang.liu at intel.com>
>> Cc: Eric Dong <eric.dong at intel.com>
>> Cc: Ray Ni <ray.ni at intel.com>
>> Cc: Laszlo Ersek <lersek at redhat.com>
>> Cc: Rahul Kumar <rahul1.kumar at intel.com>
>> Cc: Igor Mammedov <imammedo at redhat.com>
>> Cc: Boris Ostrovsky <boris.ostrovsky at oracle.com>
>> Cc: Aaron Young <aaron.young at oracle.com>
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3132
>> Signed-off-by: Ankur Arora <ankur.a.arora at oracle.com>
>> ---
>>
>> Not sure this is the right way to register a callback to be called
>> from SmmCpuFeaturesRendezvousExit(). Happy to hear suggestions for
>> a better way to accomplish this.
>
> No, it's not.
>
> SmmCpuFeaturesRendezvousExit() is an interface in the
> "UefiCpuPkg/Include/Library/SmmCpuFeaturesLib.h" class. Each platform is
> supposed to create its own instance of this library class. For example,
> OVMF's instance is at:
>
> OvmfPkg/Library/SmmCpuFeaturesLib
>
> You can modify the SmmCpuFeaturesRendezvousExit() function
> implementation in that library instance.
>
> I don't think you can easily modify the function *declaration* though,
> as that would break a whole lot of platforms that exist outside of edk2.
>
> Additionally, I don't think you can modify EFI_MM_SYSTEM_TABLE. That's a
> structure defined in the Platform Init specification. You'd first need
> to standardize (via the Platform Init Working Group of the UEFI Forum)
> the proposed change, and a compatible way to upgrade would have to be
> found. An alternative would be the "code first" procedure, which exists
> so that code can be contributed ahead of changing the published
> standards. But I'm unsure (I don't remember) how that procedure works in
> practice.
>
> Either way, I would advise against this approach. We should limit
> ourselves to modifying only the contents of
> SmmCpuFeaturesRendezvousExit() in OvmfPkg/Library/SmmCpuFeaturesLib.
>
> You should basically migrate the contents of CpuUnplugExitWork() (in
> patch#8) into SmmCpuFeaturesRendezvousExit(), without changing the
> prototype of SmmCpuFeaturesRendezvousExit(). This might require you to
> calculate "IsBSP" on the spot, possibly with the help of some global
> data that you set up in advance.
If you need "IsBSP" for a *different purpose* than unplugging the BSP
itself, then you can fetch that easily: please see the
PlatformSmmBspElection() function in
"OvmfPkg/Library/SmmCpuPlatformHookLibQemu/SmmCpuPlatformHookLibQemu.c".
The logic there is so simple that you can simply copy it into
SmmCpuFeaturesRendezvousExit()
[OvmfPkg/Library/SmmCpuPlatformHookLibQemu/SmmCpuPlatformHookLibQemu.c],
to calculate IsBSP on the spot (rather than taking it from a parameter):
MSR_IA32_APIC_BASE_REGISTER ApicBaseMsr;
ApicBaseMsr.Uint64 = AsmReadMsr64 (MSR_IA32_APIC_BASE);
IsBsp = (BOOLEAN)(ApicBaseMsr.Bits.BSP == 1);
Thanks
Laszlo
>
> *However*, regarding "IsBSP" itself -- on the QEMU platform, the BSP is
> neither unpluggable, nor switchable with any one of the APs. A removal
> for the BSP will never be attempted, so it's fine to add much simpler
> code that recognizes such an attempt (as a sanity check), and hangs hard
> on it. If that would lead to grave complications, then don't bother with
> it; just assume (or enforce elsewhere) that the BSP is never being
> unplugged.
>
> Please see the apic_designate_bsp() call sites in the QEMU source code
> -- there is exactly one of them, in "target/i386/cpu.c":
>
> /* We hard-wire the BSP to the first CPU. */
> apic_designate_bsp(cpu->apic_state, s->cpu_index == 0);
>
>
> By adding the business logic to SmmCpuFeaturesRendezvousExit() instead
> of CpuUnplugExitWork, said logic will actually be included in the
> PiSmmCpuDxeSmm binary -- but that's fine. That's why the
> SmmCpuFeaturesRendezvousExit() API exists, as a platform customization
> hook for PiSmmCpuDxeSmm.
>
>
> A formal comment in closing -- modifying two packages in the same patch,
> such as OvmfPkg and UefiCpuPkg, is almost always forbidden. It is
> permitted exceptionally, when there is absolutely no way to solve an
> issue with a gradual approach (i.e., by modifying the packages in
> question with separate patches).
>
> I think I'll skip reviewing this version beyond this email, as the
> required changes appear quite intrusive.
>
> Thanks
> Laszlo
>
>>
>> ---
>> MdeModulePkg/Core/PiSmmCore/PiSmmCore.c | 1 +
>> MdePkg/Include/Pi/PiMmCis.h | 16 ++++++++++++++++
>> MdePkg/Include/Pi/PiSmmCis.h | 2 ++
>> MdePkg/Library/MmServicesTableLib/MmServicesTableLib.c | 11 +++++++++++
>> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c | 1 +
>> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 1 +
>> 6 files changed, 32 insertions(+)
>>
>> diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
>> index cfa9922cbdb5..9d883bb06633 100644
>> --- a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
>> +++ b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
>> @@ -39,6 +39,7 @@ EFI_SMM_SYSTEM_TABLE2 gSmmCoreSmst = {
>> SmmFreePool,
>> SmmAllocatePages,
>> SmmFreePages,
>> + NULL, // SmmShutdownAp
>> NULL, // SmmStartupThisAp
>> 0, // CurrentlyExecutingCpu
>> 0, // NumberOfCpus
>> diff --git a/MdePkg/Include/Pi/PiMmCis.h b/MdePkg/Include/Pi/PiMmCis.h
>> index fdf0591a03d6..237bd8dcba76 100644
>> --- a/MdePkg/Include/Pi/PiMmCis.h
>> +++ b/MdePkg/Include/Pi/PiMmCis.h
>> @@ -77,6 +77,14 @@ EFI_STATUS
>> IN OUT VOID *ProcArguments OPTIONAL
>> );
>>
>> +typedef
>> +EFI_STATUS
>> +(EFIAPI *EFI_MM_SHUTDOWN_AP)(
>> + IN UINTN CpuNumber,
>> + IN BOOLEAN IsBSP
>> + );
>> +
>> +
>> /**
>> Function prototype for protocol install notification.
>>
>> @@ -242,6 +250,13 @@ VOID
>> IN CONST EFI_MM_ENTRY_CONTEXT *MmEntryContext
>> );
>>
>> +EFI_STATUS
>> +EFIAPI
>> +MmRegisterShutdownInterface (
>> + IN EFI_MM_SHUTDOWN_AP Procedure
>> + );
>> +
>> +
>> ///
>> /// Management Mode System Table (MMST)
>> ///
>> @@ -282,6 +297,7 @@ struct _EFI_MM_SYSTEM_TABLE {
>> ///
>> /// MP service
>> ///
>> + EFI_MM_SHUTDOWN_AP MmShutdownAp;
>> EFI_MM_STARTUP_THIS_AP MmStartupThisAp;
>>
>> ///
>> diff --git a/MdePkg/Include/Pi/PiSmmCis.h b/MdePkg/Include/Pi/PiSmmCis.h
>> index 06ef4aecd7b5..296dc01f6703 100644
>> --- a/MdePkg/Include/Pi/PiSmmCis.h
>> +++ b/MdePkg/Include/Pi/PiSmmCis.h
>> @@ -49,6 +49,7 @@ EFI_STATUS
>> IN UINTN TableSize
>> );
>>
>> +typedef EFI_MM_SHUTDOWN_AP EFI_SMM_SHUTDOWN_AP;
>> typedef EFI_MM_STARTUP_THIS_AP EFI_SMM_STARTUP_THIS_AP;
>> typedef EFI_MM_NOTIFY_FN EFI_SMM_NOTIFY_FN;
>> typedef EFI_MM_REGISTER_PROTOCOL_NOTIFY EFI_SMM_REGISTER_PROTOCOL_NOTIFY;
>> @@ -137,6 +138,7 @@ struct _EFI_SMM_SYSTEM_TABLE2 {
>> ///
>> /// MP service
>> ///
>> + EFI_SMM_SHUTDOWN_AP SmmShutdownAp;
>> EFI_SMM_STARTUP_THIS_AP SmmStartupThisAp;
>>
>> ///
>> diff --git a/MdePkg/Library/MmServicesTableLib/MmServicesTableLib.c b/MdePkg/Library/MmServicesTableLib/MmServicesTableLib.c
>> index 27f9d526e396..c7d81a0dc193 100644
>> --- a/MdePkg/Library/MmServicesTableLib/MmServicesTableLib.c
>> +++ b/MdePkg/Library/MmServicesTableLib/MmServicesTableLib.c
>> @@ -55,3 +55,14 @@ MmServicesTableLibConstructor (
>>
>> return EFI_SUCCESS;
>> }
>> +
>> +EFI_STATUS
>> +EFIAPI
>> +MmRegisterShutdownInterface (
>> + IN EFI_MM_SHUTDOWN_AP Procedure
>> + )
>> +{
>> + gMmst->MmShutdownAp = Procedure;
>> +
>> + return EFI_SUCCESS;
>> +}
>> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
>> index db68e1316ec5..f2f67e85e5e9 100644
>> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
>> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
>> @@ -22,6 +22,7 @@ SMM_CPU_PRIVATE_DATA mSmmCpuPrivateData = {
>> NULL, // Pointer to CpuSaveStateSize array
>> NULL, // Pointer to CpuSaveState array
>> { {0} }, // SmmReservedSmramRegion
>> + NULL, // SmmShutdownAp
>> {
>> SmmStartupThisAp, // SmmCoreEntryContext.SmmStartupThisAp
>> 0, // SmmCoreEntryContext.CurrentlyExecutingCpu
>> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
>> index b8aa9e1769d3..7672834a2f70 100644
>> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
>> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
>> @@ -247,6 +247,7 @@ typedef struct {
>> VOID **CpuSaveState;
>>
>> EFI_SMM_RESERVED_SMRAM_REGION SmmReservedSmramRegion[1];
>> + EFI_SMM_SHUTDOWN_AP SmmShutdownAp;
>> EFI_SMM_ENTRY_CONTEXT SmmCoreEntryContext;
>> EFI_SMM_ENTRY_POINT SmmCoreEntry;
>>
>>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#69983): https://edk2.groups.io/g/devel/message/69983
Mute This Topic: https://groups.io/mt/79507550/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list