[edk2-devel] [PATCH V2 0/2] OvmfPkg: Update TdVmCall to handle the retry for MapGPA

[edk2-devel] [PATCH V2 0/2] OvmfPkg: Update TdVmCall to handle the retry for MapGPA

[sunceping]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4572

According to section 3.2 of the [GHCI] spec, if the result is 
"TDG.VP.VMCALL_RETRY" for TDG.VP.VMCALL.MapGPA, TD must retry the
mapping for the pages in the region starting at the GPA specified in r11.

Currently, TDVF does not properly handle the retry results of MapGPA.
For this, TDVF should update the TdVmCall to return the value in R11
and must retry the mapping for the pages by the value.

How to verify the retry for MapGPA in TDVF:
Note: Since the range size of MapGPA in QEMU is limited to 64MB and
TDVF always maps 1.5GB( 2GB~3.5GB) MMIO to shared-memory for TD guest, 
the retry action is triggered always.
Pre-Config:
QEMU:
https://github.com/intel/qemu-tdx/tree/tdx-qemu-upstream | tag: tdx-qemu-upstream-2023.10.20-v8.1.0
KERNEL:
https://github.com/intel/tdx/tree/kvm-upstream-2023.10.16-v6.6-rc2

Step:
Boot with TD guest and check the log with TdVmcall(MAPGPA), as below:
TdxDxe:SetMemorySharedOrPrivate: Cr3Base=0x0 Physical=0x80000000 Length=0x60000000 Mode=Shared
SetOrClearSharedBit: TdVmcall(MAPGPA) Retry PhysicalAddress is 8000080000000, MapGpaRetryaddr is 8000084000000

Reference:
[GHCI]: TDX Guest-Host-Communication Interface v1.0
https://cdrdv2.intel.com/v1/dl/getContent/726790

v2 changes:
  - Update the code based on the comments of v1 reviewer
  - Update TdVmcall to instead of the extra API file

code: https://github.com/sunceping/edk2/tree/handleRetryMapGPA.v2

Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Michael Roth <michael.roth@amd.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Ceping Sun <cepingx.sun@intel.com>

Ceping Sun (2):
  MdePkg/BaseLib: Update TdVmcall to always output the value in R11
  OvmfPkg/BaseMemEncryptTdxLib: Handle retry result of MapGPA

 MdePkg/Library/BaseLib/X64/TdVmcall.nasm      |  4 +-
 .../BaseMemEncryptTdxLib/MemoryEncryption.c   | 43 ++++++++++++++++++-
 2 files changed, 43 insertions(+), 4 deletions(-)

-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110505): https://edk2.groups.io/g/devel/message/110505
Mute This Topic: https://groups.io/mt/102337973/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH V2 1/2] MdePkg/BaseLib: Update TdVmcall to always output the value in R11

[sunceping]

From: Ceping Sun <cepingx.sun@intel.com>

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4572

According to section 3.2 of the [GHCI] spec, if the return status
of MapGPA is "TDG.VP.VMCALL_RETRY", TD must retry this operation
for the pages in the region starting at the GPA specified in R11.

Currently, TDVF has not handled the retry results and always clears
the R11 on unsuccessful return status. For this, the TdVmcall needs
to output the value of R11 on unsuccessful return status to handle
the retry results of MapGPA.

Reference:
[GHCI]: TDX Guest-Host-Communication Interface v1.0
https://cdrdv2.intel.com/v1/dl/getContent/726790

Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Michael Roth <michael.roth@amd.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Ceping Sun <cepingx.sun@intel.com>
---
 MdePkg/Library/BaseLib/X64/TdVmcall.nasm | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/MdePkg/Library/BaseLib/X64/TdVmcall.nasm b/MdePkg/Library/BaseLib/X64/TdVmcall.nasm
index 5ecc10b17193..8dd9bfcbfa14 100644
--- a/MdePkg/Library/BaseLib/X64/TdVmcall.nasm
+++ b/MdePkg/Library/BaseLib/X64/TdVmcall.nasm
@@ -133,9 +133,7 @@ ASM_PFX(TdVmCall):
        test r9, r9
        jz .no_return_data
 
