Re: [edk2-devel] [PATCH v2 05/10] MdePkg: add MmRegisterShutdownInterface()

["Laszlo Ersek" <lersek@redhat.com>]

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;
>>  
>>
>