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 <ler...@redhat.com> > Cc: Jordan Justen <jordan.l.jus...@intel.com> > Cc: Ard Biesheuvel <ard.biesheu...@arm.com> > Cc: Igor Mammedov <imamm...@redhat.com> > Cc: Boris Ostrovsky <boris.ostrov...@oracle.com> > Cc: Aaron Young <aaron.yo...@oracle.com> > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3132 > Signed-off-by: Ankur Arora <ankur.a.ar...@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 <ler...@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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
