[PATCH v2 0/2] Add SMM support when SEV is active

[PATCH v2 0/2] Add SMM support when SEV is active

[Brijesh Singh]

The series adds the SMM support for the SEV guest.

Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>

repo: https://github.com/codomania/edk2.git
branch: smm-v2

Changes since v1:
 - add more comments to explain why we are not clearing the C-bit
   from relocated SMM Saved area.
 - restore the C-bit of initial SMM Saved Area after SMBASE is
   relocated.
 - Drop "Fvb" prefix from BeforeFlashProbe() and call this from
   Qemu flash initialization.

Brijesh Singh (2):
  OvmfPkg/AmdSevDxe: Clear the C-bit from SMM Saved State
  OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Clear C-bit when SEV is active

 OvmfPkg/AmdSevDxe/AmdSevDxe.inf                            |  4 +++
 OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf    |  1 +
 OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf  |  1 +
 OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.h         |  7 ++++
 OvmfPkg/AmdSevDxe/AmdSevDxe.c                              | 35 ++++++++++++++++++++
 OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c      | 21 ++++++++++++
 OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c | 12 +++++++
 OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c | 33 ++++++++++++++++++
 OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c         |  6 ++++
 9 files changed, 120 insertions(+)

-- 
2.14.3

[PATCH v2 1/2] OvmfPkg/AmdSevDxe: Clear the C-bit from SMM Saved State

[Brijesh Singh]

When OVMF is built with SMM, SMMSaved State area (SMM_DEFAULT_SMBASE +
SMRAM_SAVE_STATE_MAP_OFFSET) contains data which need to be accessed by
both guest and hypervisor. Since the data need to be accessed by both
hence we must map the SMMSaved State area as unencrypted (i.e C-bit
cleared).

This patch clears the SavedStateArea address before SMBASE relocation.
Currently, we do not clear the SavedStateArea address after SMBASE is
relocated due to the following reasons:

1) Guest BIOS never access the relocated SavedStateArea.

2) The C-bit works on page-aligned address, but the SavedStateArea
address is not a page-aligned. Theoretically, we could roundup the address
and clear the C-bit of aligned address but looking carefully we found
that some portion of the page contains code -- which will causes a bigger
issue for the SEV guest. When SEV is enabled, all the code must be
encrypted otherwise hardware will cause trap.

Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
 OvmfPkg/AmdSevDxe/AmdSevDxe.inf                         |  4 +++
 OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf |  1 +
 OvmfPkg/AmdSevDxe/AmdSevDxe.c                           | 35 ++++++++++++++++++++
 OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c   | 21 ++++++++++++
 4 files changed, 61 insertions(+)

diff --git a/OvmfPkg/AmdSevDxe/AmdSevDxe.inf b/OvmfPkg/AmdSevDxe/AmdSevDxe.inf
index 41635a57a454..162ed98a2fbe 100644
--- a/OvmfPkg/AmdSevDxe/AmdSevDxe.inf
+++ b/OvmfPkg/AmdSevDxe/AmdSevDxe.inf
@@ -29,6 +29,7 @@ [Packages]
   MdePkg/MdePkg.dec
   MdeModulePkg/MdeModulePkg.dec
   OvmfPkg/OvmfPkg.dec
+  UefiCpuPkg/UefiCpuPkg.dec
 
 [LibraryClasses]
   BaseLib
@@ -41,3 +42,6 @@ [LibraryClasses]
 
 [Depex]
   TRUE
+
+[FeaturePcd]
+  gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire
diff --git a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
index 31edf3a9c1fd..ba564abb787b 100644
--- a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
+++ b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
@@ -36,3 +36,4 @@ [LibraryClasses]
   PcdLib
   DebugLib
   SmmServicesTableLib
+  MemEncryptSevLib
diff --git a/OvmfPkg/AmdSevDxe/AmdSevDxe.c b/OvmfPkg/AmdSevDxe/AmdSevDxe.c
index e472096320ea..5803e8655049 100644
--- a/OvmfPkg/AmdSevDxe/AmdSevDxe.c
+++ b/OvmfPkg/AmdSevDxe/AmdSevDxe.c
@@ -25,6 +25,8 @@
 #include <Library/UefiBootServicesTableLib.h>
 #include <Library/DxeServicesTableLib.h>
 #include <Library/MemEncryptSevLib.h>
+#include <Register/SmramSaveStateMap.h>
+#include <Register/QemuSmramSaveStateMap.h>
 
 EFI_STATUS
 EFIAPI
@@ -71,5 +73,38 @@ AmdSevDxeEntryPoint (
     FreePool (AllDescMap);
   }
 
