[edk2-devel] [PATCH 06/12] OvmfPkg/AmdSevDxe: Clear encryption bit on PCIe MMCONFIG range
Lendacky, Thomas
thomas.lendacky at amd.com
Tue Jan 5 22:48:26 UTC 2021
On 1/4/21 3:04 PM, Laszlo Ersek wrote:
> On 12/15/20 21:51, Lendacky, Thomas wrote:
>> From: Tom Lendacky <thomas.lendacky at amd.com>
>>
>> BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3108&data=04%7C01%7Cthomas.lendacky%40amd.com%7Cf35ac4fb20264b713aa108d8b0f45717%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637453910773208310%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=wZg2UIdJ%2FZ2HLGbWcfli3SVzl1cSMkyI%2FvREVldOB9M%3D&reserved=0
>>
>> The PCIe MMCONFIG range should be treated as an MMIO range. However,
>> there is a comment in the code explaining why AddIoMemoryBaseSizeHob()
>> is not called. The AmdSevDxe walks the GCD map looking for MemoryMappedIo
>> or NonExistent type memory and will clear the encryption bit for these
>> ranges.
>>
>> Since the MMCONFIG range does not have one of these types, the encryption
>> bit is not cleared for this range. Add support to detect the presence of
>> the MMCONFIG range and clear the encryption bit. This will be needed for
>> follow-on support that will validate MMIO under SEV-ES.
>>
>> Cc: Jordan Justen <jordan.l.justen at intel.com>
>> Cc: Laszlo Ersek <lersek at redhat.com>
>> Cc: Ard Biesheuvel <ard.biesheuvel at arm.com>
>> Cc: Brijesh Singh <brijesh.singh at amd.com>
>> Signed-off-by: Tom Lendacky <thomas.lendacky at amd.com>
>> ---
>> OvmfPkg/AmdSevDxe/AmdSevDxe.inf | 8 +++++++-
>> OvmfPkg/AmdSevDxe/AmdSevDxe.c | 20 +++++++++++++++++++-
>> 2 files changed, 26 insertions(+), 2 deletions(-)
>>
>> diff --git a/OvmfPkg/AmdSevDxe/AmdSevDxe.inf b/OvmfPkg/AmdSevDxe/AmdSevDxe.inf
>> index dd9ecc789a20..0676fcc5b6a4 100644
>> --- a/OvmfPkg/AmdSevDxe/AmdSevDxe.inf
>> +++ b/OvmfPkg/AmdSevDxe/AmdSevDxe.inf
>> @@ -2,7 +2,7 @@
>> #
>> # Driver clears the encryption attribute from MMIO regions when SEV is enabled
>> #
>> -# Copyright (c) 2017, AMD Inc. All rights reserved.<BR>
>> +# Copyright (c) 2017 - 2020, AMD Inc. All rights reserved.<BR>
>> #
>> # SPDX-License-Identifier: BSD-2-Clause-Patent
>> #
>> @@ -39,3 +39,9 @@ [Depex]
>>
>> [FeaturePcd]
>> gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire
>> +
>> +[FixedPcd]
>> + gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
>> +
>> +[Pcd]
>> + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfHostBridgePciDevId
>> diff --git a/OvmfPkg/AmdSevDxe/AmdSevDxe.c b/OvmfPkg/AmdSevDxe/AmdSevDxe.c
>> index 595586617882..ed516fcdf956 100644
>> --- a/OvmfPkg/AmdSevDxe/AmdSevDxe.c
>> +++ b/OvmfPkg/AmdSevDxe/AmdSevDxe.c
>> @@ -4,7 +4,7 @@
>> in APRIORI. It clears C-bit from MMIO and NonExistent Memory space when SEV
>> is enabled.
>>
>> - Copyright (c) 2017, AMD Inc. All rights reserved.<BR>
>> + Copyright (c) 2017 - 2020, AMD Inc. All rights reserved.<BR>
>>
>> SPDX-License-Identifier: BSD-2-Clause-Patent
>>
>> @@ -17,6 +17,7 @@
>> #include <Library/MemEncryptSevLib.h>
>> #include <Library/MemoryAllocationLib.h>
>> #include <Library/PcdLib.h>
>> +#include <IndustryStandard/Q35MchIch9.h>
>
> (1) Please keep the #include list alphabetically sorted.
Will fix.
>
>>
>> EFI_STATUS
>> EFIAPI
>> @@ -65,6 +66,23 @@ AmdSevDxeEntryPoint (
>> FreePool (AllDescMap);
>> }
>>
>> + //
>> + // If PCI Express is enabled, the MMCONFIG area has been reserved, rather
>> + // than marked as MMIO, and so the C-bit won't be cleared by the above walk
>> + // through the GCD map. Check for the MMCONFIG area and clear the C-bit for
>> + // the range.
>> + //
>> + if (PcdGet16 (PcdOvmfHostBridgePciDevId) == INTEL_Q35_MCH_DEVICE_ID) {
>> + Status = MemEncryptSevClearPageEncMask (
>> + 0,
>> + FixedPcdGet64 (PcdPciExpressBaseAddress),
>> + EFI_SIZE_TO_PAGES (SIZE_256MB),
>> + FALSE
>> + );
>> +
>> + ASSERT_EFI_ERROR (Status);
>> + }
>> +
>> //
>> // When SMM is enabled, clear the C-bit from SMM Saved State Area
>> //
>>
>
> Very interesting. One wonders why, without this change, MMCONFIG
> accesses work at all on SEV.
>
> But then... this guest phys area is not backed by RAM in the first
> place. Whenever the guest accesses it, we trap to QEMU unconditionally.
> And so memory encryption plays no role in practice, I must think.
>
> It's different for the flash, because the flash is backed by RAM, and
> whether an access to it traps to QEMU or not depends on both the access
> (r/w/x) and the mode the flash is in (programming mode on vs. off).
>
> I now wonder whether the comment in the leading context (not visible
> above), namely the one that references the root bridge MMIO aperture,
> from which the PCI MMIO BARs are allocated, is accurate. Perhaps that
> area would work in fact even if we didn't clear the C bit for them
> (considering just the accesses themselves under SEV; not SEV-ES).
>
> (2) Please include a sentence in the commit message about the fact that
> MMCONFIG is not backed by a KVM memory slot, and so actual memory
> encryption does not take place, and that's why MMCONFIG accesses do not
> break currently under SEV / SEV-ES. (This is at least what I think happens.)
Since that address range is marked as MMIO in the nested page tables by
KVM (reserved bits set), accessing that address range will always trigger
a nested page fault (NPF).
For SEV, the hardware clears the encryption bit from the GPA provided for
the NPF, so KVM/Qemu see the base address and everything just works.
For SEV-ES, the NPF triggers a #VC. Since we run identity mapped (VA ==
PA), I use the virtual address in the VMGEXIT, which doesn't contain the
encryption bit, so, again, everything just works. The SEV-ES check for the
encryption bit being set is what uncovered this condition.
I'll write that up in the commit.
Thanks,
Tom
>
> With (1) and (2) addressed:
>
> Reviewed-by: Laszlo Ersek <lersek at redhat.com>
>
> Thanks
> Laszlo
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#69759): https://edk2.groups.io/g/devel/message/69759
Mute This Topic: https://groups.io/mt/78986163/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