[edk2-devel] [PATCH] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Use physical address with SEV-ES
Laszlo Ersek
lersek at redhat.com
Fri Jan 22 22:29:29 UTC 2021
On 01/22/21 18:55, Lendacky, Thomas wrote:
> From: Tom Lendacky <thomas.lendacky at amd.com>
>
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3183
>
> 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)?
(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;
}
(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.
(4) Please link the patch emails (v1, v2 mailing list URLs) into the
bugzilla ticket.
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 (#70696): https://edk2.groups.io/g/devel/message/70696
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