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

[Erdem Aktas via groups.io]

Hi,

This should be the [PATCH V1 2/2] I assume?

On Thu, Oct 26, 2023 at 5:58 PM sunceping <cepingx.sun at intel.com> wrote:

> From: Ceping Sun <cepingx.sun at intel.com>
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4572
>
> According to section 3.2 of the [GHCI] document, if the result of MapGPA
> is "TDG.VP.VMCALL_RETRY", TDVF must retry mapping for pages in that region,
> starting with the GPA specified in R11.
>
> Reference:
> [GHCI]: TDX Guest-Host-Communication Interface v1.0
> https://cdrdv2.intel.com/v1/dl/getContent/726790
>
> Cc: Erdem Aktas <erdemaktas at google.com>
> Cc: James Bottomley <jejb at linux.ibm.com>
> Cc: Jiewen Yao <jiewen.yao at intel.com>
> Cc: Min Xu <min.m.xu at intel.com>
> Cc: Tom Lendacky <thomas.lendacky at amd.com>
> Cc: Michael Roth <michael.roth at amd.com>
> Cc: Gerd Hoffmann <kraxel at redhat.com>
> Signed-off-by: Ceping Sun <cepingx.sun at intel.com>
> ---
>  .../BaseMemEncryptTdxLib.inf                  |  1 +
>  .../BaseMemEncryptTdxLib/MemoryEncryption.c   | 36 ++++++++++++++++++-
>  2 files changed, 36 insertions(+), 1 deletion(-)
>
> diff --git a/OvmfPkg/Library/BaseMemEncryptTdxLib/BaseMemEncryptTdxLib.inf
> b/OvmfPkg/Library/BaseMemEncryptTdxLib/BaseMemEncryptTdxLib.inf
> index 11768825f8ca..742b65a289ce 100644
> --- a/OvmfPkg/Library/BaseMemEncryptTdxLib/BaseMemEncryptTdxLib.inf
> +++ b/OvmfPkg/Library/BaseMemEncryptTdxLib/BaseMemEncryptTdxLib.inf
> @@ -30,6 +30,7 @@
>  [Sources]
>    VirtualMemory.h
>    MemoryEncryption.c
> +  X64/TdVmCallMapGPA.nasm
>
I do not think we need another TdVmCallMapGPA definition, do we?

>
>  [LibraryClasses]
>    BaseLib
> diff --git a/OvmfPkg/Library/BaseMemEncryptTdxLib/MemoryEncryption.c
> b/OvmfPkg/Library/BaseMemEncryptTdxLib/MemoryEncryption.c
> index b47f56b391a5..1f29f9194c30 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
> +
>  /**
>    This function is used to help request the host VMM to map a GPA range as
>    private or shared-memory mappings.
> @@ -546,6 +550,13 @@ SetOrClearSharedBit (
>    EFI_STATUS                    Status;
>    EDKII_MEMORY_ACCEPT_PROTOCOL  *MemoryAcceptProtocol;
>
> +  UINT64  MapGpaRetryaddr;
>
Should be replaced with MapGpaRetryAddr for consistency in variable
name casing style ?

> +  UINT32  RetryCount;
> +  UINT64  EndAddress;
> +
> +  MapGpaRetryaddr = 0;
> +  RetryCount      = 0;
> +
>    AddressEncMask = GetMemEncryptionAddressMask ();
>
>    //
> @@ -559,7 +570,30 @@ SetOrClearSharedBit (
>      PhysicalAddress   &= ~AddressEncMask;
>    }
>
> -  TdStatus = TdVmCall (TDVMCALL_MAPGPA, PhysicalAddress, Length, 0, 0,
> NULL);
> +  while (RetryCount < MAX_RETRIES_PER_PAGE) {
> +    TdStatus = TdVmCallMapGPA (PhysicalAddress, Length, &MapGpaRetryaddr);
>
Why not this?
 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));
> +
> +    EndAddress = PhysicalAddress + Length;
> +    if ((MapGpaRetryaddr < PhysicalAddress) || (MapGpaRetryaddr >
> EndAddress)) {
>
 should be?
 if ((MapGpaRetryaddr < PhysicalAddress) || (MapGpaRetryaddr >=
EndAddress))

> +      DEBUG ((DEBUG_ERROR, "%a: TdVmcall(MAPGPA) failed Retry
> PhysicalAddress is %llx, MapGpaRetryaddr is %llx\n", __func__,
> PhysicalAddress, MapGpaRetryaddr));
> +      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 (#110223): https://edk2.groups.io/g/devel/message/110223
Mute This Topic: https://groups.io/mt/102212640/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/edk2-devel-archive/attachments/20231027/063f9062/attachment.htm>