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: 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 --]