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 8E5C79403E2 for ; Tue, 31 Oct 2023 00:45:30 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=zV1RdSK+KQGKqEKMNL3YdRpV/KBqHG2AqXuPZCTWook=; 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:Content-Transfer-Encoding; s=20140610; t=1698713129; v=1; b=Tfa3O+zstsCi9vI5o4YM46BQ9aTb4tqmod8Tr8IN+nQTwLoCGM8TKfifQcmZxTFnWw2HPs3/ DZy7yW9q447Sx5eOIOpCJEQXm51243fTJP7CUvEkgA2d3MhVwQVkDVD8a5DYD1+CRZUZyCfII9h 1jrKNdNpijFADsfH/RGXXs7M= X-Received: by 127.0.0.2 with SMTP id Vj9vYY7687511x0B69cKO37n; Mon, 30 Oct 2023 17:45:29 -0700 X-Received: from mail-qv1-f47.google.com (mail-qv1-f47.google.com [209.85.219.47]) by mx.groups.io with SMTP id smtpd.web10.176420.1698713128587379061 for ; Mon, 30 Oct 2023 17:45:28 -0700 X-Received: by mail-qv1-f47.google.com with SMTP id 6a1803df08f44-66d0760cd20so42098586d6.0 for ; Mon, 30 Oct 2023 17:45:28 -0700 (PDT) X-Gm-Message-State: vfzTUWxPZml66yKhx6bCemcsx7686176AA= X-Google-Smtp-Source: AGHT+IF7qhE78oMhn8Sf/IKH2qvR6OzPb+tD0hvv7xdy43stSl9z9fHsn8bwQeLDJNBRNQSemNVDmmi0vfhM51s1KKk= X-Received: by 2002:ad4:5c61:0:b0:66d:81bb:5234 with SMTP id i1-20020ad45c61000000b0066d81bb5234mr2265386qvh.11.1698713127588; Mon, 30 Oct 2023 17:45:27 -0700 (PDT) MIME-Version: 1.0 References: <20231027005738.371-1-cepingx.sun@intel.com> <20231027005738.371-3-cepingx.sun@intel.com> In-Reply-To: From: "Erdem Aktas via groups.io" Date: Mon, 30 Oct 2023 17:45:16 -0700 Message-ID: Subject: Re: [edk2-devel] [PATCH V1 2/2] OvmfPkg/BaseMemEncryptTdxLib: Handle retry result of MapGPA To: "Sun, CepingX" Cc: "devel@edk2.groups.io" , James Bottomley , "Yao, Jiewen" , "Xu, Min M" , 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: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=Tfa3O+zs; 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 On Sun, Oct 29, 2023 at 11:42=E2=80=AFPM Sun, CepingX wrote: > > 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 upd= ate in next version to avoid the same title name. > > > On Thu, Oct 26, 2023 at 5:58=E2=80=AFPM sunceping = 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 TdVmCal= lMapGPA. > Refer the GHCI Spec , if the returns code is not successful, the R11 valu= e 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 unsuccessf= ully. If an update is required, the logic in TdVmCall could be complex. It seems like TdVmCallMapGPA function is actually duplicating most of the code that the TdVmCall function is already doing. According to the spec, R11 has meaningful data when mapgpa has RETRY, GPA_IN_USE or ALIGN_ERROR. I do not think the TdVmCall change logic would be complex (or not more than what TdVmCallMapGPA is already doing). I would like to see what others are saying on this. > > [LibraryClasses] > BaseLib > diff --git a/OvmfPkg/Library/BaseMemEncryptTdxLib/MemoryEncryption.c b/Ov= mfPkg/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 ? > Yes, it would be updated in next version. > > + 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); > The TdVmCall always clears the R11 value when unsuccessful returns as abo= ve comments, therefor add the TdVmCallMapGPA to handle it. right, the tdvmcall does not handle the R11 correctly for mapGPA. I think it should be an easy fix in that function instead of creating a whole copy of that function. Is there a reason why we think it is complicated? > > + if (TdStatus !=3D TDVMCALL_STATUS_RETRY) { > + break; > + } > + > + DEBUG ((DEBUG_VERBOSE, "%a: TdVmcall(MAPGPA) Retry PhysicalAddress i= s %llx, MapGpaRetryaddr is %llx\n", __func__, PhysicalAddress, MapGpaRetrya= ddr)); > + > + EndAddress =3D PhysicalAddress + Length; > + if ((MapGpaRetryaddr < PhysicalAddress) || (MapGpaRetryaddr > EndAdd= ress)) { > should be? > if ((MapGpaRetryaddr < PhysicalAddress) || (MapGpaRetryaddr >=3D EndAddr= ess)) > Yes, that=E2=80=99s right, it would be updated in next version. > > Thanks > Ceping > -=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 (#110350): https://edk2.groups.io/g/devel/message/110350 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-