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

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

On 05/07/21 15:29, Brijesh Singh wrote:
> 
> 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.

More precisely, that means patches 1-8 (because patch#9 should be
replaced by the module-scope override, and also moved to just before
what is currently patch#21).

Other than that, I agree, this is a good idea. I've anyway thought that
the MdePkg stuff (5 patches) could be / should be merged up-front in
separation, and then the subsequent 3 patches for OvmfPkg are basically
refactoring. We can record the resultant commit range (8 commits) in
TianoCore#3275, and keep the BZ open for the rest of the work. So go
ahead please.

Thanks!
Laszlo


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