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

[Laszlo Ersek <lersek@redhat.com>]

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