public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
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 --]

      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