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

[Brijesh Singh <brijesh.singh@amd.com>]

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