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.9663.1620310119264809053 for ; Thu, 06 May 2021 07:08:39 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=ht9szm2c; 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=1620310118; 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=PiC3UNHbI7GugpzwvPetcAbENZ57Gz8NR64MNnJbDKk=; b=ht9szm2c7ZvFmdrNKzPpIqdygYPSMVgvG2kFyysl1qlh/M+KOzUxbSS96rlH0ihWPzwHph qxQwDv5OaIh3B1EqjruDMeWbo1n2cVQDAVVpvClJd2AV+BoETqh7nOVJ4rCJ9IOA+wZc1R yMCjjB8AJBfOMPxii8kHMss6bOMs4j0= 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-353-U852tR-_PxayHUphgq9g_g-1; Thu, 06 May 2021 10:08:33 -0400 X-MC-Unique: U852tR-_PxayHUphgq9g_g-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 6CF7019251A0; Thu, 6 May 2021 14:08:31 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-113-172.ams2.redhat.com [10.36.113.172]) by smtp.corp.redhat.com (Postfix) with ESMTP id 27CA85D9DE; Thu, 6 May 2021 14:08:28 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH RFC v2 09/28] OvmfPkg/VmgExitLib: Allow PMBASE register access in Dxe phase To: brijesh.singh@amd.com, Tom Lendacky Cc: devel@edk2.groups.io, James Bottomley , Min Xu , Jiewen Yao , Jordan Justen , Ard Biesheuvel , Erdem Aktas References: <20210430115148.22267-1-brijesh.singh@amd.com> <20210430115148.22267-10-brijesh.singh@amd.com> From: "Laszlo Ersek" Message-ID: <39e81516-ae93-e737-4203-af10cb07a9f9@redhat.com> Date: Thu, 6 May 2021 16:08:28 +0200 MIME-Version: 1.0 In-Reply-To: <20210430115148.22267-10-brijesh.singh@amd.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 04/30/21 13:51, Brijesh Singh wrote: > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275 > > Commit 85b8eac59b8c5bd9c7eb9afdb64357ce1aa2e803 added support to ensure > that MMIO is only performed against the un-encrypted memory. If MMIO > is performed against encrypted memory, a #GP is raised. > > The VmgExitLib library depends on ApicTimerLib to get the APIC base > address so that it can exclude the APIC range from the un-encrypted > check. The OvmfPkg provides ApicTimerLib for the DXE phase. The > constructor AcpiTimerLibConstructor() used in the ApicTimerLib uses > the PciRead to get the PMBASE register. The PciRead() will cause an > MMIO access. > > The AmdSevDxe driver clears the memory encryption attribute from the > MMIO ranges. However, if VmgExitLib is linked to AmdSevDxe driver then the > AcpiTimerLibConstructor() will be called before AmdSevDxe driver can > clear the encryption attributes for the MMIO regions. > > Exclude the PMBASE register from the encrypted check so that we > can link VmgExitLib to the MemEncryptSevLib; which gets linked to > AmdSevDxe driver. The above explanation is inexact. There are several typos ("APIC" is incorrect, "ACPI" would be correct, for the TimerLib instance in question), but that's really just a side observation. The precise explanation is the following library instance dependency chain: OvmfPkg/AmdSevDxe/AmdSevDxe.inf -----> MemEncryptSevLib class -----> "OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLib.inf" instance -[*]-> VmgExitLib class -----> "OvmfPkg/Library/VmgExitLib/VmgExitLib.inf" instance -----> LocalApicLib class -----> "UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.inf" instance -----> TimerLib class -----> "OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf" instance -----> PciLib class -----> "OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf" instance -----> PciExpressLib class -----> "MdePkg/Library/BasePciExpressLib/BasePciExpressLib.inf" instance The link (or dependency) marked with [*] is introduced in patch #26 ("OvmfPkg/MemEncryptSevLib: Change the page state in the RMP table"). That's the change that triggers the symptom. (In combination with you testing on Q35, because on i440fx, the DxePciLibI440FxQ35 lib instance accesses PCI config space via the 0xCF8, 0xCFC IO Ports, and those are unaffected by SEV-ES.) The symptom is somewhat "unjustified", because at the end of the series, the AmdSevDxe driver makes no calls to actual TimerLib APIs (I checked -- I disassembled the "AmdSevDxe.debug" file with "objdump -S", and there is no call to any API declared in the "TimerLib.h" class header). However, the ECAM (MMCONFIG) access is still triggered, because the AcpiTimerLibConstructor() function, in "OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.c", is the constructor for the "OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf" instance, and AcpiTimerLibConstructor() calls PciRead32(). If you check the "OvmfPkg/OvmfPkgX64.dsc" file, you'll find that the PciLib class is resolved to "MdePkg/Library/BasePciLibCf8/BasePciLibCf8.inf" by default, and to "OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf" for the following module types: - DXE_DRIVER, - DXE_RUNTIME_DRIVER, - SMM_CORE, - DXE_SMM_DRIVER, - UEFI_DRIVER, - UEFI_APPLICATION. The consequence is that modules strictly after the DXE_CORE get dynamically enabled extended config space access (ECAM) on Q35 via the PciLib class, whereas all modules strictly before the DXE_CORE, and the DXE_CORE itself, are restricted to normal config space (IO Ports 0xCF8 / 0xCFC) via the PciLib class. AmdSevDxe is a DXE_DRIVER, so it used to get DxePciLibI440FxQ35 as well. The solution should be simple. In the AmdSevDxe driver specifically, we need no access to extended PCI config space. Accessing normal PCI config space, via IO Ports 0xCF8 / 0xCFC, should suffice. That can be achieved with the following module-scope override: > diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc > index 8d9a0a077601..45a02b236633 100644 > --- a/OvmfPkg/OvmfPkgX64.dsc > +++ b/OvmfPkg/OvmfPkgX64.dsc > @@ -966,7 +966,10 @@ [Components] > !endif > > OvmfPkg/PlatformDxe/Platform.inf > - OvmfPkg/AmdSevDxe/AmdSevDxe.inf > + OvmfPkg/AmdSevDxe/AmdSevDxe.inf { > + > + PciLib|MdePkg/Library/BasePciLibCf8/BasePciLibCf8.inf > + } > OvmfPkg/IoMmuDxe/IoMmuDxe.inf > OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf > ( For consistency, all DSC files that include "OvmfPkg/AmdSevDxe/AmdSevDxe.inf" should be modified similarly: - OvmfPkg/AmdSev/AmdSevX64.dsc - OvmfPkg/Bhyve/BhyveX64.dsc - OvmfPkg/OvmfPkgIa32X64.dsc - OvmfPkg/OvmfPkgX64.dsc - OvmfPkg/OvmfXen.dsc ) Therefore, please try dropping this patch, and modifying patch#26 instead -- the above module-scope override (for 5 DSC files) should be squashed into patch#26, *and* the explanation I provided above should be included in the commit message of patch#26. ... Correction: you have an independent bug in the series that affects my above analysis. Namely, you *seem* to add the VmgExitLib dependency to the "OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLib.inf" library instance, in patch#26. That's where you modify the INF file. But that's wrong: in patch#21 ("OvmfPkg/MemEncryptSevLib: Add support to validate system RAM"), you add a VmgInit() call to the same library instance, via the new file "OvmfPkg/Library/BaseMemEncryptSevLib/X64/SnpPageStateChangeInternal.c". The bug in that patch is clear from the fact that you introduce an #include directive, but that's not mirrored by an appropriate [LibraryClasses] change in the "DxeMemEncryptSevLib.inf" file. (The other two lib instance INF files, "SecMemEncryptSevLib.inf" and "PeiMemEncryptSevLib.inf" *are* modified as needed.) So you even need to move some stuff from patch#26 to patch#21, and *then* squash the above module-scope override (and explanation) into patch#21. A significant amount of work is needed on this series. I'll stop reviewing RFC v2 here, because I don't want to look at the remaining patches deeply as long as code movements etc are going to affect them. Please post the next version -- assuming no other reviewer would like to finish reviewing this version first! Thanks Laszlo > > Cc: James Bottomley > Cc: Min Xu > Cc: Jiewen Yao > Cc: Tom Lendacky > Cc: Jordan Justen > Cc: Ard Biesheuvel > Cc: Laszlo Ersek > Cc: Erdem Aktas > Signed-off-by: Brijesh Singh > --- > OvmfPkg/Library/VmgExitLib/SecVmgExitLib.inf | 4 ++ > OvmfPkg/Library/VmgExitLib/VmgExitLib.inf | 7 +++ > OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c | 45 ++++++++++++++++++++ > 3 files changed, 56 insertions(+) > > diff --git a/OvmfPkg/Library/VmgExitLib/SecVmgExitLib.inf b/OvmfPkg/Library/VmgExitLib/SecVmgExitLib.inf > index e6f6ea7972..22435a0590 100644 > --- a/OvmfPkg/Library/VmgExitLib/SecVmgExitLib.inf > +++ b/OvmfPkg/Library/VmgExitLib/SecVmgExitLib.inf > @@ -27,6 +27,7 @@ > SecVmgExitVcHandler.c > > [Packages] > + MdeModulePkg/MdeModulePkg.dec > MdePkg/MdePkg.dec > OvmfPkg/OvmfPkg.dec > UefiCpuPkg/UefiCpuPkg.dec > @@ -42,4 +43,7 @@ > [FixedPcd] > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupBase > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupSize > + gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress > > +[Pcd] > + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfHostBridgePciDevId > diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitLib.inf b/OvmfPkg/Library/VmgExitLib/VmgExitLib.inf > index c66c68726c..d3175c260e 100644 > --- a/OvmfPkg/Library/VmgExitLib/VmgExitLib.inf > +++ b/OvmfPkg/Library/VmgExitLib/VmgExitLib.inf > @@ -27,6 +27,7 @@ > PeiDxeVmgExitVcHandler.c > > [Packages] > + MdeModulePkg/MdeModulePkg.dec > MdePkg/MdePkg.dec > OvmfPkg/OvmfPkg.dec > UefiCpuPkg/UefiCpuPkg.dec > @@ -37,4 +38,10 @@ > DebugLib > LocalApicLib > MemEncryptSevLib > + PcdLib > > +[FixedPcd] > + gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress > + > +[Pcd] > + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfHostBridgePciDevId > diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c > index 24259060fd..01ac5d8c19 100644 > --- a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c > +++ b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c > @@ -14,7 +14,10 @@ > #include > #include > #include > +#include > +#include > #include > +#include > > #include "VmgExitVcHandler.h" > > @@ -596,6 +599,40 @@ UnsupportedExit ( > return Status; > } > > +STATIC > +BOOLEAN > +IsPmbaBaseAddress ( > + IN UINTN Address > + ) > +{ > + UINT16 HostBridgeDevId; > + UINTN Pmba; > + > + // > + // Query Host Bridge DID to determine platform type > + // > + HostBridgeDevId = PcdGet16 (PcdOvmfHostBridgePciDevId); > + switch (HostBridgeDevId) { > + case INTEL_82441_DEVICE_ID: > + Pmba = POWER_MGMT_REGISTER_PIIX4 (PIIX4_PMBA); > + break; > + case INTEL_Q35_MCH_DEVICE_ID: > + Pmba = POWER_MGMT_REGISTER_Q35 (ICH9_PMBASE); > + // > + // Add the MMCONFIG base address to get the Pmba base access address > + // > + Pmba += FixedPcdGet64 (PcdPciExpressBaseAddress); > + break; > + default: > + return FALSE; > + } > + > + // Round up the offset to page size > + Pmba = Pmba & ~(SIZE_4KB - 1); > + > + return (Address == Pmba); > +} > + > /** > Validate that the MMIO memory access is not to encrypted memory. > > @@ -640,6 +677,14 @@ ValidateMmioMemory ( > return 0; > } > > + // > + // Allow PMBASE accesses (which will have the encryption bit set before > + // AmdSevDxe runs in the DXE phase) > + // > + if (IsPmbaBaseAddress (Address)) { > + return 0; > + } > + > // > // Any state other than unencrypted is an error, issue a #GP. > // >