Re: [edk2-devel] [PATCH] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Use physical address with SEV-ES

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

On 1/22/21 4:29 PM, Laszlo Ersek wrote:
> On 01/22/21 18:55, Lendacky, Thomas wrote:
>> From: Tom Lendacky <thomas.lendacky@amd.com>
>>
>> BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3183&amp;data=04%7C01%7Cthomas.lendacky%40amd.com%7Ced6105527a884016335708d8bf2533d5%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637469513810518842%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=%2F%2B2OTbAeYDgp8lHsdx7%2B%2FhsCItm5x4yRFiQbxrAoTw8%3D&amp;reserved=0
>>
>> Under SEV-ES, a write to the flash device is done using a direct VMGEXIT
>> to perform an MMIO write. The address provided to the MMIO write must be
>> the physical address of the MMIO write destitnation. During boot, OVMF
>> runs with an identity mapped pagetable structure so that VA == PA and the
>> VMGEXIT MMIO write destination is just the virtual address of the flash
>> area address being written.
>>
>> However, when the UEFI SetVitualAddressMap() API is invoked, an identity
>> mapped pagetable structure may not be in place and using the virtual
>> address for the flash area address is no longer valid. This results in
>> writes to the flash not being performed successfully. This can be seen
>> by attempting to change the boot order under Linux. The update will
>> appear to be performed, based on the output of the command. But rebooting
>> the guest will show that the new boot order has not been set.
>>
>> To remedy this, update the Qemu flash services constructor to maintain a
>> copy of the original PA of the flash device. When performing the VMGEXIT
>> MMIO write, the PA can be calculated by subtracting the difference between
>> the current flash virtual address base and the original physical address
>> base. Using the resulting value in the MMIO write produces a successful
>> write during boot and during runtime services.
>>
>> Fixes: 437eb3f7a8db7681afe0e6064d3a8edb12abb766
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>> ---
>>   OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.h    |  1 +
>>   OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c    |  2 ++
>>   OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c | 10 +++++++++-
>>   3 files changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.h b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.h
>> index 219d0d6e83cf..0a91c15d0e81 100644
>> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.h
>> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.h
>> @@ -13,6 +13,7 @@
>>   #include <Protocol/FirmwareVolumeBlock.h>
>>   
>>   extern UINT8 *mFlashBase;
>> +extern UINT8 *mFlashBasePhysical;
>>   
>>   /**
>>     Read from QEMU Flash
>> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c
>> index d19997032ec9..36ae63ebde31 100644
>> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c
>> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c
>> @@ -26,6 +26,7 @@
>>   
>>   
>>   UINT8 *mFlashBase;
>> +UINT8 *mFlashBasePhysical;
>>   
>>   STATIC UINTN       mFdBlockSize = 0;
>>   STATIC UINTN       mFdBlockCount = 0;
>> @@ -251,6 +252,7 @@ QemuFlashInitialize (
>>     )
>>   {
>>     mFlashBase = (UINT8*)(UINTN) PcdGet32 (PcdOvmfFdBaseAddress);
>> +  mFlashBasePhysical = mFlashBase;
>>     mFdBlockSize = PcdGet32 (PcdOvmfFirmwareBlockSize);
>>     ASSERT(PcdGet32 (PcdOvmfFirmwareFdSize) % mFdBlockSize == 0);
>>     mFdBlockCount = PcdGet32 (PcdOvmfFirmwareFdSize) / mFdBlockSize;
>> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c
>> index 1b0742967f71..db247f324e3c 100644
>> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c
>> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c
>> @@ -52,11 +52,19 @@ QemuFlashPtrWrite (
>>     if (MemEncryptSevEsIsEnabled ()) {
>>       MSR_SEV_ES_GHCB_REGISTER  Msr;
>>       GHCB                      *Ghcb;
>> +    UINT64                    PtrPa;
>>       BOOLEAN                   InterruptState;
>>   
>>       Msr.GhcbPhysicalAddress = AsmReadMsr64 (MSR_SEV_ES_GHCB);
>>       Ghcb = Msr.Ghcb;
>>   
>> +    //
>> +    // The MMIO write needs to be to the physical address of the flash pointer.
>> +    // Since this service is available as part of the EFI runtime services,
>> +    // account for a non-identity mapped VA after SetVitualAddressMap().
>> +    //
>> +    PtrPa = (UINT64) (UINTN) (Ptr - (mFlashBase - mFlashBasePhysical));
>> +
> 
> (1) The formula seen here is indeed correct, IMO, for expressing the PA,
> *mathematically speaking*. To explain it to myself, I used a different
> formulation:
> 
> - "(Ptr - mFlashBase)" is the relative offset into the flash, where both
> "Ptr" and "mFlashBase" are virtual addresses -- the subtraction
> basically undoes a part of QemuFlashPtr(),
> 
> - and then that relative offset is added to mFlashBasePhysical:
> 
>    mFlashBasePhysical + (Ptr - mFlashBase)
> 
> Mathematically, this is exactly the sum that's in your code too, but I
> think this formulation is easier to understand. Upon reading the commit
> message, I didn't understand the goal of the (virtual - physical) shift,
> until I saw the formula.
> 
> Would you agree to reword the commit message accordingly (the last
> paragraph)?

Certainly, I can do that.

> 
> 
> (2) Although mathematically the PtrPa calculation is OK, the C
> expression itself is not so nice. It contains a subtraction between
> pointers that do not (necessarily) point into the same array (or one
> past the last element in the same array). Such a subtraction is
> undefined behavior.
> 
> But, I'll address this below, as a part of point (3).
> 
> 
> (3) It should be possible to implement this change within
> "QemuFlashDxe.c"; not having any effect on the SMM build of the driver.
> 
> (3a) I suggest introducing the following variable to "QemuFlashDxe.c":
> 
>    STATIC EFI_PHYSICAL_ADDRESS mSevEsFlashPhysBase;
> 
> (3b) The following should be prepended to the body of
> QemuFlashConvertPointers():
> 
>    if (MemEncryptSevEsIsEnabled ()) {
>      mSevEsFlashPhysBase = (UINTN)mFlashBase;
>    }

Can SetVirtualAddressMap() be called more than once? I can always make that:

   if (MemEncryptSevEsIsEnabled () && mSevEsFlashPhysBase == 0) {

> 
> (QemuFlashConvertPointers() is called from
> FvbVirtualAddressChangeEvent(), in "FwBlockServiceDxe.c", which is the
> event notification function for SetVitualAddressMap().)
> 
> (3c) I propose the following for the SEV-ES branch of
> QemuFlashPtrWrite(), in "QemuFlashDxe.c":
> 
>    EFI_PHYSICAL_ADDRESS PhysAddr;
> 
>    if (mSevEsFlashPhysBase == 0) {
>      PhysAddr = (UINTN)Ptr;
>    } else {
>      PhysAddr = mSevEsFlashPhysBase + (Ptr - mFlashBase);
>    }
> 
> (3d) EFI_PHYSICAL_ADDRESS is a typedef to UINT64, per spec, thus you can
> pass PhysAddr to the SVM_EXIT_MMIO_WRITE VmgExit() call, without a type
> cast.
> 
> I feel that this approach keeps the SEV-ES logic better contained.

Yup, I'll update it.

> 
> (4) Please link the patch emails (v1, v2 mailing list URLs) into the
> bugzilla ticket.

Ah, yes.  Will do.

Thanks,
Tom

> 
> Thanks!
> Laszlo
> 
>>       //
>>       // 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
>> @@ -68,7 +76,7 @@ QemuFlashPtrWrite (
>>       Ghcb->SharedBuffer[0] = Value;
>>       Ghcb->SaveArea.SwScratch = (UINT64) (UINTN) Ghcb->SharedBuffer;
>>       VmgSetOffsetValid (Ghcb, GhcbSwScratch);
>> -    VmgExit (Ghcb, SVM_EXIT_MMIO_WRITE, (UINT64) (UINTN) Ptr, 1);
>> +    VmgExit (Ghcb, SVM_EXIT_MMIO_WRITE, PtrPa, 1);
>>       VmgDone (Ghcb, InterruptState);
>>     } else {
>>       *Ptr = Value;
>>
>