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

[PATCH v2 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: 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                               | 28 ++++++++++++++++------
 1 file changed, 21 insertions(+), 7 deletions(-)

diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c
index 558b395dff4a..28499991a43c 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 (
+  Status = gDS->AllocateMemorySpace (
                   AllocateAddress,
-                  EfiRuntimeServicesData,
-                  EFI_SIZE_TO_PAGES (Length),
-                  &BaseAddress
+                  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 v2 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: 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                            | 51 ++++++++++++++++++++++
 .../FwBlockServiceSmm.c                            | 13 ++++++
 4 files changed, 71 insertions(+), 50 deletions(-)

diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c
index 28499991a43c..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 (
-                  AllocateAddress,
-                  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..646427bf4e2c 100644
--- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c
+++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c
@@ -22,6 +22,8 @@
 #include <Library/UefiRuntimeLib.h>
 #include <Protocol/DevicePath.h>
 #include <Protocol/FirmwareVolumeBlock.h>
+#include <Library/DxeServicesTableLib.h>
+#include <Library/MemoryAllocationLib.h>
 
 #include "FwBlockService.h"
 #include "QemuFlash.h"
@@ -155,3 +157,52 @@ InstallVirtualAddressChangeHandler (
                   );
   ASSERT_EFI_ERROR (Status);
 }
+
+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 (
+                  AllocateAddress,
+                  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..cdb073348158 100644
--- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c
+++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c
@@ -67,3 +67,16 @@ InstallVirtualAddressChangeHandler (
   // Nothing.
   //
 }
+
+EFI_STATUS
+MarkIoMemoryRangeForRuntimeAccess (
+  EFI_PHYSICAL_ADDRESS                BaseAddress,
+  UINTN                               Length
+  )
+{
+  //
+  // Nothing
+  //
+
+  return EFI_SUCCESS;
+}
-- 
2.7.4

[PATCH v2 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: 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..6bb5c2093790 100644
--- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
+++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
@@ -54,6 +54,7 @@ [LibraryClasses]
   DevicePathLib
   DxeServicesTableLib
   MemoryAllocationLib
+  MemEncryptSevLib
   PcdLib
   UefiBootServicesTableLib
   UefiDriverEntryPoint
diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c
index 646427bf4e2c..3add4bbad74c 100644
--- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c
+++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c
@@ -24,6 +24,7 @@
 #include <Protocol/FirmwareVolumeBlock.h>
 #include <Library/DxeServicesTableLib.h>
 #include <Library/MemoryAllocationLib.h>
+#include <Library/MemEncryptSevLib.h>
 
 #include "FwBlockService.h"
 #include "QemuFlash.h"
@@ -204,5 +205,21 @@ MarkIoMemoryRangeForRuntimeAccess (
                   );
   ASSERT_EFI_ERROR (Status);
 
+  //
+  // When SEV is active, AmdSevDxe should have mapped the BaseAddress with
+  // C=0 but SetMemorySpaceAttribute() remap the range with C=1. Lets 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 v2 1/3] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: mark Flash memory range as MMIO

[Laszlo Ersek]

Hi Brijesh,

On 07/05/18 16:05, Brijesh Singh wrote:
> [...]

I'll need some time before I can look at this -- meanwhile, can you
please repost the series with a dedicated cover letter email? If you
wish you can include a short summary description and v2/v3 etc updates
there; however, the main point is that the patches be nicely collected
in a threaded list view. Patches #2 and #3 shouldn't be children of
Patch #1. And, people should be able to comment on the series as a whole
-- for that, cover letters which they can respond to are the best.

https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-05

  git config format.coverletter true

or

  git config format.coverletter auto


When you do this, please collect all the CC's across the patch emails,
sort them uniquely, and add them manually to the cover letter too.

https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-23

(Note that the references above are not official edk2 material; I'm just
linking them for better explaining my request.)

Thanks!
Laszlo

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

[Brijesh Singh]

On 7/5/18 9:41 AM, Laszlo Ersek wrote:
> Hi Brijesh,
>
> On 07/05/18 16:05, Brijesh Singh wrote:
>> [...]
> I'll need some time before I can look at this -- meanwhile, can you
> please repost the series with a dedicated cover letter email? If you
> wish you can include a short summary description and v2/v3 etc updates
> there; however, the main point is that the patches be nicely collected
> in a threaded list view. Patches #2 and #3 shouldn't be children of
> Patch #1. And, people should be able to comment on the series as a whole
> -- for that, cover letters which they can respond to are the best.
>
> https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-05
>
>   git config format.coverletter true
>
> or
>
>   git config format.coverletter auto
>
>
> When you do this, please collect all the CC's across the patch emails,
> sort them uniquely, and add them manually to the cover letter too.
>
> https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-23
>
> (Note that the references above are not official edk2 material; I'm just
> linking them for better explaining my request.)

Sure, I will resend series with cover-letter. thanks