Re: [PATCH v5 1/9] OvmfPkg/CpuHotplugSmm: refactor hotplug logic

["Laszlo Ersek" <lersek@redhat.com>]

On 01/26/21 07:44, Ankur Arora wrote:
> Refactor CpuHotplugMmi() to pull out the CPU hotplug logic into
> PlugCpus(). This is in preparation for supporting CPU hot-unplug.
>
> 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 | 208 ++++++++++++++++++++++---------------
>  1 file changed, 123 insertions(+), 85 deletions(-)
>
> diff --git a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
> index cfe698ed2b5e..a5052a501e5a 100644
> --- a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
> +++ b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
> @@ -62,6 +62,124 @@ STATIC UINT32 mPostSmmPenAddress;
>  //
>  STATIC EFI_HANDLE mDispatchHandle;
>
> +/**
> +  CPU Hotplug handler function.
> +
> +  @param[in] PluggedApicIds      List of APIC IDs to be plugged.
> +
> +  @param[in] PluggedCount        Count of APIC IDs to be plugged.

(1) These comments are not optimal.

The variable names say "Plugged", meaning that the CPUs have already
been plugged, from the QEMU perspective. The purpose of this function is
to go over those CPUs whose APIC IDs have been collected with events
pending, relocate the SMBASE on each, and then expose each such CPU to
EFI_SMM_CPU_SERVICE_PROTOCOL. For a given CPU, I think the comment on
the EFI_SMM_ADD_PROCESSOR prototype
[UefiCpuPkg/Include/Protocol/SmmCpuService.h] best expresses the goal:
"Notify that a new processor has been added to the system ... The SMM
CPU driver should add the processor to the SMM CPU list".

See also the comment on QemuCpuhpCollectApicIds():

"""
  Collect the APIC IDs of
  - the CPUs that have been hot-plugged,
  - the CPUs that are about to be hot-unplugged.
"""

This closely reflects which agent (firmware vs. QEMU) is driving each
particular operation / direction.

(1a) So please replace the leading comment with:

  Process those CPUs that have been hot-added, according to
  QemuCpuhpCollectApicIds().

  For each such CPU, relocate the SMBASE, and report the CPU to PiSmmCpuDxeSmm
  via EFI_SMM_CPU_SERVICE_PROTOCOL. If a supposedly hot-added CPU is already
  known, skip it silently.

(1b) Similarly, in the parameter comments, "to be plugged" is wrong. I
suggest copying the parameter descriptions from
QemuCpuhpCollectApicIds():

  @param[out] PluggedApicIds   The APIC IDs of the CPUs that have been
                               hot-plugged.

  @param[out] PluggedCount     The number of filled-in APIC IDs in
                               PluggedApicIds.


> +
> +  @retval EFI_SUCCESS            Some of the requested APIC IDs were hot-plugged.

(2) This is inexact; on successful return, each one of the collected
CPUs has been relocated and exposed to the SMM CPU driver. (Either in
this particular invocation, or in an earlier invocation, but on success,
there is no entry in PluggedApicIds that is *not known* to the SMM CPU
driver, or whose SMBASE is not relocated.)


> +
> +  @retval EFI_INTERRUPT_PENDING  Fatal error while hot-plugging.

(3) This error code is very uncommon, and it is mostly/only required
from the function -- CpuHotplugMmi() -- that is registered with
gMmst->MmiHandlerRegister(). The meaning of the error code is, "The MMI
source could not be quiesced", which is a situation that can be
identified at the level of CpuHotplugMmi(), but not at the level of
PlugCpus().

Please list the following return values instead:

  @retval EFI_OUT_OF_RESOURCES  Out of APIC ID space in "mCpuHotPlugData".

  @return                       Error codes propagated from SmbaseRelocate()
                                and mMmCpuService->AddProcessor().

(General remark, in addition: please note the difference between
"@retval" and "@return". The latter does not name a particular value;
the set of values is described in natural language instead.)


> +
> +**/
> +STATIC
> +EFI_STATUS
> +EFIAPI

(4) There is no need to make this function EFIAPI.


