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

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

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().


Thanks
Ankur

> 
> 
> Thanks
> Laszlo
>