Re: [edk2-devel] [PATCH v2 08/15] OvmfPkg/AmdSevDxe: Clear encryption bit on PCIe MMCONFIG range

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

On 01/06/21 22:21, 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 that MMIO is not being performed to
> an encrypted address range under SEV-ES.
> 
> Even though the encryption bit was set for this range, this still worked
> under both SEV and SEV-ES because the address range is marked by the
> hypervisor as MMIO in the nested page tables:
> - For SEV, access to this address range triggers a nested page fault (NPF)
>   and the hardware supplies the guest physical address (GPA) in the VMCB's
>   EXITINFO2 field as part of the exit information. However, the encryption
>   bit is not set in the GPA, so the hypervisor can process the request
>   without any issues.
> - For SEV-ES, access to this address range triggers a #VC. Since OVMF runs
>   identity mapped (VA == PA), the virtual address is used to avoid the
>   lookup of the physical address. The virtual address does not have the
>   encryption bit set, so the hypervisor can process the request without
>   any issues.
> 
> 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>
> Reviewed-by: Laszlo Ersek <lersek@redhat.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(-)

Thanks for the updates!
Laszlo

> 
> 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..689bfb376d03 100644
> --- a/OvmfPkg/AmdSevDxe/AmdSevDxe.c
> +++ b/OvmfPkg/AmdSevDxe/AmdSevDxe.c
> @@ -4,12 +4,13 @@
>    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
>  
>  **/
>  
> +#include <IndustryStandard/Q35MchIch9.h>
>  #include <Library/BaseLib.h>
>  #include <Library/BaseMemoryLib.h>
>  #include <Library/DebugLib.h>
> @@ -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
>    //
>