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

[PATCH v2] 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, save the value of the flash base physical address before
converting the address as part of SetVirtualAddressMap(). The physical
address can then be calculated by obtaining the offset of the MMIO target
virtual address relative to the flash base virtual address and adding that
to the original flash base physical address. The resulting value produces
a successful MMIO write 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>
---
 .../QemuFlashDxe.c                            | 20 ++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c
index 1b0742967f71..d303b0078b08 100644
--- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c
+++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c
@@ -16,11 +16,17 @@
 
 #include "QemuFlash.h"
 
+STATIC EFI_PHYSICAL_ADDRESS mSevEsFlashPhysBase;
+
 VOID
 QemuFlashConvertPointers (
   VOID
   )
 {
+  if (MemEncryptSevEsIsEnabled ()) {
+    mSevEsFlashPhysBase = (UINTN) mFlashBase;
+  }
+
   EfiConvertPointer (0x0, (VOID **) &mFlashBase);
 }
 
@@ -52,11 +58,23 @@ QemuFlashPtrWrite (
   if (MemEncryptSevEsIsEnabled ()) {
     MSR_SEV_ES_GHCB_REGISTER  Msr;
     GHCB                      *Ghcb;
+    EFI_PHYSICAL_ADDRESS      PhysAddr;
     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().
+    //
+    if (mSevEsFlashPhysBase == 0) {
+      PhysAddr = (UINTN) Ptr;
+    } else {
+      PhysAddr = mSevEsFlashPhysBase + (Ptr - mFlashBase);
+    }
+
     //
     // 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 +86,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, PhysAddr, 1);
     VmgDone (Ghcb, InterruptState);
   } else {
     *Ptr = Value;
-- 
2.30.0

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

[Lendacky, Thomas]

On 1/23/21 7:57 AM, Tom Lendacky 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, save the value of the flash base physical address before
> converting the address as part of SetVirtualAddressMap(). The physical
> address can then be calculated by obtaining the offset of the MMIO target
> virtual address relative to the flash base virtual address and adding that
> to the original flash base physical address. The resulting value produces
> a successful MMIO write 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> > ---
>   .../QemuFlashDxe.c                            | 20 ++++++++++++++++++-
>   1 file changed, 19 insertions(+), 1 deletion(-)

Forgot to add the changelog and use the diffstat options.

The changes address the comments from Laszlo on the v1 version of the patch:

- Update the last paragraph of the commit message
- Limit changes to only the QemuFlashDxe.c file
   - Update expression for calculating the physical address

Thanks,
Tom

> 
> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c
> index 1b0742967f71..d303b0078b08 100644
> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c
> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c
> @@ -16,11 +16,17 @@
>   
>   #include "QemuFlash.h"
>   
> +STATIC EFI_PHYSICAL_ADDRESS mSevEsFlashPhysBase;
> +
>   VOID
>   QemuFlashConvertPointers (
>     VOID
>     )
>   {
> +  if (MemEncryptSevEsIsEnabled ()) {
> +    mSevEsFlashPhysBase = (UINTN) mFlashBase;
> +  }
> +
>     EfiConvertPointer (0x0, (VOID **) &mFlashBase);
>   }
>   
> @@ -52,11 +58,23 @@ QemuFlashPtrWrite (
>     if (MemEncryptSevEsIsEnabled ()) {
>       MSR_SEV_ES_GHCB_REGISTER  Msr;
>       GHCB                      *Ghcb;
> +    EFI_PHYSICAL_ADDRESS      PhysAddr;
>       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().
> +    //
> +    if (mSevEsFlashPhysBase == 0) {
> +      PhysAddr = (UINTN) Ptr;
> +    } else {
> +      PhysAddr = mSevEsFlashPhysBase + (Ptr - mFlashBase);
> +    }
> +
>       //
>       // 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 +86,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, PhysAddr, 1);
>       VmgDone (Ghcb, InterruptState);
>     } else {
>       *Ptr = Value;
> 

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

[Lendacky, Thomas]

On 1/23/21 7:57 AM, Tom Lendacky 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

s/SetVitualAddressMap/SetVirtualAddressMap/

I can fix that if another version is required, otherwise, can it be fixed
on commit?

Thanks,
Tom

> 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, save the value of the flash base physical address before
> converting the address as part of SetVirtualAddressMap(). The physical
> address can then be calculated by obtaining the offset of the MMIO target
> virtual address relative to the flash base virtual address and adding that
> to the original flash base physical address. The resulting value produces
> a successful MMIO write 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>
> ---
>  .../QemuFlashDxe.c                            | 20 ++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c
> index 1b0742967f71..d303b0078b08 100644
> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c
> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c
> @@ -16,11 +16,17 @@
>  
>  #include "QemuFlash.h"
>  
> +STATIC EFI_PHYSICAL_ADDRESS mSevEsFlashPhysBase;
> +
>  VOID
>  QemuFlashConvertPointers (
>    VOID
>    )
>  {
> +  if (MemEncryptSevEsIsEnabled ()) {
> +    mSevEsFlashPhysBase = (UINTN) mFlashBase;
> +  }
> +
>    EfiConvertPointer (0x0, (VOID **) &mFlashBase);
>  }
>  
> @@ -52,11 +58,23 @@ QemuFlashPtrWrite (
>    if (MemEncryptSevEsIsEnabled ()) {
>      MSR_SEV_ES_GHCB_REGISTER  Msr;
>      GHCB                      *Ghcb;
> +    EFI_PHYSICAL_ADDRESS      PhysAddr;
>      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().
> +    //
> +    if (mSevEsFlashPhysBase == 0) {
> +      PhysAddr = (UINTN) Ptr;
> +    } else {
> +      PhysAddr = mSevEsFlashPhysBase + (Ptr - mFlashBase);
> +    }
> +
>      //
>      // 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 +86,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, PhysAddr, 1);
>      VmgDone (Ghcb, InterruptState);
>    } else {
>      *Ptr = Value;
> 

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

[Laszlo Ersek]

On 01/25/21 14:55, Tom Lendacky wrote:
> On 1/23/21 7:57 AM, Tom Lendacky 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
> 
> s/SetVitualAddressMap/SetVirtualAddressMap/
> 
> I can fix that if another version is required, otherwise, can it be fixed
> on commit?

Yes, I'll fix it on merge. I made the exact same typo in my v1 comment
-- and that was because I copied the mistyped string from the BZ :) (See
c#0, c#1.)

Thanks!
Laszlo

> 
> Thanks,
> Tom
> 
>> 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, save the value of the flash base physical address before
>> converting the address as part of SetVirtualAddressMap(). The physical
>> address can then be calculated by obtaining the offset of the MMIO target
>> virtual address relative to the flash base virtual address and adding that
>> to the original flash base physical address. The resulting value produces
>> a successful MMIO write 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>
>> ---
>>  .../QemuFlashDxe.c                            | 20 ++++++++++++++++++-
>>  1 file changed, 19 insertions(+), 1 deletion(-)
>>
>> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c
>> index 1b0742967f71..d303b0078b08 100644
>> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c
>> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c
>> @@ -16,11 +16,17 @@
>>  
>>  #include "QemuFlash.h"
>>  
>> +STATIC EFI_PHYSICAL_ADDRESS mSevEsFlashPhysBase;
>> +
>>  VOID
>>  QemuFlashConvertPointers (
>>    VOID
>>    )
>>  {
>> +  if (MemEncryptSevEsIsEnabled ()) {
>> +    mSevEsFlashPhysBase = (UINTN) mFlashBase;
>> +  }
>> +
>>    EfiConvertPointer (0x0, (VOID **) &mFlashBase);
>>  }
>>  
>> @@ -52,11 +58,23 @@ QemuFlashPtrWrite (
>>    if (MemEncryptSevEsIsEnabled ()) {
>>      MSR_SEV_ES_GHCB_REGISTER  Msr;
>>      GHCB                      *Ghcb;
>> +    EFI_PHYSICAL_ADDRESS      PhysAddr;
>>      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().
>> +    //
>> +    if (mSevEsFlashPhysBase == 0) {
>> +      PhysAddr = (UINTN) Ptr;
>> +    } else {
>> +      PhysAddr = mSevEsFlashPhysBase + (Ptr - mFlashBase);
>> +    }
>> +
>>      //
>>      // 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 +86,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, PhysAddr, 1);
>>      VmgDone (Ghcb, InterruptState);
>>    } else {
>>      *Ptr = Value;
>>
> 

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

[Lendacky, Thomas]

On 1/25/21 4:17 PM, Laszlo Ersek wrote:
> On 01/25/21 14:55, Tom Lendacky wrote:
>> On 1/23/21 7:57 AM, Tom Lendacky 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%7Ce7ba4f968a0d4bc42ea108d8c17f0a9b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637472098694077769%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=jGAzh8XsefGmSnQfjjz%2BQmLJoUu25o67pOxuUORzw7g%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
>>
>> s/SetVitualAddressMap/SetVirtualAddressMap/
>>
>> I can fix that if another version is required, otherwise, can it be fixed
>> on commit?
> 
> Yes, I'll fix it on merge. I made the exact same typo in my v1 comment
> -- and that was because I copied the mistyped string from the BZ :) (See
> c#0, c#1.)

Well, looks like I was only able to get it right once...  I just noticed
down in the code comment it is wrong, too. Ugh, my fingers just couldn't
type that 'r' I guess.

Thanks,
Tom

> 
> Thanks!
> Laszlo
> 
>>
>> Thanks,
>> Tom
>>
>>> 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, save the value of the flash base physical address before
>>> converting the address as part of SetVirtualAddressMap(). The physical
>>> address can then be calculated by obtaining the offset of the MMIO target
>>> virtual address relative to the flash base virtual address and adding that
>>> to the original flash base physical address. The resulting value produces
>>> a successful MMIO write 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>
>>> ---
>>>  .../QemuFlashDxe.c                            | 20 ++++++++++++++++++-
>>>  1 file changed, 19 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c
>>> index 1b0742967f71..d303b0078b08 100644
>>> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c
>>> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c
>>> @@ -16,11 +16,17 @@
>>>  
>>>  #include "QemuFlash.h"
>>>  
>>> +STATIC EFI_PHYSICAL_ADDRESS mSevEsFlashPhysBase;
>>> +
>>>  VOID
>>>  QemuFlashConvertPointers (
>>>    VOID
>>>    )
>>>  {
>>> +  if (MemEncryptSevEsIsEnabled ()) {
>>> +    mSevEsFlashPhysBase = (UINTN) mFlashBase;
>>> +  }
>>> +
>>>    EfiConvertPointer (0x0, (VOID **) &mFlashBase);
>>>  }
>>>  
>>> @@ -52,11 +58,23 @@ QemuFlashPtrWrite (
>>>    if (MemEncryptSevEsIsEnabled ()) {
>>>      MSR_SEV_ES_GHCB_REGISTER  Msr;
>>>      GHCB                      *Ghcb;
>>> +    EFI_PHYSICAL_ADDRESS      PhysAddr;
>>>      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().
>>> +    //
>>> +    if (mSevEsFlashPhysBase == 0) {
>>> +      PhysAddr = (UINTN) Ptr;
>>> +    } else {
>>> +      PhysAddr = mSevEsFlashPhysBase + (Ptr - mFlashBase);
>>> +    }
>>> +
>>>      //
>>>      // 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 +86,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, PhysAddr, 1);
>>>      VmgDone (Ghcb, InterruptState);
>>>    } else {
>>>      *Ptr = Value;
>>>
>>
> 

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

[Laszlo Ersek]

On 01/23/21 14:57, 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, save the value of the flash base physical address before
> converting the address as part of SetVirtualAddressMap(). The physical
> address can then be calculated by obtaining the offset of the MMIO target
> virtual address relative to the flash base virtual address and adding that
> to the original flash base physical address. The resulting value produces
> a successful MMIO write 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>
> ---
>  .../QemuFlashDxe.c                            | 20 ++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c
> index 1b0742967f71..d303b0078b08 100644
> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c
> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c
> @@ -16,11 +16,17 @@
>  
>  #include "QemuFlash.h"
>  
> +STATIC EFI_PHYSICAL_ADDRESS mSevEsFlashPhysBase;
> +
>  VOID
>  QemuFlashConvertPointers (
>    VOID
>    )
>  {
> +  if (MemEncryptSevEsIsEnabled ()) {
> +    mSevEsFlashPhysBase = (UINTN) mFlashBase;
> +  }
> +
>    EfiConvertPointer (0x0, (VOID **) &mFlashBase);
>  }
>  
> @@ -52,11 +58,23 @@ QemuFlashPtrWrite (
>    if (MemEncryptSevEsIsEnabled ()) {
>      MSR_SEV_ES_GHCB_REGISTER  Msr;
>      GHCB                      *Ghcb;
> +    EFI_PHYSICAL_ADDRESS      PhysAddr;
>      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().
> +    //
> +    if (mSevEsFlashPhysBase == 0) {
> +      PhysAddr = (UINTN) Ptr;
> +    } else {
> +      PhysAddr = mSevEsFlashPhysBase + (Ptr - mFlashBase);
> +    }
> +
>      //
>      // 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 +86,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, PhysAddr, 1);
>      VmgDone (Ghcb, InterruptState);
>    } else {
>      *Ptr = Value;
> 

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

I've fixed up the typo as discussed elsewhere in this thread, in both
the commit message and the code comment.

Merged as commit 3a3501862f73, via
<https://github.com/tianocore/edk2/pull/1389>.

Thanks
Laszlo