* [PATCH] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Use physical address with SEV-ES @ 2021-01-22 17:55 Lendacky, Thomas 2021-01-22 22:29 ` [edk2-devel] " Laszlo Ersek 0 siblings, 1 reply; 6+ messages in thread From: Lendacky, Thomas @ 2021-01-22 17:55 UTC (permalink / raw) To: devel; +Cc: Brijesh Singh, Jordan Justen, Laszlo Ersek, Ard Biesheuvel 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)); + // // 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; -- 2.30.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [edk2-devel] [PATCH] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Use physical address with SEV-ES 2021-01-22 17:55 [PATCH] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Use physical address with SEV-ES Lendacky, Thomas @ 2021-01-22 22:29 ` Laszlo Ersek 2021-01-22 22:40 ` Lendacky, Thomas 0 siblings, 1 reply; 6+ messages in thread From: Laszlo Ersek @ 2021-01-22 22:29 UTC (permalink / raw) To: devel, thomas.lendacky; +Cc: Brijesh Singh, Jordan Justen, Ard Biesheuvel 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; > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [edk2-devel] [PATCH] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Use physical address with SEV-ES 2021-01-22 22:29 ` [edk2-devel] " Laszlo Ersek @ 2021-01-22 22:40 ` Lendacky, Thomas 2021-01-23 0:25 ` Laszlo Ersek 0 siblings, 1 reply; 6+ messages in thread From: Lendacky, Thomas @ 2021-01-22 22:40 UTC (permalink / raw) To: Laszlo Ersek, devel; +Cc: Brijesh Singh, Jordan Justen, Ard Biesheuvel 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&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@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; >> > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [edk2-devel] [PATCH] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Use physical address with SEV-ES 2021-01-22 22:40 ` Lendacky, Thomas @ 2021-01-23 0:25 ` Laszlo Ersek 2021-01-23 1:29 ` Andrew Fish 0 siblings, 1 reply; 6+ messages in thread From: Laszlo Ersek @ 2021-01-23 0:25 UTC (permalink / raw) To: Tom Lendacky, devel; +Cc: Brijesh Singh, Jordan Justen, Ard Biesheuvel On 01/22/21 23:40, Tom Lendacky wrote: > Can SetVirtualAddressMap() be called more than once? According to the UEFI spec: no, it can't. The call to SetVirtualAddressMap() must be done with the physical mappings. On successful return from this function, the system must then make any future calls with the newly assigned virtual mappings. [...] The SetVirtualAddressMap() and ConvertPointer() services are only callable in physical mode, so they do not need to be converted from physical pointers to virtual pointers. [...] A virtual address map may only be applied one time. Once the runtime system is in virtual mode, calls to this function return EFI_UNSUPPORTED. (I seem to detect a bit of contradiction between quotes #1+#2 and #3 -- the first two quotes seem to explain that a second call is expected to be impossible, whereas the third quote explains how a second or later call should behave. But, anyway, the intent is clear.) Thanks, Laszlo ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [edk2-devel] [PATCH] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Use physical address with SEV-ES 2021-01-23 0:25 ` Laszlo Ersek @ 2021-01-23 1:29 ` Andrew Fish 2021-01-23 2:24 ` Laszlo Ersek 0 siblings, 1 reply; 6+ messages in thread From: Andrew Fish @ 2021-01-23 1:29 UTC (permalink / raw) To: edk2-devel-groups-io, Laszlo Ersek Cc: Tom Lendacky, Brijesh Singh, Jordan Justen, Ard Biesheuvel [-- Attachment #1: Type: text/plain, Size: 2905 bytes --] > On Jan 22, 2021, at 4:25 PM, Laszlo Ersek <lersek@redhat.com> wrote: > > On 01/22/21 23:40, Tom Lendacky wrote: > >> Can SetVirtualAddressMap() be called more than once? > > According to the UEFI spec: no, it can't. > > The call to SetVirtualAddressMap() must be done with the physical > mappings. On successful return from this function, the system must > then make any future calls with the newly assigned virtual mappings. > > [...] > > The SetVirtualAddressMap() and ConvertPointer() services are only > callable in physical mode, so they do not need to be converted from > physical pointers to virtual pointers. > > [...] > > A virtual address map may only be applied one time. Once the runtime > system is in virtual mode, calls to this function return > EFI_UNSUPPORTED. > > (I seem to detect a bit of contradiction between quotes #1+#2 and #3 -- > the first two quotes seem to explain that a second call is expected to > be impossible, whereas the third quote explains how a second or later > call should behave. But, anyway, the intent is clear.) > Laszlo, So the answer is no you can’t call it more than once. While the text is confusing it describes the only possible behavior. If you look at the EFI_UNSUPPORTED Status Codes Returned: "EFI firmware is not at runtime, or the EFI firmware is already in virtual address mapped mode.” This is what the edk2 implementation does [1]. And combine that with this text "Once all events have been notified, the EFI firmware reapplies image “fix-up” information to virtually relocate all runtime images to their new addresses. “. So basically that implies that after the1st call all the Runtime drivers have been fixed up to run at their virtual address. So if you called them again at their physical mode address they would likely crash. That crash could be implementation dependent I guess based on when the EFI virtual mappings are produced. It is perfectly legal to only produce them in kernel after ExitBootServices. Remember all the notification functions are registered via their physical address. [1] https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/RuntimeDxe/Runtime.c#L245 it enforces the call only once. // // Can only switch to virtual addresses once the memory map is locked down, // and can only set it once // if (!mRuntime.AtRuntime || mRuntime.VirtualMode) { return EFI_UNSUPPORTED; } // // Only understand the original descriptor format // if (DescriptorVersion != EFI_MEMORY_DESCRIPTOR_VERSION || DescriptorSize < sizeof (EFI_MEMORY_DESCRIPTOR)) { return EFI_INVALID_PARAMETER; } // // We are now committed to go to virtual mode, so lets get to it! // mRuntime.VirtualMode = TRUE; Thanks, Andrew Fish > Thanks, > Laszlo > > > > > > [-- Attachment #2: Type: text/html, Size: 23234 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [edk2-devel] [PATCH] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Use physical address with SEV-ES 2021-01-23 1:29 ` Andrew Fish @ 2021-01-23 2:24 ` Laszlo Ersek 0 siblings, 0 replies; 6+ messages in thread From: Laszlo Ersek @ 2021-01-23 2:24 UTC (permalink / raw) To: Andrew Fish, edk2-devel-groups-io Cc: Tom Lendacky, Brijesh Singh, Jordan Justen, Ard Biesheuvel On 01/23/21 02:29, Andrew Fish wrote: > >> On Jan 22, 2021, at 4:25 PM, Laszlo Ersek <lersek@redhat.com> wrote: >> >> On 01/22/21 23:40, Tom Lendacky wrote: >> >>> Can SetVirtualAddressMap() be called more than once? >> >> According to the UEFI spec: no, it can't. >> >> The call to SetVirtualAddressMap() must be done with the physical >> mappings. On successful return from this function, the system must >> then make any future calls with the newly assigned virtual >> mappings. >> >> [...] >> >> The SetVirtualAddressMap() and ConvertPointer() services are only >> callable in physical mode, so they do not need to be converted >> from physical pointers to virtual pointers. >> >> [...] >> >> A virtual address map may only be applied one time. Once the >> runtime system is in virtual mode, calls to this function return >> EFI_UNSUPPORTED. >> >> (I seem to detect a bit of contradiction between quotes #1+#2 and #3 >> -- the first two quotes seem to explain that a second call is >> expected to be impossible, whereas the third quote explains how a >> second or later call should behave. But, anyway, the intent is >> clear.) >> > > Laszlo, > > So the answer is no you can't call it more than once. While the text > is confusing it describes the only possible behavior. > > If you look at the EFI_UNSUPPORTED Status Codes Returned: "EFI > firmware is not at runtime, or the EFI firmware is already in virtual > address mapped mode." This is what the edk2 implementation does [1]. > And combine that with this text 'Once all events have been notified, > the EFI firmware reapplies image "fix-up" information to virtually > relocate all runtime images to their new addresses'. So basically > that implies that after the 1st call all the Runtime drivers have been > fixed up to run at their virtual address. So if you called them again > at their physical mode address they would likely crash. Yes, exactly; the question is whether it is even possible to make a second call to SetVirtualAddressMap(), after the first call returns successfully, without crashing immediately in the call instruction. Put differently: whether there is any way where SetVirtualAddressMap() could remain *accessible* for a 2nd call, after the 1st call succeeds. If there is no such way, then the last quoted paragraph is useless, because it specifies a situation that can never happen. Put yet differently: - SetVirtualAddressMap can only be called in physical mode, - when SetVirtualAddressMap returns with success, we're in virtual mode, - so SetVirtualAddressMap must not be called *at all* for a 2nd time, - so why give details on *how* SetVirtualAddressMap behaves when its calling contract is violated? Ultimately, I would replace: Once the runtime system is in virtual mode, calls to this function return EFI_UNSUPPORTED. with: Once the runtime system is in virtual mode, calls to this function are impossible to make. Thanks! Laszlo ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-01-23 2:24 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-01-22 17:55 [PATCH] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Use physical address with SEV-ES Lendacky, Thomas 2021-01-22 22:29 ` [edk2-devel] " Laszlo Ersek 2021-01-22 22:40 ` Lendacky, Thomas 2021-01-23 0:25 ` Laszlo Ersek 2021-01-23 1:29 ` Andrew Fish 2021-01-23 2:24 ` Laszlo Ersek
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox