From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id 8ADF6740046 for ; Fri, 27 Oct 2023 16:44:52 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=KSiEe9ARV/0R7VH0j7SIMKUm9GpoSsNPVvsJrSRhVUc=; c=relaxed/simple; d=groups.io; h=MIME-Version:References:In-Reply-To:From:Date:Message-ID:Subject:To:Cc:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Type; s=20140610; t=1698425091; v=1; b=qsfElAgTvy2R4t4OM1+JxUZAPoT4F/+ZN/HH3f1nWcVy0AdibKugzkj8ck4vlpDwSYZ7TW13 oMjxhvciQ1rqIV/EDFcCZ+QQh0cK8MYJq9VN8DHA38c0IhrzEDQxWq4S75nViMxbZ9i7YbfRouZ 1SuulmHb9qJj52X3tiimf3dA= X-Received: by 127.0.0.2 with SMTP id znWtYY7687511xDlbVCZBQP1; Fri, 27 Oct 2023 09:44:51 -0700 X-Received: from mail-qk1-f175.google.com (mail-qk1-f175.google.com [209.85.222.175]) by mx.groups.io with SMTP id smtpd.web10.11838.1698425090556177660 for ; Fri, 27 Oct 2023 09:44:50 -0700 X-Received: by mail-qk1-f175.google.com with SMTP id af79cd13be357-7789a4c01ddso167647485a.1 for ; Fri, 27 Oct 2023 09:44:50 -0700 (PDT) X-Gm-Message-State: HxRsY66VKW9euNe0CEU2Gd2ex7686176AA= X-Google-Smtp-Source: AGHT+IE3MApMRWQKL6LZSHMI6dJVLRwJFu4JCBS8s+r1MM1c4FhyAGwMn670+nCfgzdDOblDxWovo0wGP04oXsJ3hMI= X-Received: by 2002:a05:6214:448b:b0:66f:bcc5:cf70 with SMTP id on11-20020a056214448b00b0066fbcc5cf70mr1763849qvb.42.1698425089343; Fri, 27 Oct 2023 09:44:49 -0700 (PDT) MIME-Version: 1.0 References: <20231027005738.371-1-cepingx.sun@intel.com> <20231027005738.371-3-cepingx.sun@intel.com> In-Reply-To: <20231027005738.371-3-cepingx.sun@intel.com> From: "Erdem Aktas via groups.io" Date: Fri, 27 Oct 2023 09:44:36 -0700 Message-ID: Subject: Re: [edk2-devel] [PATCH V1 2/2] OvmfPkg/BaseMemEncryptTdxLib: Handle retry result of MapGPA To: sunceping Cc: devel@edk2.groups.io, James Bottomley , Jiewen Yao , Min Xu , Tom Lendacky , Michael Roth , Gerd Hoffmann Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,erdemaktas@google.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: Content-Type: multipart/alternative; boundary="0000000000002197e70608b568f5" X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=qsfElAgT; dmarc=none; spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io --0000000000002197e70608b568f5 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hi, This should be the [PATCH V1 2/2] I assume? On Thu, Oct 26, 2023 at 5:58=E2=80=AFPM sunceping w= rote: > From: Ceping Sun > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D4572 > > 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 regio= n, > 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 > Cc: James Bottomley > Cc: Jiewen Yao > Cc: Min Xu > Cc: Tom Lendacky > Cc: Michael Roth > Cc: Gerd Hoffmann > Signed-off-by: Ceping Sun > --- > .../BaseMemEncryptTdxLib.inf | 1 + > .../BaseMemEncryptTdxLib/MemoryEncryption.c | 36 ++++++++++++++++++- > 2 files changed, 36 insertions(+), 1 deletion(-) > > diff --git a/OvmfPkg/Library/BaseMemEncryptTdxLib/BaseMemEncryptTdxLib.in= f > 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 =3D 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 =3D 0; > + RetryCount =3D 0; > + > AddressEncMask =3D GetMemEncryptionAddressMask (); > > // > @@ -559,7 +570,30 @@ SetOrClearSharedBit ( > PhysicalAddress &=3D ~AddressEncMask; > } > > - TdStatus =3D TdVmCall (TDVMCALL_MAPGPA, PhysicalAddress, Length, 0, 0, > NULL); > + while (RetryCount < MAX_RETRIES_PER_PAGE) { > + TdStatus =3D TdVmCallMapGPA (PhysicalAddress, Length, &MapGpaRetryad= dr); > Why not this? TdStatus =3D TdVmCall (TDVMCALL_MAPGPA, PhysicalAddress, Length, 0, 0, &MapGpaRetryaddr); > + if (TdStatus !=3D TDVMCALL_STATUS_RETRY) { > + break; > + } > + > + DEBUG ((DEBUG_VERBOSE, "%a: TdVmcall(MAPGPA) Retry PhysicalAddress i= s > %llx, MapGpaRetryaddr is %llx\n", __func__, PhysicalAddress, > MapGpaRetryaddr)); > + > + EndAddress =3D PhysicalAddress + Length; > + if ((MapGpaRetryaddr < PhysicalAddress) || (MapGpaRetryaddr > > EndAddress)) { > should be? if ((MapGpaRetryaddr < PhysicalAddress) || (MapGpaRetryaddr >=3D EndAddress)) > + DEBUG ((DEBUG_ERROR, "%a: TdVmcall(MAPGPA) failed Retry > PhysicalAddress is %llx, MapGpaRetryaddr is %llx\n", __func__, > PhysicalAddress, MapGpaRetryaddr)); > + break; > + } > + > + if (MapGpaRetryaddr =3D=3D PhysicalAddress) { > + RetryCount++; > + continue; > + } > + > + PhysicalAddress =3D MapGpaRetryaddr; > + Length =3D EndAddress - PhysicalAddress; > + RetryCount =3D 0; > + } > + > if (TdStatus !=3D 0) { > DEBUG ((DEBUG_ERROR, "%a: TdVmcall(MAPGPA) failed with %llx\n", > __func__, TdStatus)); > ASSERT (FALSE); > -- > 2.34.1 > > -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- 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/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- --0000000000002197e70608b568f5 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi,

