[edk2-devel] [PATCH RFC v2 09/28] OvmfPkg/VmgExitLib: Allow PMBASE register access in Dxe phase
Brijesh Singh
brijesh.singh at amd.com
Fri May 7 13:29:27 UTC 2021
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 at linux.ibm.com>
>> Cc: Min Xu <min.m.xu at intel.com>
>> Cc: Jiewen Yao <jiewen.yao at intel.com>
>> Cc: Tom Lendacky <thomas.lendacky at amd.com>
>> Cc: Jordan Justen <jordan.l.justen at intel.com>
>> Cc: Ard Biesheuvel <ardb+tianocore at kernel.org>
>> Cc: Laszlo Ersek <lersek at redhat.com>
>> Cc: Erdem Aktas <erdemaktas at google.com>
>> Signed-off-by: Brijesh Singh <brijesh.singh at 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/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list