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

[PATCH v3 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 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

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                            | 68 ++++++++++++++++++++++
 .../FwBlockServiceSmm.c                            | 13 +++++
 5 files changed, 90 insertions(+), 37 deletions(-)

-- 
2.7.4

[PATCH v3 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                               | 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 v3 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                            | 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 v3 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..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 v3 1/3] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: mark Flash memory range as MMIO

[Laszlo Ersek]

On 07/05/18 21:12, Brijesh Singh wrote:
> 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                               | 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,

(1) This should be changed to "EfiGcdAllocateAddress". (I didn't notice
this in your previous submission; however, to my excuse, I did suggest
it correctly in
<http://mid.mail-archive.com/fbba354d-1df7-0da5-80a6-40df601b95ea@redhat.com>
:) )

Note that this omission does not invalidate your testing, because
"AllocateAddress" (from "MdePkg/Include/Uefi/UefiSpec.h") has value 2,
and "EfiGcdAllocateAddress" (from "MdePkg/Include/Pi/PiDxeCis.h") has
value 2 as well.

> -                  EfiRuntimeServicesData,
> -                  EFI_SIZE_TO_PAGES (Length),
> -                  &BaseAddress
> +                  EfiGcdMemoryTypeMemoryMappedIo,
> +                  0,
> +                  Length,
> +                  &BaseAddress,
> +                  gImageHandle,
> +                  NULL
> +                  );
> +  ASSERT_EFI_ERROR (Status);
> +
> +  Status      = gDS->GetMemorySpaceDescriptor (BaseAddress, &GcdDescriptor);

(2) The extra space before the equal sign (=) looks uncalled for.

> +  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
> 

Thanks!
Laszlo

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

[Laszlo Ersek]

On 07/05/18 21:12, Brijesh Singh wrote:
> 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                            | 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>

(1) Can you drop "MemoryAllocationLib.h"? I don't think it is needed.

(2) Also, can you add the "DxeServicesTableLib.h" include directive so
that the Library #include list remains sorted?

>  
>  #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;
> +}

(3) Please don't forget to update this function (post-move) as I
requested for patch #1.

> 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;
> +}
> 

Looks OK to me otherwise.

Thanks
Laszlo

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

[Laszlo Ersek]

On 07/06/18 13:35, Laszlo Ersek wrote:
> On 07/05/18 21:12, Brijesh Singh wrote:
>> 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                            | 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>
> 
> (1) Can you drop "MemoryAllocationLib.h"? I don't think it is needed.
> 
> (2) Also, can you add the "DxeServicesTableLib.h" include directive so
> that the Library #include list remains sorted?
> 
>>  
>>  #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;
>> +}
> 
> (3) Please don't forget to update this function (post-move) as I
> requested for patch #1.
> 
>> 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;
>> +}
>>
> 
> Looks OK to me otherwise.

Sorry, got another remark:

(4) In "FwBlockService.h", you declare the function prototype -- very
correctly -- with the "IN" parameter decorators. Can you please add the
same "IN" decorators to both *definitions* of the function as well?

Thank you very much!
Laszlo

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

[Laszlo Ersek]

On 07/05/18 21:12, Brijesh Singh wrote:
> 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..6bb5c2093790 100644
> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
> @@ -54,6 +54,7 @@ [LibraryClasses]
>    DevicePathLib
>    DxeServicesTableLib
>    MemoryAllocationLib
> +  MemEncryptSevLib
>    PcdLib
>    UefiBootServicesTableLib
>    UefiDriverEntryPoint

(1) Please keep this list sorted.

(I realize I must have missed this dis-order when we added the
MemEncryptSevLib class to the "FvbServicesSmm.inf" file. If you want,
you can include a patch in v4 for fixing that order, but it's really not
necessary. Let's just not increase the dis-order if we can manage.)

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

(2) Here too, please keep the Library #include list sorted.

>  
>  #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.
> +  //

(3) A couple of cleanups, please:

(3a) I suggest s/should have mapped/mapped/, because AmdSevDxe is really
not to blame here,

(3b) s/SetMemorySpaceAttribute/SetMemorySpaceAttributes/ (plural)

(3c) s/remap/remaps/

(3d) s/Lets/Let's/

(3e) Please check that, after the typo fixes, we are still under 80
chars per line.

> +  if (MemEncryptSevIsEnabled()) {

(4) missing space before the "()"

> +    Status = MemEncryptSevClearPageEncMask (
> +               0,
> +               BaseAddress,
> +               EFI_SIZE_TO_PAGES (Length),
> +               FALSE
> +               );

I'm glad that you had documented the "Flush" parameter earlier! :)
"Flush the caches before clearing the bit (mostly TRUE except MMIO
addresses)". So, this looks fine.

> +    ASSERT_EFI_ERROR (Status);
> +  }
> +
>    return Status;
>  }
> 

I've requested a few changes, but none of those affect functionality in
practice. So, I will go ahead and collect some test results. I'll report
back.

Thanks!
Laszlo

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

[Laszlo Ersek]

On 07/05/18 21:12, 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 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
> 
> 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                            | 68 ++++++++++++++++++++++
>  .../FwBlockServiceSmm.c                            | 13 +++++
>  5 files changed, 90 insertions(+), 37 deletions(-)
> 

Regression tested -- only without SEV -- on top of commit 9090c8b53301,
using the configs below.

The "rdmsr" and "efibootmgr" checks are explained at
<https://github.com/tianocore/tianocore.github.io/wiki/Testing-SMM-with-QEMU,-KVM-and-libvirt>.


* i440fx, X64, without SMM:

(01) Fedora 28 guest, efibootmgr + rdmsr checks after boot and S3: PASS

(02) RHEL 6 guest, efibootmgr check after boot: PASS

(03) RHEL 7 guest, efibootmgr check after boot and S3: PASS

(04) Windows 7 guest, boot and S3: PASS


* q35, IA32, with SMM:

(05) Fedora 26 guest, efibootmgr check after boot and S3: PASS


* q35, IA32X64, with SMM:

(06) Fedora 28 guest, efibootmgr + rdmsr checks after boot and S3: PASS

(07) RHEL 7 guest, efibootmgr check after boot and S3: PASS

(08) Windows 10 guest, boot and S3: PASS

(09) Windows Server 2016 guest, boot: PASS
     (can't test S3 due to signing requirements for the QXL driver)

(10) Windows 8.1 guest, boot and S3: PASS

(11) Windows Server 2012 R2 guest, boot and S3: PASS

(12) Windows 7 guest, boot and S3: PASS

(13) Windows Server 2008 R2 guest, boot and S3: PASS


Once you submit v4 with the superficial updates requested, I will add:

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

I'll leave the SEV testing (with and without SMM) up to you :)

Thanks!
Laszlo