Re: [PATCH v3 3/3] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Restore C-bit when SEV is active

[Laszlo Ersek <lersek@redhat.com>]

On 07/05/18 21:12, Brijesh Singh wrote:
> AmdSevDxe maps the flash memory range with C=0, but
> SetMemorySpaceAttributes() unconditionally resets the C-bit to '1'. Lets
> restore the mapping back to C=0.
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Anthony Perard <anthony.perard@citrix.com>
> Cc: Julien Grall <julien.grall@linaro.org>
> 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>
> ---
>  .../FvbServicesRuntimeDxe.inf                           |  1 +
>  .../QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c  | 17 +++++++++++++++++
>  2 files changed, 18 insertions(+)
> 
> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
> index d7b4ec06c4e6..6bb5c2093790 100644
> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
> @@ -54,6 +54,7 @@ [LibraryClasses]
>    DevicePathLib
>    DxeServicesTableLib
>    MemoryAllocationLib
> +  MemEncryptSevLib
>    PcdLib
>    UefiBootServicesTableLib
>    UefiDriverEntryPoint

(1) Please keep this list sorted.

(I realize I must have missed this dis-order when we added the
MemEncryptSevLib class to the "FvbServicesSmm.inf" file. If you want,
you can include a patch in v4 for fixing that order, but it's really not
necessary. Let's just not increase the dis-order if we can manage.)

> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c
> index 646427bf4e2c..3add4bbad74c 100644
> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c
> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServiceDxe.c
> @@ -24,6 +24,7 @@
>  #include <Protocol/FirmwareVolumeBlock.h>
>  #include <Library/DxeServicesTableLib.h>
>  #include <Library/MemoryAllocationLib.h>
> +#include <Library/MemEncryptSevLib.h>

(2) Here too, please keep the Library #include list sorted.

>  
>  #include "FwBlockService.h"
>  #include "QemuFlash.h"
> @@ -204,5 +205,21 @@ MarkIoMemoryRangeForRuntimeAccess (
>                    );
>    ASSERT_EFI_ERROR (Status);
>  
> +  //
> +  // When SEV is active, AmdSevDxe should have mapped the BaseAddress with
> +  // C=0 but SetMemorySpaceAttribute() remap the range with C=1. Lets restore
> +  // the mapping so that both guest and hyervisor can access the flash
> +  // memory range.
> +  //

(3) A couple of cleanups, please:

(3a) I suggest s/should have mapped/mapped/, because AmdSevDxe is really
not to blame here,

(3b) s/SetMemorySpaceAttribute/SetMemorySpaceAttributes/ (plural)

(3c) s/remap/remaps/

(3d) s/Lets/Let's/

(3e) Please check that, after the typo fixes, we are still under 80
chars per line.

> +  if (MemEncryptSevIsEnabled()) {

(4) missing space before the "()"

> +    Status = MemEncryptSevClearPageEncMask (
> +               0,
> +               BaseAddress,
> +               EFI_SIZE_TO_PAGES (Length),
> +               FALSE
> +               );

I'm glad that you had documented the "Flush" parameter earlier! :)
"Flush the caches before clearing the bit (mostly TRUE except MMIO
addresses)". So, this looks fine.

> +    ASSERT_EFI_ERROR (Status);
> +  }
> +
>    return Status;
>  }
> 

I've requested a few changes, but none of those affect functionality in
practice. So, I will go ahead and collect some test results. I'll report
back.

Thanks!
Laszlo