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.
+
+ @retval EFI_SUCCESS Some of the requested APIC IDs were hot-plugged.
+
+ @retval EFI_INTERRUPT_PENDING Fatal error while hot-plugging.
+
+**/
+STATIC
+EFI_STATUS
+EFIAPI
+PlugCpus(
+ IN APIC_ID *PluggedApicIds,
+ IN UINT32 PluggedCount
+ )
+{
+ EFI_STATUS Status;
+ UINT32 PluggedIdx;
+ UINT32 NewSlot;
+
+ //
+ // 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);
+
+ 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;
+ }
+
+ //
+ // 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.
+ //
+ return EFI_SUCCESS;
+
+RevokeNewSlot:
+ mCpuHotPlugData->ApicId[NewSlot] = MAX_UINT64;
+
+Fatal:
+ return EFI_INTERRUPT_PENDING;
+}
/**
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);
+ }
- 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;
}
//
@@ -267,9 +308,6 @@ CpuHotplugMmi (
//
return EFI_SUCCESS;
-RevokeNewSlot:
- mCpuHotPlugData->ApicId[NewSlot] = MAX_UINT64;
-
Fatal:
ASSERT (FALSE);
CpuDeadLoop ();
--
2.9.3
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#70760): https://edk2.groups.io/g/devel/message/70760
Mute This Topic: https://groups.io/mt/80125308/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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
-=-=-=-=-=-=-=-=-=-=-=-
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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 2021-01-26 11:01 a.m., Laszlo Ersek wrote:
> 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.
Acking the comments above. Meanwhile let me reprocess the series
in light of the comments above.
Ankur
>
> Thanks
> Laszlo
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#70786): https://edk2.groups.io/g/devel/message/70786
Mute This Topic: https://groups.io/mt/80125308/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 01/26/21 20:15, Ankur Arora wrote: > On 2021-01-26 11:01 a.m., Laszlo Ersek wrote: >> I'll continue the review later this week. > > Acking the comments above. Thank you! > Meanwhile let me reprocess the series in light of the comments above. I'm at such a point now, during the v5 review, that I think I can easily re-sync. In general, I don't mind the posting of a new version of a series mid-review, *IF* we agree about it in advance. If you prefer to post a v6, for addressing the comments I've made thus far, I'm OK with that. If you'd like me to continue reviewing v5, I'm also OK with that. So it's up to you -- please state your decision, so that I know if I should proceed with v5 (later this week), or wait for v6. Thanks! Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#70811): https://edk2.groups.io/g/devel/message/70811 Mute This Topic: https://groups.io/mt/80125308/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On 2021-01-26 1:07 p.m., Laszlo Ersek wrote: > On 01/26/21 20:15, Ankur Arora wrote: >> On 2021-01-26 11:01 a.m., Laszlo Ersek wrote: > >>> I'll continue the review later this week. >> >> Acking the comments above. > > Thank you! > >> Meanwhile let me reprocess the series in light of the comments above. > > I'm at such a point now, during the v5 review, that I think I can easily > re-sync. > > In general, I don't mind the posting of a new version of a series > mid-review, *IF* we agree about it in advance. > > If you prefer to post a v6, for addressing the comments I've made thus > far, I'm OK with that. If you'd like me to continue reviewing v5, I'm > also OK with that. > > So it's up to you -- please state your decision, so that I know if I > should proceed with v5 (later this week), or wait for v6. I think I would prefer to send v6. Looking at the v5 comments so far, I'm sure that there's a lot of non conforming coding style issues. Addressing them now (or at least a hopefully significant subset) would probably save time. I'm looking at sending these out by Thursday morning PT, and given that you plan to continue later this week, sounds like it might not lose too much review time either. Thanks Ankur > > Thanks! > Laszlo > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#70812): https://edk2.groups.io/g/devel/message/70812 Mute This Topic: https://groups.io/mt/80125308/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On 01/26/21 22:17, Ankur Arora wrote: > On 2021-01-26 1:07 p.m., Laszlo Ersek wrote: >> On 01/26/21 20:15, Ankur Arora wrote: >>> On 2021-01-26 11:01 a.m., Laszlo Ersek wrote: >> >>>> I'll continue the review later this week. >>> >>> Acking the comments above. >> >> Thank you! >> >>> Meanwhile let me reprocess the series in light of the comments above. >> >> I'm at such a point now, during the v5 review, that I think I can easily >> re-sync. >> >> In general, I don't mind the posting of a new version of a series >> mid-review, *IF* we agree about it in advance. >> >> If you prefer to post a v6, for addressing the comments I've made thus >> far, I'm OK with that. If you'd like me to continue reviewing v5, I'm >> also OK with that. >> >> So it's up to you -- please state your decision, so that I know if I >> should proceed with v5 (later this week), or wait for v6. > > I think I would prefer to send v6. Looking at the v5 comments so far, I'm > sure that there's a lot of non conforming coding style issues. > Addressing them now (or at least a hopefully significant subset) would > probably save time. I agree; thank you. (And, for all the yelling that ECC does, I'm really surprised it didn't catch the "missing space between function designator and opening paren" wart!) > I'm looking at sending these out by Thursday morning PT, and given that > you plan to continue later this week, sounds like it might not lose too > much review time either. Yes, that should work fine. Thank you, Ankur! Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#70813): https://edk2.groups.io/g/devel/message/70813 Mute This Topic: https://groups.io/mt/80125308/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On 2021-01-26 1:32 p.m., Laszlo Ersek wrote: > On 01/26/21 22:17, Ankur Arora wrote: >> On 2021-01-26 1:07 p.m., Laszlo Ersek wrote: >>> On 01/26/21 20:15, Ankur Arora wrote: >>>> On 2021-01-26 11:01 a.m., Laszlo Ersek wrote: >>> >>>>> I'll continue the review later this week. >>>> >>>> Acking the comments above. >>> >>> Thank you! >>> >>>> Meanwhile let me reprocess the series in light of the comments above. >>> >>> I'm at such a point now, during the v5 review, that I think I can easily >>> re-sync. >>> >>> In general, I don't mind the posting of a new version of a series >>> mid-review, *IF* we agree about it in advance. >>> >>> If you prefer to post a v6, for addressing the comments I've made thus >>> far, I'm OK with that. If you'd like me to continue reviewing v5, I'm >>> also OK with that. >>> >>> So it's up to you -- please state your decision, so that I know if I >>> should proceed with v5 (later this week), or wait for v6. >> >> I think I would prefer to send v6. Looking at the v5 comments so far, I'm >> sure that there's a lot of non conforming coding style issues. >> Addressing them now (or at least a hopefully significant subset) would >> probably save time. > > I agree; thank you. > > (And, for all the yelling that ECC does, I'm really surprised it didn't > catch the "missing space between function designator and opening paren" > wart!) Heh. Yeah, I was surprised at how little fault ECC found in the code. Thanks for reviewing. Ankur > >> I'm looking at sending these out by Thursday morning PT, and given that >> you plan to continue later this week, sounds like it might not lose too >> much review time either. > > Yes, that should work fine. > > Thank you, Ankur! > Laszlo > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#70814): https://edk2.groups.io/g/devel/message/70814 Mute This Topic: https://groups.io/mt/80125308/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On 01/26/21 20:01, Laszlo Ersek wrote:
> On 01/26/21 07:44, Ankur Arora wrote:
>> + 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().
(14) Missing space after "EFI_ERROR".
(I'll not point out further instances of this issue; please review all
the patches with regard to it.)
Thanks
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#70785): https://edk2.groups.io/g/devel/message/70785
Mute This Topic: https://groups.io/mt/80125308/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 01/26/21 20:01, Laszlo Ersek wrote: > (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. (Of course, in this location, the parameters are [in], not [out].) Thanks Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#70784): https://edk2.groups.io/g/devel/message/70784 Mute This Topic: https://groups.io/mt/80125308/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2024 Red Hat, Inc.