This should be th= e=C2=A0[PATCH V1 2/2] I assume?=C2=A0

On Thu, Oct 26, 2023 at 5:58=E2=80=AFPM= sunceping <cepingx.sun@intel.c= om> wrote:
cepingx.sun@intel.com>

REF: https://bugzilla.tianocore.org/show_bug.c= gi?id=3D4572

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 th= at 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@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>
---
=C2=A0.../BaseMemEncryptTdxLib.inf=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 |=C2=A0 1 +
=C2=A0.../BaseMemEncryptTdxLib/MemoryEncryption.c=C2=A0 =C2=A0| 36 ++++++++= ++++++++++-
=C2=A02 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 @@
=C2=A0[Sources]
=C2=A0 =C2=A0VirtualMemory.h
=C2=A0 =C2=A0MemoryEncryption.c
+=C2=A0 X64/TdVmCallMapGPA.nasm
I do not think we need= another TdVmCallMapGPA definition, do we?=C2=A0

=C2=A0[LibraryClasses]
=C2=A0 =C2=A0BaseLib
diff --git a/OvmfPkg/Library/BaseMemEncryptTdxLib/MemoryEncryption.c b/Ovmf= Pkg/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 {

=C2=A0STATIC PAGE_TABLE_POOL=C2=A0 *mPageTablePool =3D NULL;

+#define TDVMCALL_STATUS_RETRY=C2=A0 0x1
+
+#define MAX_RETRIES_PER_PAGE=C2=A0 3
+
=C2=A0/**
=C2=A0 =C2=A0This function is used to help request the host VMM to map a GP= A range as
=C2=A0 =C2=A0private or shared-memory mappings.
@@ -546,6 +550,13 @@ SetOrClearSharedBit (
=C2=A0 =C2=A0EFI_STATUS=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 Status;
=C2=A0 =C2=A0EDKII_MEMORY_ACCEPT_PROTOCOL=C2=A0 *MemoryAcceptProtocol;

+=C2=A0 UINT64=C2=A0 MapGpaRetryaddr;
Should be replac= ed with MapGpaRetryAddr for consistency in variable name=C2=A0casing style = ?=C2=A0
+=C2=A0 UINT32=C2=A0 RetryCount;
+=C2=A0 UINT64=C2=A0 EndAddress;
+
+=C2=A0 MapGpaRetryaddr =3D 0;
+=C2=A0 RetryCount=C2=A0 =C2=A0 =C2=A0 =3D 0;
+
=C2=A0 =C2=A0AddressEncMask =3D GetMemEncryptionAddressMask ();

=C2=A0 =C2=A0//
@@ -559,7 +570,30 @@ SetOrClearSharedBit (
=C2=A0 =C2=A0 =C2=A0PhysicalAddress=C2=A0 =C2=A0&=3D ~AddressEncMask; =C2=A0 =C2=A0}

-=C2=A0 TdStatus =3D TdVmCall (TDVMCALL_MAPGPA, PhysicalAddress, Length, 0,= 0, NULL);
+=C2=A0 while (RetryCount < MAX_RETRIES_PER_PAGE) {
+=C2=A0 =C2=A0 TdStatus =3D TdVmCallMapGPA (PhysicalAddress, Length, &M= apGpaRetryaddr);
Why not this?
=C2=A0TdStatu= s =3D TdVmCall (TDVMCALL_MAPGPA, PhysicalAddress, Length, 0, 0, &MapGpa= Retryaddr);
+=C2=A0 =C2=A0 if (TdStatus !=3D TDVMCALL_STATUS_RETRY) {
+=C2=A0 =C2=A0 =C2=A0 break;
+=C2=A0 =C2=A0 }
+
+=C2=A0 =C2=A0 DEBUG ((DEBUG_VERBOSE, "%a: TdVmcall(MAPGPA) Retry Phys= icalAddress is %llx, MapGpaRetryaddr is %llx\n", __func__, PhysicalAdd= ress, MapGpaRetryaddr));
+
+=C2=A0 =C2=A0 EndAddress =3D PhysicalAddress + Length;
+=C2=A0 =C2=A0 if ((MapGpaRetryaddr < PhysicalAddress) || (MapGpaRetryad= dr > EndAddress)) {
=C2=A0should be?
=C2= =A0if ((MapGpaRetryaddr < PhysicalAddress) || (MapGpaRetryaddr >=3D E= ndAddress))=C2=A0
_._,_._,_
Groups.io Links:

=20 You receive all messages sent to this group. =20 =20

View/Reply Online (#110223) | =20 | Mute= This Topic | New Topic
Your Subscriptio= n | Contact Group Owner | Unsubscribe [rebecca@openfw.io]

_._,_._,_
--0000000000002197e70608b568f5--