* [PATCH V2 1/1] OvmfPkg/BaseMemEncryptTdxLib: Refactor error handle of SetOrClearSharedBit @ 2023-01-17 23:52 Min Xu 2023-01-18 0:15 ` Yao, Jiewen ` (2 more replies) 0 siblings, 3 replies; 4+ messages in thread From: Min Xu @ 2023-01-17 23:52 UTC (permalink / raw) To: devel Cc: Min M Xu, Erdem Aktas, James Bottomley, Jiewen Yao, Gerd Hoffmann, Tom Lendacky, Michael Roth 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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH V2 1/1] OvmfPkg/BaseMemEncryptTdxLib: Refactor error handle of SetOrClearSharedBit 2023-01-17 23:52 [PATCH V2 1/1] OvmfPkg/BaseMemEncryptTdxLib: Refactor error handle of SetOrClearSharedBit Min Xu @ 2023-01-18 0:15 ` Yao, Jiewen [not found] ` <173B3F0145D7A22F.14781@groups.io> 2023-01-18 9:00 ` Gerd Hoffmann 2 siblings, 0 replies; 4+ messages in thread From: Yao, Jiewen @ 2023-01-18 0:15 UTC (permalink / raw) To: Xu, Min M, devel@edk2.groups.io Cc: Aktas, Erdem, James Bottomley, Gerd Hoffmann, Tom Lendacky, Michael Roth 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
[parent not found: <173B3F0145D7A22F.14781@groups.io>]
* Re: [edk2-devel] [PATCH V2 1/1] OvmfPkg/BaseMemEncryptTdxLib: Refactor error handle of SetOrClearSharedBit [not found] ` <173B3F0145D7A22F.14781@groups.io> @ 2023-01-18 5:13 ` Yao, Jiewen 0 siblings, 0 replies; 4+ messages in thread From: Yao, Jiewen @ 2023-01-18 5:13 UTC (permalink / raw) To: devel@edk2.groups.io, Yao, Jiewen, Xu, Min M Cc: Aktas, Erdem, James Bottomley, Gerd Hoffmann, Tom Lendacky, Michael Roth 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 > > > > > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH V2 1/1] OvmfPkg/BaseMemEncryptTdxLib: Refactor error handle of SetOrClearSharedBit 2023-01-17 23:52 [PATCH V2 1/1] OvmfPkg/BaseMemEncryptTdxLib: Refactor error handle of SetOrClearSharedBit Min Xu 2023-01-18 0:15 ` Yao, Jiewen [not found] ` <173B3F0145D7A22F.14781@groups.io> @ 2023-01-18 9:00 ` Gerd Hoffmann 2 siblings, 0 replies; 4+ messages in thread From: Gerd Hoffmann @ 2023-01-18 9:00 UTC (permalink / raw) To: Min Xu Cc: devel, Erdem Aktas, James Bottomley, Jiewen Yao, Tom Lendacky, Michael Roth On Wed, Jan 18, 2023 at 07:52:32AM +0800, Min Xu wrote: > 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. Acked-by: Gerd Hoffmann <kraxel@redhat.com> ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-01-18 9:00 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-01-17 23:52 [PATCH V2 1/1] OvmfPkg/BaseMemEncryptTdxLib: Refactor error handle of SetOrClearSharedBit Min Xu 2023-01-18 0:15 ` Yao, Jiewen [not found] ` <173B3F0145D7A22F.14781@groups.io> 2023-01-18 5:13 ` [edk2-devel] " Yao, Jiewen 2023-01-18 9:00 ` Gerd Hoffmann
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox