Re: [PATCH v5 06/14] OvmfPkg:AmdSevDxe: Add AmdSevDxe driver

[Laszlo Ersek <lersek@redhat.com>]

cosmetic comments if a v6 is needed:

On 05/22/17 17:23, Brijesh Singh wrote:
> When SEV is enabled, the MMIO memory range must be mapped as unencrypted
> (i.e C-bit cleared).
> 
> We need to clear the C-bit for MMIO GCD entries in order to cover the
> ranges that were added during the PEI phase (through memory resource
> descriptor HOBs). Additionally, the NonExistent ranges are processed
> in order to cover, in advance, MMIO ranges added later in the DXE phase
> by various device drivers, via the appropriate DXE memory space services.
> 
> The approach is not transparent for later addition of system memory ranges
> to the GCD memory space map. (Such ranges should be encrypted.) OVMF does
> not do such a thing at the moment, so this approach should be OK.
> 
> The driver is being added to the APRIORI DXE file so that, we clear the
> C-bit from MMIO regions before any driver accesses it.
> 
> 
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Leo Duran <leo.duran@amd.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Suggested-by: Jiewen Yao <jiewen.yao@intel.com>
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> Reviewed-by: Jiewen Yao <jiewen.yao@intel.com>
> ---
>  OvmfPkg/OvmfPkgIa32X64.dsc      |  1 +
>  OvmfPkg/OvmfPkgX64.dsc          |  1 +
>  OvmfPkg/OvmfPkgIa32X64.fdf      |  2 +
>  OvmfPkg/OvmfPkgX64.fdf          |  2 +
>  OvmfPkg/AmdSevDxe/AmdSevDxe.inf | 43 ++++++++++++
>  OvmfPkg/AmdSevDxe/AmdSevDxe.c   | 71 ++++++++++++++++++++
>  6 files changed, 120 insertions(+)
> 
> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
> index ef245635224c..daf2faadea35 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> @@ -822,6 +822,7 @@ [Components.X64]
>  !endif
>  
>    OvmfPkg/PlatformDxe/Platform.inf
> +  OvmfPkg/AmdSevDxe/AmdSevDxe.inf
>  
>  !if $(SMM_REQUIRE) == TRUE
>    OvmfPkg/SmmAccess/SmmAccess2Dxe.inf
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index 0a693f2772a7..6189088da86c 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -820,6 +820,7 @@ [Components]
>  !endif
>  
>    OvmfPkg/PlatformDxe/Platform.inf
> +  OvmfPkg/AmdSevDxe/AmdSevDxe.inf
>  
>  !if $(SMM_REQUIRE) == TRUE
>    OvmfPkg/SmmAccess/SmmAccess2Dxe.inf
> diff --git a/OvmfPkg/OvmfPkgIa32X64.fdf b/OvmfPkg/OvmfPkgIa32X64.fdf
> index 5233314139bc..12871860d001 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.fdf
> +++ b/OvmfPkg/OvmfPkgIa32X64.fdf
> @@ -190,6 +190,7 @@ [FV.DXEFV]
>  APRIORI DXE {
>    INF  MdeModulePkg/Universal/DevicePathDxe/DevicePathDxe.inf
>    INF  MdeModulePkg/Universal/PCD/Dxe/Pcd.inf
> +  INF  OvmfPkg/AmdSevDxe/AmdSevDxe.inf
>  !if $(SMM_REQUIRE) == FALSE
>    INF  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
>  !endif
> @@ -351,6 +352,7 @@ [FV.DXEFV]
>  INF  OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf
>  INF  OvmfPkg/VirtioGpuDxe/VirtioGpu.inf
>  INF  OvmfPkg/PlatformDxe/Platform.inf
> +INF  OvmfPkg/AmdSevDxe/AmdSevDxe.inf
>  
>  !if $(SMM_REQUIRE) == TRUE
>  INF  OvmfPkg/SmmAccess/SmmAccess2Dxe.inf
> diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
> index 36150101e784..ae6e66a1c08d 100644
> --- a/OvmfPkg/OvmfPkgX64.fdf
> +++ b/OvmfPkg/OvmfPkgX64.fdf
> @@ -190,6 +190,7 @@ [FV.DXEFV]
>  APRIORI DXE {
>    INF  MdeModulePkg/Universal/DevicePathDxe/DevicePathDxe.inf
>    INF  MdeModulePkg/Universal/PCD/Dxe/Pcd.inf
> +  INF  OvmfPkg/AmdSevDxe/AmdSevDxe.inf
>  !if $(SMM_REQUIRE) == FALSE
>    INF  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
>  !endif
> @@ -351,6 +352,7 @@ [FV.DXEFV]
>  INF  OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf
>  INF  OvmfPkg/VirtioGpuDxe/VirtioGpu.inf
>  INF  OvmfPkg/PlatformDxe/Platform.inf
> +INF  OvmfPkg/AmdSevDxe/AmdSevDxe.inf
>  
>  !if $(SMM_REQUIRE) == TRUE
>  INF  OvmfPkg/SmmAccess/SmmAccess2Dxe.inf
> diff --git a/OvmfPkg/AmdSevDxe/AmdSevDxe.inf b/OvmfPkg/AmdSevDxe/AmdSevDxe.inf
> new file mode 100644
> index 000000000000..41635a57a454
> --- /dev/null
> +++ b/OvmfPkg/AmdSevDxe/AmdSevDxe.inf
> @@ -0,0 +1,43 @@
> +#/** @file
> +#
> +#  Driver clears the encryption attribute from MMIO regions when SEV is enabled
> +#
> +#  Copyright (c) 2017, AMD Inc. All rights reserved.<BR>
> +#
> +#  This program and the accompanying materials
> +#  are licensed and made available under the terms and conditions of the BSD
> +#  License which accompanies this distribution.  The full text of the license may
> +#  be found at http://opensource.org/licenses/bsd-license.php
> +#
> +#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +#  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +#
> +#**/
> +
> +[Defines]
> +  INF_VERSION                    = 1.25
> +  BASE_NAME                      = AmdSevDxe
> +  FILE_GUID                      = 2ec9da37-ee35-4de9-86c5-6d9a81dc38a7
> +  MODULE_TYPE                    = DXE_DRIVER
> +  VERSION_STRING                 = 1.0
> +  ENTRY_POINT                    = AmdSevDxeEntryPoint
> +
> +[Sources]
> +  AmdSevDxe.c
> +
> +[Packages]
> +  MdePkg/MdePkg.dec
> +  MdeModulePkg/MdeModulePkg.dec
> +  OvmfPkg/OvmfPkg.dec
> +
> +[LibraryClasses]
> +  BaseLib
> +  UefiLib
> +  UefiDriverEntryPoint
> +  UefiBootServicesTableLib
> +  DxeServicesTableLib
> +  DebugLib
> +  MemEncryptSevLib
> +
> +[Depex]
> +  TRUE
> diff --git a/OvmfPkg/AmdSevDxe/AmdSevDxe.c b/OvmfPkg/AmdSevDxe/AmdSevDxe.c
> new file mode 100644
> index 000000000000..c483ae1419fd
> --- /dev/null
> +++ b/OvmfPkg/AmdSevDxe/AmdSevDxe.c
> @@ -0,0 +1,71 @@
> +/** @file
> +
> +  AMD Sev Dxe driver. The driver runs in APRIORI phase and clears C-bit from

(1) "APRIORI" is not a phase, but a file in the firmware volume that
instructs the DXE core to do things. Please refer to "10.3 The A Priori
File" in Volume 2 of the Platform Init spec.

So the comment should say, "this driver is dispatched early in DXE, due
to being listed in the APRIORI DXE file".

> +  MMIO and NonExistent Memory space when SEV is enabled.
> +
> +  Copyright (c) 2017, AMD Inc. All rights reserved.<BR>
> +
> +  This program and the accompanying materials
> +  are licensed and made available under the terms and conditions of the BSD
> +  License which accompanies this distribution.  The full text of the license may
> +  be found at http://opensource.org/licenses/bsd-license.php
> +
> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +
> +**/
> +
> +#include <PiDxe.h>
> +
> +#include <Library/BaseLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/BaseMemoryLib.h>
> +#include <Library/MemoryAllocationLib.h>
> +#include <Library/UefiBootServicesTableLib.h>
> +#include <Library/DxeServicesTableLib.h>
> +#include <Library/MemEncryptSevLib.h>
> +
> +EFI_STATUS
> +EFIAPI
> +AmdSevDxeEntryPoint (
> +  IN EFI_HANDLE         ImageHandle,
> +  IN EFI_SYSTEM_TABLE   *SystemTable
> +  )
> +{
> +  EFI_STATUS                       Status;
> +  EFI_GCD_MEMORY_SPACE_DESCRIPTOR  *AllDescMap;
> +  UINTN                            NumEntries;
> +  UINTN                            Index;
> +
> +  //
> +  // Do nothing when SEV is not enabled
> +  //
> +  if (!MemEncryptSevIsEnabled ()) {
> +    return EFI_UNSUPPORTED;
> +  }
> +
> +  //
> +  // Iterate through the GCD map and clear the C-bit from MMIO and NonExistent
> +  // memory space. The NonExistent memory space will be used for mapping the MMIO
> +  // space added later (eg PciRootBridge). By clearing both known MMIO and NonExistent
> +  // memory space can gurantee that current and furture MMIO adds will have
> +  // C-bit cleared.
> +  //
> +  Status = gDS->GetMemorySpaceMap (&NumEntries, &AllDescMap);
> +  if (Status == EFI_SUCCESS) {

(2) It is a bit more idiomatic to write:

  !EFI_ERROR (Status))

> +    for (Index = 0; Index < NumEntries; Index++) {
> +      CONST EFI_GCD_MEMORY_SPACE_DESCRIPTOR *Desc;
> +
> +      Desc = &AllDescMap[Index];
> +      if (Desc->GcdMemoryType == EfiGcdMemoryTypeMemoryMappedIo ||
> +          Desc->GcdMemoryType == EfiGcdMemoryTypeNonExistent) {
> +        Status = MemEncryptSevClearPageEncMask (0, Desc->BaseAddress, EFI_SIZE_TO_PAGES(Desc->Length), FALSE);
> +        ASSERT_EFI_ERROR(Status);

(3) Missing space after opening paren.

> +      }
> +    }
> +
> +    FreePool (AllDescMap);
> +  }
> +
> +  return EFI_SUCCESS;
> +}
> 

(4) The lines are too long in this file.


If no v6 is necessary, I'm OK with the patch as-is.

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

Thanks
Laszlo