Re: [edk2-devel] [PATCH V2 1/1] OvmfPkg/BaseMemEncryptTdxLib: Refactor error handle of SetOrClearSharedBit

["Yao, Jiewen" <jiewen.yao@intel.com>]

Merged https://github.com/tianocore/edk2/pull/3921

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Yao,
> Jiewen
> Sent: Wednesday, January 18, 2023 8:16 AM
> To: Xu, Min M <min.m.xu@intel.com>; devel@edk2.groups.io
> Cc: Aktas, Erdem <erdemaktas@google.com>; James Bottomley
> <jejb@linux.ibm.com>; Gerd Hoffmann <kraxel@redhat.com>; Tom
> Lendacky <thomas.lendacky@amd.com>; Michael Roth
> <michael.roth@amd.com>
> Subject: Re: [edk2-devel] [PATCH V2 1/1] OvmfPkg/BaseMemEncryptTdxLib:
> Refactor error handle of SetOrClearSharedBit
> 
> Reviewed-by: Jiewen Yao <Jiewen.yao@intel.com>
> 
> > -----Original Message-----
> > From: Xu, Min M <min.m.xu@intel.com>
> > Sent: Wednesday, January 18, 2023 7:53 AM
> > To: devel@edk2.groups.io
> > Cc: Xu, Min M <min.m.xu@intel.com>; Aktas, Erdem
> > <erdemaktas@google.com>; James Bottomley <jejb@linux.ibm.com>; Yao,
> > Jiewen <jiewen.yao@intel.com>; Gerd Hoffmann <kraxel@redhat.com>;
> > Tom Lendacky <thomas.lendacky@amd.com>; Michael Roth
> > <michael.roth@amd.com>
> > Subject: [PATCH V2 1/1] OvmfPkg/BaseMemEncryptTdxLib: Refactor error
> > handle of SetOrClearSharedBit
> >
> > From: Min M Xu <min.m.xu@intel.com>
> >
> > The previous implementation of SetOrClearSharedBit doesn't handle the
> > error correctly. In this patch SetOrClearSharedBit is changed to return
> > error code so that the caller can handle it.
> >
> > Cc: Erdem Aktas <erdemaktas@google.com>
> > Cc: James Bottomley <jejb@linux.ibm.com>
> > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > Cc: Gerd Hoffmann <kraxel@redhat.com>
> > Cc: Tom Lendacky <thomas.lendacky@amd.com>
> > Cc: Michael Roth <michael.roth@amd.com>
> > Reviewed-by: Jiewen Yao <jiewen.yao@intel.com>
> > Signed-off-by: Min Xu <min.m.xu@intel.com>
> > ---
> >  .../BaseMemEncryptTdxLib/MemoryEncryption.c   | 48 +++++++++++++++--
> --
> >  1 file changed, 40 insertions(+), 8 deletions(-)
> >
> > diff --git a/OvmfPkg/Library/BaseMemEncryptTdxLib/MemoryEncryption.c
> > b/OvmfPkg/Library/BaseMemEncryptTdxLib/MemoryEncryption.c
> > index 503f626d75c6..5b13042512ad 100644
> > --- a/OvmfPkg/Library/BaseMemEncryptTdxLib/MemoryEncryption.c
> > +++ b/OvmfPkg/Library/BaseMemEncryptTdxLib/MemoryEncryption.c
> > @@ -510,8 +510,11 @@ Split1GPageTo2M (
> >    @param[in]      PagetablePoint        Page table entry pointer (PTE).
> >    @param[in]      Mode                  Set or Clear shared bit
> >
> > +  @retval         EFI_SUCCESS           Successfully set or clear the memory
> > shared bit
> > +  @retval         Others                Other error as indicated
> >  **/
> > -STATIC VOID
> > +STATIC
> > +EFI_STATUS
> >  SetOrClearSharedBit (
> >    IN   OUT     UINT64              *PageTablePointer,
> >    IN           TDX_PAGETABLE_MODE  Mode,
> > @@ -520,7 +523,8 @@ SetOrClearSharedBit (
> >    )
> >  {
> >    UINT64                        AddressEncMask;
> > -  UINT64                        Status;
> > +  UINT64                        TdStatus;
> > +  EFI_STATUS                    Status;
> >    EDKII_MEMORY_ACCEPT_PROTOCOL  *MemoryAcceptProtocol;
> >
> >    AddressEncMask = GetMemEncryptionAddressMask ();
> > @@ -536,16 +540,30 @@ SetOrClearSharedBit (
> >      PhysicalAddress   &= ~AddressEncMask;
> >    }
> >
> > -  Status = TdVmCall (TDVMCALL_MAPGPA, PhysicalAddress, Length, 0, 0,
> > NULL);
> > +  TdStatus = TdVmCall (TDVMCALL_MAPGPA, PhysicalAddress, Length, 0, 0,
> > NULL);
> > +  if (TdStatus != 0) {
> > +    DEBUG ((DEBUG_ERROR, "%a: TdVmcall(MAPGPA) failed with %llx\n",
> > __FUNCTION__, TdStatus));
> > +    ASSERT (FALSE);
> > +    return EFI_DEVICE_ERROR;
> > +  }
> >
> >    //
> >    // If changing shared to private, must accept-page again
> >    //
> >    if (Mode == ClearSharedBit) {
> >      Status = gBS->LocateProtocol (&gEdkiiMemoryAcceptProtocolGuid, NULL,
> > (VOID **)&MemoryAcceptProtocol);
> > -    ASSERT (!EFI_ERROR (Status));
> > +    if (EFI_ERROR (Status)) {
> > +      DEBUG ((DEBUG_ERROR, "%a: Failed to locate MemoryAcceptProtocol
> > with %r\n", __FUNCTION__, Status));
> > +      ASSERT (FALSE);
> > +      return Status;
> > +    }
> > +
> >      Status = MemoryAcceptProtocol->AcceptMemory
> (MemoryAcceptProtocol,
> > PhysicalAddress, Length);
> > -    ASSERT (!EFI_ERROR (Status));
> > +    if (EFI_ERROR (Status)) {
> > +      DEBUG ((DEBUG_ERROR, "%a: Failed to AcceptMemory with %r\n",
> > __FUNCTION__, Status));
> > +      ASSERT (FALSE);
> > +      return Status;
> > +    }
> >    }
> >
> >    DEBUG ((
> > @@ -558,6 +576,8 @@ SetOrClearSharedBit (
> >      Mode,
> >      Status
> >      ));
> > +
> > +  return EFI_SUCCESS;
> >  }
> >
> >  /**
> > @@ -747,7 +767,11 @@ SetMemorySharedOrPrivate (
> >        // If we have at least 1GB to go, we can just update this entry
> >        //
> >        if (!(PhysicalAddress & (BIT30 - 1)) && (Length >= BIT30)) {
> > -        SetOrClearSharedBit (&PageDirectory1GEntry->Uint64, Mode,
> > PhysicalAddress, BIT30);
> > +        Status = SetOrClearSharedBit (&PageDirectory1GEntry->Uint64, Mode,
> > PhysicalAddress, BIT30);
> > +        if (EFI_ERROR (Status)) {
> > +          goto Done;
> > +        }
> > +
> >          DEBUG ((
> >            DEBUG_VERBOSE,
> >            "%a:%a: updated 1GB entry for Physical=0x%Lx\n",
> > @@ -809,7 +833,11 @@ SetMemorySharedOrPrivate (
> >          // If we have at least 2MB left to go, we can just update this entry
> >          //
> >          if (!(PhysicalAddress & (BIT21-1)) && (Length >= BIT21)) {
> > -          SetOrClearSharedBit (&PageDirectory2MEntry->Uint64, Mode,
> > PhysicalAddress, BIT21);
> > +          Status = SetOrClearSharedBit (&PageDirectory2MEntry->Uint64,
> Mode,
> > PhysicalAddress, BIT21);
> > +          if (EFI_ERROR (Status)) {
> > +            goto Done;
> > +          }
> > +
> >            PhysicalAddress += BIT21;
> >            Length          -= BIT21;
> >          } else {
> > @@ -856,7 +884,11 @@ SetMemorySharedOrPrivate (
> >            goto Done;
> >          }
> >
> > -        SetOrClearSharedBit (&PageTableEntry->Uint64, Mode,
> PhysicalAddress,
> > EFI_PAGE_SIZE);
> > +        Status = SetOrClearSharedBit (&PageTableEntry->Uint64, Mode,
> > PhysicalAddress, EFI_PAGE_SIZE);
> > +        if (EFI_ERROR (Status)) {
> > +          goto Done;
> > +        }
> > +
> >          PhysicalAddress += EFI_PAGE_SIZE;
> >          Length          -= EFI_PAGE_SIZE;
> >        }
> > --
> > 2.29.2.windows.2
> 
> 
> 
> 
>