Re: [PATCH v6 4/9] OvmfPkg/CpuHotplugSmm: introduce UnplugCpus()

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

On 2021-01-31 7:13 p.m., Laszlo Ersek wrote:
> On 01/29/21 01:59, Ankur Arora wrote:
>> Introduce UnplugCpus() which maps each APIC ID being unplugged
>> onto the hardware ID of the processor and informs PiSmmCpuDxeSmm
>> of removal by calling EFI_SMM_CPU_SERVICE_PROTOCOL.RemoveProcessor().
>>
>> With this change we handle the first phase of unplug where we collect
>> the CPUs that need to be unplugged and mark them for removal in SMM
>> data structures.
>>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
>> Cc: Igor Mammedov <imammedo@redhat.com>
>> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>> Cc: Aaron Young <aaron.young@oracle.com>
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3132
>> Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com>
>> ---
>>   OvmfPkg/CpuHotplugSmm/CpuHotplug.c | 84 ++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 84 insertions(+)
>>
>> diff --git a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
>> index 05b1f8cb63a6..70d69f6ed65b 100644
>> --- a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
>> +++ b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
>> @@ -188,6 +188,88 @@ RevokeNewSlot:
>>   }
>>
>>   /**
>> +  Process to be hot-unplugged CPUs, per QemuCpuhpCollectApicIds().
>> +
>> +  For each such CPU, report the CPU to PiSmmCpuDxeSmm via
>> +  EFI_SMM_CPU_SERVICE_PROTOCOL. If the to be hot-unplugged CPU is
>> +  unknown, skip it silently.
>> +
>> +  @param[in] ToUnplugApicIds    The APIC IDs of the CPUs that are about to be
>> +                                hot-unplugged.
>> +
>> +  @param[in] ToUnplugCount      The number of filled-in APIC IDs in
>> +                                ToUnplugApicIds.
>> +
>> +  @retval EFI_SUCCESS           Known APIC IDs have been removed from SMM data
>> +                                structures.
>> +
>> +  @return                       Error codes propagated from
>> +                                mMmCpuService->RemoveProcessor().
>> +
> 
> (1) Please drop this empty line (just before the '**/').
> 
> 
>> +**/
>> +STATIC
>> +EFI_STATUS
>> +UnplugCpus (
>> +  IN APIC_ID                      *ToUnplugApicIds,
>> +  IN UINT32                       ToUnplugCount
>> +  )
>> +{
>> +  EFI_STATUS Status;
>> +  UINT32     ToUnplugIdx;
>> +  UINTN      ProcessorNum;
>> +
>> +  ToUnplugIdx = 0;
>> +  while (ToUnplugIdx < ToUnplugCount) {
>> +    APIC_ID    RemoveApicId;
>> +
>> +    RemoveApicId = ToUnplugApicIds[ToUnplugIdx];
>> +
>> +    //
>> +    // mCpuHotPlugData->ApicId maps ProcessorNum -> ApicId. Use it to find
>> +    // the ProcessorNum for the APIC ID to be removed.
>> +    //
>> +    for (ProcessorNum = 0;
>> +         ProcessorNum < mCpuHotPlugData->ArrayLength;
>> +         ProcessorNum++) {
>> +      if (mCpuHotPlugData->ApicId[ProcessorNum] == RemoveApicId) {
>> +        break;
>> +      }
>> +    }
>> +
>> +    //
>> +    // Ignore the unplug if APIC ID not found
>> +    //
>> +    if (ProcessorNum == mCpuHotPlugData->ArrayLength) {
>> +      DEBUG ((DEBUG_INFO, "%a: did not find APIC ID " FMT_APIC_ID
>> +          " to unplug\n", __FUNCTION__, RemoveApicId));
> 
> (2) Please use DEBUG_VERBOSE here.
> 
> (I agree that we should have *one* DEBUG_INFO message that relates to
> the removal of an individual processor; however, I think we should emit
> that message when we finally signal QEMU to eject the processor.)

Based on our discussion around establishing the correspondence between
The ProcessorNum, APIC-ID and CPU selector, I'll change this to
DEBUG_VERBOSE and add a new DEBUG_INFO print after successfully
putting it in the APICIdMap.

> 
> 
> (3) Please un-indent ("outdent"?) the second line by two spaces.
> 
> 
>> +      ToUnplugIdx++;
>> +      continue;
>> +    }
>> +
>> +    //
>> +    // Mark ProcessorNum for removal from SMM data structures
>> +    //
>> +    Status = mMmCpuService->RemoveProcessor (mMmCpuService, ProcessorNum);
>> +
> 
> (4) It would be more idiomatic to remove this empty line (between Status
> assignment and check).
> 
> 
>> +    if (EFI_ERROR (Status)) {
>> +      DEBUG ((DEBUG_ERROR, "%a: RemoveProcessor(" FMT_APIC_ID "): %r\n",
>> +        __FUNCTION__, RemoveApicId, Status));
>> +      goto Fatal;
> 
> (5) Please just "return Status" here, and drop the "Fatal" label.
> 
> 
>> +    }
>> +
>> +    ToUnplugIdx++;
>> +  }
>> +
>> +  //
>> +  // We've removed this set of APIC IDs from SMM data structures.
>> +  //
>> +  return EFI_SUCCESS;
>> +
>> +Fatal:
>> +  return Status;
>> +}
>> +
>> +/**
>>     CPU Hotplug MMI handler function.
>>
>>     This is a root MMI handler.
>> @@ -303,6 +385,8 @@ CpuHotplugMmi (
>>
>>     if (PluggedCount > 0) {
>>       Status = ProcessHotAddedCpus (mPluggedApicIds, PluggedCount);
>> +  } else if (ToUnplugCount > 0) {
>> +    Status = UnplugCpus (mToUnplugApicIds, ToUnplugCount);
>>     }
>>
>>     if (EFI_ERROR(Status)) {
>>
> 
> (6) Hmm... What's the reason for the exclusivity?
> 
> Why is the following not better:
> 
>    if (PluggedCount > 0) {
>      Status = ProcessHotAddedCpus (mPluggedApicIds, PluggedCount);
>      if (EFI_ERROR (Status)) {
>        goto Fatal;
>      }
>    }
>    if (ToUnplugCount > 0) {
>      Status = UnplugCpus (mToUnplugApicIds, ToUnplugCount);
>      if (EFI_ERROR (Status)) {
>        goto Fatal;
>      }
>    }
> 
> QemuCpuhpCollectApicIds() intentionally populates both arrays in a
> single go. As I suggested earlier:
> 
> https://lists.gnu.org/archive/html/qemu-devel/2020-11/msg06711.html
> msgid: <a92b50df-f693-ebda-e549-7bc9e6d2d7a5@redhat.com>
> 
>> [...] please handle plugs first, for which unused slots in
>> mCpuHotPlugData.ApicId will be populated, and *then* handle removals
>> (in the same invocation of CpuHotplugMmi()).
> 
> Did that turn out as unviable (the "same invocation of CpuHotplugMmi()"
> part)?

No I had some confusion while looking at the underlying AddProcessor() and
RemoveProcessor() logic.

Looking at it again, it should work. Will fix.

Also acking the rest of your comments here.

Thanks
Ankur

> 
> 
> (7) As a side note, addressing point (6) above would allow you to
> address my point (13) from the v5 patch#1 thread, too; i.e., nesting the
> Status check under (PluggedCount > 0).
> 
> Thanks!
> Laszlo
>