Re: [edk2-devel] [PATCH RFC v2 09/28] OvmfPkg/VmgExitLib: Allow PMBASE register access in Dxe phase

["Laszlo Ersek" <lersek@redhat.com>]

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.

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.
>    //
>