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

["Ankur Arora" <ankur.a.arora@oracle.com>]

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
>