[PATCH v4 0/3] OvmfPkg: mark flash memory range as MMIO

[PATCH v4 0/3] OvmfPkg: mark flash memory range as MMIO

[Brijesh Singh]

The Qemu flash range is marked as 'system ram' in EFI runtime memmap
but it is actually an IO address. The patch series updates the
EFI runtime memmap to add this range as Memory Mapped IO address.

Changes since v3:
- minor changes to address Laszlo's review feedback.

Changes since v2:
 - added cover letter and extend CC list to add all OvmfPkg
   maintainers/reviewers.

Changes since v1:
 - split the OvmfPkg single patch into three patches based on Laszlo's feedback.
  a) mark memory as MMIO for non SEV case
  b) do not adding EFI runtime mapping for SMM build
  c) clear C-bit when SEV is active for non SMM builds only

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Anthony Perard <anthony.perard@citrix.com>
Cc: Julien Grall <julien.grall@linaro.org>
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>

Complete tree is available:
url: https://github.com/codomania/edk2.git
branch: qemu-flash-mmio-v4

Brijesh Singh (3):
  OvmfPkg/QemuFlashFvbServicesRuntimeDxe: mark Flash memory range as
    MMIO
  OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Do not expose MMIO in SMM
    build
  OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Restore C-bit when SEV is
    active

 .../FvbServicesRuntimeDxe.inf                      |  1 +
 .../FwBlockService.c                               | 38 +-----------
 .../FwBlockService.h                               |  7 +++
 .../FwBlockServiceDxe.c                            | 67 ++++++++++++++++++++++
 .../FwBlockServiceSmm.c                            | 13 +++++
 5 files changed, 89 insertions(+), 37 deletions(-)

-- 
2.7.4

[PATCH v4 1/3] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: mark Flash memory range as MMIO

[Brijesh Singh]

The flash memory range is an IO address and should be presented as Memory
Mapped IO in EFI Runtime mapping. This information can be used by OS
when mapping the flash memory range.

It is especially helpful in SEV guest case, in which IO addresses should
be mapped as unencrypted. If memory region is not marked as MMIO then OS
maps the range as encrypted.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Anthony Perard <anthony.perard@citrix.com>
Cc: Julien Grall <julien.grall@linaro.org>
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>
---
 .../FwBlockService.c                               | 30 ++++++++++++++++------
 1 file changed, 22 insertions(+), 8 deletions(-)

diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c
index 558b395dff4a..b3f428bb4284 100644
--- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c
+++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c
@@ -831,12 +831,13 @@ ValidateFvHeader (
 
 STATIC
 EFI_STATUS
-MarkMemoryRangeForRuntimeAccess (
+MarkIoMemoryRangeForRuntimeAccess (
   EFI_PHYSICAL_ADDRESS                BaseAddress,
   UINTN                               Length
   )
 {
   EFI_STATUS                          Status;
+  EFI_GCD_MEMORY_SPACE_DESCRIPTOR     GcdDescriptor;
 
   //
   // Mark flash region as runtime memory
@@ -847,18 +848,31 @@ MarkMemoryRangeForRuntimeAccess (
                   );
 
   Status = gDS->AddMemorySpace (
-                  EfiGcdMemoryTypeSystemMemory,
+                  EfiGcdMemoryTypeMemoryMappedIo,
                   BaseAddress,
                   Length,
                   EFI_MEMORY_UC | EFI_MEMORY_RUNTIME
                   );
   ASSERT_EFI_ERROR (Status);
 
-  Status = gBS->AllocatePages (
-                  AllocateAddress,
-                  EfiRuntimeServicesData,
-                  EFI_SIZE_TO_PAGES (Length),
-                  &BaseAddress
+  Status = gDS->AllocateMemorySpace (
+                  EfiGcdAllocateAddress,
+                  EfiGcdMemoryTypeMemoryMappedIo,
+                  0,
+                  Length,
+                  &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);
 
@@ -1091,7 +1105,7 @@ FvbInitialize (
   //
   InstallProtocolInterfaces (FvbDevice);
 
-  MarkMemoryRangeForRuntimeAccess (BaseAddress, Length);
+  MarkIoMemoryRangeForRuntimeAccess (BaseAddress, Length);
 
   //
   // Set several PCD values to point to flash
-- 
2.7.4

[PATCH v4 2/3] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Do not expose MMIO in SMM build

[Brijesh Singh]

In the SMM build, only an SMM driver is using the address range hence we
do not need to expose the flash MMIO range in EFI runtime mapping.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Anthony Perard <anthony.perard@citrix.com>
Cc: Julien Grall <julien.grall@linaro.org>
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>
---
 .../FwBlockService.c                               | 50 ----------------------
 .../FwBlockService.h                               |  7 +++
 .../FwBlockServiceDxe.c                            | 50 ++++++++++++++++++++++
 .../FwBlockServiceSmm.c                            | 13 ++++++
 4 files changed, 70 insertions(+), 50 deletions(-)

diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c
index b3f428bb4284..eec8b1b1ae9d 100644
--- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c
+++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c
@@ -831,56 +831,6 @@ ValidateFvHeader (
 
 STATIC
 EFI_STATUS
-MarkIoMemoryRangeForRuntimeAccess (
-  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 (
-                  EfiGcdAllocateAddress,
-                  EfiGcdMemoryTypeMemoryMappedIo,
-                  0,
-                  Length,
-                  &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);
-
-  return Status;
-}
-
-STATIC
-EFI_STATUS
 InitializeVariableFvHeader (
   VOID
   )
diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.h b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.h
index 1f9287b08769..178f578d49f0 100644
--- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.h
+++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.h
@@ -189,4 +189,11 @@ VOID
 InstallVirtualAddressChangeHandler (
   VOID
   );
+
+EFI_STATUS
+MarkIoMemoryRangeForRuntimeAccess (
+  IN EFI_PHYSICAL_ADDRESS   BaseAddress,
+  IN UINTN                  Length
+  );
+
 #endif
diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c
index 63b308658e36..37deece363e6 100644
--- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c
+++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c
@@ -17,6 +17,7 @@
 #include <Guid/EventGroup.h>
 #include <Library/DebugLib.h>
 #include <Library/DevicePathLib.h>
+#include <Library/DxeServicesTableLib.h>
 #include <Library/PcdLib.h>
 #include <Library/UefiBootServicesTableLib.h>
 #include <Library/UefiRuntimeLib.h>
@@ -155,3 +156,52 @@ InstallVirtualAddressChangeHandler (
                   );
   ASSERT_EFI_ERROR (Status);
 }
+
+EFI_STATUS
+MarkIoMemoryRangeForRuntimeAccess (
+  IN EFI_PHYSICAL_ADDRESS                BaseAddress,
+  IN 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 (
+                  EfiGcdAllocateAddress,
+                  EfiGcdMemoryTypeMemoryMappedIo,
+                  0,
+                  Length,
+                  &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);
+
+  return Status;
+}
diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c
index e0617f2503a2..af08fa69d489 100644
--- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c
+++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c
@@ -67,3 +67,16 @@ InstallVirtualAddressChangeHandler (
   // Nothing.
   //
 }
+
+EFI_STATUS
+MarkIoMemoryRangeForRuntimeAccess (
+  IN EFI_PHYSICAL_ADDRESS                BaseAddress,
+  IN UINTN                               Length
+  )
+{
+  //
+  // Nothing
+  //
+
+  return EFI_SUCCESS;
+}
-- 
2.7.4

[PATCH v4 3/3] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Restore C-bit when SEV is active

[Brijesh Singh]

AmdSevDxe maps the flash memory range with C=0, but
SetMemorySpaceAttributes() unconditionally resets the C-bit to '1'. Lets
restore the mapping back to C=0.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Anthony Perard <anthony.perard@citrix.com>
Cc: Julien Grall <julien.grall@linaro.org>
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>
---
 .../FvbServicesRuntimeDxe.inf                           |  1 +
 .../QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c  | 17 +++++++++++++++++
 2 files changed, 18 insertions(+)

diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
index d7b4ec06c4e6..86b244a0095b 100644
--- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
+++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
@@ -53,6 +53,7 @@ [LibraryClasses]
   DebugLib
   DevicePathLib
   DxeServicesTableLib
+  MemEncryptSevLib
   MemoryAllocationLib
   PcdLib
   UefiBootServicesTableLib
diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c
index 37deece363e6..1fbe1342a57c 100644
--- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c
+++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c
@@ -18,6 +18,7 @@
 #include <Library/DebugLib.h>
 #include <Library/DevicePathLib.h>
 #include <Library/DxeServicesTableLib.h>
+#include <Library/MemEncryptSevLib.h>
 #include <Library/PcdLib.h>
 #include <Library/UefiBootServicesTableLib.h>
 #include <Library/UefiRuntimeLib.h>
@@ -203,5 +204,21 @@ MarkIoMemoryRangeForRuntimeAccess (
                   );
   ASSERT_EFI_ERROR (Status);
 
+  //
+  // When SEV is active, AmdSevDxe mapped the BaseAddress with C=0 but
+  // SetMemorySpaceAttributes() remaps the range with C=1. Let's restore
+  // the mapping so that both guest and hyervisor can access the flash
+  // memory range.
+  //
+  if (MemEncryptSevIsEnabled ()) {
+    Status = MemEncryptSevClearPageEncMask (
+               0,
+               BaseAddress,
+               EFI_SIZE_TO_PAGES (Length),
+               FALSE
+               );
+    ASSERT_EFI_ERROR (Status);
+  }
+
   return Status;
 }
-- 
2.7.4

Re: [PATCH v4 0/3] OvmfPkg: mark flash memory range as MMIO

[Laszlo Ersek]

On 07/06/18 17:00, Brijesh Singh wrote:
> The Qemu flash range is marked as 'system ram' in EFI runtime memmap
> but it is actually an IO address. The patch series updates the
> EFI runtime memmap to add this range as Memory Mapped IO address.
> 
> Changes since v3:
> - minor changes to address Laszlo's review feedback.
> 
> Changes since v2:
>  - added cover letter and extend CC list to add all OvmfPkg
>    maintainers/reviewers.
> 
> Changes since v1:
>  - split the OvmfPkg single patch into three patches based on Laszlo's feedback.
>   a) mark memory as MMIO for non SEV case
>   b) do not adding EFI runtime mapping for SMM build
>   c) clear C-bit when SEV is active for non SMM builds only
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Anthony Perard <anthony.perard@citrix.com>
> Cc: Julien Grall <julien.grall@linaro.org>
> 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>
> 
> Complete tree is available:
> url: https://github.com/codomania/edk2.git
> branch: qemu-flash-mmio-v4
> 
> Brijesh Singh (3):
>   OvmfPkg/QemuFlashFvbServicesRuntimeDxe: mark Flash memory range as
>     MMIO
>   OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Do not expose MMIO in SMM
>     build
>   OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Restore C-bit when SEV is
>     active
> 
>  .../FvbServicesRuntimeDxe.inf                      |  1 +
>  .../FwBlockService.c                               | 38 +-----------
>  .../FwBlockService.h                               |  7 +++
>  .../FwBlockServiceDxe.c                            | 67 ++++++++++++++++++++++
>  .../FwBlockServiceSmm.c                            | 13 +++++
>  5 files changed, 89 insertions(+), 37 deletions(-)
> 

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

Commit range 9090c8b53301..75b7aa9528bd.

Thank you for the contribution! The first two patches are a good general
improvement for non-SEV guests as well.

Laszlo