[edk2-devel] [PATCH v5 1/9] OvmfPkg/CpuHotplugSmm: refactor hotplug logic
Ankur Arora
ankur.a.arora at oracle.com
Tue Jan 26 19:15:31 UTC 2021
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 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.
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/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