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 <[email protected]>
> Cc: Anthony Perard <[email protected]>
> Cc: Julien Grall <[email protected]>
> Cc: Justen Jordan L <[email protected]>
> Cc: Laszlo Ersek <[email protected]>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Brijesh Singh <[email protected]>
> ---
> .../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
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
