From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by mx.groups.io with SMTP id smtpd.web09.2124.1611969335127867005 for ; Fri, 29 Jan 2021 17:15:35 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=Sn/HOqyU; spf=pass (domain: redhat.com, ip: 216.205.24.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1611969334; 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=8RBgf87vLrsCwtDXzN6zG57HiQXOB/FBzGYonml1KcY=; b=Sn/HOqyU4EKhzq9OTQABHPaye/vfWhvUzZ7PFEie1R7I1XWcmIQ2qKn24l2ktiakILA4H3 i2UZHpPgAnvJd1cocO08Y4v714bPEMIxLOnHtGaJjsaRXvRr4iUfU2w/G7Ht9WS+RRqHbb KnMV2N3MZh7Y+Q+FtahWwWdiMRHhmDk= 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-583-Zv14M_g0Pw2kqd_mnGtmzA-1; Fri, 29 Jan 2021 20:15:30 -0500 X-MC-Unique: Zv14M_g0Pw2kqd_mnGtmzA-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id AB424107ACF6; Sat, 30 Jan 2021 01:15:28 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-14.ams2.redhat.com [10.36.112.14]) by smtp.corp.redhat.com (Postfix) with ESMTP id 4FB206F7E7; Sat, 30 Jan 2021 01:15:25 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v6 1/9] OvmfPkg/CpuHotplugSmm: refactor hotplug logic To: devel@edk2.groups.io, ankur.a.arora@oracle.com Cc: imammedo@redhat.com, boris.ostrovsky@oracle.com, Jordan Justen , Ard Biesheuvel , Aaron Young References: <20210129005950.467638-1-ankur.a.arora@oracle.com> <20210129005950.467638-2-ankur.a.arora@oracle.com> From: "Laszlo Ersek" Message-ID: Date: Sat, 30 Jan 2021 02:15:25 +0100 MIME-Version: 1.0 In-Reply-To: <20210129005950.467638-2-ankur.a.arora@oracle.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 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/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 > 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 > --- > > 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 Thanks Laszlo > + goto Fatal; > } > > // > @@ -267,9 +314,6 @@ CpuHotplugMmi ( > // > return EFI_SUCCESS; > > -RevokeNewSlot: > - mCpuHotPlugData->ApicId[NewSlot] = MAX_UINT64; > - > Fatal: > ASSERT (FALSE); > CpuDeadLoop (); >