+  //
+  // When SMM is enabled, clear the C-bit from SMM Saved State Area
+  //
+  // NOTES: The SavedStateArea address cleared here is before SMBASE
+  // relocation. Currently, we do not clear the SavedStateArea address after
+  // SMBASE is relocated due to the following reasons:
+  //
+  // 1) Guest BIOS never access the relocated SavedStateArea.
+  //
+  // 2) The C-bit works on page-aligned address, but the SavedStateArea
+  // address is not a page-aligned. Theoretically, we could roundup the address
+  // and clear the C-bit of aligned address but looking carefully we found
+  // that some portion of the page contains code -- which will causes a bigger
+  // issues for SEV guest. When SEV is enabled, all the code must be encrypted
+  // otherwise hardware will cause trap.
+  //
+  // We restore the C-bit for this SMM Saved State Area after SMBASE relocation
+  // is completed (See OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c).
+  //
+  if (FeaturePcdGet (PcdSmmSmramRequire)) {
+    EFI_PHYSICAL_ADDRESS  SmmSavedStateAreaAddress;
+
+    SmmSavedStateAreaAddress = SMM_DEFAULT_SMBASE + SMRAM_SAVE_STATE_MAP_OFFSET;
+
+    Status = MemEncryptSevClearPageEncMask (
+               0,
+               SmmSavedStateAreaAddress,
+               EFI_SIZE_TO_PAGES (sizeof(QEMU_SMRAM_SAVE_STATE_MAP)),
+               FALSE
+               );
+    ASSERT_EFI_ERROR (Status);
+  }
+
   return EFI_SUCCESS;
 }
diff --git a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
index a307f64c9c61..946294701c62 100644
--- a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
+++ b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
@@ -20,6 +20,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
 #include <Library/MemoryAllocationLib.h>
 #include <Library/SmmServicesTableLib.h>
 #include <Library/DebugLib.h>
+#include <Library/MemEncryptSevLib.h>
 #include <Register/QemuSmramSaveStateMap.h>
 
 //
@@ -183,6 +184,26 @@ SmmCpuFeaturesSmmRelocationComplete (
   VOID
   )
 {
+  EFI_STATUS Status;
+  EFI_PHYSICAL_ADDRESS  SmmSavedStateAreaAddress;
+
+  //
+  // When SEV is enabled, the SMM SavedState is mapped with C=0
+  // (See OvmfPkg/AmdSevDxe/AmdSevDxe.c). Now the SMBASE is relocated hence we
+  // remap the address with C=1.
+  //
+  if (!MemEncryptSevIsEnabled ()) {
+    return;
+  }
+
+  SmmSavedStateAreaAddress = SMM_DEFAULT_SMBASE + SMRAM_SAVE_STATE_MAP_OFFSET;
+  Status = MemEncryptSevSetPageEncMask (
+             0,
+             SmmSavedStateAreaAddress,
+             EFI_SIZE_TO_PAGES (sizeof(QEMU_SMRAM_SAVE_STATE_MAP)),
+             FALSE
+             );
+  ASSERT_EFI_ERROR (Status);
 }
 
 /**
-- 
2.14.3

[PATCH v2 2/2] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Clear C-bit when SEV is active

[Brijesh Singh]

Commit:24e4ad7 (OvmfPkg: Add AmdSevDxe driver) added a driver which runs
early in DXE phase and clears the C-bit from all MMIO regions (including
Qemu Flash). When SMM is enabled, we build two sets of page tables; first
page table is used when executing code in non SMM mode (SMM-less-pgtable)
and second page table is used when we are executing code in SMM mode
(SMM-pgtable).

During boot time, AmdSevDxe driver clears the C-bit from the
SMM-less-pgtable. But when SMM is enabled, Qemu Flash services are used
from SMM mode.

In this patch we explicitly clear the C-bit from Qemu flash MMIO range
before we probe the flash. When OVMF is built with SMM_REQUIRE then
call to initialize the flash services happen after the SMM-pgtable is
created and processor is serving the first SMI. At this time we will
have access to the SMM-pgtable.

Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
 OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf  |  1 +
 OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.h         |  7 +++++
 OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c | 12 +++++++
 OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c | 33 ++++++++++++++++++++
 OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c         |  6 ++++
 5 files changed, 59 insertions(+)

diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf
index ba2d3679a46d..d365e27cbe59 100644
--- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf
+++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf
@@ -53,6 +53,7 @@ [LibraryClasses]
   DevicePathLib
   DxeServicesTableLib
   MemoryAllocationLib
+  MemEncryptSevLib
   PcdLib
   SmmServicesTableLib
   UefiBootServicesTableLib
diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.h b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.h
index 8d83dca7a52c..6c4099c140e8 100644
--- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.h
+++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.h
@@ -88,5 +88,12 @@ QemuFlashConvertPointers (
   VOID
   );
 
+VOID
+BeforeFlashProbe (
+  EFI_PHYSICAL_ADDRESS    BaseAddress,
+  UINTN                   FdBlockSize,
+  UINTN                   FdBlockCount
+  );
+
 #endif
 
diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c
index 63b308658e36..a4614de3c901 100644
--- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c
+++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c
@@ -155,3 +155,15 @@ InstallVirtualAddressChangeHandler (
                   );
   ASSERT_EFI_ERROR (Status);
 }
+
+VOID
+BeforeFlashProbe (
+  EFI_PHYSICAL_ADDRESS    BaseAddress,
+  UINTN                   FdBlockSize,
+  UINTN                   FdBlockCount
+  )
+{
+  //
+  // Do nothing
+  //
+}
diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c
index e0617f2503a2..a6cad5af223b 100644
--- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c
+++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c
@@ -17,6 +17,7 @@
 #include <Library/DebugLib.h>
 #include <Library/PcdLib.h>
 #include <Library/SmmServicesTableLib.h>
+#include <Library/MemEncryptSevLib.h>
 #include <Protocol/DevicePath.h>
 #include <Protocol/SmmFirmwareVolumeBlock.h>
 
@@ -67,3 +68,35 @@ InstallVirtualAddressChangeHandler (
   // Nothing.
   //
 }
+
+VOID
+BeforeFlashProbe (
+  EFI_PHYSICAL_ADDRESS    BaseAddress,
+  UINTN                   FdBlockSize,
+  UINTN                   FdBlockCount
+  )
+{
+  EFI_STATUS              Status;
+
+  ASSERT (FeaturePcdGet (PcdSmmSmramRequire));
+
+  if (!MemEncryptSevIsEnabled()) {
+    return;
+  }
+
+  //
+  // When SEV is enabled, AmdSevDxe runs early in DXE phase and clears the C-bit
+  // from the MMIO space (including flash ranges) but the driver runs in non SMM
+  // context hence it cleared the flash ranges from non SMM page table.
+  // When SMM is enabled, the flash services are accessed from the SMM mode
+  // hence we explicitly clear the C-bit on flash ranges from SMM page table.
+  //
+
+  Status = MemEncryptSevClearPageEncMask (
+             0,
+             BaseAddress,
+             EFI_SIZE_TO_PAGES (FdBlockSize * FdBlockCount),
+             FALSE
+            );
+  ASSERT_EFI_ERROR (Status);
+}
diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c
index 5677b5ee119c..f63e11723415 100644
--- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c
+++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c
@@ -244,6 +244,12 @@ QemuFlashInitialize (
   ASSERT(PcdGet32 (PcdOvmfFirmwareFdSize) % mFdBlockSize == 0);
   mFdBlockCount = PcdGet32 (PcdOvmfFirmwareFdSize) / mFdBlockSize;
 
+  //
+  // execute platform specific hooks before probing the flash
+  //
+  BeforeFlashProbe ((EFI_PHYSICAL_ADDRESS)(UINTN) mFlashBase,
+                    mFdBlockSize, mFdBlockCount);
+
   if (!QemuFlashDetected ()) {
     ASSERT (!FeaturePcdGet (PcdSmmSmramRequire));
     return EFI_WRITE_PROTECTED;
-- 
2.14.3

Re: [PATCH v2 1/2] OvmfPkg/AmdSevDxe: Clear the C-bit from SMM Saved State

[Laszlo Ersek]

Hi Brijesh,

On 02/28/18 17:14, Brijesh Singh wrote:
> When OVMF is built with SMM, SMMSaved State area (SMM_DEFAULT_SMBASE +
> SMRAM_SAVE_STATE_MAP_OFFSET) contains data which need to be accessed by
> both guest and hypervisor. Since the data need to be accessed by both
> hence we must map the SMMSaved State area as unencrypted (i.e C-bit
> cleared).
> 
> This patch clears the SavedStateArea address before SMBASE relocation.
> Currently, we do not clear the SavedStateArea address after SMBASE is
> relocated due to the following reasons:
> 
> 1) Guest BIOS never access the relocated SavedStateArea.
> 
> 2) The C-bit works on page-aligned address, but the SavedStateArea
> address is not a page-aligned. Theoretically, we could roundup the address
> and clear the C-bit of aligned address but looking carefully we found
> that some portion of the page contains code -- which will causes a bigger
> issue for the SEV guest. When SEV is enabled, all the code must be
> encrypted otherwise hardware will cause trap.
> 
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  OvmfPkg/AmdSevDxe/AmdSevDxe.inf                         |  4 +++
>  OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf |  1 +
>  OvmfPkg/AmdSevDxe/AmdSevDxe.c                           | 35 ++++++++++++++++++++
>  OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c   | 21 ++++++++++++
>  4 files changed, 61 insertions(+)

I've been staring at this patch for ~2 hours now (I've also read your
other email). I like this approach (and the comments / commit message),
but IMO an important detail is missing.

I started writing up my notes, but the list got very long. Is it OK with
you if I send my ideas as a patch set (replacing just this patch)? I
think I'd like to turn this patch into 3-4 patches.

Thanks!
Laszlo


> diff --git a/OvmfPkg/AmdSevDxe/AmdSevDxe.inf b/OvmfPkg/AmdSevDxe/AmdSevDxe.inf
> index 41635a57a454..162ed98a2fbe 100644
> --- a/OvmfPkg/AmdSevDxe/AmdSevDxe.inf
> +++ b/OvmfPkg/AmdSevDxe/AmdSevDxe.inf
> @@ -29,6 +29,7 @@ [Packages]
>    MdePkg/MdePkg.dec
>    MdeModulePkg/MdeModulePkg.dec
>    OvmfPkg/OvmfPkg.dec
> +  UefiCpuPkg/UefiCpuPkg.dec
>  
>  [LibraryClasses]
>    BaseLib
> @@ -41,3 +42,6 @@ [LibraryClasses]
>  
>  [Depex]
>    TRUE
> +
> +[FeaturePcd]
> +  gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire
> diff --git a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
> index 31edf3a9c1fd..ba564abb787b 100644
> --- a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
> +++ b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
> @@ -36,3 +36,4 @@ [LibraryClasses]
>    PcdLib
>    DebugLib
>    SmmServicesTableLib
> +  MemEncryptSevLib
> diff --git a/OvmfPkg/AmdSevDxe/AmdSevDxe.c b/OvmfPkg/AmdSevDxe/AmdSevDxe.c
> index e472096320ea..5803e8655049 100644
> --- a/OvmfPkg/AmdSevDxe/AmdSevDxe.c
> +++ b/OvmfPkg/AmdSevDxe/AmdSevDxe.c
> @@ -25,6 +25,8 @@
>  #include <Library/UefiBootServicesTableLib.h>
>  #include <Library/DxeServicesTableLib.h>
>  #include <Library/MemEncryptSevLib.h>
> +#include <Register/SmramSaveStateMap.h>
> +#include <Register/QemuSmramSaveStateMap.h>
>  
>  EFI_STATUS
>  EFIAPI
> @@ -71,5 +73,38 @@ AmdSevDxeEntryPoint (
>      FreePool (AllDescMap);
>    }
>  
> +  //
> +  // When SMM is enabled, clear the C-bit from SMM Saved State Area
> +  //
> +  // NOTES: The SavedStateArea address cleared here is before SMBASE
> +  // relocation. Currently, we do not clear the SavedStateArea address after
> +  // SMBASE is relocated due to the following reasons:
> +  //
> +  // 1) Guest BIOS never access the relocated SavedStateArea.
> +  //
> +  // 2) The C-bit works on page-aligned address, but the SavedStateArea
> +  // address is not a page-aligned. Theoretically, we could roundup the address
> +  // and clear the C-bit of aligned address but looking carefully we found
> +  // that some portion of the page contains code -- which will causes a bigger
> +  // issues for SEV guest. When SEV is enabled, all the code must be encrypted
> +  // otherwise hardware will cause trap.
> +  //
> +  // We restore the C-bit for this SMM Saved State Area after SMBASE relocation
> +  // is completed (See OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c).
> +  //
> +  if (FeaturePcdGet (PcdSmmSmramRequire)) {
> +    EFI_PHYSICAL_ADDRESS  SmmSavedStateAreaAddress;
> +
> +    SmmSavedStateAreaAddress = SMM_DEFAULT_SMBASE + SMRAM_SAVE_STATE_MAP_OFFSET;
> +
> +    Status = MemEncryptSevClearPageEncMask (
> +               0,
> +               SmmSavedStateAreaAddress,
> +               EFI_SIZE_TO_PAGES (sizeof(QEMU_SMRAM_SAVE_STATE_MAP)),
> +               FALSE
> +               );
> +    ASSERT_EFI_ERROR (Status);
> +  }
> +
>    return EFI_SUCCESS;
>  }
> diff --git a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> index a307f64c9c61..946294701c62 100644
> --- a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> +++ b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> @@ -20,6 +20,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>  #include <Library/MemoryAllocationLib.h>
>  #include <Library/SmmServicesTableLib.h>
>  #include <Library/DebugLib.h>
> +#include <Library/MemEncryptSevLib.h>
>  #include <Register/QemuSmramSaveStateMap.h>
>  
>  //
> @@ -183,6 +184,26 @@ SmmCpuFeaturesSmmRelocationComplete (
>    VOID
>    )
>  {
> +  EFI_STATUS Status;
> +  EFI_PHYSICAL_ADDRESS  SmmSavedStateAreaAddress;
> +
> +  //
> +  // When SEV is enabled, the SMM SavedState is mapped with C=0
> +  // (See OvmfPkg/AmdSevDxe/AmdSevDxe.c). Now the SMBASE is relocated hence we
> +  // remap the address with C=1.
> +  //
> +  if (!MemEncryptSevIsEnabled ()) {
> +    return;
> +  }
> +
> +  SmmSavedStateAreaAddress = SMM_DEFAULT_SMBASE + SMRAM_SAVE_STATE_MAP_OFFSET;
> +  Status = MemEncryptSevSetPageEncMask (
> +             0,
> +             SmmSavedStateAreaAddress,
> +             EFI_SIZE_TO_PAGES (sizeof(QEMU_SMRAM_SAVE_STATE_MAP)),
> +             FALSE
> +             );
> +  ASSERT_EFI_ERROR (Status);
>  }
>  
>  /**
> 

Re: [PATCH v2 1/2] OvmfPkg/AmdSevDxe: Clear the C-bit from SMM Saved State

[Brijesh Singh]

Hi Laszlo,


On 2/28/18 1:06 PM, Laszlo Ersek wrote:
> Hi Brijesh,
>
> On 02/28/18 17:14, Brijesh Singh wrote:
>> When OVMF is built with SMM, SMMSaved State area (SMM_DEFAULT_SMBASE +
>> SMRAM_SAVE_STATE_MAP_OFFSET) contains data which need to be accessed by
>> both guest and hypervisor. Since the data need to be accessed by both
>> hence we must map the SMMSaved State area as unencrypted (i.e C-bit
>> cleared).
>>
>> This patch clears the SavedStateArea address before SMBASE relocation.
>> Currently, we do not clear the SavedStateArea address after SMBASE is
>> relocated due to the following reasons:
>>
>> 1) Guest BIOS never access the relocated SavedStateArea.
>>
>> 2) The C-bit works on page-aligned address, but the SavedStateArea
>> address is not a page-aligned. Theoretically, we could roundup the address
>> and clear the C-bit of aligned address but looking carefully we found
>> that some portion of the page contains code -- which will causes a bigger
>> issue for the SEV guest. When SEV is enabled, all the code must be
>> encrypted otherwise hardware will cause trap.
>>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
>> ---
>>  OvmfPkg/AmdSevDxe/AmdSevDxe.inf                         |  4 +++
>>  OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf |  1 +
>>  OvmfPkg/AmdSevDxe/AmdSevDxe.c                           | 35 ++++++++++++++++++++
>>  OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c   | 21 ++++++++++++
>>  4 files changed, 61 insertions(+)
> I've been staring at this patch for ~2 hours now (I've also read your
> other email). I like this approach (and the comments / commit message),
> but IMO an important detail is missing.
>
> I started writing up my notes, but the list got very long. Is it OK with
> you if I send my ideas as a patch set (replacing just this patch)? I
> think I'd like to turn this patch into 3-4 patches.

Yes,  patches are appreciated.  thanks.

>
> Thanks!
> Laszlo
>
>
>> diff --git a/OvmfPkg/AmdSevDxe/AmdSevDxe.inf b/OvmfPkg/AmdSevDxe/AmdSevDxe.inf
>> index 41635a57a454..162ed98a2fbe 100644
>> --- a/OvmfPkg/AmdSevDxe/AmdSevDxe.inf
>> +++ b/OvmfPkg/AmdSevDxe/AmdSevDxe.inf
>> @@ -29,6 +29,7 @@ [Packages]
>>    MdePkg/MdePkg.dec
>>    MdeModulePkg/MdeModulePkg.dec
>>    OvmfPkg/OvmfPkg.dec
>> +  UefiCpuPkg/UefiCpuPkg.dec
>>  
>>  [LibraryClasses]
>>    BaseLib
>> @@ -41,3 +42,6 @@ [LibraryClasses]
>>  
>>  [Depex]
>>    TRUE
>> +
>> +[FeaturePcd]
>> +  gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire
>> diff --git a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
>> index 31edf3a9c1fd..ba564abb787b 100644
>> --- a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
>> +++ b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
>> @@ -36,3 +36,4 @@ [LibraryClasses]
>>    PcdLib
>>    DebugLib
>>    SmmServicesTableLib
>> +  MemEncryptSevLib
>> diff --git a/OvmfPkg/AmdSevDxe/AmdSevDxe.c b/OvmfPkg/AmdSevDxe/AmdSevDxe.c
>> index e472096320ea..5803e8655049 100644
>> --- a/OvmfPkg/AmdSevDxe/AmdSevDxe.c
>> +++ b/OvmfPkg/AmdSevDxe/AmdSevDxe.c
>> @@ -25,6 +25,8 @@
>>  #include <Library/UefiBootServicesTableLib.h>
>>  #include <Library/DxeServicesTableLib.h>
>>  #include <Library/MemEncryptSevLib.h>
>> +#include <Register/SmramSaveStateMap.h>
>> +#include <Register/QemuSmramSaveStateMap.h>
>>  
>>  EFI_STATUS
>>  EFIAPI
>> @@ -71,5 +73,38 @@ AmdSevDxeEntryPoint (
>>      FreePool (AllDescMap);
>>    }
>>  
>> +  //
>> +  // When SMM is enabled, clear the C-bit from SMM Saved State Area
>> +  //
>> +  // NOTES: The SavedStateArea address cleared here is before SMBASE
>> +  // relocation. Currently, we do not clear the SavedStateArea address after
>> +  // SMBASE is relocated due to the following reasons:
>> +  //
>> +  // 1) Guest BIOS never access the relocated SavedStateArea.
>> +  //
>> +  // 2) The C-bit works on page-aligned address, but the SavedStateArea
>> +  // address is not a page-aligned. Theoretically, we could roundup the address
>> +  // and clear the C-bit of aligned address but looking carefully we found
>> +  // that some portion of the page contains code -- which will causes a bigger
>> +  // issues for SEV guest. When SEV is enabled, all the code must be encrypted
>> +  // otherwise hardware will cause trap.
>> +  //
>> +  // We restore the C-bit for this SMM Saved State Area after SMBASE relocation
>> +  // is completed (See OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c).
>> +  //
>> +  if (FeaturePcdGet (PcdSmmSmramRequire)) {
>> +    EFI_PHYSICAL_ADDRESS  SmmSavedStateAreaAddress;
>> +
>> +    SmmSavedStateAreaAddress = SMM_DEFAULT_SMBASE + SMRAM_SAVE_STATE_MAP_OFFSET;
>> +
>> +    Status = MemEncryptSevClearPageEncMask (
>> +               0,
>> +               SmmSavedStateAreaAddress,
>> +               EFI_SIZE_TO_PAGES (sizeof(QEMU_SMRAM_SAVE_STATE_MAP)),
>> +               FALSE
>> +               );
>> +    ASSERT_EFI_ERROR (Status);
>> +  }
>> +
>>    return EFI_SUCCESS;
>>  }
>> diff --git a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
>> index a307f64c9c61..946294701c62 100644
>> --- a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
>> +++ b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
>> @@ -20,6 +20,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>>  #include <Library/MemoryAllocationLib.h>
>>  #include <Library/SmmServicesTableLib.h>
>>  #include <Library/DebugLib.h>
>> +#include <Library/MemEncryptSevLib.h>
>>  #include <Register/QemuSmramSaveStateMap.h>
>>  
>>  //
>> @@ -183,6 +184,26 @@ SmmCpuFeaturesSmmRelocationComplete (
>>    VOID
>>    )
>>  {
>> +  EFI_STATUS Status;
>> +  EFI_PHYSICAL_ADDRESS  SmmSavedStateAreaAddress;
>> +
>> +  //
>> +  // When SEV is enabled, the SMM SavedState is mapped with C=0
>> +  // (See OvmfPkg/AmdSevDxe/AmdSevDxe.c). Now the SMBASE is relocated hence we
>> +  // remap the address with C=1.
>> +  //
>> +  if (!MemEncryptSevIsEnabled ()) {
>> +    return;
>> +  }
>> +
>> +  SmmSavedStateAreaAddress = SMM_DEFAULT_SMBASE + SMRAM_SAVE_STATE_MAP_OFFSET;
>> +  Status = MemEncryptSevSetPageEncMask (
>> +             0,
>> +             SmmSavedStateAreaAddress,
>> +             EFI_SIZE_TO_PAGES (sizeof(QEMU_SMRAM_SAVE_STATE_MAP)),
>> +             FALSE
>> +             );
>> +  ASSERT_EFI_ERROR (Status);
>>  }
>>  
>>  /**
>>

Re: [PATCH v2 2/2] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Clear C-bit when SEV is active

[Laszlo Ersek]

On 02/28/18 17:14, Brijesh Singh wrote:
> Commit:24e4ad7 (OvmfPkg: Add AmdSevDxe driver) added a driver which runs
> early in DXE phase and clears the C-bit from all MMIO regions (including
> Qemu Flash).

(1) This appears incorrect / inexact; AmdSevDxe is dispatched from
APRIORI DXE before the flash driver is dispatched, and the MMIO GCD
entry is only added by the flash driver. So in this case, AmdSevDxe
clears the C-bit on a NonExistent entry that will later be split and
accommodate the flash MMIO range.

> When SMM is enabled, we build two sets of page tables; first
> page table is used when executing code in non SMM mode (SMM-less-pgtable)
> and second page table is used when we are executing code in SMM mode
> (SMM-pgtable).
> 
> During boot time, AmdSevDxe driver clears the C-bit from the
> SMM-less-pgtable. But when SMM is enabled, Qemu Flash services are used
> from SMM mode.
> 
> In this patch we explicitly clear the C-bit from Qemu flash MMIO range
> before we probe the flash. When OVMF is built with SMM_REQUIRE then
> call to initialize the flash services happen after the SMM-pgtable is
> created and processor is serving the first SMI. At this time we will
> have access to the SMM-pgtable.

(2) Please replace "is serving" with "has served".

> 
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf  |  1 +
>  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.h         |  7 +++++
>  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c | 12 +++++++
>  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c | 33 ++++++++++++++++++++
>  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c         |  6 ++++
>  5 files changed, 59 insertions(+)
> 
> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf
> index ba2d3679a46d..d365e27cbe59 100644
> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf
> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf
> @@ -53,6 +53,7 @@ [LibraryClasses]
>    DevicePathLib
>    DxeServicesTableLib
>    MemoryAllocationLib
> +  MemEncryptSevLib
>    PcdLib
>    SmmServicesTableLib
>    UefiBootServicesTableLib
> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.h b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.h
> index 8d83dca7a52c..6c4099c140e8 100644
> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.h
> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.h
> @@ -88,5 +88,12 @@ QemuFlashConvertPointers (
>    VOID
>    );
>  
> +VOID
> +BeforeFlashProbe (
> +  EFI_PHYSICAL_ADDRESS    BaseAddress,
> +  UINTN                   FdBlockSize,
> +  UINTN                   FdBlockCount
> +  );
> +
>  #endif

(3) Sorry that I'm again requesting a name change for this function. Can
we call it QemuFlashBeforeProbe()? To be consistent with the other
function names in this header file.

(4) Please add "IN" decorators (also to the function definitions).

>  
> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c
> index 63b308658e36..a4614de3c901 100644
> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c
> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c
> @@ -155,3 +155,15 @@ InstallVirtualAddressChangeHandler (
>                    );
>    ASSERT_EFI_ERROR (Status);
>  }
> +
> +VOID
> +BeforeFlashProbe (
> +  EFI_PHYSICAL_ADDRESS    BaseAddress,
> +  UINTN                   FdBlockSize,
> +  UINTN                   FdBlockCount
> +  )
> +{
> +  //
> +  // Do nothing
> +  //
> +}

(5) This function definition should go into the existent file
"QemuFlashDxe.c".

> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c
> index e0617f2503a2..a6cad5af223b 100644
> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c
> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c
> @@ -17,6 +17,7 @@
>  #include <Library/DebugLib.h>
>  #include <Library/PcdLib.h>
>  #include <Library/SmmServicesTableLib.h>
> +#include <Library/MemEncryptSevLib.h>
>  #include <Protocol/DevicePath.h>
>  #include <Protocol/SmmFirmwareVolumeBlock.h>
>  
> @@ -67,3 +68,35 @@ InstallVirtualAddressChangeHandler (
>    // Nothing.
>    //
>  }
> +
> +VOID
> +BeforeFlashProbe (
> +  EFI_PHYSICAL_ADDRESS    BaseAddress,
> +  UINTN                   FdBlockSize,
> +  UINTN                   FdBlockCount
> +  )
> +{
> +  EFI_STATUS              Status;
> +
> +  ASSERT (FeaturePcdGet (PcdSmmSmramRequire));
> +
> +  if (!MemEncryptSevIsEnabled()) {
> +    return;
> +  }
> +
> +  //
> +  // When SEV is enabled, AmdSevDxe runs early in DXE phase and clears the C-bit
> +  // from the MMIO space (including flash ranges) but the driver runs in non SMM

(6) Please update the comment according to (1).

> +  // context hence it cleared the flash ranges from non SMM page table.
> +  // When SMM is enabled, the flash services are accessed from the SMM mode
> +  // hence we explicitly clear the C-bit on flash ranges from SMM page table.
> +  //
> +
> +  Status = MemEncryptSevClearPageEncMask (
> +             0,
> +             BaseAddress,
> +             EFI_SIZE_TO_PAGES (FdBlockSize * FdBlockCount),
> +             FALSE
> +            );

(7) The closing paren is not indented correctly, it should be aligned
with the arguments.

> +  ASSERT_EFI_ERROR (Status);
> +}

(8) This function definition should go into a new file called
"QemuFlashSmm.c" -- please make sure you add a license block at the top,
and use CRLF line endings --, and the new file should be added to
"FvbServicesSmm.inf".

> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c
> index 5677b5ee119c..f63e11723415 100644
> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c
> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c
> @@ -244,6 +244,12 @@ QemuFlashInitialize (
>    ASSERT(PcdGet32 (PcdOvmfFirmwareFdSize) % mFdBlockSize == 0);
>    mFdBlockCount = PcdGet32 (PcdOvmfFirmwareFdSize) / mFdBlockSize;
>  
> +  //
> +  // execute platform specific hooks before probing the flash
> +  //

(9) Please replace "platform" with "module type".

> +  BeforeFlashProbe ((EFI_PHYSICAL_ADDRESS)(UINTN) mFlashBase,
> +                    mFdBlockSize, mFdBlockCount);
> +

(10) The indentation is not idiomatic.

>    if (!QemuFlashDetected ()) {
>      ASSERT (!FeaturePcdGet (PcdSmmSmramRequire));
>      return EFI_WRITE_PROTECTED;
> 

I think this patch is good, just a few warts left to clean up.

Thanks!
Laszlo

Re: [PATCH v2 2/2] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Clear C-bit when SEV is active

[Brijesh Singh]

On 02/28/2018 01:41 PM, Laszlo Ersek wrote:
> On 02/28/18 17:14, Brijesh Singh wrote:
>> Commit:24e4ad7 (OvmfPkg: Add AmdSevDxe driver) added a driver which runs
>> early in DXE phase and clears the C-bit from all MMIO regions (including
>> Qemu Flash).
> 
> (1) This appears incorrect / inexact; AmdSevDxe is dispatched from
> APRIORI DXE before the flash driver is dispatched, and the MMIO GCD
> entry is only added by the flash driver. So in this case, AmdSevDxe
> clears the C-bit on a NonExistent entry that will later be split and
> accommodate the flash MMIO range.
> 

Okay, I will update it.


>> When SMM is enabled, we build two sets of page tables; first
>> page table is used when executing code in non SMM mode (SMM-less-pgtable)
>> and second page table is used when we are executing code in SMM mode
>> (SMM-pgtable).
>>
>> During boot time, AmdSevDxe driver clears the C-bit from the
>> SMM-less-pgtable. But when SMM is enabled, Qemu Flash services are used
>> from SMM mode.
>>
>> In this patch we explicitly clear the C-bit from Qemu flash MMIO range
>> before we probe the flash. When OVMF is built with SMM_REQUIRE then
>> call to initialize the flash services happen after the SMM-pgtable is
>> created and processor is serving the first SMI. At this time we will
>> have access to the SMM-pgtable.
> 
> (2) Please replace "is serving" with "has served".
> 

Will do

>>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
>> ---
>>   OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf  |  1 +
>>   OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.h         |  7 +++++
>>   OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c | 12 +++++++
>>   OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c | 33 ++++++++++++++++++++
>>   OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c         |  6 ++++
>>   5 files changed, 59 insertions(+)
>>
>> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf
>> index ba2d3679a46d..d365e27cbe59 100644
>> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf
>> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf
>> @@ -53,6 +53,7 @@ [LibraryClasses]
>>     DevicePathLib
>>     DxeServicesTableLib
>>     MemoryAllocationLib
>> +  MemEncryptSevLib
>>     PcdLib
>>     SmmServicesTableLib
>>     UefiBootServicesTableLib
>> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.h b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.h
>> index 8d83dca7a52c..6c4099c140e8 100644
>> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.h
>> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.h
>> @@ -88,5 +88,12 @@ QemuFlashConvertPointers (
>>     VOID
>>     );
>>   
>> +VOID
>> +BeforeFlashProbe (
>> +  EFI_PHYSICAL_ADDRESS    BaseAddress,
>> +  UINTN                   FdBlockSize,
>> +  UINTN                   FdBlockCount
>> +  );
>> +
>>   #endif
> 
> (3) Sorry that I'm again requesting a name change for this function. Can
> we call it QemuFlashBeforeProbe()? To be consistent with the other
> function names in this header file.
> 
> (4) Please add "IN" decorators (also to the function definitions).
> 

Will do

>>   
>> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c
>> index 63b308658e36..a4614de3c901 100644
>> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c
>> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c
>> @@ -155,3 +155,15 @@ InstallVirtualAddressChangeHandler (
>>                     );
>>     ASSERT_EFI_ERROR (Status);
>>   }
>> +
>> +VOID
>> +BeforeFlashProbe (
>> +  EFI_PHYSICAL_ADDRESS    BaseAddress,
>> +  UINTN                   FdBlockSize,
>> +  UINTN                   FdBlockCount
>> +  )
>> +{
>> +  //
>> +  // Do nothing
>> +  //
>> +}
> 
> (5) This function definition should go into the existent file
> "QemuFlashDxe.c".


I will look into it.


> 
>> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c
>> index e0617f2503a2..a6cad5af223b 100644
>> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c
>> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceSmm.c
>> @@ -17,6 +17,7 @@
>>   #include <Library/DebugLib.h>
>>   #include <Library/PcdLib.h>
>>   #include <Library/SmmServicesTableLib.h>
>> +#include <Library/MemEncryptSevLib.h>
>>   #include <Protocol/DevicePath.h>
>>   #include <Protocol/SmmFirmwareVolumeBlock.h>
>>   
>> @@ -67,3 +68,35 @@ InstallVirtualAddressChangeHandler (
>>     // Nothing.
>>     //
>>   }
>> +
>> +VOID
>> +BeforeFlashProbe (
>> +  EFI_PHYSICAL_ADDRESS    BaseAddress,
>> +  UINTN                   FdBlockSize,
>> +  UINTN                   FdBlockCount
>> +  )
>> +{
>> +  EFI_STATUS              Status;
>> +
>> +  ASSERT (FeaturePcdGet (PcdSmmSmramRequire));
>> +
>> +  if (!MemEncryptSevIsEnabled()) {
>> +    return;
>> +  }
>> +
>> +  //
>> +  // When SEV is enabled, AmdSevDxe runs early in DXE phase and clears the C-bit
>> +  // from the MMIO space (including flash ranges) but the driver runs in non SMM
> 
> (6) Please update the comment according to (1).
> 

Will do


>> +  // context hence it cleared the flash ranges from non SMM page table.
>> +  // When SMM is enabled, the flash services are accessed from the SMM mode
>> +  // hence we explicitly clear the C-bit on flash ranges from SMM page table.
>> +  //
>> +
>> +  Status = MemEncryptSevClearPageEncMask (
>> +             0,
>> +             BaseAddress,
>> +             EFI_SIZE_TO_PAGES (FdBlockSize * FdBlockCount),
>> +             FALSE
>> +            );
> 
> (7) The closing paren is not indented correctly, it should be aligned
> with the arguments.
> 

Will fix it

>> +  ASSERT_EFI_ERROR (Status);
>> +}
> 
> (8) This function definition should go into a new file called
> "QemuFlashSmm.c" -- please make sure you add a license block at the top,
> and use CRLF line endings --, and the new file should be added to
> "FvbServicesSmm.inf".
> 

Will do


>> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c
>> index 5677b5ee119c..f63e11723415 100644
>> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c
>> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c
>> @@ -244,6 +244,12 @@ QemuFlashInitialize (
>>     ASSERT(PcdGet32 (PcdOvmfFirmwareFdSize) % mFdBlockSize == 0);
>>     mFdBlockCount = PcdGet32 (PcdOvmfFirmwareFdSize) / mFdBlockSize;
>>   
>> +  //
>> +  // execute platform specific hooks before probing the flash
>> +  //
> 
> (9) Please replace "platform" with "module type".
> 

Will do


>> +  BeforeFlashProbe ((EFI_PHYSICAL_ADDRESS)(UINTN) mFlashBase,
>> +                    mFdBlockSize, mFdBlockCount);
>> +
> 
> (10) The indentation is not idiomatic.
> 
>>     if (!QemuFlashDetected ()) {
>>       ASSERT (!FeaturePcdGet (PcdSmmSmramRequire));
>>       return EFI_WRITE_PROTECTED;
>>
>