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

["Brijesh Singh" <brijesh.singh@amd.com>]

On 5/6/21 9:08 AM, Laszlo Ersek wrote:
> On 04/30/21 13:51, Brijesh Singh wrote:
>> BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3275&data=04%7C01%7Cbrijesh.singh%40amd.com%7C01d3e5c5268043c18bdf08d910987251%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637559069206222390%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=CN2hZrjsKzfSqMAxcQLtoHTUqBOlZmdDEO9vY9XT%2FTQ%3D&reserved=0
>>
>> 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:

Thanks Laszlo, I was not aware of the module-scope override. I will go
with this approach and make sure it works after the inclusion of the
VmgExitLib.


>
>> 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!


Sounds good. What's your thought if I take out patch 1 - 9 from this RFC
series and submit them as non-RFC for the further review and acceptance
? The patch# 1-9 are basically prepatch before we get into SNP specific
bits.


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