-       ; On success, propagate TDVMCALL output value to output param
-       test rax, rax
-       jnz .no_return_data
+       ; Propagate TDVMCALL output value to output param
        mov [r9], r11
 .no_return_data:
        tdcall_regs_postamble
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110506): https://edk2.groups.io/g/devel/message/110506
Mute This Topic: https://groups.io/mt/102337975/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH V2 2/2] OvmfPkg/BaseMemEncryptTdxLib: Handle retry result of MapGPA

[sunceping]

From: Ceping Sun <cepingx.sun@intel.com>

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4572

According to section 3.2 of the [GHCI] document, if the return status
of MapGPA is "TDG.VP.VMCALL_RETRY", TD must retry this operation for the
pages in the region starting at the GPA specified in R11.

In this patch, when a retry state is detected, TDVF needs to retry the
mapping with the specified address from the output results of TdVmCall.

Reference:
[GHCI]: TDX Guest-Host-Communication Interface v1.0
https://cdrdv2.intel.com/v1/dl/getContent/726790

Cc: Erdem Aktas <erdemaktas@google.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Michael Roth <michael.roth@amd.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Ceping Sun <cepingx.sun@intel.com>
---
 .../BaseMemEncryptTdxLib/MemoryEncryption.c   | 43 ++++++++++++++++++-
 1 file changed, 42 insertions(+), 1 deletion(-)

