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

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

On 12/15/20 21:51, Lendacky, Thomas wrote:
> From: Tom Lendacky <thomas.lendacky@amd.com>
> 
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3108
> 
> 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.

>  
>  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.)

With (1) and (2) addressed:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo