1. Patchew
  2. EDK2
  3. [edk2] [PATCH v2 0/2] Add SMM support when SEV is active
  4. View patch

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

Brijesh Singh posted 2 patches 6 years, 9 months ago
[edk2] [PATCH v2 2/2] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Clear C-bit when SEV is active
Posted by Brijesh Singh 6 years, 9 months ago 
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

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2 2/2] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Clear C-bit when SEV is active
Posted by Laszlo Ersek 6 years, 9 months ago 
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2 2/2] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Clear C-bit when SEV is active
Posted by Brijesh Singh 6 years, 9 months ago 

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

Patchew 2.1-devel-8c64711

© 2016 - 2024 Red Hat, Inc.