Re: [PATCH 8/9] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Disable interrupts when using GHCB

["Laszlo Ersek" <lersek@redhat.com>]

On 10/10/20 18:07, Tom Lendacky wrote:
> From: Tom Lendacky <thomas.lendacky@amd.com>
> 
> The QemuFlashPtrWrite() flash services runtime uses the GHCB and VmgExit()
> directly to perform the flash write when running as an SEV-ES guest. If an
> interrupt arrives

(1) please clarify what kind of interrupt you've seen in practice (my
guess: timer interrupt)

> between VmgInit() and VmgDone(),

(2) VmgDone() is currently an empty function (both library instances) --
did you mean VmgExit()?


> the Dr7 read in the
> interrupt handler

(3) please clarify what interrupt handler you have in mind (function
name and file with full path would be helpful)

> will generate a #VC, which can overwrite information in
> the GHCB that QemuFlashPtrWrite() has set. Prevent this from occurring by
> disabling interrupts around the usage of the GHCB.

(4) I like the last sentence, because it seems to support my suspicion
that the problem is generic. Should we push the interrupt disablement /
re-enablement logic into VmgInit() and VmgDone()?

For that, the pre-VmgInit() interrupt state would have to be saved (for
restoration) somewhere.

(5) I wonder if raising the TPL to TPL_HIGH_LEVEL, rather than messing
with interrupts explicitly, works too. (Search the UEFI spec for
"TPL_HIGH_LEVEL".) Managing the TPL feels cleaner.

... Either way, VmgInit() would have to output either the "old TPL", or
the "old interrupt state", for VmgDone() to restore.

... Hm wait, VmgInit() is called from ApWakeupFunction()
[UefiCpuPkg/Library/MpInitLib/MpLib.c], which is built for PEI too --
there is no "TPL" concept in PEI.

So I guess we should indeed manipulate the interrupts briefly, but I
believe we should still have that logic occur every time we are setting
up a VmgExit(). And so VmgInit() / VmgDone() look like the perfect
bracket for that. What's your take?

Thanks!
Laszlo

> 
> Fixes: 437eb3f7a8db ("OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Bypass flash detection with SEV-ES")
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> ---
>  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c
> index 5d5a117c48e0..872e58db7cc0 100644
> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c
> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c
> @@ -9,6 +9,7 @@
>  
>  **/
>  
> +#include <Library/BaseLib.h>
>  #include <Library/UefiRuntimeLib.h>
>  #include <Library/MemEncryptSevLib.h>
>  #include <Library/VmgExitLib.h>
> @@ -54,6 +55,7 @@ QemuFlashPtrWrite (
>      GHCB                      *Ghcb;
>      UINT32                    ScratchIndex;
>      UINT32                    ScratchBit;
> +    BOOLEAN                   InterruptsEnabled;
>  
>      Msr.GhcbPhysicalAddress = AsmReadMsr64 (MSR_SEV_ES_GHCB);
>      Ghcb = Msr.Ghcb;
> @@ -61,6 +63,15 @@ QemuFlashPtrWrite (
>      ScratchIndex = GhcbSwScratch / 8;
>      ScratchBit   = GhcbSwScratch & 0x07;
>  
> +    //
> +    // Be sure that an interrupt can't cause a #VC while the GHCB is
> +    // being used.
> +    //
> +    InterruptsEnabled = GetInterruptState ();
> +    if (InterruptsEnabled) {
> +      DisableInterrupts ();
> +    }
> +
>      //
>      // 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
> @@ -74,6 +85,10 @@ QemuFlashPtrWrite (
>      Ghcb->SaveArea.ValidBitmap[ScratchIndex] |= (1 << ScratchBit);
>      VmgExit (Ghcb, SVM_EXIT_MMIO_WRITE, (UINT64) (UINTN) Ptr, 1);
>      VmgDone (Ghcb);
> +
> +    if (InterruptsEnabled) {
> +      EnableInterrupts ();
> +    }
>    } else {
>      *Ptr = Value;
>    }
>