Re: [PATCH 2/2] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: mark Runtime Data as MMIO when SEV is active

[Laszlo Ersek <lersek@redhat.com>]

Hi Brijesh,

On 07/03/18 05:11, Brijesh Singh wrote:
> When SEV is active, the flash memory range is mapped as unencrypted by
> AmdSevDxe. Mark the flash memory range with EfiGcdMemoryTypeMemoryMappedIo
> so that OS maps this memory range as unencrypted.
> 
> Cc: Justen Jordan L <jordan.l.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
> 
> Hi Laszlo,
> 
> I have tried marking flash memory range as MMIO for non SEV guest, and
> everything seems to be working fine but I was not sure if we will break
> something else in non SEV case. Because of this I have created a new
> routine which marks the range as MMIO only when SEV is active.
> 
>  .../FvbServicesRuntimeDxe.inf                      |  1 +
>  .../FwBlockService.c                               | 69 +++++++++++++++++++++-
>  2 files changed, 69 insertions(+), 1 deletion(-)

Here's what I suggest:

(1) Please send the patch for MdeModulePkg/VariableDxe separately, with
the updates that Star requests. We should commit that patch first (and
separately), so that we can collaborate on the OvmfPkg patches without
disturbing Star with further reposts.

(2) For OvmfPkg, I'd like you to split this change in three parts:

(2a) In the first patch, please switch MarkMemoryRangeForRuntimeAccess()
from the runtime memory marking to the runtime MMIO marking. This should
extend up to and including the SetMemorySpaceAttributes() call.

Also, rename MarkMemoryRangeForRuntimeAccess() to
MarkIoMemoryRangeForRuntimeAccess().

In my opinion, this is something we should do regardless of SEV.

(2b) In the second patch, please declare
MarkIoMemoryRangeForRuntimeAccess() at the end of "FwBlockService.h",
and move the implementation to "FwBlockServiceDxe.c".

For the SMM build, add an empty stub to "FwBlockServiceSmm.c".

This is because the "MMIO+runtime" marking stands for "a runtime DXE
driver or service is using this address range". However, in the SMM
build, only an SMM driver is using the address range; I don't think we
need to expose the range in the UEFI memmap at all.

(2c) In the third patch, modify the implementation in
"FwBlockServiceDxe.c", so that the SEV case is handled at the end of
MarkIoMemoryRangeForRuntimeAccess(). (By clearing the C-bit again.)


The end result *should* be that:
- in the non-SMM build,
  - we expose the area as runtime MMIO in the UEFI memmap,
  - we handle SEV explicitly,
  - and the variable DXE driver leaves the attributes alone,

- in the SMM build,
  - we don't expose the area at all,
  - we clear the C-bit *earlier* (in QemuFlashBeforeProbe()),
  - the variable SMM driver leaves the GCD / UEFI maps alone.

Obviously we'll have to regression test both builds with a number of
guest OSes; I will help with that (I can test going back to RHEL-6 and
Windows-7).

I have two more comments below:

> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
> index d7b4ec06c4e6..1af675852c86 100644
> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
> @@ -58,6 +58,7 @@ [LibraryClasses]
>    UefiBootServicesTableLib
>    UefiDriverEntryPoint
>    UefiRuntimeLib
> +  MemEncryptSevLib
>  
>  [Guids]
>    gEfiEventVirtualAddressChangeGuid   # ALWAYS_CONSUMED
> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c
> index 558b395dff4a..3aa21466556a 100644
> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c
> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c
> @@ -36,6 +36,7 @@
>  #include <Library/DxeServicesTableLib.h>
>  #include <Library/MemoryAllocationLib.h>
>  #include <Library/UefiBootServicesTableLib.h>
> +#include <Library/MemEncryptSevLib.h>
>  
>  #include "FwBlockService.h"
>  #include "QemuFlash.h"
> @@ -867,6 +868,64 @@ MarkMemoryRangeForRuntimeAccess (
>  
>  STATIC
>  EFI_STATUS
> +SevMarkMemoryRangeForRuntimeAccess (
> +  EFI_PHYSICAL_ADDRESS                BaseAddress,
> +  UINTN                               Length
> +  )
> +{
> +  EFI_STATUS                          Status;
> +  EFI_GCD_MEMORY_SPACE_DESCRIPTOR     GcdDescriptor;
> +
> +  //
> +  // Mark flash region as runtime memory
> +  //
> +  Status = gDS->RemoveMemorySpace (
> +                  BaseAddress,
> +                  Length
> +                  );
> +
> +  Status = gDS->AddMemorySpace (
> +                  EfiGcdMemoryTypeMemoryMappedIo,
> +                  BaseAddress,
> +                  Length,
> +                  EFI_MEMORY_UC | EFI_MEMORY_RUNTIME
> +                  );
> +  ASSERT_EFI_ERROR (Status);
> +
> +  Status = gDS->AllocateMemorySpace (
> +                  AllocateAddress,
> +                  EfiGcdMemoryTypeMemoryMappedIo,
> +                  0,
> +                  EFI_SIZE_TO_PAGES (Length),

(3) This seems wrong; AllocateMemorySpace() takes a number of bytes.

(I realize you pasted this verbatim from an earlier suggestion that I
made; sorry about that. :) )

> +                  &BaseAddress,
> +                  gImageHandle,
> +                  NULL
> +                  );
> +  ASSERT_EFI_ERROR (Status);
> +
> +  Status      = gDS->GetMemorySpaceDescriptor (BaseAddress, &GcdDescriptor);
> +  ASSERT_EFI_ERROR (Status);
> +
> +  Status = gDS->SetMemorySpaceAttributes (
> +                  BaseAddress,
> +                  Length,
> +                  GcdDescriptor.Attributes | EFI_MEMORY_RUNTIME
> +                  );
> +  ASSERT_EFI_ERROR (Status);
> +
> +  Status = MemEncryptSevClearPageEncMask (
> +             0,
> +             BaseAddress,
> +             EFI_SIZE_TO_PAGES (Length),
> +             FALSE
> +             );
> +  ASSERT_EFI_ERROR (Status);

(4) Please add a comment before MemEncryptSevClearPageEncMask(),
explaining that the call is necessary because SetMemorySpaceAttributes()
re-sets the C-bit, and we need to undo that.

Thanks!
Laszlo

> +
> +  return Status;
> +}
> +
> +STATIC
> +EFI_STATUS
>  InitializeVariableFvHeader (
>    VOID
>    )
> @@ -1091,7 +1150,15 @@ FvbInitialize (
>    //
>    InstallProtocolInterfaces (FvbDevice);
>  
> -  MarkMemoryRangeForRuntimeAccess (BaseAddress, Length);
> +  //
> +  // When SEV is enabled, mark the flash region as MMIO to hint the OS that
> +  // the memory range need to be mapped as unencrypted.
> +  //
> +  if (MemEncryptSevIsEnabled()) {
> +    SevMarkMemoryRangeForRuntimeAccess (BaseAddress, Length);
> +  } else {
> +    MarkMemoryRangeForRuntimeAccess (BaseAddress, Length);
> +  }
>  
>    //
>    // Set several PCD values to point to flash
>