diff --git a/OvmfPkg/Library/BaseMemEncryptTdxLib/MemoryEncryption.c b/OvmfPkg/Library/BaseMemEncryptTdxLib/MemoryEncryption.c
index a01dc98852b8..b9de699a6489 100644
--- a/OvmfPkg/Library/BaseMemEncryptTdxLib/MemoryEncryption.c
+++ b/OvmfPkg/Library/BaseMemEncryptTdxLib/MemoryEncryption.c
@@ -38,6 +38,10 @@ typedef enum {
 
 STATIC PAGE_TABLE_POOL  *mPageTablePool = NULL;
 
+#define TDVMCALL_STATUS_RETRY  0x1
+
+#define MAX_RETRIES_PER_PAGE  3
+
 /**
   Returns boolean to indicate whether to indicate which, if any, memory encryption is enabled
 
@@ -527,6 +531,13 @@ SetOrClearSharedBit (
   EFI_STATUS                    Status;
   EDKII_MEMORY_ACCEPT_PROTOCOL  *MemoryAcceptProtocol;
 
+  UINT64  MapGpaRetryAddr;
+  UINT32  RetryCount;
+  UINT64  EndAddress;
+
+  MapGpaRetryAddr = 0;
+  RetryCount      = 0;
+
   AddressEncMask = GetMemEncryptionAddressMask ();
 
   //
@@ -540,7 +551,37 @@ SetOrClearSharedBit (
     PhysicalAddress   &= ~AddressEncMask;
   }
 
-  TdStatus = TdVmCall (TDVMCALL_MAPGPA, PhysicalAddress, Length, 0, 0, NULL);
+  EndAddress = PhysicalAddress + Length;
+  while (RetryCount < MAX_RETRIES_PER_PAGE) {
+    TdStatus = TdVmCall (TDVMCALL_MAPGPA, PhysicalAddress, Length, 0, 0, &MapGpaRetryAddr);
+    if (TdStatus != TDVMCALL_STATUS_RETRY) {
+      break;
+    }
+
+    DEBUG ((DEBUG_VERBOSE, "%a: TdVmcall(MAPGPA) Retry PhysicalAddress is %llx, MapGpaRetryAddr is %llx\n", __func__, PhysicalAddress, MapGpaRetryAddr));
+
+    if ((MapGpaRetryAddr < PhysicalAddress) || (MapGpaRetryAddr >= EndAddress)) {
+      DEBUG ((
+        DEBUG_ERROR,
+        "%a: TdVmcall(MAPGPA) failed with MapGpaRetryAddr(%llx) less than PhysicalAddress(%llx) or more than or equal to EndAddress(%llx) \n",
+        __func__,
+        MapGpaRetryAddr,
+        PhysicalAddress,
+        EndAddress
+        ));
+      break;
+    }
+
+    if (MapGpaRetryAddr == PhysicalAddress) {
+      RetryCount++;
+      continue;
+    }
+
+    PhysicalAddress = MapGpaRetryAddr;
+    Length          = EndAddress - PhysicalAddress;
+    RetryCount      = 0;
+  }
+
   if (TdStatus != 0) {
     DEBUG ((DEBUG_ERROR, "%a: TdVmcall(MAPGPA) failed with %llx\n", __func__, TdStatus));
     ASSERT (FALSE);
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110507): https://edk2.groups.io/g/devel/message/110507
Mute This Topic: https://groups.io/mt/102337977/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH V2 0/2] OvmfPkg: Update TdVmCall to handle the retry for MapGPA

[Gerd Hoffmann]

On Thu, Nov 02, 2023 at 05:09:55PM +0800, sunceping wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4572
> 
> According to section 3.2 of the [GHCI] spec, if the result is 
> "TDG.VP.VMCALL_RETRY" for TDG.VP.VMCALL.MapGPA, TD must retry the
> mapping for the pages in the region starting at the GPA specified in r11.
> 
> Currently, TDVF does not properly handle the retry results of MapGPA.
> For this, TDVF should update the TdVmCall to return the value in R11
> and must retry the mapping for the pages by the value.
> 
> How to verify the retry for MapGPA in TDVF:
> Note: Since the range size of MapGPA in QEMU is limited to 64MB and
> TDVF always maps 1.5GB( 2GB~3.5GB) MMIO to shared-memory for TD guest, 
> the retry action is triggered always.
> Pre-Config:
> QEMU:
> https://github.com/intel/qemu-tdx/tree/tdx-qemu-upstream | tag: tdx-qemu-upstream-2023.10.20-v8.1.0
> KERNEL:
> https://github.com/intel/tdx/tree/kvm-upstream-2023.10.16-v6.6-rc2
> 
> Step:
> Boot with TD guest and check the log with TdVmcall(MAPGPA), as below:
> TdxDxe:SetMemorySharedOrPrivate: Cr3Base=0x0 Physical=0x80000000 Length=0x60000000 Mode=Shared
> SetOrClearSharedBit: TdVmcall(MAPGPA) Retry PhysicalAddress is 8000080000000, MapGpaRetryaddr is 8000084000000
> 
> Reference:
> [GHCI]: TDX Guest-Host-Communication Interface v1.0
> https://cdrdv2.intel.com/v1/dl/getContent/726790
> 
> v2 changes:
>   - Update the code based on the comments of v1 reviewer
>   - Update TdVmcall to instead of the extra API file

Series:
Acked-by: Gerd Hoffmann <kraxel@redhat.com>



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110825): https://edk2.groups.io/g/devel/message/110825
Mute This Topic: https://groups.io/mt/102337973/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH V2 1/2] MdePkg/BaseLib: Update TdVmcall to always output the value in R11

[Yao, Jiewen]

Reviewed-by: Jiewen Yao <Jiewen.yao@intel.com>

> -----Original Message-----
> From: Sun, CepingX <cepingx.sun@intel.com>
> Sent: Thursday, November 2, 2023 5:10 PM
> To: devel@edk2.groups.io
> Cc: Sun, CepingX <cepingx.sun@intel.com>; Gao, Liming
> <gaoliming@byosoft.com.cn>; Kinney, Michael D <michael.d.kinney@intel.com>;
> Aktas, Erdem <erdemaktas@google.com>; James Bottomley
> <jejb@linux.ibm.com>; Yao, Jiewen <jiewen.yao@intel.com>; Xu, Min M
> <min.m.xu@intel.com>; Tom Lendacky <thomas.lendacky@amd.com>; Michael
> Roth <michael.roth@amd.com>; Gerd Hoffmann <kraxel@redhat.com>
> Subject: [PATCH V2 1/2] MdePkg/BaseLib: Update TdVmcall to always output the
> value in R11
> 
> From: Ceping Sun <cepingx.sun@intel.com>
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4572
> 
> According to section 3.2 of the [GHCI] spec, if the return status
> of MapGPA is "TDG.VP.VMCALL_RETRY", TD must retry this operation
> for the pages in the region starting at the GPA specified in R11.
> 
> Currently, TDVF has not handled the retry results and always clears
> the R11 on unsuccessful return status. For this, the TdVmcall needs
> to output the value of R11 on unsuccessful return status to handle
> the retry results of MapGPA.
> 
> Reference:
> [GHCI]: TDX Guest-Host-Communication Interface v1.0
> https://cdrdv2.intel.com/v1/dl/getContent/726790
> 
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Erdem Aktas <erdemaktas@google.com>
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Min Xu <min.m.xu@intel.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Michael Roth <michael.roth@amd.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Signed-off-by: Ceping Sun <cepingx.sun@intel.com>
> ---
>  MdePkg/Library/BaseLib/X64/TdVmcall.nasm | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/MdePkg/Library/BaseLib/X64/TdVmcall.nasm
> b/MdePkg/Library/BaseLib/X64/TdVmcall.nasm
> index 5ecc10b17193..8dd9bfcbfa14 100644
> --- a/MdePkg/Library/BaseLib/X64/TdVmcall.nasm
> +++ b/MdePkg/Library/BaseLib/X64/TdVmcall.nasm
> @@ -133,9 +133,7 @@ ASM_PFX(TdVmCall):
>         test r9, r9
>         jz .no_return_data
> 
> -       ; On success, propagate TDVMCALL output value to output param
> -       test rax, rax
> -       jnz .no_return_data
> +       ; Propagate TDVMCALL output value to output param
>         mov [r9], r11
>  .no_return_data:
>         tdcall_regs_postamble
> --
> 2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110894): https://edk2.groups.io/g/devel/message/110894
Mute This Topic: https://groups.io/mt/102337975/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH V2 2/2] OvmfPkg/BaseMemEncryptTdxLib: Handle retry result of MapGPA

[Yao, Jiewen]

I think the macro definition (#define TDVMCALL_STATUS_RETRY  0x1) should be in https://github.com/tianocore/edk2/blob/master/MdePkg/Include/IndustryStandard/Tdx.h, together with other TDX definition.

Thank you
Yao, Jiewen

> -----Original Message-----
> From: Sun, CepingX <cepingx.sun@intel.com>
> Sent: Thursday, November 2, 2023 5:10 PM
> To: devel@edk2.groups.io
> Cc: Sun, CepingX <cepingx.sun@intel.com>; Aktas, Erdem
> <erdemaktas@google.com>; James Bottomley <jejb@linux.ibm.com>; Yao,
> Jiewen <jiewen.yao@intel.com>; Xu, Min M <min.m.xu@intel.com>; Tom
> Lendacky <thomas.lendacky@amd.com>; Michael Roth
> <michael.roth@amd.com>; Gerd Hoffmann <kraxel@redhat.com>
> Subject: [PATCH V2 2/2] OvmfPkg/BaseMemEncryptTdxLib: Handle retry result of
> MapGPA
> 
> From: Ceping Sun <cepingx.sun@intel.com>
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4572
> 
> According to section 3.2 of the [GHCI] document, if the return status
> of MapGPA is "TDG.VP.VMCALL_RETRY", TD must retry this operation for the
> pages in the region starting at the GPA specified in R11.
> 
> In this patch, when a retry state is detected, TDVF needs to retry the
> mapping with the specified address from the output results of TdVmCall.
> 
> Reference:
> [GHCI]: TDX Guest-Host-Communication Interface v1.0
> https://cdrdv2.intel.com/v1/dl/getContent/726790
> 
> Cc: Erdem Aktas <erdemaktas@google.com>
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Min Xu <min.m.xu@intel.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Michael Roth <michael.roth@amd.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Signed-off-by: Ceping Sun <cepingx.sun@intel.com>
> ---
>  .../BaseMemEncryptTdxLib/MemoryEncryption.c   | 43 ++++++++++++++++++-
>  1 file changed, 42 insertions(+), 1 deletion(-)
> 
> diff --git a/OvmfPkg/Library/BaseMemEncryptTdxLib/MemoryEncryption.c
> b/OvmfPkg/Library/BaseMemEncryptTdxLib/MemoryEncryption.c
> index a01dc98852b8..b9de699a6489 100644
> --- a/OvmfPkg/Library/BaseMemEncryptTdxLib/MemoryEncryption.c
> +++ b/OvmfPkg/Library/BaseMemEncryptTdxLib/MemoryEncryption.c
> @@ -38,6 +38,10 @@ typedef enum {
> 
>  STATIC PAGE_TABLE_POOL  *mPageTablePool = NULL;
> 
> +#define TDVMCALL_STATUS_RETRY  0x1
> +
> +#define MAX_RETRIES_PER_PAGE  3
> +
>  /**
>    Returns boolean to indicate whether to indicate which, if any, memory
> encryption is enabled
> 
> @@ -527,6 +531,13 @@ SetOrClearSharedBit (
>    EFI_STATUS                    Status;
>    EDKII_MEMORY_ACCEPT_PROTOCOL  *MemoryAcceptProtocol;
> 
> +  UINT64  MapGpaRetryAddr;
> +  UINT32  RetryCount;
> +  UINT64  EndAddress;
> +
> +  MapGpaRetryAddr = 0;
> +  RetryCount      = 0;
> +
>    AddressEncMask = GetMemEncryptionAddressMask ();
> 
>    //
> @@ -540,7 +551,37 @@ SetOrClearSharedBit (
>      PhysicalAddress   &= ~AddressEncMask;
>    }
> 
> -  TdStatus = TdVmCall (TDVMCALL_MAPGPA, PhysicalAddress, Length, 0, 0,
> NULL);
> +  EndAddress = PhysicalAddress + Length;
> +  while (RetryCount < MAX_RETRIES_PER_PAGE) {
> +    TdStatus = TdVmCall (TDVMCALL_MAPGPA, PhysicalAddress, Length, 0, 0,
> &MapGpaRetryAddr);
> +    if (TdStatus != TDVMCALL_STATUS_RETRY) {
> +      break;
> +    }
> +
> +    DEBUG ((DEBUG_VERBOSE, "%a: TdVmcall(MAPGPA) Retry PhysicalAddress
> is %llx, MapGpaRetryAddr is %llx\n", __func__, PhysicalAddress,
> MapGpaRetryAddr));
> +
> +    if ((MapGpaRetryAddr < PhysicalAddress) || (MapGpaRetryAddr >=
> EndAddress)) {
> +      DEBUG ((
> +        DEBUG_ERROR,
> +        "%a: TdVmcall(MAPGPA) failed with MapGpaRetryAddr(%llx) less than
> PhysicalAddress(%llx) or more than or equal to EndAddress(%llx) \n",
> +        __func__,
> +        MapGpaRetryAddr,
> +        PhysicalAddress,
> +        EndAddress
> +        ));
> +      break;
> +    }
> +
> +    if (MapGpaRetryAddr == PhysicalAddress) {
> +      RetryCount++;
> +      continue;
> +    }
> +
> +    PhysicalAddress = MapGpaRetryAddr;
> +    Length          = EndAddress - PhysicalAddress;
> +    RetryCount      = 0;
> +  }
> +
>    if (TdStatus != 0) {
>      DEBUG ((DEBUG_ERROR, "%a: TdVmcall(MAPGPA) failed with %llx\n",
> __func__, TdStatus));
>      ASSERT (FALSE);
> --
> 2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110895): https://edk2.groups.io/g/devel/message/110895
Mute This Topic: https://groups.io/mt/102337977/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-