public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Laszlo Ersek" <lersek@redhat.com>
To: brijesh.singh@amd.com, Tom Lendacky <thomas.lendacky@amd.com>
Cc: devel@edk2.groups.io, James Bottomley <jejb@linux.ibm.com>,
	Min Xu <min.m.xu@intel.com>, Jiewen Yao <jiewen.yao@intel.com>,
	Jordan Justen <jordan.l.justen@intel.com>,
	Ard Biesheuvel <ardb+tianocore@kernel.org>,
	Erdem Aktas <erdemaktas@google.com>
Subject: Re: [edk2-devel] [PATCH RFC v2 09/28] OvmfPkg/VmgExitLib: Allow PMBASE register access in Dxe phase
Date: Thu, 6 May 2021 16:12:32 +0200	[thread overview]
Message-ID: <d1e0e088-a6a1-2d8c-5659-a556920c97b8@redhat.com> (raw)
In-Reply-To: <39e81516-ae93-e737-4203-af10cb07a9f9@redhat.com>

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 {
>> +    <LibraryClasses>
>> +      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 <Library/VmgExitLib.h> 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 <jejb@linux.ibm.com>
>> Cc: Min Xu <min.m.xu@intel.com>
>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>> Cc: Tom Lendacky <thomas.lendacky@amd.com>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Erdem Aktas <erdemaktas@google.com>
>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
>> ---
>>  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 <Library/VmgExitLib.h>
>>  #include <Register/Amd/Msr.h>
>>  #include <Register/Intel/Cpuid.h>
>> +#include <IndustryStandard/Q35MchIch9.h>
>> +#include <IndustryStandard/I440FxPiix4.h>
>>  #include <IndustryStandard/InstructionParsing.h>
>> +#include <Library/PcdLib.h>
>>
>>  #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.
>>    //
>>
> 


  reply	other threads:[~2021-05-06 14:12 UTC|newest]

Thread overview: 81+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-30 11:51 [PATCH RFC v2 00/28] Add AMD Secure Nested Paging (SEV-SNP) support Brijesh Singh
2021-04-30 11:51 ` [PATCH RFC v2 01/28] MdePkg: Expand the SEV MSR to include the SNP definition Brijesh Singh
2021-05-03  8:39   ` [edk2-devel] " Laszlo Ersek
2021-05-03 11:42     ` Brijesh Singh
2021-04-30 11:51 ` [PATCH RFC v2 02/28] MdePkg: Define the GHCB Hypervisor features Brijesh Singh
2021-05-03 10:10   ` [edk2-devel] " Laszlo Ersek
2021-05-03 12:20     ` Brijesh Singh
2021-05-03 13:40       ` Laszlo Ersek
2021-04-30 11:51 ` [PATCH RFC v2 03/28] MdePkg: Define the GHCB GPA structure Brijesh Singh
2021-05-03 10:24   ` [edk2-devel] " Laszlo Ersek
2021-05-03 12:19     ` Laszlo Ersek
2021-05-03 12:55       ` Brijesh Singh
2021-05-03 13:50         ` Laszlo Ersek
2021-05-03 13:55           ` Laszlo Ersek
2021-04-30 11:51 ` [PATCH RFC v2 04/28] MdePkg: Define the Page State Change VMGEXIT structures Brijesh Singh
2021-05-04 12:33   ` [edk2-devel] " Laszlo Ersek
2021-05-04 13:59     ` Laszlo Ersek
2021-05-04 14:48       ` Lendacky, Thomas
2021-05-04 18:07         ` Laszlo Ersek
2021-05-04 18:53     ` Brijesh Singh
2021-05-05 18:24       ` Laszlo Ersek
2021-05-05 19:27         ` Brijesh Singh
2021-04-30 11:51 ` [PATCH RFC v2 05/28] MdePkg: Add AsmPvalidate() support Brijesh Singh
2021-05-04 13:58   ` [edk2-devel] " Laszlo Ersek
2021-05-04 14:09     ` Laszlo Ersek
2021-05-04 19:07     ` Brijesh Singh
2021-05-05 18:56       ` Laszlo Ersek
     [not found]     ` <167BF2A01FA60569.6407@groups.io>
2021-05-04 19:55       ` Brijesh Singh
2021-05-05 19:10         ` Laszlo Ersek
     [not found]       ` <167BF53DA09B327E.22277@groups.io>
2021-05-04 20:28         ` Brijesh Singh
2021-05-04 23:03           ` Brijesh Singh
2021-05-05 19:19             ` Laszlo Ersek
2021-05-05 19:17           ` Laszlo Ersek
2021-04-30 11:51 ` [PATCH RFC v2 06/28] OvmfPkg/BaseMemEncryptSevLib: Introduce MemEncryptSevClearMmioPageEncMask() Brijesh Singh
2021-05-06 10:39   ` [edk2-devel] " Laszlo Ersek
2021-05-06 19:18     ` Brijesh Singh
2021-04-30 11:51 ` [PATCH RFC v2 07/28] OvmfPkg: Use MemEncryptSevClearMmioPageEncMask() to clear EncMask from Mmio Brijesh Singh
2021-05-06 10:50   ` [edk2-devel] " Laszlo Ersek
2021-05-06 19:20     ` Brijesh Singh
2021-04-30 11:51 ` [PATCH RFC v2 08/28] OvmfPkg/BaseMemEncryptSevLib: Remove CacheFlush parameter Brijesh Singh
2021-05-06 11:08   ` [edk2-devel] " Laszlo Ersek
2021-04-30 11:51 ` [PATCH RFC v2 09/28] OvmfPkg/VmgExitLib: Allow PMBASE register access in Dxe phase Brijesh Singh
2021-05-06 14:08   ` [edk2-devel] " Laszlo Ersek
2021-05-06 14:12     ` Laszlo Ersek [this message]
2021-05-07 13:29     ` Brijesh Singh
2021-05-07 15:10       ` Laszlo Ersek
2021-05-07 15:19         ` Brijesh Singh
2021-05-07 15:47           ` Laszlo Ersek
2021-04-30 11:51 ` [PATCH RFC v2 10/28] OvmfPkg/MemEncryptSevLib: add MemEncryptSevSnpEnabled() Brijesh Singh
2021-04-30 11:51 ` [PATCH RFC v2 11/28] OvmfPkg: Reserve Secrets page in MEMFD Brijesh Singh
2021-05-05  6:42   ` [edk2-devel] " Dov Murik
2021-05-05 13:11     ` Brijesh Singh
2021-05-05 19:33       ` Laszlo Ersek
2021-05-06 10:57         ` Dov Murik
2021-05-06 15:06           ` Laszlo Ersek
2021-05-06 16:12           ` James Bottomley
2021-05-06 16:02         ` James Bottomley
2021-04-30 11:51 ` [PATCH RFC v2 12/28] OvmfPkg: Reserve CPUID page for the SEV-SNP guest Brijesh Singh
2021-04-30 11:51 ` [PATCH RFC v2 13/28] OvmfPkg: Validate the data pages used in the Reset vector and SEC phase Brijesh Singh
2021-04-30 11:51 ` [PATCH RFC v2 14/28] UefiCpuPkg: Define the SEV-SNP specific dynamic PCDs Brijesh Singh
2021-04-30 11:51 ` [PATCH RFC v2 15/28] OvmfPkg/MemEncryptSevLib: extend the workarea to include SNP enabled field Brijesh Singh
2021-04-30 11:51 ` [PATCH RFC v2 16/28] OvmfPkg/MemEncryptSevLib: Extend Es Workarea to include hv features Brijesh Singh
2021-04-30 11:51 ` [PATCH RFC v2 17/28] OvmfPkg/ResetVector: Invalidate the GHCB page Brijesh Singh
2021-05-03 13:05   ` Erdem Aktas
2021-05-03 14:28     ` Brijesh Singh
2021-04-30 11:51 ` [PATCH RFC v2 18/28] OvmfPkg: Add a library to support registering GHCB GPA Brijesh Singh
2021-04-30 11:51 ` [PATCH RFC v2 19/28] OvmfPkg: register GHCB gpa for the SEV-SNP guest Brijesh Singh
2021-04-30 11:51 ` [PATCH RFC v2 20/28] UefiCpuPkg/MpLib: add support to register GHCB GPA when SEV-SNP is enabled Brijesh Singh
2021-04-30 11:51 ` [PATCH RFC v2 21/28] OvmfPkg/MemEncryptSevLib: Add support to validate system RAM Brijesh Singh
2021-05-03 14:04   ` Erdem Aktas
2021-05-03 18:56     ` Brijesh Singh
2021-04-30 11:51 ` [PATCH RFC v2 22/28] OvmfPkg/BaseMemEncryptSevLib: Skip the pre-validated " Brijesh Singh
2021-04-30 11:51 ` [PATCH RFC v2 23/28] OvmfPkg/MemEncryptSevLib: Add support to validate > 4GB memory in PEI phase Brijesh Singh
2021-04-30 11:51 ` [PATCH RFC v2 24/28] OvmfPkg/SecMain: Pre-validate the memory used for decompressing Fv Brijesh Singh
2021-04-30 11:51 ` [PATCH RFC v2 25/28] OvmfPkg/PlatformPei: Validate the system RAM when SNP is active Brijesh Singh
2021-04-30 11:51 ` [PATCH RFC v2 26/28] OvmfPkg/MemEncryptSevLib: Change the page state in the RMP table Brijesh Singh
2021-04-30 11:51 ` [PATCH RFC v2 27/28] OvmfPkg/AmdSev: Expose the SNP reserved pages through configuration table Brijesh Singh
2021-05-05  7:10   ` [edk2-devel] " Dov Murik
2021-05-05 19:37     ` Laszlo Ersek
2021-04-30 11:51 ` [PATCH RFC v2 28/28] MdePkg/GHCB: Increase the GHCB protocol max version Brijesh Singh
2021-04-30 16:49 ` [edk2-devel] [PATCH RFC v2 00/28] Add AMD Secure Nested Paging (SEV-SNP) support Laszlo Ersek

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=d1e0e088-a6a1-2d8c-5659-a556920c97b8@redhat.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox