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@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Zhiguang Liu <zhiguang.liu@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Aaron Young <aaron.young@oracle.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3132
Signed-off-by: Ankur Arora <ankur.a.arora@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.
---
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;
--
2.9.3
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#69977): https://edk2.groups.io/g/devel/message/69977
Mute This Topic: https://groups.io/mt/79507550/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Zhiguang Liu <zhiguang.liu@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Cc: Aaron Young <aaron.young@oracle.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3132
> Signed-off-by: Ankur Arora <ankur.a.arora@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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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@intel.com>
>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>> Cc: Liming Gao <gaoliming@byosoft.com.cn>
>> Cc: Zhiguang Liu <zhiguang.liu@intel.com>
>> Cc: Eric Dong <eric.dong@intel.com>
>> Cc: Ray Ni <ray.ni@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Rahul Kumar <rahul1.kumar@intel.com>
>> Cc: Igor Mammedov <imammedo@redhat.com>
>> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>> Cc: Aaron Young <aaron.young@oracle.com>
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3132
>> Signed-off-by: Ankur Arora <ankur.a.arora@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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 2021-01-07 12:48 p.m., 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@intel.com>
>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>> Cc: Liming Gao <gaoliming@byosoft.com.cn>
>> Cc: Zhiguang Liu <zhiguang.liu@intel.com>
>> Cc: Eric Dong <eric.dong@intel.com>
>> Cc: Ray Ni <ray.ni@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Rahul Kumar <rahul1.kumar@intel.com>
>> Cc: Igor Mammedov <imammedo@redhat.com>
>> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>> Cc: Aaron Young <aaron.young@oracle.com>
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3132
>> Signed-off-by: Ankur Arora <ankur.a.arora@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.
Thanks, yeah I'm definitely not looking to do that. I did look through
the spec but not enough to get any kind of familiarity with it.
>
> Either way, I would advise against this approach. We should limit
> ourselves to modifying only the contents of
> SmmCpuFeaturesRendezvousExit() in OvmfPkg/Library/SmmCpuFeaturesLib.
That was my initial approach. The problem that ran into was that which
CPUs to eject depends on state held in OvmfPkg/CpuHotPlugSmm (see patch 8)
which, I could not figure out a way to make that available given that the
libraries are linked statically.
The other way I can think of doing this would be to just reprobe this state
from QEMU. The problem with that is that then there's zero shared state
between the removal and the ejection.
>
> 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.
The reason for trying to capture IsBSP was that mSmmMpSyncData->BspIndex
has gone at this point. If, as you say, there are significant problems
with changing the prototype, then I can figure out a way to avoid that,
especially given that we get the BSP by reading an APIC MSR anyway.
>
> *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).
Thanks for letting me know. I'll avoid that in future versions.
>
> I think I'll skip reviewing this version beyond this email, as the
> required changes appear quite intrusive.
Could you take a look at patch 8? That's really the crux of the series and
your comments on that would be helpful.
Thanks
Ankur
>
> 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 (#69984): https://edk2.groups.io/g/devel/message/69984
Mute This Topic: https://groups.io/mt/79507550/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 01/07/21 22:19, Ankur Arora wrote: > On 2021-01-07 12:48 p.m., Laszlo Ersek wrote: >> We should limit ourselves to modifying only the contents of >> SmmCpuFeaturesRendezvousExit() in OvmfPkg/Library/SmmCpuFeaturesLib. > > That was my initial approach. The problem that ran into was that which > CPUs to eject depends on state held in OvmfPkg/CpuHotPlugSmm (see patch 8) > which, I could not figure out a way to make that available given that the > libraries are linked statically. [...] > Could you take a look at patch 8? That's really the crux of the series Indeed. The proposal I just sent elsewhere in this patch#5 sub-thread addresses exactly that. It's a frequent cross-driver communication pattern in edk2, utilizing dynamic PCDs (platform configuration database entries). The tricky part with this pattern is always proper serialization (make sure the publishing driver always performs the PcdSet before the consuming driver performs the PcdGet). This is often achieved with a DEPEX. Luckily, in this case, such a depex (and indeed an earlier instance of this exact same pattern) is already in place between PiSmmCpuDxeSmm and CpuHotplugSmm, so you can imitate it. In the other email, I laid out the suggested steps. Thanks! Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#69986): https://edk2.groups.io/g/devel/message/69986 Mute This Topic: https://groups.io/mt/79507550/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On 01/07/21 21:48, Laszlo Ersek wrote: > 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. [...] > 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. If you need PiSmmCpuDxeSmm and CpuHotplugSmm to collaborate on a dynamically sized array, such as "mHotUnplugWork", here's a possible approach. Consider how the somewhat similar "mCpuHotPlugData" object is allocated, and then shared between both drivers, in SMRAM. Line numbers at commit 85b8eac59b8c. (1) in the PiCpuSmmEntry() function, PiSmmCpuDxeSmm first sets "mMaxNumberOfCpus" to the value of "PcdCpuMaxLogicalProcessorNumber". (PcdCpuHotPlugSupport is TRUE in our case.) Line 624. (2) "mCpuHotPlugData.ArrayLength" is set to "mMaxNumberOfCpus". Line 824. (3) SmmCpuFeaturesSmmRelocationComplete() is called [OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c]. Line 930. (4) The address of "mCpuHotPlugData" is published through "PcdCpuHotPlugDataAddress", on line 1021. (Again, PcdCpuHotPlugSupport is TRUE.) (5) "OvmfPkg/CpuHotplugSmm/CpuHotplug.c" then locates "mCpuHotPlugData" as follows, in the CpuHotplugEntry() function: 17cb8ddba39b5 329) // 17cb8ddba39b5 330) // Our DEPEX on EFI_SMM_CPU_SERVICE_PROTOCOL guarantees that PiSmmCpuDxeSmm 17cb8ddba39b5 331) // has pointed PcdCpuHotPlugDataAddress to CPU_HOT_PLUG_DATA in SMRAM. 17cb8ddba39b5 332) // 17cb8ddba39b5 333) mCpuHotPlugData = (VOID *)(UINTN)PcdGet64 (PcdCpuHotPlugDataAddress); The same pattern can be repeated for the new data structure that you (might) need: (a) Extend the SmmCpuFeaturesSmmRelocationComplete() function in "OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c" with the following logic -- probably implemented as a new STATIC helper function: - fetch "PcdCpuMaxLogicalProcessorNumber", so that you know how to size the "mHotUnplugWork" array - allocate the "mHotUnplugWork" array in SMRAM. For the allocation, you can simply use the AllocatePool() function from MemoryAllocationLib, as in this SMM library instance, it will allocate SMRAM. - if the allocation fails, hang hard (there's really nothing else to do) - publish the array's address via a new UINT64 PCD -- introduce a new PCD in "OvmfPkg.dec", as dynamic-only, set a default 0 value in the DSC files, and then use a PcdSet64S() call in the code. (b) The same DEPEX guarantee continues working in the CpuHotplugSmm driver as before. Thus, in the CpuHotplugEntry() function, you can locate "mHotUnplugWork" with another PcdGet64() function call, referring to the new PCD. (c) In the SmmCpuFeaturesRendezvousExit() function -- which is part of the same library instance as SmmCpuFeaturesSmmRelocationComplete() --, you can simply refer to "mHotUnplugWork" by name. In summary, the ownership of of "mHotUnplugWork" moves from CpuHotplugSmm to PiSmmCpuSmmDxe. PiSmmCpuSmmDxe itself does not know about this, because all the new logic is hooked into it through existent hook functions in "OvmfPkg/Library/SmmCpuFeaturesLib". And CpuHotplugSmm retains access to "mHotUnplugWork" by reusing the dynamic PCD pattern that's already employed for "mCpuHotPlugData". (Note that the PCD itself exists in normal RAM, but this is safe, because the transfer of the "mHotUnplugWork" address through the PCD occurs way before such code could execute that is not part of the platform firmware.) Thanks, Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#69985): https://edk2.groups.io/g/devel/message/69985 Mute This Topic: https://groups.io/mt/79507550/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On 2021-01-07 1:45 p.m., Laszlo Ersek wrote: > On 01/07/21 21:48, Laszlo Ersek wrote: > >> 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. > > [...] > >> 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. > > If you need PiSmmCpuDxeSmm and CpuHotplugSmm to collaborate on a > dynamically sized array, such as "mHotUnplugWork", here's a possible > approach. > > Consider how the somewhat similar "mCpuHotPlugData" object is allocated, > and then shared between both drivers, in SMRAM. > > Line numbers at commit 85b8eac59b8c. > > (1) in the PiCpuSmmEntry() function, PiSmmCpuDxeSmm first sets > "mMaxNumberOfCpus" to the value of "PcdCpuMaxLogicalProcessorNumber". > (PcdCpuHotPlugSupport is TRUE in our case.) Line 624. > > (2) "mCpuHotPlugData.ArrayLength" is set to "mMaxNumberOfCpus". Line > 824. > > (3) SmmCpuFeaturesSmmRelocationComplete() is called > [OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c]. Line 930. > > (4) The address of "mCpuHotPlugData" is published through > "PcdCpuHotPlugDataAddress", on line 1021. (Again, PcdCpuHotPlugSupport > is TRUE.) Aah, yeah. Thanks, this is exactly what I needed to do. > > (5) "OvmfPkg/CpuHotplugSmm/CpuHotplug.c" then locates "mCpuHotPlugData" > as follows, in the CpuHotplugEntry() function: > > 17cb8ddba39b5 329) // > 17cb8ddba39b5 330) // Our DEPEX on EFI_SMM_CPU_SERVICE_PROTOCOL guarantees that PiSmmCpuDxeSmm > 17cb8ddba39b5 331) // has pointed PcdCpuHotPlugDataAddress to CPU_HOT_PLUG_DATA in SMRAM. > 17cb8ddba39b5 332) // > 17cb8ddba39b5 333) mCpuHotPlugData = (VOID *)(UINTN)PcdGet64 (PcdCpuHotPlugDataAddress); > > > The same pattern can be repeated for the new data structure that you > (might) need: > > (a) Extend the SmmCpuFeaturesSmmRelocationComplete() function in > "OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c" with the > following logic -- probably implemented as a new STATIC helper function: > > - fetch "PcdCpuMaxLogicalProcessorNumber", so that you know how to size > the "mHotUnplugWork" array > > - allocate the "mHotUnplugWork" array in SMRAM. For the allocation, you > can simply use the AllocatePool() function from MemoryAllocationLib, as > in this SMM library instance, it will allocate SMRAM. > > - if the allocation fails, hang hard (there's really nothing else to do) > > - publish the array's address via a new UINT64 PCD -- introduce a new > PCD in "OvmfPkg.dec", as dynamic-only, set a default 0 value in the DSC > files, and then use a PcdSet64S() call in the code. > > (b) The same DEPEX guarantee continues working in the CpuHotplugSmm > driver as before. Thus, in the CpuHotplugEntry() function, you can > locate "mHotUnplugWork" with another PcdGet64() function call, referring > to the new PCD. > > (c) In the SmmCpuFeaturesRendezvousExit() function -- which is part of > the same library instance as SmmCpuFeaturesSmmRelocationComplete() --, > you can simply refer to "mHotUnplugWork" by name. > > In summary, the ownership of of "mHotUnplugWork" moves from > CpuHotplugSmm to PiSmmCpuSmmDxe. PiSmmCpuSmmDxe itself does not know > about this, because all the new logic is hooked into it through existent > hook functions in "OvmfPkg/Library/SmmCpuFeaturesLib". And CpuHotplugSmm > retains access to "mHotUnplugWork" by reusing the dynamic PCD pattern > that's already employed for "mCpuHotPlugData". Right, this makes perfect sense now. Let me fix up the patches and resend in light of your comments. Thanks Ankur > > (Note that the PCD itself exists in normal RAM, but this is safe, > because the transfer of the "mHotUnplugWork" address through the PCD > occurs way before such code could execute that is not part of the > platform firmware.) > > Thanks, > Laszlo > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#69992): https://edk2.groups.io/g/devel/message/69992 Mute This Topic: https://groups.io/mt/79507550/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2024 Red Hat, Inc.