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

Brijesh Singh posted 28 patches 1 month, 2 weeks ago

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

Posted by Brijesh Singh 1 month, 2 weeks ago
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.

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



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#74636): https://edk2.groups.io/g/devel/message/74636
Mute This Topic: https://groups.io/mt/82479056/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


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

Posted by Laszlo Ersek 1 month, 1 week ago
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.
>    //
>



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#74795): https://edk2.groups.io/g/devel/message/74795
Mute This Topic: https://groups.io/mt/82479056/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


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

Posted by Brijesh Singh 1 month, 1 week ago
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&amp;data=04%7C01%7Cbrijesh.singh%40amd.com%7C01d3e5c5268043c18bdf08d910987251%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637559069206222390%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=CN2hZrjsKzfSqMAxcQLtoHTUqBOlZmdDEO9vY9XT%2FTQ%3D&amp;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.
>>    //
>>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#74814): https://edk2.groups.io/g/devel/message/74814
Mute This Topic: https://groups.io/mt/82479056/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


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

Posted by Laszlo Ersek 1 month, 1 week ago
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&amp;data=04%7C01%7Cbrijesh.singh%40amd.com%7C01d3e5c5268043c18bdf08d910987251%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637559069206222390%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=CN2hZrjsKzfSqMAxcQLtoHTUqBOlZmdDEO9vY9XT%2FTQ%3D&amp;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.
>>>    //
>>>
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#74818): https://edk2.groups.io/g/devel/message/74818
Mute This Topic: https://groups.io/mt/82479056/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


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

Posted by Brijesh Singh 1 month, 1 week ago
On 5/7/21 10:10 AM, Laszlo Ersek wrote:
>
>> 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.

Yes, I will keep patch#9 in SNP series.

FYI, I will add couple of more patches in MdePkg to define the macros
for AP creation and RMPAJUST instruction. Now that GHCB spec is final,
we are working to get the AP creation implemented for the next version.

-Brijesh



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#74819): https://edk2.groups.io/g/devel/message/74819
Mute This Topic: https://groups.io/mt/82479056/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


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

Posted by Laszlo Ersek 1 month, 1 week ago
On 05/07/21 17:19, Brijesh Singh wrote:
> 
> On 5/7/21 10:10 AM, Laszlo Ersek wrote:
>>
>>> 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.
> 
> Yes, I will keep patch#9 in SNP series.
> 
> FYI, I will add couple of more patches in MdePkg to define the macros
> for AP creation and RMPAJUST instruction. Now that GHCB spec is final,
> we are working to get the AP creation implemented for the next version.

If the spec is final, then extending the MdePkg patches makes sense, yes.

Thanks
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#74820): https://edk2.groups.io/g/devel/message/74820
Mute This Topic: https://groups.io/mt/82479056/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


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

Posted by Laszlo Ersek 1 month, 1 week ago
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.
>>    //
>>
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#74796): https://edk2.groups.io/g/devel/message/74796
Mute This Topic: https://groups.io/mt/82479056/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-