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

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel