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.133.124]) by mx.groups.io with SMTP id smtpd.web10.9909.1620310359966335699 for ; Thu, 06 May 2021 07:12:40 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=eCP32etK; spf=pass (domain: redhat.com, ip: 170.10.133.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1620310359; 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=kkGEJNnzElJF0I+9ReRquoHgR6GwN8MllYQhwO5uTcE=; b=eCP32etKocD3M86ghNTevhB35c3fJIEMLrqo2EEsEm5o0yS88Fa9x27lCDq9MQTvGfiLSV 4IJP/bV5nMQKTEs7+f10FprT/fuYgzceR3AGGDFexYbplHgVZuggV5JN9jHLODg6GMTPZU RoLwUlDZPdiqL66+rwohycZLLaVfeJI= 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-220-irh7QzpVMCCMbf7um2vj4A-1; Thu, 06 May 2021 10:12:37 -0400 X-MC-Unique: irh7QzpVMCCMbf7um2vj4A-1 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id CDA758015F7; Thu, 6 May 2021 14:12:35 +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 950501001B2C; Thu, 6 May 2021 14:12:33 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH RFC v2 09/28] OvmfPkg/VmgExitLib: Allow PMBASE register access in Dxe phase From: "Laszlo Ersek" 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> <39e81516-ae93-e737-4203-af10cb07a9f9@redhat.com> Message-ID: Date: Thu, 6 May 2021 16:12:32 +0200 MIME-Version: 1.0 In-Reply-To: <39e81516-ae93-e737-4203-af10cb07a9f9@redhat.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 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 05/06/21 16:08, Laszlo Ersek wrote: > 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. Actually, I've changed my mind, regarding the squashing. Patch#21 is already large, and the module-scope PciLib override requires a fair bit of justification (commit message, 5 DSC files modified etc). Therefore, please do implement the module-scope PciLib override in its own dedicated patch; *replacing* this one. The new patch with the module-scoped PciLib override should be inserted in the series just before the patch that establishes the dependency marked with [*], that is, just before patch#21. Thanks Laszlo > > 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. >> // >> >