Re: [edk2-devel] [PATCH v3 04/10] UefiCpuPkg: add CPU ejection support

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

On 2021-01-15 10:44 a.m., Laszlo Ersek wrote:
> On 01/15/21 19:16, Ankur Arora wrote:
>> On 2021-01-15 12:17 a.m., Laszlo Ersek wrote:
>>> Hi Ankur,
>>>
>>> On 01/15/21 08:45, Ankur Arora wrote:
>>>> Define CPU_HOT_EJECT_DATA and add PCD PcdCpuHotEjectDataAddress,
>>>> which would be used to share CPU ejection state between
>>>> PiSmmCpuDxeSmm and OvmfPkg/CpuHotPlugSmm.
>>>>
[...]
>>>
>>> Second, if that's not possible -- please explain why --, then a function
>>> pointer might be justified after all, but the information channel still
>>> seems to have zero relevance for UefiCpuPkg.
>>
>> The reason for keeping the PCD in UefiCpuPkg was that its declaration and
>> init are in SmmCpuFeaturesLib which gets folded into the
>> UefiCpuPkg/PiSmmCpuDxe
>> execution context and so the export happening via OvmfPkg.dec seemed off.
> 
> No, it's not off, that's precisely the goal. SmmCpuFeaturesLib is a
> dedicated platform customization interface for PiSmmCpuDxeSmm. (By
> platform, I mean "firmware platform"; such as OvmfPkg.)
> SmmCpuFeaturesLib exists becuase PiSmmCpuDxeSmm wants it to exist.
> 
> If we can solve a task by hiding it entirely in SmmCpuFeaturesLib, in
> connection with other parts of the firmware platform, we should do that.
> That's why the SmmCpuFeaturesLib class was invented, with carefully
> selected hook points. UefiCpuPkg in general is a core package, not a
> firmware platform package, so we only modify UefiCpuPkg for platform
> needs if that is absolutely unavoidable.
> 
> We are modifying the SmmCpuFeaturesLib instance provided by OvmfPkg, so
> we should strive for keeping the internals of that solution internal to
> OvmfPkg -- such as a PCD declared in OvmfPkg.dec, a structure type
> defined in an OvmfPkg include file, and so on. We're welcome to stuff as
> much platform logic into PiSmmCpuDxeSmm through our platform's
> SmmCpuFeaturesLib instance as possible, so long as we have an actual
> justified purpose with that "stuffing", and we honor the interface
> contracts.
> 
>> That said, I guess your underlying point is that this adds unnecessary
>> state to non OVMF builds (?), which it does, so I can move the PCD to OvmfPkg.dec.
> 
> Yes, that's my point. Ideally, the diffstat of the series should
> entirely stay within OvmfPkg.
> 
> I would suggest even splitting off the last patch (for CpuDeadLoop())
> into its own submission. That patch could be merged sooner than, and
> independently of, the unplug feature for OVMF.
> 
> Is it OK with you if I ask you to submit a v4 like that, before I start
> going through the series in detail?

Sure. Let me send a v4 with these changes.

Ankur

> 
> A bit more feedback on folding this UefiCpuPkg content into OvmfPkg:
> 
> - "OvmfPkg/Include/CpuHotUnplug.h" looks good to me, for the header file
> (feel free to replace Unplug with Eject though, if you like the latter more)
> 
> - in INF files, in every section, such as [Sources], [Pcds] etc, please
> keep entries (filenames, PCD names) alphabetically sorted -- unless the
> preexistent order already breaks this property
> 
> - don't bother about a .uni file under OvmfPkg
> 
> - in "OvmfPkg.dec", please find the PCD with the highest token value,
> and for introducing the new PCD, use a new token value that's one
> greater than the current maximum.
> 
> Thank you!
> Laszlo
>