[edk2-devel] [PATCH v5 1/9] OvmfPkg/CpuHotplugSmm: refactor hotplug logic
Laszlo Ersek
lersek at redhat.com
Tue Jan 26 19:01:18 UTC 2021
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 at redhat.com>
> Cc: Jordan Justen <jordan.l.justen at intel.com>
> Cc: Ard Biesheuvel <ard.biesheuvel at arm.com>
> Cc: Igor Mammedov <imammedo at redhat.com>
> Cc: Boris Ostrovsky <boris.ostrovsky at oracle.com>
> Cc: Aaron Young <aaron.young at oracle.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3132
> Signed-off-by: Ankur Arora <ankur.a.arora at 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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#70783): https://edk2.groups.io/g/devel/message/70783
Mute This Topic: https://groups.io/mt/80125308/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list