[edk2-devel] [PATCH] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Use physical address with SEV-ES
Lendacky, Thomas
thomas.lendacky at amd.com
Fri Jan 22 22:40:36 UTC 2021
On 1/22/21 4:29 PM, Laszlo Ersek wrote:
> On 01/22/21 18:55, Lendacky, Thomas wrote:
>> From: Tom Lendacky <thomas.lendacky at amd.com>
>>
>> BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3183&data=04%7C01%7Cthomas.lendacky%40amd.com%7Ced6105527a884016335708d8bf2533d5%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637469513810518842%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=%2F%2B2OTbAeYDgp8lHsdx7%2B%2FhsCItm5x4yRFiQbxrAoTw8%3D&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 at intel.com>
>> Cc: Laszlo Ersek <lersek at redhat.com>
>> Cc: Ard Biesheuvel <ardb+tianocore at kernel.org>
>> Signed-off-by: Tom Lendacky <thomas.lendacky at 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;
>>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#70697): https://edk2.groups.io/g/devel/message/70697
Mute This Topic: https://groups.io/mt/80036772/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list