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
