From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [63.128.21.124]) by mx.groups.io with SMTP id smtpd.web12.17242.1611354577838462872 for ; Fri, 22 Jan 2021 14:29:38 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=U90GhoAk; spf=pass (domain: redhat.com, ip: 63.128.21.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1611354577; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=h9OqZlVvnbzuhpeIyotugK3AY6rmnZZ2UgSn2y/Nbss=; b=U90GhoAkBbHYJ6aLqGs/0w80CSiSUUIdN5dbjU9Zu/I1Z0IykZAHx1TXOhJUrVMeR92wfp bX/YIZ0O0f2OaJjDkKpGVBj7Gr6emaSUEzlB9WQh7NCjHqK27YoHJJL7EE+iSq+Sprm8IR LX6yMVBXBApLEPAriM+8uudNpIQqVcc= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-54-vbuRmHV4OHihsBlWAaFjEg-1; Fri, 22 Jan 2021 17:29:32 -0500 X-MC-Unique: vbuRmHV4OHihsBlWAaFjEg-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 8BA7A180A095; Fri, 22 Jan 2021 22:29:31 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-113-81.ams2.redhat.com [10.36.113.81]) by smtp.corp.redhat.com (Postfix) with ESMTP id 095E79CA0; Fri, 22 Jan 2021 22:29:29 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Use physical address with SEV-ES To: devel@edk2.groups.io, thomas.lendacky@amd.com Cc: Brijesh Singh , Jordan Justen , Ard Biesheuvel References: <8bba19920e58813f32889dec522bbcd8de113219.1611338106.git.thomas.lendacky@amd.com> From: "Laszlo Ersek" Message-ID: <8d660790-fafd-c0d5-f1eb-1e02830a7fa9@redhat.com> Date: Fri, 22 Jan 2021 23:29:29 +0100 MIME-Version: 1.0 In-Reply-To: <8bba19920e58813f32889dec522bbcd8de113219.1611338106.git.thomas.lendacky@amd.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 01/22/21 18:55, Lendacky, Thomas wrote: > From: Tom Lendacky > > 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 > Cc: Laszlo Ersek > Cc: Ard Biesheuvel > Signed-off-by: Tom Lendacky > --- > 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 > > 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; >