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

["sunceping" <cepingx.sun@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 3488 bytes --]

On Saturday, October 28, 2023 12:45 AM, Erdem Aktas wrote:
This should be the [PATCH V1 2/2] I assume?
Yes, the name is same with [PATCH v1 0/2] , may be confusion, I would update in next version to avoid the same title name.


On Thu, Oct 26, 2023 at 5:58 PM sunceping <cepingx.sun@intel.com<mailto:cepingx.sun@intel.com>> wrote:
 [Sources]
   VirtualMemory.h
   MemoryEncryption.c
+  X64/TdVmCallMapGPA.nasm
I do not think we need another TdVmCallMapGPA definition, do we?
Currently,  the TdVmCall always clears the R11 if the return code is not successful,  which means we need to change TdVmCall if we don't add TdVmCallMapGPA.
Refer the GHCI Spec , if the returns code is not successful,  the R11 value is not valid for the sub-functions except MapGPA,  which means  TdVmCall should clear the value on
unsuccessful returns and only save the value if MapGPA returns unsuccessfully.  If an update is required, the logic in TdVmCall could be complex.

 [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 ?
Yes, it would be updated in next version.

+  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);
The TdVmCall always clears the R11 value when unsuccessful returns as above comments, therefor add the TdVmCallMapGPA to handle it.

+    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))
Yes, that’s right, it would be updated in next version.

Thanks
 Ceping



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



[-- Attachment #2: Type: text/html, Size: 7481 bytes --]