From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by mx.groups.io with SMTP id smtpd.web10.53994.1673528240907673319 for ; Thu, 12 Jan 2023 04:57:21 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=AbXSut/3; spf=pass (domain: redhat.com, ip: 170.10.129.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1673528240; 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=HNobe5bv5eZ8hhcT+cNd0IFsP+4+6NigBxJQeJ+bFJU=; b=AbXSut/3LE+nIzaeSNzs9IFkw2r/tQU0Ssbr+3otFfI35WSMRR+lzVVoAHCi5G+hioywzG 44CR6DCSlpMsb7A9rbrUe845fYusK0/vvQpnHsxufhJ7gEzhm1S2hHldBraEu2T78cLAL/ I70g2S95n7FSfKTAhhKGS1mxwF9htCU= Received: from mimecast-mx02.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-671-eVkF2kZTNdOoNO-njlk_Lw-1; Thu, 12 Jan 2023 07:57:17 -0500 X-MC-Unique: eVkF2kZTNdOoNO-njlk_Lw-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.rdu2.redhat.com [10.11.54.3]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 7F5433814953; Thu, 12 Jan 2023 12:57:16 +0000 (UTC) Received: from [10.39.192.93] (unknown [10.39.192.93]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 13C711121314; Thu, 12 Jan 2023 12:57:14 +0000 (UTC) Message-ID: <5ceca637-f2db-d521-1fd6-3843587acd75@redhat.com> Date: Thu, 12 Jan 2023 13:57:13 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH v2] UefiCpuPkg: Support SMM Relocated SmBase handling From: "Laszlo Ersek" To: devel@edk2.groups.io, jiaxin.wu@intel.com Cc: Eric Dong , Ray Ni , Zeng Star , Gerd Hoffmann , Abdul Lateef Attar References: <20230111083507.8792-1-jiaxin.wu@intel.com> <5caa15ee-4c89-c2bc-6c0f-77707cfbcf61@redhat.com> In-Reply-To: <5caa15ee-4c89-c2bc-6c0f-77707cfbcf61@redhat.com> X-Scanned-By: MIMEDefang 3.1 on 10.11.54.3 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit On 1/12/23 13:37, Laszlo Ersek wrote: > On 1/11/23 09:35, Wu, Jiaxin wrote: >> Mainly changes as below: >> 1. Add Smm Base HOB, which is used to store the information of >> Smm Relocated SmBase array for each Processors; >> 2. Combine 2 SMIs (gcSmmInitTemplate & gcSmiHandlerTemplate) into one >> (gcSmiHandlerTemplate), the new SMI handler needs to run to 2 paths: one >> to SmmCpuFeaturesInitializeProcessor(), the other to SMM Core Entry Point. >> 3. Issue SMI IPI (All Excluding Self SMM IPI + BSP SMM IPI) for SMM init >> before normal SMI sources happen. >> 4. Call SmmCpuFeaturesInitializeProcessor() in parallel. >> >> v2: >> - Refine the coding style >> - Rename hob to gSmmBaseHobGuid >> - Update SmmInitHandler() to handle the SMM relocation >> - Correct the S3 for SMM relocation >> >> v1: >> - Thread: https://edk2.groups.io/g/devel/message/97748 >> >> Change-Id: Iec7bf25166bfeefb44a202285465a35b5debbce4 > > (1) Please don't include this in upstream patches. > >> Cc: Eric Dong >> Cc: Ray Ni >> Cc: Zeng Star >> Signed-off-by: Jiaxin Wu > > (2) This patch is for UefiCpuPkg, but Gerd has not been CC'd, as far as > I can tell. CC'ing him now. (Please refer to commit 0aca5901e344, > "Maintainers.txt: designate Gerd Hoffmann as UefiCpuPkg reviewer", > 2023-01-06). > >> --- >> UefiCpuPkg/Include/Guid/SmmBaseHob.h | 36 +++++ >> .../Library/SmmCpuFeaturesLib/CpuFeaturesLib.h | 2 + >> .../SmmCpuFeaturesLib/IntelSmmCpuFeaturesLib.c | 24 +++- >> .../SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf | 4 + >> .../SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf | 1 + >> UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmStm.c | 1 - >> .../StandaloneMmCpuFeaturesLib.inf | 4 + >> UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c | 39 +++++- >> UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 25 +++- >> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c | 149 ++++++++++++++++----- >> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 21 ++- >> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf | 1 + >> UefiCpuPkg/UefiCpuPkg.dec | 3 + >> 13 files changed, 261 insertions(+), 49 deletions(-) >> create mode 100644 UefiCpuPkg/Include/Guid/SmmBaseHob.h > > (3) The patch extends the interface between PiSmmCpuDxeSmm and multipe > SmmCpuFeaturesLib instances. There is no concise and complete design > description, either in the commit message, or in a TianoCore bugzilla > ticket (no reference in your commit message), or in the new header file > "SmmBaseHob.h". > > (4) The commit modifies multiple modules at once. In a producer-consumer > scenario (which is usually characteristic of HOBs), we tend to extend > the producer first (if there are multiple producers, then each in > separation), and then the consumers. Usually the consumers are expected > to keep compatibility with the lack of the HOB, if possible. > > The compatibility aspects are not explained, and the modifications are > squashed together in a single patch. Unacceptable. > > (5) OvmfPkg includes its own SmmCpuFeaturesLib instance, but there's not > a peep about OvmfPkg in the patch (code or commit message), not even an > explanation why OvmfPkg is supposed to be unaffected. Unacceptable. > > Nacked-by: Laszlo Ersek > (6) BTW, does this patch conflict with (or at least require coordination with): [edk2-devel] [PATCH v2 0/6] Adds AmdSmmCpuFeaturesLib https://edk2.groups.io/g/devel/message/98270 ? Cc'ing Abdul. Laszlo