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 <j...@linux.ibm.com>
>>> Cc: Min Xu <min.m...@intel.com>
>>> Cc: Jiewen Yao <jiewen....@intel.com>
>>> Cc: Tom Lendacky <thomas.lenda...@amd.com>
>>> Cc: Jordan Justen <jordan.l.jus...@intel.com>
>>> Cc: Ard Biesheuvel <ardb+tianoc...@kernel.org>
>>> Cc: Laszlo Ersek <ler...@redhat.com>
>>> Cc: Erdem Aktas <erdemak...@google.com>
>>> Signed-off-by: Brijesh Singh <brijesh.si...@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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
