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

Lendacky, Thomas posted 15 patches 3 years, 10 months ago
There is a newer version of this series
[edk2-devel] [PATCH v2 08/15] OvmfPkg/AmdSevDxe: Clear encryption bit on PCIe MMCONFIG range
Posted by Lendacky, Thomas 3 years, 10 months ago
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(-)

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
   //
-- 
2.30.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#69866): https://edk2.groups.io/g/devel/message/69866
Mute This Topic: https://groups.io/mt/79485062/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


Re: [edk2-devel] [PATCH v2 08/15] OvmfPkg/AmdSevDxe: Clear encryption bit on PCIe MMCONFIG range
Posted by Laszlo Ersek 3 years, 10 months ago
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
>    //
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#69935): https://edk2.groups.io/g/devel/message/69935
Mute This Topic: https://groups.io/mt/79485062/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-