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

["Lendacky, Thomas" <thomas.lendacky@amd.com>]

On 10/15/20 4:45 AM, Laszlo Ersek wrote:
> 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?

Yes, I believe that will work nicely. I can add a second parameter to 
VmgInit() that takes a pointer to a BOOLEAN to return the interrupt state 
and then pass that to VmgDone(). To keep builds from breaking, I'll need 
to do the UefiCpuPkg and OvmfPkg changes in the same patch.

Thanks,
Tom

> 
> 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;
>>     }
>>
>