From: "Laszlo Ersek" <lersek@redhat.com>
To: devel@edk2.groups.io, thomas.lendacky@amd.com
Cc: Brijesh Singh <brijesh.singh@amd.com>,
Jordan Justen <jordan.l.justen@intel.com>,
Ard Biesheuvel <ardb+tianocore@kernel.org>
Subject: Re: [edk2-devel] [PATCH] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Use physical address with SEV-ES
Date: Fri, 22 Jan 2021 23:29:29 +0100 [thread overview]
Message-ID: <8d660790-fafd-c0d5-f1eb-1e02830a7fa9@redhat.com> (raw)
In-Reply-To: <8bba19920e58813f32889dec522bbcd8de113219.1611338106.git.thomas.lendacky@amd.com>
On 01/22/21 18:55, Lendacky, Thomas wrote:
> From: Tom Lendacky <thomas.lendacky@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@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)?
(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;
>
next prev parent reply other threads:[~2021-01-22 22:29 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-01-22 17:55 [PATCH] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Use physical address with SEV-ES Lendacky, Thomas
2021-01-22 22:29 ` Laszlo Ersek [this message]
2021-01-22 22:40 ` [edk2-devel] " Lendacky, Thomas
2021-01-23 0:25 ` Laszlo Ersek
2021-01-23 1:29 ` Andrew Fish
2021-01-23 2:24 ` Laszlo Ersek
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=8d660790-fafd-c0d5-f1eb-1e02830a7fa9@redhat.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox