Re: [edk2-devel] [PATCH 06/12] OvmfPkg/AmdSevDxe: Clear encryption bit on PCIe MMCONFIG range

["Lendacky, Thomas" <thomas.lendacky@amd.com>]

On 1/4/21 3:04 PM, Laszlo Ersek wrote:
> On 12/15/20 21:51, Lendacky, Thomas wrote:
>> From: Tom Lendacky <thomas.lendacky@amd.com>
>>
>> BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3108&amp;data=04%7C01%7Cthomas.lendacky%40amd.com%7Cf35ac4fb20264b713aa108d8b0f45717%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637453910773208310%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=wZg2UIdJ%2FZ2HLGbWcfli3SVzl1cSMkyI%2FvREVldOB9M%3D&amp;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@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
>> Cc: Brijesh Singh <brijesh.singh@amd.com>
>> Signed-off-by: Tom Lendacky <thomas.lendacky@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@redhat.com>
> 
> Thanks
> Laszlo
>