From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id 354677803DA for ; Thu, 20 Jul 2023 13:46:06 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=lg5CiZ5c0EsmKot09w/nklRE8Pp5liiDqZ+rXaw5jK8=; c=relaxed/simple; d=groups.io; h=X-Received:X-Received:X-Received:X-Received:From:To:Cc:Subject:Date:Message-Id:MIME-Version:Precedence:List-Unsubscribe:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:X-Gm-Message-State:Content-Transfer-Encoding; s=20140610; t=1689860764; v=1; b=q1tgsE2ZaSYhppa196rdKY+TY+/PihXpC/LgTeJPac+1e+hQzPVB9er3sOe7t1ghw73yoiqR jpz7eSHuPtrJ49TLbXk53Z76r6ZLwr0WQzl2Uc0xcE4KMN/PsZSIHXsFR9xRiqwtfLfsR0nbe57 zas3S6PSKevR9DZGIOqFp6A8= X-Received: by 127.0.0.2 with SMTP id 3kPLYY7687511xpCQ4w2GhMi; Thu, 20 Jul 2023 06:46:04 -0700 X-Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by mx.groups.io with SMTP id smtpd.web11.13350.1689860764060245796 for ; Thu, 20 Jul 2023 06:46:04 -0700 X-Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 46CBC61AED; Thu, 20 Jul 2023 13:46:03 +0000 (UTC) X-Received: by smtp.kernel.org (Postfix) with ESMTPSA id 4856FC433C8; Thu, 20 Jul 2023 13:46:01 +0000 (UTC) From: "Ard Biesheuvel" To: devel@edk2.groups.io Cc: Ard Biesheuvel , Gerd Hoffmann , Jiewen Yao , Michael Brown Subject: [edk2-devel] [RFC/RFT PATCH] OvmfPkg/IoMmuDxe: don't rely on TPLs for re-entrancy Date: Thu, 20 Jul 2023 15:45:57 +0200 Message-Id: <20230720134557.3903923-1-ardb@kernel.org> MIME-Version: 1.0 Precedence: Bulk List-Unsubscribe: List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,ardb@kernel.org X-Gm-Message-State: 6TFTzMiqfp4vD2Ctbaulov6Zx7686176AA= Content-Transfer-Encoding: quoted-printable X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=q1tgsE2Z; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=kernel.org (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io Instead of relying on raising the TPL to protect the critical sections=0D that manipulate the global bitmask that keeps track of bounce buffer=0D allocations, use compare-and-exchange to manage the global variable, and=0D tweak the logic to line up with that.=0D =0D Given that IoMmuDxe implements a singleton protocol that is shared=0D between multiple drivers, and considering the elaborate and confusing=0D requirements in the UEFP spec regarding TPL levels at which protocol=0D methods may be invoked, not relying on TPL levels at all is a more=0D robust approach in this case.=0D =0D Cc: Gerd Hoffmann =0D Cc: Jiewen Yao =0D Cc: Michael Brown =0D Link: https://bugzilla.redhat.com/show_bug.cgi?id=3D2211060=0D Signed-off-by: Ard Biesheuvel =0D ---=0D OvmfPkg/IoMmuDxe/IoMmuBuffer.c | 100 ++++++++++++--------=0D OvmfPkg/IoMmuDxe/IoMmuDxe.inf | 1 +=0D 2 files changed, 60 insertions(+), 41 deletions(-)=0D =0D diff --git a/OvmfPkg/IoMmuDxe/IoMmuBuffer.c b/OvmfPkg/IoMmuDxe/IoMmuBuffer.= c=0D index 103003cae376b93f..2764c35044ac23a7 100644=0D --- a/OvmfPkg/IoMmuDxe/IoMmuBuffer.c=0D +++ b/OvmfPkg/IoMmuDxe/IoMmuBuffer.c=0D @@ -12,6 +12,7 @@=0D #include =0D #include =0D #include =0D +#include =0D #include =0D #include "IoMmuInternal.h"=0D =0D @@ -268,16 +269,17 @@ InternalAllocateBuffer (=0D IN EFI_ALLOCATE_TYPE Type,=0D IN EFI_MEMORY_TYPE MemoryType,=0D IN UINTN Pages,=0D - IN OUT UINT32 *ReservedMemBitmap,=0D + OUT UINT32 *ReservedMemBit,=0D IN OUT EFI_PHYSICAL_ADDRESS *PhysicalAddress=0D )=0D {=0D UINT32 MemBitmap;=0D + UINT32 ReservedMemBitmap;=0D UINT8 Index;=0D IOMMU_RESERVED_MEM_RANGE *MemRange;=0D UINTN PagesOfLastMemRange;=0D =0D - *ReservedMemBitmap =3D 0;=0D + *ReservedMemBit =3D 0;=0D =0D if (Pages =3D=3D 0) {=0D ASSERT (FALSE);=0D @@ -309,23 +311,31 @@ InternalAllocateBuffer (=0D =0D MemRange =3D &mReservedMemRanges[Index];=0D =0D - if ((mReservedMemBitmap & MemRange->BitmapMask) =3D=3D MemRange->BitmapM= ask) {=0D - // The reserved memory is exausted. Turn to legacy allocate.=0D - goto LegacyAllocateBuffer;=0D - }=0D + do {=0D + ReservedMemBitmap =3D mReservedMemBitmap;=0D =0D - MemBitmap =3D (mReservedMemBitmap & MemRange->BitmapMask) >> MemRange->S= hift;=0D + if ((ReservedMemBitmap & MemRange->BitmapMask) =3D=3D MemRange->Bitmap= Mask) {=0D + // The reserved memory is exhausted. Turn to legacy allocate.=0D + goto LegacyAllocateBuffer;=0D + }=0D +=0D + MemBitmap =3D (ReservedMemBitmap & MemRange->BitmapMask) >> MemRange->= Shift;=0D =0D - for (Index =3D 0; Index < MemRange->Slots; Index++) {=0D - if ((MemBitmap & (UINT8)(1<Slots; Index++) {=0D + if ((MemBitmap & (UINT8)(1<Slots);=0D + ASSERT (Index !=3D MemRange->Slots);=0D =0D - *PhysicalAddress =3D MemRange->StartAddressOfMemRange + Index * SIZE_O= F_MEM_RANGE (MemRange) + MemRange->HeaderSize;=0D - *ReservedMemBitmap =3D (UINT32)(1 << (Index + MemRange->Shift));=0D + *PhysicalAddress =3D MemRange->StartAddressOfMemRange + Index * SIZE_O= F_MEM_RANGE (MemRange) + MemRange->HeaderSize;=0D + *ReservedMemBit =3D (UINT32)(1 << (Index + MemRange->Shift));=0D + } while (ReservedMemBitmap !=3D InterlockedCompareExchange32 (=0D + &mReservedMemBitmap,=0D + ReservedMemBitmap,=0D + ReservedMemBitmap | *ReservedMemBit=0D + ));=0D =0D DEBUG ((=0D DEBUG_VERBOSE,=0D @@ -334,16 +344,16 @@ InternalAllocateBuffer (=0D MemRange->DataSize,=0D *PhysicalAddress,=0D Pages,=0D - *ReservedMemBitmap,=0D - mReservedMemBitmap,=0D - mReservedMemBitmap | *ReservedMemBitmap=0D + *ReservedMemBit,=0D + ReservedMemBitmap,=0D + ReservedMemBitmap | *ReservedMemBit=0D ));=0D =0D return EFI_SUCCESS;=0D =0D LegacyAllocateBuffer:=0D =0D - *ReservedMemBitmap =3D 0;=0D + *ReservedMemBit =3D 0;=0D return gBS->AllocatePages (Type, MemoryType, Pages, PhysicalAddress);=0D }=0D =0D @@ -366,27 +376,41 @@ IoMmuAllocateBounceBuffer (=0D )=0D {=0D EFI_STATUS Status;=0D - UINT32 ReservedMemBitmap;=0D - EFI_TPL OldTpl;=0D -=0D - OldTpl =3D gBS->RaiseTPL (TPL_NOTIFY);=0D - ReservedMemBitmap =3D 0;=0D - Status =3D InternalAllocateBuffer (=0D - Type,=0D - MemoryType,=0D - MapInfo->NumberOfPages,=0D - &ReservedMemBitmap,=0D - &MapInfo->PlainTextAddress=0D - );=0D - MapInfo->ReservedMemBitmap =3D ReservedMemBitmap;=0D - mReservedMemBitmap |=3D ReservedMemBitmap;=0D - gBS->RestoreTPL (OldTpl);=0D =0D + Status =3D InternalAllocateBuffer (=0D + Type,=0D + MemoryType,=0D + MapInfo->NumberOfPages,=0D + &MapInfo->ReservedMemBitmap,=0D + &MapInfo->PlainTextAddress=0D + );=0D ASSERT (Status =3D=3D EFI_SUCCESS);=0D =0D return Status;=0D }=0D =0D +/**=0D + * Clear a bit in the reserved memory bitmap in a thread safe manner=0D + *=0D + * @param ReservedMemBit The bit to clear=0D + */=0D +STATIC=0D +VOID=0D +ClearReservedMemBit (=0D + IN UINT32 ReservedMemBit=0D + )=0D +{=0D + UINT32 ReservedMemBitmap;=0D +=0D + do {=0D + ReservedMemBitmap =3D mReservedMemBitmap;=0D + } while (ReservedMemBitmap !=3D InterlockedCompareExchange32 (=0D + &mReservedMemBitmap,=0D + ReservedMemBitmap,=0D + ReservedMemBitmap & ~ReservedMemBit=0D + ));=0D +}=0D +=0D /**=0D * Free the bounce buffer allocated in IoMmuAllocateBounceBuffer.=0D *=0D @@ -398,8 +422,6 @@ IoMmuFreeBounceBuffer (=0D IN OUT MAP_INFO *MapInfo=0D )=0D {=0D - EFI_TPL OldTpl;=0D -=0D if (MapInfo->ReservedMemBitmap =3D=3D 0) {=0D gBS->FreePages (MapInfo->PlainTextAddress, MapInfo->NumberOfPages);=0D } else {=0D @@ -412,11 +434,9 @@ IoMmuFreeBounceBuffer (=0D mReservedMemBitmap,=0D mReservedMemBitmap & ((UINT32)(~MapInfo->ReservedMemBitmap))=0D ));=0D - OldTpl =3D gBS->RaiseTPL (TPL_NOTIFY);=0D MapInfo->PlainTextAddress =3D 0;=0D - mReservedMemBitmap &=3D (UINT32)(~MapInfo->ReservedMemBitmap);= =0D + ClearReservedMemBit (MapInfo->ReservedMemBitmap);=0D MapInfo->ReservedMemBitmap =3D 0;=0D - gBS->RestoreTPL (OldTpl);=0D }=0D =0D return EFI_SUCCESS;=0D @@ -452,8 +472,6 @@ IoMmuAllocateCommonBuffer (=0D );=0D ASSERT (Status =3D=3D EFI_SUCCESS);=0D =0D - mReservedMemBitmap |=3D *ReservedMemBitmap;=0D -=0D if (*ReservedMemBitmap !=3D 0) {=0D *PhysicalAddress -=3D SIZE_4KB;=0D }=0D @@ -494,7 +512,7 @@ IoMmuFreeCommonBuffer (=0D mReservedMemBitmap & ((UINT32)(~CommonBufferHeader->ReservedMemBitmap)= )=0D ));=0D =0D - mReservedMemBitmap &=3D (UINT32)(~CommonBufferHeader->ReservedMemBitmap)= ;=0D + ClearReservedMemBit (CommonBufferHeader->ReservedMemBitmap);=0D return EFI_SUCCESS;=0D =0D LegacyFreeCommonBuffer:=0D diff --git a/OvmfPkg/IoMmuDxe/IoMmuDxe.inf b/OvmfPkg/IoMmuDxe/IoMmuDxe.inf= =0D index 17fca52856925da0..d08f7e59e2b665f4 100644=0D --- a/OvmfPkg/IoMmuDxe/IoMmuDxe.inf=0D +++ b/OvmfPkg/IoMmuDxe/IoMmuDxe.inf=0D @@ -35,6 +35,7 @@ [LibraryClasses]=0D MemEncryptSevLib=0D MemEncryptTdxLib=0D MemoryAllocationLib=0D + SynchronizationLib=0D UefiBootServicesTableLib=0D UefiDriverEntryPoint=0D =0D -- =0D 2.39.2=0D =0D -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#107100): https://edk2.groups.io/g/devel/message/107100 Mute This Topic: https://groups.io/mt/100256049/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=-