> +PlugCpus(

(5) Space character missing between function name and opening
parenthesis.

Please check every function prototype and function call for this -- one
space char before the opening paren is required, except in the
definition of function-like macros (where the language standard requires
the "(" to be directly adjacent).


(6) According to the discussion above, this function should be called
ProcessHotAddedCpus().

... The best hint for this function name is actually the comment that
sits atop the stretch of code you are extracting, namely:

  //
  // Process hot-added CPUs.
  //


> +  IN APIC_ID                      *PluggedApicIds,
> +  IN UINT32                       PluggedCount
> +  )
> +{
> +  EFI_STATUS Status;
> +  UINT32     PluggedIdx;
> +  UINT32     NewSlot;
> +
> +  //
> +  // Process hot-added CPUs.

(7) This short introductory comment is no longer needed, as it should be
promoted to the name of the function.


> +  //
> +  // The Post-SMM Pen need not be reinstalled multiple times within a single
> +  // root MMI handling. Even reinstalling once per root MMI is only prudence;
> +  // in theory installing the pen in the driver's entry point function should
> +  // suffice.
> +  //
> +  SmbaseReinstallPostSmmPen (mPostSmmPenAddress);
> +
> +  PluggedIdx = 0;
> +  NewSlot = 0;
> +  while (PluggedIdx < PluggedCount) {
> +    APIC_ID NewApicId;
> +    UINT32  CheckSlot;
> +    UINTN   NewProcessorNumberByProtocol;
> +
> +    NewApicId = PluggedApicIds[PluggedIdx];
> +
> +    //
> +    // Check if the supposedly hot-added CPU is already known to us.
> +    //
> +    for (CheckSlot = 0;
> +         CheckSlot < mCpuHotPlugData->ArrayLength;
> +         CheckSlot++) {
> +      if (mCpuHotPlugData->ApicId[CheckSlot] == NewApicId) {
> +        break;
> +      }
> +    }
> +    if (CheckSlot < mCpuHotPlugData->ArrayLength) {
> +      DEBUG ((DEBUG_VERBOSE, "%a: APIC ID " FMT_APIC_ID " was hot-plugged "
> +        "before; ignoring it\n", __FUNCTION__, NewApicId));
> +      PluggedIdx++;
> +      continue;
> +    }
> +
> +    //
> +    // Find the first empty slot in CPU_HOT_PLUG_DATA.
> +    //
> +    while (NewSlot < mCpuHotPlugData->ArrayLength &&
> +           mCpuHotPlugData->ApicId[NewSlot] != MAX_UINT64) {
> +      NewSlot++;
> +    }
> +    if (NewSlot == mCpuHotPlugData->ArrayLength) {
> +      DEBUG ((DEBUG_ERROR, "%a: no room for APIC ID " FMT_APIC_ID "\n",
> +        __FUNCTION__, NewApicId));
> +      goto Fatal;

(8) Please replace the "goto" with "return EFI_OUT_OF_RESOURCES".


> +    }
> +
> +    //
> +    // Store the APIC ID of the new processor to the slot.
> +    //
> +    mCpuHotPlugData->ApicId[NewSlot] = NewApicId;
> +
> +    //
> +    // Relocate the SMBASE of the new CPU.
> +    //
> +    Status = SmbaseRelocate (NewApicId, mCpuHotPlugData->SmBase[NewSlot],
> +               mPostSmmPenAddress);
> +    if (EFI_ERROR (Status)) {
> +      goto RevokeNewSlot;
> +    }
> +
> +    //
> +    // Add the new CPU with EFI_SMM_CPU_SERVICE_PROTOCOL.
> +    //
> +    Status = mMmCpuService->AddProcessor (mMmCpuService, NewApicId,
> +                              &NewProcessorNumberByProtocol);
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((DEBUG_ERROR, "%a: AddProcessor(" FMT_APIC_ID "): %r\n",
> +        __FUNCTION__, NewApicId, Status));
> +      goto RevokeNewSlot;
> +    }
> +
> +    DEBUG ((DEBUG_INFO, "%a: hot-added APIC ID " FMT_APIC_ID ", SMBASE 0x%Lx, "
> +      "EFI_SMM_CPU_SERVICE_PROTOCOL assigned number %Lu\n", __FUNCTION__,
> +      NewApicId, (UINT64)mCpuHotPlugData->SmBase[NewSlot],
> +      (UINT64)NewProcessorNumberByProtocol));
> +
> +    NewSlot++;
> +    PluggedIdx++;
> +  }
> +
> +  //
> +  // We've handled this hotplug.
> +  //

(9) I suggest: "We've processed this batch of hot-added CPUs.".


> +  return EFI_SUCCESS;
> +
> +RevokeNewSlot:
> +  mCpuHotPlugData->ApicId[NewSlot] = MAX_UINT64;
> +
> +Fatal:

(10) Please drop this label.


> +  return EFI_INTERRUPT_PENDING;

(11) This should be "return Status".


