From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [63.128.21.124]) by mx.groups.io with SMTP id smtpd.web12.17292.1611687692427520246 for ; Tue, 26 Jan 2021 11:01:33 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=GTLjsN6I; spf=pass (domain: redhat.com, ip: 63.128.21.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1611687691; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=QYBKORE8LtKYwnhO47IUvYuA21l5M99yvqzUhy0z4fw=; b=GTLjsN6I0X+ZbHr1vbXHdKolaZdpZ4mBq7zqyjNBpTjUHVnpKvl2aIURQDtRaUkOUaOvK2 L7Mdv/MJCidx8SjiydnXXt4mN2EJsjR0rK8NIRfhh5OLC/ub0duZPqD6KbmOLTz0IqdAz+ JZdZy4z3veylX2FEAJ91B1oaKEpaTFA= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-74-oMcx6tu5PPusiFQ88CcdDQ-1; Tue, 26 Jan 2021 14:01:26 -0500 X-MC-Unique: oMcx6tu5PPusiFQ88CcdDQ-1 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 4596410054FF; Tue, 26 Jan 2021 19:01:25 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-219.ams2.redhat.com [10.36.112.219]) by smtp.corp.redhat.com (Postfix) with ESMTP id 24F8D5D97A; Tue, 26 Jan 2021 19:01:18 +0000 (UTC) Subject: Re: [PATCH v5 1/9] OvmfPkg/CpuHotplugSmm: refactor hotplug logic To: Ankur Arora , devel@edk2.groups.io Cc: imammedo@redhat.com, boris.ostrovsky@oracle.com, Jordan Justen , Ard Biesheuvel , Aaron Young References: <20210126064440.299596-1-ankur.a.arora@oracle.com> <20210126064440.299596-2-ankur.a.arora@oracle.com> From: "Laszlo Ersek" Message-ID: <3f3fd01d-78a9-315c-85e3-b5788b8d6489@redhat.com> Date: Tue, 26 Jan 2021 20:01:18 +0100 MIME-Version: 1.0 In-Reply-To: <20210126064440.299596-2-ankur.a.arora@oracle.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 > Cc: Jordan Justen > Cc: Ard Biesheuvel > Cc: Igor Mammedov > Cc: Boris Ostrovsky > Cc: Aaron Young > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3132 > Signed-off-by: Ankur Arora > --- > 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