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

["Laszlo Ersek" <lersek@redhat.com>]

On 01/05/21 23:48, Tom Lendacky wrote:
> 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.

Thank you!
Laszlo