> +}
>
>  /**
>    CPU Hotplug MMI handler function.
> @@ -122,8 +240,6 @@ CpuHotplugMmi (
>    UINT8      ApmControl;
>    UINT32     PluggedCount;
>    UINT32     ToUnplugCount;
> -  UINT32     PluggedIdx;
> -  UINT32     NewSlot;
>
>    //
>    // Assert that we are entering this function due to our root MMI handler
> @@ -179,87 +295,12 @@ CpuHotplugMmi (
>      goto Fatal;
>    }
>
> -  //
> -  // Process hot-added CPUs.
> -  //
> -  // The Post-SMM Pen need not be reinstalled multiple times within a single
> -  // root MMI handling. Even reinstalling once per root MMI is only prudence;
> -  // in theory installing the pen in the driver's entry point function should
> -  // suffice.
> -  //
> -  SmbaseReinstallPostSmmPen (mPostSmmPenAddress);
> +  if (PluggedCount > 0) {
> +    Status = PlugCpus(mPluggedApicIds, PluggedCount);

(12) Space missing before "(".


> +  }
>
> -  PluggedIdx = 0;
> -  NewSlot = 0;
> -  while (PluggedIdx < PluggedCount) {
> -    APIC_ID NewApicId;
> -    UINT32  CheckSlot;
> -    UINTN   NewProcessorNumberByProtocol;
> -
> -    NewApicId = mPluggedApicIds[PluggedIdx];
> -
> -    //
> -    // Check if the supposedly hot-added CPU is already known to us.
> -    //
> -    for (CheckSlot = 0;
> -         CheckSlot < mCpuHotPlugData->ArrayLength;
> -         CheckSlot++) {
> -      if (mCpuHotPlugData->ApicId[CheckSlot] == NewApicId) {
> -        break;
> -      }
> -    }
> -    if (CheckSlot < mCpuHotPlugData->ArrayLength) {
> -      DEBUG ((DEBUG_VERBOSE, "%a: APIC ID " FMT_APIC_ID " was hot-plugged "
> -        "before; ignoring it\n", __FUNCTION__, NewApicId));
> -      PluggedIdx++;
> -      continue;
> -    }
> -
> -    //
> -    // Find the first empty slot in CPU_HOT_PLUG_DATA.
> -    //
> -    while (NewSlot < mCpuHotPlugData->ArrayLength &&
> -           mCpuHotPlugData->ApicId[NewSlot] != MAX_UINT64) {
> -      NewSlot++;
> -    }
> -    if (NewSlot == mCpuHotPlugData->ArrayLength) {
> -      DEBUG ((DEBUG_ERROR, "%a: no room for APIC ID " FMT_APIC_ID "\n",
> -        __FUNCTION__, NewApicId));
> -      goto Fatal;
> -    }
> -
> -    //
> -    // Store the APIC ID of the new processor to the slot.
> -    //
> -    mCpuHotPlugData->ApicId[NewSlot] = NewApicId;
> -
> -    //
> -    // Relocate the SMBASE of the new CPU.
> -    //
> -    Status = SmbaseRelocate (NewApicId, mCpuHotPlugData->SmBase[NewSlot],
> -               mPostSmmPenAddress);
> -    if (EFI_ERROR (Status)) {
> -      goto RevokeNewSlot;
> -    }
> -
> -    //
> -    // Add the new CPU with EFI_SMM_CPU_SERVICE_PROTOCOL.
> -    //
> -    Status = mMmCpuService->AddProcessor (mMmCpuService, NewApicId,
> -                              &NewProcessorNumberByProtocol);
> -    if (EFI_ERROR (Status)) {
> -      DEBUG ((DEBUG_ERROR, "%a: AddProcessor(" FMT_APIC_ID "): %r\n",
> -        __FUNCTION__, NewApicId, Status));
> -      goto RevokeNewSlot;
> -    }
> -
> -    DEBUG ((DEBUG_INFO, "%a: hot-added APIC ID " FMT_APIC_ID ", SMBASE 0x%Lx, "
> -      "EFI_SMM_CPU_SERVICE_PROTOCOL assigned number %Lu\n", __FUNCTION__,
> -      NewApicId, (UINT64)mCpuHotPlugData->SmBase[NewSlot],
> -      (UINT64)NewProcessorNumberByProtocol));
> -
> -    NewSlot++;
> -    PluggedIdx++;
> +  if (EFI_ERROR(Status)) {
> +    goto Fatal;
>    }

(13) Without having seen the rest of the patches, I think this error
check should be nested under the same (PluggedCount > 0) condition; in
other words, I think it only makes sense to check Status after we
actually call ProcessHotAddedCpus().


>
>    //
> @@ -267,9 +308,6 @@ CpuHotplugMmi (
>    //
>    return EFI_SUCCESS;
>
> -RevokeNewSlot:
> -  mCpuHotPlugData->ApicId[NewSlot] = MAX_UINT64;
> -
>  Fatal:
>    ASSERT (FALSE);
>    CpuDeadLoop ();
>

I'll continue the review later this week.

Thanks
Laszlo