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

[Laszlo Ersek <lersek@redhat.com>]

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