From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id CFF19D81115 for ; Mon, 30 Oct 2023 14:04:11 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=lmhasnG8UKIupvjTbvoksf/tGjg0R7Abd3k0+K5WqCM=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Language:Content-Type:Content-Transfer-Encoding; s=20140610; t=1698674650; v=1; b=ayNayPwhPxhxoqTd3VmzZ5bEnuoDcKZMW0JoRQc/TnvJ+q/ZuPpiIjcT4hMVEyf80hOF0RR4 fe/czklP7nBe/enyTsGstTSnqfPhZynq54plT/dgryY/OL4tSoPru/4oZ6E6h/RrENK5HFYfuwQ BY2UhMpEPPdZlUQjtu6SEbDM= X-Received: by 127.0.0.2 with SMTP id G2IQYY7687511xjIGrFPCdal; Mon, 30 Oct 2023 07:04:10 -0700 X-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.web11.149791.1698674649726990777 for ; Mon, 30 Oct 2023 07:04:09 -0700 X-Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-119-3o89SvwoNPyT5P_DRXXvaw-1; Mon, 30 Oct 2023 10:04:04 -0400 X-MC-Unique: 3o89SvwoNPyT5P_DRXXvaw-1 X-Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id B93BC90EAA8; Mon, 30 Oct 2023 14:04:01 +0000 (UTC) X-Received: from [10.39.194.199] (unknown [10.39.194.199]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 9C3275027; Mon, 30 Oct 2023 14:04:00 +0000 (UTC) Message-ID: Date: Mon, 30 Oct 2023 15:03:59 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg/MmSaveStateLib: Remove checking Smm Rev ID in AMD MmSaveStateLib To: devel@edk2.groups.io, hsienchieh.lin@amd.com Cc: Abdul Lateef Attar , Abner Chang , Gerd Hoffmann , Paolo Bonzini References: <20231030061203.1166-1-hsienchieh.lin@amd.com> From: "Laszlo Ersek" In-Reply-To: <20231030061203.1166-1-hsienchieh.lin@amd.com> X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.5 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,lersek@redhat.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: WSJ5cqTtxbn5bnrXRwkNW1HNx7686176AA= Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=ayNayPwh; spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=redhat.com (policy=none) +Gerd, +Paolo On 10/30/23 07:12, Jacque Lin via groups.io wrote: > Remove checking SMM Rev ID in AMD Save State lib when reading Save > State Register EFI_MM_SAVE_STATE_REGISTER_IO. > For AMD, it is not necessary to check SmmRevId when reading Save State > Register EFI_MM_SAVE_STATE_REGISTER_IO. > > Cc: Abdul Lateef Attar > Cc: Abner Chang > Signed-off-by: Jacque Lin > --- > UefiCpuPkg/Library/MmSaveStateLib/AmdMmSaveState.c | 13 ------------- > 1 file changed, 13 deletions(-) > > diff --git a/UefiCpuPkg/Library/MmSaveStateLib/AmdMmSaveState.c b/UefiCpu= Pkg/Library/MmSaveStateLib/AmdMmSaveState.c > index 3315a6cc44..c4bf6ad4bb 100644 > --- a/UefiCpuPkg/Library/MmSaveStateLib/AmdMmSaveState.c > +++ b/UefiCpuPkg/Library/MmSaveStateLib/AmdMmSaveState.c > @@ -102,7 +102,6 @@ MmSaveStateReadRegister ( > OUT VOID *Buffer > > ) > > { > > - UINT32 SmmRevId; > > EFI_MM_SAVE_STATE_IO_INFO *IoInfo; > > AMD_SMRAM_SAVE_STATE_MAP *CpuSaveState; > > UINT8 DataWidth; > > @@ -124,18 +123,6 @@ MmSaveStateReadRegister ( > > > // Check for special EFI_MM_SAVE_STATE_REGISTER_IO > > if (Register =3D=3D EFI_MM_SAVE_STATE_REGISTER_IO) { > > - // > > - // Get SMM Revision ID > > - // > > - MmSaveStateReadRegisterByIndex (CpuIndex, AMD_MM_SAVE_STATE_REGISTER= _SMMREVID_INDEX, sizeof (SmmRevId), &SmmRevId); > > - > > - // > > - // See if the CPU supports the IOMisc register in the save state > > - // > > - if (SmmRevId < AMD_SMM_MIN_REV_ID_X64) { > > - return EFI_NOT_FOUND; > > - } > > - > > // Check if IO Restart Dword [IO Trap] is valid or not using bit 1. > > if (!(CpuSaveState->x64.IO_DWord & 0x02u)) { > > return EFI_NOT_FOUND; > I still don't understand. Are you saying that the SmmRevId < AMD_SMM_MIN_REV_ID_X64 check will always evaluate to FALSE, and so the "return EFI_NOT_FOUND" branch is dead code? If that's the case, then the patch seems right, but *why* is SmmRevId always greater than or equal to AMD_SMM_MIN_REV_ID_X64? SmmRevId is used to tell apart x86 from x64 (i.e., to distinguish the two formats of the save state map). Is the intent of this patch to remove 32-bit (x86) support? That makes me uncomfortable, because it could break SMM support in *IA32* OVMF. Note that commit f2188fe5d155 ("OvmfPkg: Uses MmSaveStateLib library", 2023-07-03) also updated "OvmfPkg/OvmfPkgIa32.dsc". In fact, I'm worried that the conversion of OVMF to MmSaveStateLib may have been incorrect. Note that commit f2188fe5d155 removed the following comment from "OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c": > // > // No support for I/O restart > // Furthermore, commit f2188fe5d155 removed the following functions: GetRegisterIndex(), ReadSaveStateRegisterByIndex(), SmmCpuFeaturesReadSaveStateRegister(), SmmCpuFeaturesWriteSaveStateRegister(). In particular, the latter two functions contained comments and code like: > // > // Check for special EFI_SMM_SAVE_STATE_REGISTER_IO > // > if (Register =3D=3D EFI_SMM_SAVE_STATE_REGISTER_IO) { > return EFI_NOT_FOUND; > } and > // > // Writes to EFI_SMM_SAVE_STATE_REGISTER_IO are not supported > // > if (Register =3D=3D EFI_SMM_SAVE_STATE_REGISTER_IO) { > return EFI_NOT_FOUND; > } All of these came from Paolo's original commit 4036b4e57cce ("OvmfPkg: SmmCpuFeaturesLib: implement SMRAM state save map access", 2015-11-30). Consider the following commits from Paolo (including 4036b4e57cce): > commit 86d71589c1fdb099c04a0f9e548fe868a2fef266 > Author: Paolo Bonzini > Date: Mon Nov 30 18:46:27 2015 +0000 > > OvmfPkg: import SmmCpuFeaturesLib from UefiCpuPkg > > The next patches will customize the implementation, but let's start f= rom > the common version to better show the changes. and > commit d7e71b2925012c9706d1d044ca466173aac802a8 > Author: Paolo Bonzini > Date: Mon Nov 30 18:46:32 2015 +0000 > > OvmfPkg: SmmCpuFeaturesLib: remove unnecessary bits > > SMRR, MTRR, and SMM Feature Control support is not needed on a virtua= l > platform. and > commit 4036b4e57ccef4e0fa48d8389acf6390826c2bed > Author: Paolo Bonzini > Date: Mon Nov 30 18:46:37 2015 +0000 > > OvmfPkg: SmmCpuFeaturesLib: implement SMRAM state save map access > > This implementation copies SMRAM state save map access from the > PiSmmCpuDxeSmm module. > > The most notable change is: > > - dropping support for EFI_SMM_SAVE_STATE_REGISTER_IO > > - changing the implementation of EFI_SMM_SAVE_STATE_REGISTER_LMA to u= se > the SMM revision id instead of a local variable (which > UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c initializes from CPUID's= LM > bit). This accounts for QEMU's implementation of x86_64, which alw= ays > uses revision 0x20064 even if the LM bit is zero. and > commit c1fcd80bf42e6b1e91c1c742d222f1ba421b1d1d > Author: Paolo Bonzini > Date: Mon Nov 30 18:46:42 2015 +0000 > > OvmfPkg: SmmCpuFeaturesLib: customize state save map format > > This adjusts the previously introduced state save map access function= s, to > account for QEMU and KVM's 64-bit state save map following the AMD sp= ec > rather than the Intel one. Were these QEMU-specific differences considered when OVMF was migrated to MmSaveStateLib, in commit? Before the patches for TianoCore Bugzilla 4182 (commit range ad7d3ace1ad4..f2188fe5d155), we used to have the following call tree, for EFI_SMM_CPU_PROTOCOL.ReadSaveState, in OVMF: SmmReadSaveState() [UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmC= puDxeSmm.c] SmmCpuFeaturesReadSaveStateRegister() [OvmfPkg/Library/SmmCpuFeaturesLi= b/SmmCpuFeaturesLib.c] return EFI_NOT_FOUND for EFI_SMM_SAVE_STATE_REGISTER_IO Thus, the protocol member function would propagate EFI_NOT_FOUND outwards, and ReadSaveStateRegister() [UefiCpuPkg/PiSmmCpuDxeSmm/SmramSaveState.c] would not be reached. In particular, the register would not be accessed at all. After commit f2188fe5d155 however, we have this, in OVMF: SmmReadSaveState() [UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c] MmSaveStateReadRegister() [UefiCpuPkg/Library/MmSaveStateLib/AmdMmSaveS= tate.c] Namely: > // Check for special EFI_MM_SAVE_STATE_REGISTER_IO > if (Register =3D=3D EFI_MM_SAVE_STATE_REGISTER_IO) { > // > // Get SMM Revision ID > // > MmSaveStateReadRegisterByIndex (CpuIndex, AMD_MM_SAVE_STATE_REGISTER_= SMMREVID_INDEX, sizeof (SmmRevId), &SmmRevId); > > // > // See if the CPU supports the IOMisc register in the save state > // > if (SmmRevId < AMD_SMM_MIN_REV_ID_X64) { > return EFI_NOT_FOUND; > } > > // Check if IO Restart Dword [IO Trap] is valid or not using bit 1. > if (!(CpuSaveState->x64.IO_DWord & 0x02u)) { > return EFI_NOT_FOUND; > } So, indeed. The patches for TianoCore Bugzilla 4182 were not correct for OVMF. And *this* patch would make things even worse. As Paolo's commit 4036b4e57cce above states, QEMU #defines SMM_REVISION_ID as 0x00020064 (see "target/i386/tcg/sysemu/smm_helper.c"). That means that, *even after* commit f2188fe5d155, the check if (SmmRevId < AMD_SMM_MIN_REV_ID_X64) { return EFI_NOT_FOUND; } would prevent OVMF from accessing EFI_MM_SAVE_STATE_REGISTER_IO, because AMD_SMM_MIN_REV_ID_X64 is #defined as 0x30064 in "MdePkg/Include/Register/Amd/SmramSaveStateMap.h". So the condition would actually evaluate to TRUE, MmSaveStateReadRegister() would return EFI_NOT_FOUND, and SmmReadSaveState() -- the EFI_SMM_CPU_PROTOCOL.ReadSaveState protocol member function -- would also return EFI_NOT_FOUND. Put differently, that particular check would maintain the consistency of OVMF's behavior regarding EFI_MM_SAVE_STATE_REGISTER_IO, *even though* the patches for TianoCore Bugzilla 4182 (commit range ad7d3ace1ad4..f2188fe5d155) may have been incorrect in other aspects. And then the *present* patch would break *that* last resort. In short, the statement "QEMU/OVMF uses the AMD save state map" is not accurate. In most senses, yes, but not exactly. Therefore this patch is not acceptable for OVMF (unless you can prove that it makes absolutely no difference for either of IA32, IA32X64, and X64 OVMF). ... Back to the already upstream patches. Again from Paolo's original commit 4036b4e57cce ("OvmfPkg: SmmCpuFeaturesLib: implement SMRAM state save map access", 2015-11-30), we used to have the following access logic for EFI_SMM_SAVE_STATE_REGISTER_LMA, at commit ad7d3ace1ad4 (i.e., before the conversion), in SmmCpuFeaturesReadSaveStateRegister() [OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c] > // > // Check for special EFI_SMM_SAVE_STATE_REGISTER_LMA > // > if (Register =3D=3D EFI_SMM_SAVE_STATE_REGISTER_LMA) { > // > // Only byte access is supported for this register > // > if (Width !=3D 1) { > return EFI_INVALID_PARAMETER; > } > > CpuSaveState =3D (QEMU_SMRAM_SAVE_STATE_MAP *)gSmst->CpuSaveState[Cpu= Index]; > > // > // Check CPU mode > // > if ((CpuSaveState->x86.SMMRevId & 0xFFFF) =3D=3D 0) { > *(UINT8 *)Buffer =3D 32; > } else { > *(UINT8 *)Buffer =3D 64; > } > > return EFI_SUCCESS; > } But now we have, in MmSaveStateReadRegister() [UefiCpuPkg/Library/MmSaveStateLib/AmdMmSaveState.c]: > // Check for special EFI_MM_SAVE_STATE_REGISTER_LMA > if (Register =3D=3D EFI_MM_SAVE_STATE_REGISTER_LMA) { > // Only byte access is supported for this register > if (Width !=3D 1) { > return EFI_INVALID_PARAMETER; > } > > *(UINT8 *)Buffer =3D MmSaveStateGetRegisterLma (); > > return EFI_SUCCESS; > } where MmSaveStateGetRegisterLma() [UefiCpuPkg/Library/MmSaveStateLib/AmdMmSaveState.c] does the following: > UINT32 LMAValue; > > LMAValue =3D (UINT32)AsmReadMsr64 (EFER_ADDRESS) & LMA; > if (LMAValue) { > return EFI_MM_SAVE_STATE_REGISTER_LMA_64BIT; > } > > return EFI_MM_SAVE_STATE_REGISTER_LMA_32BIT; I don't know if this is still correct on QEMU or not, but this is not what the original code did. The original code checked SMMRevId, as Paolo said in the message of commit 4036b4e57cce; the new code reads the EFER MSR. Laszlo -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#110317): https://edk2.groups.io/g/devel/message/110317 Mute This Topic: https://groups.io/mt/102269908/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/19134562= 12/xyzzy [rebecca@openfw.io] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-