On 05/19/20 23:51, Lendacky, Thomas wrote:
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2198
>
> The flash detection routine will attempt to determine how the flash
> device behaves (e.g. ROM, RAM, Flash). But when SEV-ES is enabled and
> the flash device behaves as a ROM device (meaning it is marked read-only
> by the hypervisor), this check may result in an infinite nested page fault
> because of the attempted write. Since the instruction cannot be emulated
> when SEV-ES is enabled, the RIP is never advanced, resulting in repeated
> nested page faults.
>
> When SEV-ES is enabled, exit the flash detection early and assume that
> the FD behaves as Flash. This will result in QemuFlashWrite() being called
> to store EFI variables, which will also result in an infinite nested page
> fault when the write is performed. In this case, update QemuFlashWrite()
> to use the VMGEXIT MMIO write support to have the hypervisor perform the
> write without having to emulate the instruction.
>
> Cc: Jordan Justen <jordan.l.jus...@intel.com>
> Cc: Laszlo Ersek <ler...@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheu...@linaro.org>
> Reviewed-by: Laszlo Ersek <ler...@redhat.com>
> Signed-off-by: Tom Lendacky <thomas.lenda...@amd.com>
> ---
> .../FvbServicesRuntimeDxe.inf | 2 +
> .../QemuFlash.h | 13 ++++++
> .../QemuFlash.c | 23 +++++++++--
> .../QemuFlashDxe.c | 40 +++++++++++++++++++
> .../QemuFlashSmm.c | 16 ++++++++
> 5 files changed, 91 insertions(+), 3 deletions(-)
- subject line has been cleaned up relative to v6, OK
- commit message updated in sync with open-coding VmgMmioWrite() in this
patch; and VmgMmioWrite() has been documented in the v8 blurb -- OK
>
> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
> b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
> index 72cabba4357d..8bb2325157ea 100644
> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
> @@ -38,6 +38,7 @@ [Sources]
> [Packages]
> MdePkg/MdePkg.dec
> MdeModulePkg/MdeModulePkg.dec
> + UefiCpuPkg/UefiCpuPkg.dec
> OvmfPkg/OvmfPkg.dec
>
> [LibraryClasses]
> @@ -52,6 +53,7 @@ [LibraryClasses]
> UefiBootServicesTableLib
> UefiDriverEntryPoint
> UefiRuntimeLib
> + VmgExitLib
>
> [Guids]
> gEfiEventVirtualAddressChangeGuid # ALWAYS_CONSUMED
> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.h
> b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.h
> index f1afabcbe6ae..219d0d6e83cf 100644
> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.h
> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.h
> @@ -89,5 +89,18 @@ QemuFlashBeforeProbe (
> IN UINTN FdBlockCount
> );
>
> +/**
> + Write to QEMU Flash
> +
> + @param[in] Ptr Pointer to the location to write.
> + @param[in] Value The value to write.
> +
> +**/
> +VOID
> +QemuFlashPtrWrite (
> + IN volatile UINT8 *Ptr,
> + IN UINT8 Value
> + );
> +
> #endif
>
- New comment, OK.
> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c
> b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c
> index 1b0d6c053f1a..0d29bf701aca 100644
> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c
> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c
> @@ -9,6 +9,7 @@
>
> #include <Library/BaseMemoryLib.h>
> #include <Library/DebugLib.h>
> +#include <Library/MemEncryptSevLib.h>
> #include <Library/PcdLib.h>
>
> #include "QemuFlash.h"
> @@ -80,6 +81,21 @@ QemuFlashDetected (
>
> DEBUG ((DEBUG_INFO, "QEMU Flash: Attempting flash detection at %p\n",
> Ptr));
>
> + if (MemEncryptSevEsIsEnabled ()) {
> + //
> + // When SEV-ES is enabled, the check below can result in an infinite
> + // loop with respect to a nested page fault. When the memslot is mapped
> + // read-only, the nested page table entry is read-only. The check below
> + // will cause a nested page fault that cannot be emulated, causing
> + // the instruction to retried over and over. For SEV-ES, acknowledge that
> + // the FD appears as ROM and not as FLASH, but report FLASH anyway
> because
> + // FLASH behavior can be simulated using VMGEXIT.
> + //
> + DEBUG ((DEBUG_INFO,
> + "QEMU Flash: SEV-ES enabled, assuming FD behaves as FLASH\n"));
> + return TRUE;
> + }
> +
> OriginalUint8 = *Ptr;
> *Ptr = CLEAR_STATUS_CMD;
> ProbeUint8 = *Ptr;
> @@ -181,8 +197,9 @@ QemuFlashWrite (
> //
> Ptr = QemuFlashPtr (Lba, Offset);
> for (Loop = 0; Loop < *NumBytes; Loop++) {
> - *Ptr = WRITE_BYTE_CMD;
> - *Ptr = Buffer[Loop];
> + QemuFlashPtrWrite (Ptr, WRITE_BYTE_CMD);
> + QemuFlashPtrWrite (Ptr, Buffer[Loop]);
> +
> Ptr++;
> }
>
> @@ -190,7 +207,7 @@ QemuFlashWrite (
> // Restore flash to read mode
> //
> if (*NumBytes > 0) {
> - *(Ptr - 1) = READ_ARRAY_CMD;
> + QemuFlashPtrWrite (Ptr - 1, READ_ARRAY_CMD);
> }
>
> return EFI_SUCCESS;
> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c
> b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c
> index 5aabe9d7b59c..565383ee26d2 100644
> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c
> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c
> @@ -10,6 +10,9 @@
> **/
>
> #include <Library/UefiRuntimeLib.h>
> +#include <Library/MemEncryptSevLib.h>
> +#include <Library/VmgExitLib.h>
> +#include <Register/Amd/Msr.h>
- new include directive, most likely due to flattening VmgMmioWrite; OK
>
> #include "QemuFlash.h"
>
> @@ -32,3 +35,40 @@ QemuFlashBeforeProbe (
> // Do nothing
> //
> }
> +
> +/**
> + Write to QEMU Flash
> +
> + @param[in] Ptr Pointer to the location to write.
> + @param[in] Value The value to write.
> +
> +**/
- new comment, OK
> +VOID
> +QemuFlashPtrWrite (
> + IN volatile UINT8 *Ptr,
> + IN UINT8 Value
> + )
> +{
> + if (MemEncryptSevEsIsEnabled ()) {
> + MSR_SEV_ES_GHCB_REGISTER Msr;
> + GHCB *Ghcb;
> +
> + Msr.GhcbPhysicalAddress = AsmReadMsr64 (MSR_SEV_ES_GHCB);
> + Ghcb = Msr.Ghcb;
> +
> + //
> + // Writing to flash is emulated by the hypervisor through the use of
> write
> + // protection. This won't work for an SEV-ES guest because the write
> won't
> + // be recognized as a true MMIO write, which would result in the required
> + // #VC exception. Instead, use the the VMGEXIT MMIO write support
> directly
> + // to perform the update.
> + //
> + VmgInit (Ghcb);
> + Ghcb->SharedBuffer[0] = Value;
> + Ghcb->SaveArea.SwScratch = (UINT64) (UINTN) Ghcb->SharedBuffer;
> + VmgExit (Ghcb, SVM_EXIT_MMIO_WRITE, (UINT64) (UINTN) Ptr, 1);
> + VmgDone (Ghcb);
- this is what replaces VmgMmioWrite(); seems reasonable
> + } else {
> + *Ptr = Value;
> + }
> +}
> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashSmm.c
> b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashSmm.c
> index 7eb426e03855..7eb80bfeffae 100644
> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashSmm.c
> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashSmm.c
> @@ -46,3 +46,19 @@ QemuFlashBeforeProbe (
> );
> ASSERT_EFI_ERROR (Status);
> }
> +
> +/**
> + Write to QEMU Flash
> +
> + @param[in] Ptr Pointer to the location to write.
> + @param[in] Value The value to write.
> +
> +**/
- new comment.
> +VOID
> +QemuFlashPtrWrite (
> + IN volatile UINT8 *Ptr,
> + IN UINT8 Value
> + )
> +{
> + *Ptr = Value;
> +}
>
Reviewed-by: Laszlo Ersek <ler...@redhat.com>
Thanks
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#60276): https://edk2.groups.io/g/devel/message/60276
Mute This Topic: https://groups.io/mt/74336601/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
