[PATCH 1/2] MdeModulePkg/Variable: Check EFI_MEMORY_RUNTIME attribute before setting it

[PATCH 1/2] MdeModulePkg/Variable: Check EFI_MEMORY_RUNTIME attribute before setting it

[Brijesh Singh]

Set the EFI_MEMORY_RUNTIME attribute in FtwNotificationEvent() only if
the attribute is not already present. This will ensure that the attributes
set by the platform drivers (e.g Ovmf pflash) is not lost.

Cc: Dong Eric <eric.dong@intel.com>
Cc: Justen Jordan L <jordan.l.justen@intel.com>
Cc: Zeng Star <star.zeng@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
 MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c
index 6b04f4f7b394..f5ab6641ef28 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c
@@ -412,13 +412,15 @@ FtwNotificationEvent (
   if (EFI_ERROR (Status)) {
     DEBUG ((DEBUG_WARN, "Variable driver failed to get flash memory attribute.\n"));
   } else {
-    Status = gDS->SetMemorySpaceAttributes (
-                    BaseAddress,
-                    Length,
-                    GcdDescriptor.Attributes | EFI_MEMORY_RUNTIME
-                    );
-    if (EFI_ERROR (Status)) {
-      DEBUG ((DEBUG_WARN, "Variable driver failed to add EFI_MEMORY_RUNTIME attribute to Flash.\n"));
+    if (!(GcdDescriptor.Attributes & EFI_MEMORY_RUNTIME)) {
+      Status = gDS->SetMemorySpaceAttributes (
+                      BaseAddress,
+                      Length,
+                      GcdDescriptor.Attributes | EFI_MEMORY_RUNTIME
+                      );
+      if (EFI_ERROR (Status)) {
+        DEBUG ((DEBUG_WARN, "Variable driver failed to add EFI_MEMORY_RUNTIME attribute to Flash.\n"));
+      }
     }
   }
 
-- 
2.7.4

[PATCH 2/2] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: mark Runtime Data as MMIO when SEV is active

[Brijesh Singh]

When SEV is active, the flash memory range is mapped as unencrypted by
AmdSevDxe. Mark the flash memory range with EfiGcdMemoryTypeMemoryMappedIo
so that OS maps this memory range as unencrypted.

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

Hi Laszlo,

I have tried marking flash memory range as MMIO for non SEV guest, and
everything seems to be working fine but I was not sure if we will break
something else in non SEV case. Because of this I have created a new
routine which marks the range as MMIO only when SEV is active.

 .../FvbServicesRuntimeDxe.inf                      |  1 +
 .../FwBlockService.c                               | 69 +++++++++++++++++++++-
 2 files changed, 69 insertions(+), 1 deletion(-)

diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
index d7b4ec06c4e6..1af675852c86 100644
--- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
+++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
@@ -58,6 +58,7 @@ [LibraryClasses]
   UefiBootServicesTableLib
   UefiDriverEntryPoint
   UefiRuntimeLib
+  MemEncryptSevLib
 
 [Guids]
   gEfiEventVirtualAddressChangeGuid   # ALWAYS_CONSUMED
diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c
index 558b395dff4a..3aa21466556a 100644
--- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c
+++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c
@@ -36,6 +36,7 @@
 #include <Library/DxeServicesTableLib.h>
 #include <Library/MemoryAllocationLib.h>
 #include <Library/UefiBootServicesTableLib.h>
+#include <Library/MemEncryptSevLib.h>
 
 #include "FwBlockService.h"
 #include "QemuFlash.h"
@@ -867,6 +868,64 @@ MarkMemoryRangeForRuntimeAccess (
 
 STATIC
 EFI_STATUS
+SevMarkMemoryRangeForRuntimeAccess (
+  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,
+                  EFI_SIZE_TO_PAGES (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);
+
+  Status = MemEncryptSevClearPageEncMask (
+             0,
+             BaseAddress,
+             EFI_SIZE_TO_PAGES (Length),
+             FALSE
+             );
+  ASSERT_EFI_ERROR (Status);
+
+  return Status;
+}
+
+STATIC
+EFI_STATUS
 InitializeVariableFvHeader (
   VOID
   )
@@ -1091,7 +1150,15 @@ FvbInitialize (
   //
   InstallProtocolInterfaces (FvbDevice);
 
-  MarkMemoryRangeForRuntimeAccess (BaseAddress, Length);
+  //
+  // When SEV is enabled, mark the flash region as MMIO to hint the OS that
+  // the memory range need to be mapped as unencrypted.
+  //
+  if (MemEncryptSevIsEnabled()) {
+    SevMarkMemoryRangeForRuntimeAccess (BaseAddress, Length);
+  } else {
+    MarkMemoryRangeForRuntimeAccess (BaseAddress, Length);
+  }
 
   //
   // Set several PCD values to point to flash
-- 
2.7.4

Re: [PATCH 1/2] MdeModulePkg/Variable: Check EFI_MEMORY_RUNTIME attribute before setting it

[Zeng, Star]

Please use
if ((GcdDescriptor.Attributes & EFI_MEMORY_RUNTIME) == 0) {
instead of
if (!(GcdDescriptor.Attributes & EFI_MEMORY_RUNTIME)) {

There is coding style and ECC tool check about it. See https://edk2-docs.gitbooks.io/edk-ii-c-coding-standards-specification/content/5_source_files/57_c_programming.html#table-10-predicate-expression-examples.


With the change above, Reviewed-by: Star Zeng <star.zeng@intel.com>


Thanks,
Star
-----Original Message-----
From: Brijesh Singh [mailto:brijesh.singh@amd.com] 
Sent: Tuesday, July 3, 2018 11:11 AM
To: edk2-devel@lists.01.org
Cc: Tom Lendacky <Thomas.Lendacky@amd.com>; Brijesh Singh <brijesh.singh@amd.com>; Dong, Eric <eric.dong@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; Zeng, Star <star.zeng@intel.com>; Laszlo Ersek <lersek@redhat.com>
Subject: [PATCH 1/2] MdeModulePkg/Variable: Check EFI_MEMORY_RUNTIME attribute before setting it

Set the EFI_MEMORY_RUNTIME attribute in FtwNotificationEvent() only if the attribute is not already present. This will ensure that the attributes set by the platform drivers (e.g Ovmf pflash) is not lost.

Cc: Dong Eric <eric.dong@intel.com>
Cc: Justen Jordan L <jordan.l.justen@intel.com>
Cc: Zeng Star <star.zeng@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
 MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c
index 6b04f4f7b394..f5ab6641ef28 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c
@@ -412,13 +412,15 @@ FtwNotificationEvent (
   if (EFI_ERROR (Status)) {
     DEBUG ((DEBUG_WARN, "Variable driver failed to get flash memory attribute.\n"));
   } else {
-    Status = gDS->SetMemorySpaceAttributes (
-                    BaseAddress,
-                    Length,
-                    GcdDescriptor.Attributes | EFI_MEMORY_RUNTIME
-                    );
-    if (EFI_ERROR (Status)) {
-      DEBUG ((DEBUG_WARN, "Variable driver failed to add EFI_MEMORY_RUNTIME attribute to Flash.\n"));
+    if (!(GcdDescriptor.Attributes & EFI_MEMORY_RUNTIME)) {
+      Status = gDS->SetMemorySpaceAttributes (
+                      BaseAddress,
+                      Length,
+                      GcdDescriptor.Attributes | EFI_MEMORY_RUNTIME
+                      );
+      if (EFI_ERROR (Status)) {
+        DEBUG ((DEBUG_WARN, "Variable driver failed to add EFI_MEMORY_RUNTIME attribute to Flash.\n"));
+      }
     }
   }
 
--
2.7.4

Re: [PATCH 1/2] MdeModulePkg/Variable: Check EFI_MEMORY_RUNTIME attribute before setting it

[Brijesh Singh]

On 7/2/18 10:21 PM, Zeng, Star wrote:
> Please use
> if ((GcdDescriptor.Attributes & EFI_MEMORY_RUNTIME) == 0) {
> instead of
> if (!(GcdDescriptor.Attributes & EFI_MEMORY_RUNTIME)) {
>
> There is coding style and ECC tool check about it. See https://edk2-docs.gitbooks.io/edk-ii-c-coding-standards-specification/content/5_source_files/57_c_programming.html#table-10-predicate-expression-examples.

thanks for quick feedback, I will update the patch with coding style fix
in next rev.

>
> With the change above, Reviewed-by: Star Zeng <star.zeng@intel.com>
>
>
> Thanks,
> Star
> -----Original Message-----
> From: Brijesh Singh [mailto:brijesh.singh@amd.com] 
> Sent: Tuesday, July 3, 2018 11:11 AM
> To: edk2-devel@lists.01.org
> Cc: Tom Lendacky <Thomas.Lendacky@amd.com>; Brijesh Singh <brijesh.singh@amd.com>; Dong, Eric <eric.dong@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; Zeng, Star <star.zeng@intel.com>; Laszlo Ersek <lersek@redhat.com>
> Subject: [PATCH 1/2] MdeModulePkg/Variable: Check EFI_MEMORY_RUNTIME attribute before setting it
>
> Set the EFI_MEMORY_RUNTIME attribute in FtwNotificationEvent() only if the attribute is not already present. This will ensure that the attributes set by the platform drivers (e.g Ovmf pflash) is not lost.
>
> Cc: Dong Eric <eric.dong@intel.com>
> Cc: Justen Jordan L <jordan.l.justen@intel.com>
> Cc: Zeng Star <star.zeng@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c
> index 6b04f4f7b394..f5ab6641ef28 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c
> @@ -412,13 +412,15 @@ FtwNotificationEvent (
>    if (EFI_ERROR (Status)) {
>      DEBUG ((DEBUG_WARN, "Variable driver failed to get flash memory attribute.\n"));
>    } else {
> -    Status = gDS->SetMemorySpaceAttributes (
> -                    BaseAddress,
> -                    Length,
> -                    GcdDescriptor.Attributes | EFI_MEMORY_RUNTIME
> -                    );
> -    if (EFI_ERROR (Status)) {
> -      DEBUG ((DEBUG_WARN, "Variable driver failed to add EFI_MEMORY_RUNTIME attribute to Flash.\n"));
> +    if (!(GcdDescriptor.Attributes & EFI_MEMORY_RUNTIME)) {
> +      Status = gDS->SetMemorySpaceAttributes (
> +                      BaseAddress,
> +                      Length,
> +                      GcdDescriptor.Attributes | EFI_MEMORY_RUNTIME
> +                      );
> +      if (EFI_ERROR (Status)) {
> +        DEBUG ((DEBUG_WARN, "Variable driver failed to add EFI_MEMORY_RUNTIME attribute to Flash.\n"));
> +      }
>      }
>    }
>  
> --
> 2.7.4
>

Re: [PATCH 2/2] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: mark Runtime Data as MMIO when SEV is active

[Laszlo Ersek]

Hi Brijesh,

On 07/03/18 05:11, Brijesh Singh wrote:
> When SEV is active, the flash memory range is mapped as unencrypted by
> AmdSevDxe. Mark the flash memory range with EfiGcdMemoryTypeMemoryMappedIo
> so that OS maps this memory range as unencrypted.
> 
> 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>
> ---
> 
> Hi Laszlo,
> 
> I have tried marking flash memory range as MMIO for non SEV guest, and
> everything seems to be working fine but I was not sure if we will break
> something else in non SEV case. Because of this I have created a new
> routine which marks the range as MMIO only when SEV is active.
> 
>  .../FvbServicesRuntimeDxe.inf                      |  1 +
>  .../FwBlockService.c                               | 69 +++++++++++++++++++++-
>  2 files changed, 69 insertions(+), 1 deletion(-)

Here's what I suggest:

(1) Please send the patch for MdeModulePkg/VariableDxe separately, with
the updates that Star requests. We should commit that patch first (and
separately), so that we can collaborate on the OvmfPkg patches without
disturbing Star with further reposts.

(2) For OvmfPkg, I'd like you to split this change in three parts:

(2a) In the first patch, please switch MarkMemoryRangeForRuntimeAccess()
from the runtime memory marking to the runtime MMIO marking. This should
extend up to and including the SetMemorySpaceAttributes() call.

Also, rename MarkMemoryRangeForRuntimeAccess() to
MarkIoMemoryRangeForRuntimeAccess().

In my opinion, this is something we should do regardless of SEV.

(2b) In the second patch, please declare
MarkIoMemoryRangeForRuntimeAccess() at the end of "FwBlockService.h",
and move the implementation to "FwBlockServiceDxe.c".

For the SMM build, add an empty stub to "FwBlockServiceSmm.c".

This is because the "MMIO+runtime" marking stands for "a runtime DXE
driver or service is using this address range". However, in the SMM
build, only an SMM driver is using the address range; I don't think we
need to expose the range in the UEFI memmap at all.

(2c) In the third patch, modify the implementation in
"FwBlockServiceDxe.c", so that the SEV case is handled at the end of
MarkIoMemoryRangeForRuntimeAccess(). (By clearing the C-bit again.)


The end result *should* be that:
- in the non-SMM build,
  - we expose the area as runtime MMIO in the UEFI memmap,
  - we handle SEV explicitly,
  - and the variable DXE driver leaves the attributes alone,

- in the SMM build,
  - we don't expose the area at all,
  - we clear the C-bit *earlier* (in QemuFlashBeforeProbe()),
  - the variable SMM driver leaves the GCD / UEFI maps alone.

Obviously we'll have to regression test both builds with a number of
guest OSes; I will help with that (I can test going back to RHEL-6 and
Windows-7).

I have two more comments below:

> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
> index d7b4ec06c4e6..1af675852c86 100644
> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
> @@ -58,6 +58,7 @@ [LibraryClasses]
>    UefiBootServicesTableLib
>    UefiDriverEntryPoint
>    UefiRuntimeLib
> +  MemEncryptSevLib
>  
>  [Guids]
>    gEfiEventVirtualAddressChangeGuid   # ALWAYS_CONSUMED
> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c
> index 558b395dff4a..3aa21466556a 100644
> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c
> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c
> @@ -36,6 +36,7 @@
>  #include <Library/DxeServicesTableLib.h>
>  #include <Library/MemoryAllocationLib.h>
>  #include <Library/UefiBootServicesTableLib.h>
> +#include <Library/MemEncryptSevLib.h>
>  
>  #include "FwBlockService.h"
>  #include "QemuFlash.h"
> @@ -867,6 +868,64 @@ MarkMemoryRangeForRuntimeAccess (
>  
>  STATIC
>  EFI_STATUS
> +SevMarkMemoryRangeForRuntimeAccess (
> +  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,
> +                  EFI_SIZE_TO_PAGES (Length),

(3) This seems wrong; AllocateMemorySpace() takes a number of bytes.

(I realize you pasted this verbatim from an earlier suggestion that I
made; sorry about that. :) )

> +                  &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);
> +
> +  Status = MemEncryptSevClearPageEncMask (
> +             0,
> +             BaseAddress,
> +             EFI_SIZE_TO_PAGES (Length),
> +             FALSE
> +             );
> +  ASSERT_EFI_ERROR (Status);

(4) Please add a comment before MemEncryptSevClearPageEncMask(),
explaining that the call is necessary because SetMemorySpaceAttributes()
re-sets the C-bit, and we need to undo that.

Thanks!
Laszlo

> +
> +  return Status;
> +}
> +
> +STATIC
> +EFI_STATUS
>  InitializeVariableFvHeader (
>    VOID
>    )
> @@ -1091,7 +1150,15 @@ FvbInitialize (
>    //
>    InstallProtocolInterfaces (FvbDevice);
>  
> -  MarkMemoryRangeForRuntimeAccess (BaseAddress, Length);
> +  //
> +  // When SEV is enabled, mark the flash region as MMIO to hint the OS that
> +  // the memory range need to be mapped as unencrypted.
> +  //
> +  if (MemEncryptSevIsEnabled()) {
> +    SevMarkMemoryRangeForRuntimeAccess (BaseAddress, Length);
> +  } else {
> +    MarkMemoryRangeForRuntimeAccess (BaseAddress, Length);
> +  }
>  
>    //
>    // Set several PCD values to point to flash
>