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