[PATCH] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Use physical address with SEV-ES

[PATCH] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Use physical address with SEV-ES

[Lendacky, Thomas]

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

Re: [edk2-devel] [PATCH] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Use physical address with SEV-ES

[Laszlo Ersek]

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;
> 

Re: [edk2-devel] [PATCH] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Use physical address with SEV-ES

[Lendacky, Thomas]

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&amp;data=04%7C01%7Cthomas.lendacky%40amd.com%7Ced6105527a884016335708d8bf2533d5%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637469513810518842%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=%2F%2B2OTbAeYDgp8lHsdx7%2B%2FhsCItm5x4yRFiQbxrAoTw8%3D&amp;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;
>>
> 

Re: [edk2-devel] [PATCH] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Use physical address with SEV-ES

[Laszlo Ersek]

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

Re: [edk2-devel] [PATCH] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Use physical address with SEV-ES

[Andrew Fish]

[-- 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 --]

Re: [edk2-devel] [PATCH] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Use physical address with SEV-ES

[Laszlo Ersek]

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