Refactor CpuHotplugMmi() to pull out the CPU hotplug logic into
ProcessHotAddedCpus(). 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>
---
Notes:
> + 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().
Addresses all comments from v5, except for this one, since the (lack) of
nesting makes more sense after patch 4, "OvmfPkg/CpuHotplugSmm: introduce
UnplugCpus()".
OvmfPkg/CpuHotplugSmm/CpuHotplug.c | 214 ++++++++++++++++++++++---------------
1 file changed, 129 insertions(+), 85 deletions(-)
diff --git a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
index cfe698ed2b5e..05b1f8cb63a6 100644
--- a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
+++ b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
@@ -62,6 +62,130 @@ STATIC UINT32 mPostSmmPenAddress;
//
STATIC EFI_HANDLE mDispatchHandle;
+/**
+ Process CPUs that have been hot-added, per QemuCpuhpCollectApicIds().
+
+ For each such CPU, relocate the SMBASE, and report the CPU to PiSmmCpuDxeSmm
+ via EFI_SMM_CPU_SERVICE_PROTOCOL. If the supposedly hot-added CPU is already
+ known, skip it silently.
+
+ @param[in] PluggedApicIds The APIC IDs of the CPUs that have been
+ hot-plugged.
+
+ @param[in] PluggedCount The number of filled-in APIC IDs in
+ PluggedApicIds.
+
+ @retval EFI_SUCCESS CPUs corresponding to all the APIC IDs are
+ populated.
+
+ @retval EFI_OUT_OF_RESOURCES Out of APIC ID space in "mCpuHotPlugData".
+
+ @return Error codes propagated from SmbaseRelocate()
+ and mMmCpuService->AddProcessor().
+
+**/
+STATIC
+EFI_STATUS
+ProcessHotAddedCpus (
+ IN APIC_ID *PluggedApicIds,
+ IN UINT32 PluggedCount
+ )
+{
+ EFI_STATUS Status;
+ UINT32 PluggedIdx;
+ UINT32 NewSlot;
+
+ //
+ // 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));
+ 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 processed this batch of hot-added CPUs.
+ //
+ return EFI_SUCCESS;
+
+RevokeNewSlot:
+ mCpuHotPlugData->ApicId[NewSlot] = MAX_UINT64;
+
+ return Status;
+}
/**
CPU Hotplug MMI handler function.
@@ -122,8 +246,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 +301,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 = ProcessHotAddedCpus (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 +314,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 (#70869): https://edk2.groups.io/g/devel/message/70869
Mute This Topic: https://groups.io/mt/80199901/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 01/29/21 01:59, Ankur Arora wrote:
> Refactor CpuHotplugMmi() to pull out the CPU hotplug logic into
> ProcessHotAddedCpus(). 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>
> ---
>
> Notes:
> > + 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().
>
> Addresses all comments from v5, except for this one, since the (lack) of
> nesting makes more sense after patch 4, "OvmfPkg/CpuHotplugSmm: introduce
> UnplugCpus()".
>
> OvmfPkg/CpuHotplugSmm/CpuHotplug.c | 214 ++++++++++++++++++++++---------------
> 1 file changed, 129 insertions(+), 85 deletions(-)
I've got one more trivial comment for this patch, to improve style
consistency with the rest of this driver:
>
> diff --git a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
> index cfe698ed2b5e..05b1f8cb63a6 100644
> --- a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
> +++ b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
> @@ -62,6 +62,130 @@ STATIC UINT32 mPostSmmPenAddress;
> //
> STATIC EFI_HANDLE mDispatchHandle;
>
> +/**
> + Process CPUs that have been hot-added, per QemuCpuhpCollectApicIds().
> +
> + For each such CPU, relocate the SMBASE, and report the CPU to PiSmmCpuDxeSmm
> + via EFI_SMM_CPU_SERVICE_PROTOCOL. If the supposedly hot-added CPU is already
> + known, skip it silently.
> +
> + @param[in] PluggedApicIds The APIC IDs of the CPUs that have been
> + hot-plugged.
> +
> + @param[in] PluggedCount The number of filled-in APIC IDs in
> + PluggedApicIds.
> +
> + @retval EFI_SUCCESS CPUs corresponding to all the APIC IDs are
> + populated.
> +
> + @retval EFI_OUT_OF_RESOURCES Out of APIC ID space in "mCpuHotPlugData".
> +
> + @return Error codes propagated from SmbaseRelocate()
> + and mMmCpuService->AddProcessor().
> +
(2) This empty line is not "wrong" in any case, just a bit inconsistent
with the rest of this driver; please drop it.
Thanks
Laszlo
> +**/
> +STATIC
> +EFI_STATUS
> +ProcessHotAddedCpus (
> + IN APIC_ID *PluggedApicIds,
> + IN UINT32 PluggedCount
> + )
> +{
> + EFI_STATUS Status;
> + UINT32 PluggedIdx;
> + UINT32 NewSlot;
> +
> + //
> + // 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));
> + 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 processed this batch of hot-added CPUs.
> + //
> + return EFI_SUCCESS;
> +
> +RevokeNewSlot:
> + mCpuHotPlugData->ApicId[NewSlot] = MAX_UINT64;
> +
> + return Status;
> +}
>
> /**
> CPU Hotplug MMI handler function.
> @@ -122,8 +246,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 +301,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 = ProcessHotAddedCpus (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 +314,6 @@ CpuHotplugMmi (
> //
> return EFI_SUCCESS;
>
> -RevokeNewSlot:
> - mCpuHotPlugData->ApicId[NewSlot] = MAX_UINT64;
> -
> Fatal:
> ASSERT (FALSE);
> CpuDeadLoop ();
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#70997): https://edk2.groups.io/g/devel/message/70997
Mute This Topic: https://groups.io/mt/80199901/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 01/29/21 01:59, Ankur Arora wrote:
> Refactor CpuHotplugMmi() to pull out the CPU hotplug logic into
> ProcessHotAddedCpus(). 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>
> ---
>
> Notes:
> > + 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().
>
> Addresses all comments from v5, except for this one, since the (lack) of
> nesting makes more sense after patch 4, "OvmfPkg/CpuHotplugSmm: introduce
> UnplugCpus()".
>
> OvmfPkg/CpuHotplugSmm/CpuHotplug.c | 214 ++++++++++++++++++++++---------------
> 1 file changed, 129 insertions(+), 85 deletions(-)
>
> diff --git a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
> index cfe698ed2b5e..05b1f8cb63a6 100644
> --- a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
> +++ b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
> @@ -62,6 +62,130 @@ STATIC UINT32 mPostSmmPenAddress;
> //
> STATIC EFI_HANDLE mDispatchHandle;
>
> +/**
> + Process CPUs that have been hot-added, per QemuCpuhpCollectApicIds().
> +
> + For each such CPU, relocate the SMBASE, and report the CPU to PiSmmCpuDxeSmm
> + via EFI_SMM_CPU_SERVICE_PROTOCOL. If the supposedly hot-added CPU is already
> + known, skip it silently.
> +
> + @param[in] PluggedApicIds The APIC IDs of the CPUs that have been
> + hot-plugged.
> +
> + @param[in] PluggedCount The number of filled-in APIC IDs in
> + PluggedApicIds.
> +
> + @retval EFI_SUCCESS CPUs corresponding to all the APIC IDs are
> + populated.
> +
> + @retval EFI_OUT_OF_RESOURCES Out of APIC ID space in "mCpuHotPlugData".
> +
> + @return Error codes propagated from SmbaseRelocate()
> + and mMmCpuService->AddProcessor().
> +
> +**/
> +STATIC
> +EFI_STATUS
> +ProcessHotAddedCpus (
> + IN APIC_ID *PluggedApicIds,
> + IN UINT32 PluggedCount
> + )
> +{
> + EFI_STATUS Status;
> + UINT32 PluggedIdx;
> + UINT32 NewSlot;
> +
> + //
> + // 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));
> + 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 processed this batch of hot-added CPUs.
> + //
> + return EFI_SUCCESS;
> +
> +RevokeNewSlot:
> + mCpuHotPlugData->ApicId[NewSlot] = MAX_UINT64;
> +
> + return Status;
> +}
>
> /**
> CPU Hotplug MMI handler function.
> @@ -122,8 +246,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 +301,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 = ProcessHotAddedCpus (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)) {
(1) I understand why you skipped point (13) from the previous review,
but you missed point (14) as well -- space character missing after
"EFI_ERROR":
https://edk2.groups.io/g/devel/message/70785
Anyway, in case v7 will not be necessary, I can fix this up myself.
With the space character added:
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Thanks
Laszlo
> + goto Fatal;
> }
>
> //
> @@ -267,9 +314,6 @@ CpuHotplugMmi (
> //
> return EFI_SUCCESS;
>
> -RevokeNewSlot:
> - mCpuHotPlugData->ApicId[NewSlot] = MAX_UINT64;
> -
> Fatal:
> ASSERT (FALSE);
> CpuDeadLoop ();
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#70915): https://edk2.groups.io/g/devel/message/70915
Mute This Topic: https://groups.io/mt/80199901/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 2021-01-29 5:15 p.m., Laszlo Ersek wrote:
> On 01/29/21 01:59, Ankur Arora wrote:
>> Refactor CpuHotplugMmi() to pull out the CPU hotplug logic into
>> ProcessHotAddedCpus(). 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>
>> ---
>>
>> Notes:
>> > + 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().
>>
>> Addresses all comments from v5, except for this one, since the (lack) of
>> nesting makes more sense after patch 4, "OvmfPkg/CpuHotplugSmm: introduce
>> UnplugCpus()".
>>
>> OvmfPkg/CpuHotplugSmm/CpuHotplug.c | 214 ++++++++++++++++++++++---------------
>> 1 file changed, 129 insertions(+), 85 deletions(-)
>>
>> diff --git a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
>> index cfe698ed2b5e..05b1f8cb63a6 100644
>> --- a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
>> +++ b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
>> @@ -62,6 +62,130 @@ STATIC UINT32 mPostSmmPenAddress;
>> //
>> STATIC EFI_HANDLE mDispatchHandle;
>>
>> +/**
>> + Process CPUs that have been hot-added, per QemuCpuhpCollectApicIds().
>> +
>> + For each such CPU, relocate the SMBASE, and report the CPU to PiSmmCpuDxeSmm
>> + via EFI_SMM_CPU_SERVICE_PROTOCOL. If the supposedly hot-added CPU is already
>> + known, skip it silently.
>> +
>> + @param[in] PluggedApicIds The APIC IDs of the CPUs that have been
>> + hot-plugged.
>> +
>> + @param[in] PluggedCount The number of filled-in APIC IDs in
>> + PluggedApicIds.
>> +
>> + @retval EFI_SUCCESS CPUs corresponding to all the APIC IDs are
>> + populated.
>> +
>> + @retval EFI_OUT_OF_RESOURCES Out of APIC ID space in "mCpuHotPlugData".
>> +
>> + @return Error codes propagated from SmbaseRelocate()
>> + and mMmCpuService->AddProcessor().
>> +
>> +**/
>> +STATIC
>> +EFI_STATUS
>> +ProcessHotAddedCpus (
>> + IN APIC_ID *PluggedApicIds,
>> + IN UINT32 PluggedCount
>> + )
>> +{
>> + EFI_STATUS Status;
>> + UINT32 PluggedIdx;
>> + UINT32 NewSlot;
>> +
>> + //
>> + // 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));
>> + 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 processed this batch of hot-added CPUs.
>> + //
>> + return EFI_SUCCESS;
>> +
>> +RevokeNewSlot:
>> + mCpuHotPlugData->ApicId[NewSlot] = MAX_UINT64;
>> +
>> + return Status;
>> +}
>>
>> /**
>> CPU Hotplug MMI handler function.
>> @@ -122,8 +246,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 +301,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 = ProcessHotAddedCpus (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)) {
>
> (1) I understand why you skipped point (13) from the previous review,
> but you missed point (14) as well -- space character missing after
> "EFI_ERROR":
>
> https://edk2.groups.io/g/devel/message/70785
>
> Anyway, in case v7 will not be necessary, I can fix this up myself.
>
> With the space character added:
>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Thanks. Will fix in v7 (not quite sure how this one escaped me; my eyes
are too used to not having the additional space, but I did have a
grep command that should have caught this one as well.)
Thanks
Ankur
>
> Thanks
> Laszlo
>
>
>> + goto Fatal;
>> }
>>
>> //
>> @@ -267,9 +314,6 @@ CpuHotplugMmi (
>> //
>> return EFI_SUCCESS;
>>
>> -RevokeNewSlot:
>> - mCpuHotPlugData->ApicId[NewSlot] = MAX_UINT64;
>> -
>> Fatal:
>> ASSERT (FALSE);
>> CpuDeadLoop ();
>>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#71050): https://edk2.groups.io/g/devel/message/71050
Mute This Topic: https://groups.io/mt/80199901/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2024 Red Hat, Inc.