[edk2-devel] [PATCH v2 05/10] MdePkg: add MmRegisterShutdownInterface()
Laszlo Ersek
lersek at redhat.com
Thu Jan 7 20:48:27 UTC 2021
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.
*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 (#69982): https://edk2.groups.io/g/devel/message/69982
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