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

2018-07-06 Thread 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 
>> Cc: Anthony Perard 
>> Cc: Julien Grall 
>> Cc: Justen Jordan L 
>> Cc: Laszlo Ersek 
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Brijesh Singh 
>> ---
>>  .../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 2841a43c..eec8b1b1ae9d 100644
>> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c
>> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c
>> @@ -831,56 +831,6 @@ ValidateFvHeader (
>>  
>>  STATIC
>>  EFI_STATUS
>> -MarkIoMemoryRangeForRuntimeAccess (
>> -  EFI_PHYSICAL_ADDRESSBaseAddress,
>> -  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,
>> -  ,
>> -  gImageHandle,
>> -  NULL
>> -  );
>> -  ASSERT_EFI_ERROR (Status);
>> -
>> -  Status  = gDS->GetMemorySpaceDescriptor (BaseAddress, );
>> -  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 
>>  #include 
>>  #include 
>> +#include 
>> +#include 
> 
> (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_ADDRESSBaseAddress,
>> +  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,
>> +

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

2018-07-06 Thread 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 
> Cc: Anthony Perard 
> Cc: Julien Grall 
> Cc: Justen Jordan L 
> Cc: Laszlo Ersek 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Brijesh Singh 
> ---
>  .../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 2841a43c..eec8b1b1ae9d 100644
> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c
> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c
> @@ -831,56 +831,6 @@ ValidateFvHeader (
>  
>  STATIC
>  EFI_STATUS
> -MarkIoMemoryRangeForRuntimeAccess (
> -  EFI_PHYSICAL_ADDRESSBaseAddress,
> -  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,
> -  ,
> -  gImageHandle,
> -  NULL
> -  );
> -  ASSERT_EFI_ERROR (Status);
> -
> -  Status  = gDS->GetMemorySpaceDescriptor (BaseAddress, );
> -  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 
>  #include 
>  #include 
> +#include 
> +#include 

(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_ADDRESSBaseAddress,
> +  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,
> +  ,
> +  gImageHandle,
> +  NULL
> +  );
> +  ASSERT_EFI_ERROR (Status);
> +
> +  

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

2018-07-05 Thread 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 
Cc: Anthony Perard 
Cc: Julien Grall 
Cc: Justen Jordan L 
Cc: Laszlo Ersek 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Brijesh Singh 
---
 .../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 2841a43c..eec8b1b1ae9d 100644
--- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c
+++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c
@@ -831,56 +831,6 @@ ValidateFvHeader (
 
 STATIC
 EFI_STATUS
-MarkIoMemoryRangeForRuntimeAccess (
-  EFI_PHYSICAL_ADDRESSBaseAddress,
-  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,
-  ,
-  gImageHandle,
-  NULL
-  );
-  ASSERT_EFI_ERROR (Status);
-
-  Status  = gDS->GetMemorySpaceDescriptor (BaseAddress, );
-  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 
 #include 
 #include 
+#include 
+#include 
 
 #include "FwBlockService.h"
 #include "QemuFlash.h"
@@ -155,3 +157,52 @@ InstallVirtualAddressChangeHandler (
   );
   ASSERT_EFI_ERROR (Status);
 }
+
+EFI_STATUS
+MarkIoMemoryRangeForRuntimeAccess (
+  EFI_PHYSICAL_ADDRESSBaseAddress,
+  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,
+  ,
+  gImageHandle,
+  NULL
+  );
+  ASSERT_EFI_ERROR (Status);
+
+  Status  = gDS->GetMemorySpaceDescriptor (BaseAddress, );
+  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
---