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.34751.1613999216769963704 for ; Mon, 22 Feb 2021 05:06:56 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=UPC81AVL; 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=1613999216; 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=3dAjkZjUIpxp3EFCKpT3ZudR2GUnghMQyjohawWEd1k=; b=UPC81AVLBf1byab1r8+v9zHqRk37v+6VQCfpdfcTslR4O45NlEPdweoV+qLDh22SXDc4VO YWLjJ74AACpvXaxywuC9a7vfzhe0z5Z3CM2CaC++EjWV5pZoBaV9fvasTuWQgJse0Ypnqt FvDH1Rag9cH1Q+xp2C8Qx59iHP1XVUk= 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-41-0-hZC5U2OUKQAoSUMliqQQ-1; Mon, 22 Feb 2021 08:06:52 -0500 X-MC-Unique: 0-hZC5U2OUKQAoSUMliqQQ-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id D6696107ACE3; Mon, 22 Feb 2021 13:06:50 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-113-67.ams2.redhat.com [10.36.113.67]) by smtp.corp.redhat.com (Postfix) with ESMTP id DD0E019C45; Mon, 22 Feb 2021 13:06:48 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v8 05/10] OvmfPkg/CpuHotplugSmm: define CPU_HOT_EJECT_DATA 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: <20210222071928.1401820-1-ankur.a.arora@oracle.com> <20210222071928.1401820-6-ankur.a.arora@oracle.com> From: "Laszlo Ersek" Message-ID: Date: Mon, 22 Feb 2021 14:06:47 +0100 MIME-Version: 1.0 In-Reply-To: <20210222071928.1401820-6-ankur.a.arora@oracle.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 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 02/22/21 08:19, Ankur Arora wrote: > Define CPU_HOT_EJECT_DATA and add PCD PcdCpuHotEjectDataAddress, which > will be used to share CPU ejection state between OvmfPkg/CpuHotPlugSmm > and PiSmmCpuDxeSmm. > > 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: > Addresses the following review comments in v6: > (1) Dropped modifications to LibraryClasses in OvmfPkg.dec > (2,3) Cleanup comments around PCD PcdCpuHotEjectDataAddress. > (4) Move PCD PcdCpuHotEjectDataAddress declaration in CpuHotplugSmm.inf > to a patch-7 where it actually gets used. > (5a,5b) Change the comment in the top block to use Laszlo's language. > Also detail when the PCD would contain a valid value. > (6) Move Library/CpuHotEjectData.h to Pcd/CpuHotEjectData.h > (7,15,16) Fixup guard macro to be C namespace compliant. Also fixup the > comment style near the endif guard. > (8-10) Rename CPU_HOT_EJECT_FN to a more EDK2 compliant style. Also add > a comment block and fix spacing. > () Rename ApicIdMap -> QemuSelectorMap while keeping the type as UINT64. > Related to a comment in patch-8 ("... add worker to do CPU ejection".) > (11a,11b) Rename CPU_EJECT_INVALID to CPU_EJECT_QEMU_SELECTOR_INVALID > and add a comment about it. > () Remove CPU_EJECT_WORKER based on review comment on a patch 8. > (12,14) Remove CPU_HOT_EJECT_DATA fields Revision and Reserved. > Reorder CPU_HOT_EJECT_DATA to minimize internal padding > and ensure elements are properly aligned. > (13a,13b) Change CpuIndex->ApicId map to ProcessorNum -> QemuSelector > () Make CPU_HOT_EJECT_HANDLER->Handler, > CPU_HOT_EJECT_HANDLER->QemuSelectorMap volatile. > > OvmfPkg/OvmfPkg.dec | 4 +++ > OvmfPkg/Include/Pcd/CpuHotEjectData.h | 52 +++++++++++++++++++++++++++++++++++ > 2 files changed, 56 insertions(+) > create mode 100644 OvmfPkg/Include/Pcd/CpuHotEjectData.h Very nice updates; I only have superficial comments this time: (1) The patch -- correctly -- no longer touches CpuHotplugSmm, so please remove "/CpuHotplugSmm" from the subject line. > > diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec > index 4348bb45c64a..9629707020ba 100644 > --- a/OvmfPkg/OvmfPkg.dec > +++ b/OvmfPkg/OvmfPkg.dec > @@ -352,6 +352,10 @@ [PcdsDynamic, PcdsDynamicEx] > # This PCD is only accessed if PcdSmmSmramRequire is TRUE (see below). > gUefiOvmfPkgTokenSpaceGuid.PcdQ35SmramAtDefaultSmbase|FALSE|BOOLEAN|0x34 > > + ## This PCD adds a communication channel between OVMF's SmmCpuFeaturesLib > + # instance in PiSmmCpuDxeSmm, and CpuHotplugSmm. > + gUefiOvmfPkgTokenSpaceGuid.PcdCpuHotEjectDataAddress|0|UINT64|0x46 > + > [PcdsFeatureFlag] > gUefiOvmfPkgTokenSpaceGuid.PcdQemuBootOrderPciTranslation|TRUE|BOOLEAN|0x1c > gUefiOvmfPkgTokenSpaceGuid.PcdQemuBootOrderMmioTranslation|FALSE|BOOLEAN|0x1d > diff --git a/OvmfPkg/Include/Pcd/CpuHotEjectData.h b/OvmfPkg/Include/Pcd/CpuHotEjectData.h > new file mode 100644 > index 000000000000..024a92726869 > --- /dev/null > +++ b/OvmfPkg/Include/Pcd/CpuHotEjectData.h > @@ -0,0 +1,52 @@ > +/** @file > + Definition for the CPU_HOT_EJECT_DATA structure, which shares > + CPU hot-eject state between OVMF's SmmCpuFeaturesLib instance in > + PiSmmCpuDxeSmm, and CpuHotplugSmm. > + > + CPU_HOT_EJECT_DATA is allocated in SMRAM, and pointed-to by > + PcdCpuHotEjectDataAddress. > + > + PcdCpuHotEjectDataAddress is valid when SMM_REQUIRE is TRUE > + and MaxNumberOfCpus > 1. (2) Looks good, thanks; please clarify it as follows though: s/MaxNumberOfCpus/PcdCpuMaxLogicalProcessorNumber/ > + > + Copyright (C) 2021, Oracle Corporation. > + > + SPDX-License-Identifier: BSD-2-Clause-Patent > +**/ > + > +#ifndef CPU_HOT_EJECT_DATA_H_ > +#define CPU_HOT_EJECT_DATA_H_ > + > +/** > + CPU Hot-eject handler, called from SmmCpuFeaturesRendezvousExit() > + on each CPU at exit from SMM. > + > + @param[in] ProcessorNum ProcessorNum denotes the CPU exiting SMM, > + and will be used as an index into > + CPU_HOT_EJECT_DATA->QemuSelectorMap. It is > + identical to the processor handle in > + EFI_SMM_CPU_SERVICE_PROTOCOL. > +**/ > +typedef > +VOID > +(EFIAPI *CPU_HOT_EJECT_HANDLER) ( > + IN UINTN ProcessorNum > + ); > + > +// > +// CPU_EJECT_QEMU_SELECTOR_INVALID marks CPUs not being ejected in > +// CPU_HOT_EJECT_DATA->QemuSelectorMap. > +// > +// QEMU CPU Selector is UINT32, so we choose an invalid value larger > +// than that type. > +// > +#define CPU_EJECT_QEMU_SELECTOR_INVALID (MAX_UINT64) > + > +typedef struct { > + volatile UINT64 *QemuSelectorMap; // Maps ProcessorNum -> QemuSelector > + // for pending hot-ejects > + volatile CPU_HOT_EJECT_HANDLER Handler; // Handler to do the CPU ejection > + UINT32 ArrayLength; // Entries in the QemuSelectorMap (3) Please move "*QemuSelectorMap" and "ArrayLength" to the right, by 9 spaces, so that they line up with "Handler". Now... I realize that such an update would result in the awkwarly wide code snippet: > typedef struct { > volatile UINT64 *QemuSelectorMap; // Maps ProcessorNum -> QemuSelector > // for pending hot-ejects > volatile CPU_HOT_EJECT_HANDLER Handler; // Handler to do the CPU ejection > UINT32 ArrayLength; // Entries in the QemuSelectorMap > }; where it is not really possible to re-wrap the comments usefully. With that in mind, the following is likely the best approach: > typedef struct { > // > // Maps ProcessorNum -> QemuSelector for pending hot-ejects > // > volatile UINT64 *QemuSelectorMap; > // > // Handler to do the CPU ejection > // > volatile CPU_HOT_EJECT_HANDLER Handler; > // > // Entries in the QemuSelectorMap > // > UINT32 ArrayLength; > }; This way, you have ample room for the comments. Plus, the field names need not care about any visual alignment, because they are separated by the comments anyway. Thanks! Laszlo > +} CPU_HOT_EJECT_DATA; > + > +#endif // CPU_HOT_EJECT_DATA_H_ >