From: "sunceping" <cepingx.sun@intel.com>
To: "Aktas, Erdem" <erdemaktas@google.com>
Cc: "devel@edk2.groups.io" <devel@edk2.groups.io>,
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: Re: [edk2-devel] [PATCH V1 2/2] OvmfPkg/BaseMemEncryptTdxLib: Handle retry result of MapGPA
Date: Thu, 2 Nov 2023 09:25:05 +0000 [thread overview]
Message-ID: <SN6PR11MB27839C1BCB596556DE5F37DBE7A6A@SN6PR11MB2783.namprd11.prod.outlook.com> (raw)
In-Reply-To: <CAAYXXYwfEC1FTwMaXLQ1eCCv24ZWVgtZY1wZmb-vsqQnBmgdYw@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 5148 bytes --]
On Tuesday, October 31, 2023 8:45 AM, Erdem Aktas wrote:
>
> On Sun, Oct 29, 2023 at 11:42 PM Sun, CepingX <cepingx.sun@intel.com<mailto:cepingx.sun@intel.com>>
> 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
> 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.
>
> 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.
@Erdem Aktas<mailto:erdemaktas@google.com> In v2 Patch, use the updated TdVmcall to instead of the TdVmCallMapGPA.nasm .
The v2 path mail( [PATCH V2 0/2] OvmfPkg: Update TdVmCall to handle the retry for MapGPA) has been sent , please review it.
> >
> > [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.
> 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 != 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 (#110508): https://edk2.groups.io/g/devel/message/110508
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: 11826 bytes --]
prev parent reply other threads:[~2023-11-02 9:25 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-10-27 0:57 [edk2-devel] [PATCH V1 0/2] OvmfPkg/BaseMemEncryptTdxLib: Handle retry result of MapGPA sunceping
2023-10-27 0:57 ` [edk2-devel] [PATCH V1 1/2] OvmfPkg/BaseMemEncryptTdxLib: Add TdVmCallMapGPA sunceping
2023-10-27 0:57 ` [edk2-devel] [PATCH V1 2/2] OvmfPkg/BaseMemEncryptTdxLib: Handle retry result of MapGPA sunceping
2023-10-27 11:04 ` Gerd Hoffmann
2023-10-30 2:26 ` sunceping
2023-10-27 16:44 ` Erdem Aktas via groups.io
2023-10-30 6:41 ` sunceping
2023-10-31 0:45 ` Erdem Aktas via groups.io
2023-10-31 7:46 ` sunceping
2023-11-02 9:25 ` sunceping [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=SN6PR11MB27839C1BCB596556DE5F37DBE7A6A@SN6PR11MB2783.namprd11.prod.outlook.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox