Re: [RFC] ich9:cpuhp: add support for cpu hot-unplug with SMI broadcast enabled

[Laszlo Ersek]

On 11/28/20 01:43, Ankur Arora wrote:
> On 2020-11-27 7:19 a.m., Laszlo Ersek wrote:
>> On 11/27/20 05:10, Ankur Arora wrote:
>>
>>> Yeah I was wondering what would happen for simultaneous hot add and
>>> remove.
>>> I guess we would always do remove first and then the add, unless we hit
>>> the break due to max_cpus_per_pass and switch to hot-add mode.
>>
>> Considering the firmware only, I disagree with remove-then-add.
>>
>> EFI_SMM_CPU_SERVICE_PROTOCOL.AddProcessor() and
>> EFI_SMM_CPU_SERVICE_PROTOCOL.RemoveProcessor() (implemented in
>> SmmAddProcessor() and SmmRemoveProcessor() in
>> "UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c", respectively) only mark the
>> processors for addition/removal. The actual processing is done only
>> later, in BSPHandler() --> SmmCpuUpdate(), when "all SMI handlers are
>> finished" (see the comment in SmmRemoveProcessor()).
>>
>> Consequently, I would not suggest replacing a valid APIC ID in a
>> particular mCpuHotPlugData.ApicId[Index] slot with INVALID_APIC_ID
>> (corresponding to the unplug operation), and then possibly replacing
>> INVALID_APIC_ID in the *same slot* with the APIC ID of the newly plugged
>> CPU, in the exact same SMI invocation (= in the same execution of
>> CpuHotplugMmi()). That might cause some component in edk2 to see the
>> APIC ID in mCpuHotPlugData.ApicId[Index] to change from one valid ACPI
>> ID to another valid APIC ID, and I don't even want to think about what
>> kind of mess that could cause.
> 
> Shudders.
> 
>>
>> So no, please handle plugs first, for which unused slots in
>> mCpuHotPlugData.ApicId will be populated, and *then* handle removals (in
>> the same invocation of CpuHotplugMmi()).
> 
> Yeah, that ordering makes complete sense.
> 
>>
>> By the way, for unplug, you will not have to re-set
>> mCpuHotPlugData.ApicId[Index] to INVALID_APIC_ID, as
>> SmmRemoveProcessor() does that internally. You just have to locate the
>> Index for the APIC ID being removed, for calling
>> EFI_SMM_CPU_SERVICE_PROTOCOL.RemoveProcessor().
> 
> Right. The hotplug is more involved (given the need to pen the new CPU)
> but for the unplug, AFAICS all the actual handling for removal is in
> .RemoveProcessor() and at SMI exit in SmmCpuUpdate().

Yes, I got the same impression (without having tried to implement it, of
course).

Laszlo