On Wed, Jul 19, 2023 at 04:04:28PM +0000, Michael Brown wrote: > On 19/07/2023 12:33, Gerd Hoffmann wrote: > > Searching for an unused bounce buffer in mReservedMemBitmap and > > reserving the buffer by flipping the bit is a critical section > > which must not be interrupted. Raise the TPL level to ensure > > that. > > > > Without this fix it can happen that IoMmuDxe hands out the same > > bounce buffer twice, causing trouble down the road. Seen happening > > in practice with VirtioNetDxe setting up the network interface (and > > calling into IoMmuDxe from a polling timer callback) in parallel with > > Boot Manager doing some disk I/O. An ASSERT() in VirtioNet caught > > the buffer inconsistency. > > > > Full story with lots of details and discussions is available here: > > https://bugzilla.redhat.com/show_bug.cgi?id=2211060 > > > > Signed-off-by: Gerd Hoffmann <kra...@redhat.com> > > --- > > OvmfPkg/IoMmuDxe/IoMmuBuffer.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/OvmfPkg/IoMmuDxe/IoMmuBuffer.c b/OvmfPkg/IoMmuDxe/IoMmuBuffer.c > > index c8f6cf4818e8..7f8a0368ab5d 100644 > > --- a/OvmfPkg/IoMmuDxe/IoMmuBuffer.c > > +++ b/OvmfPkg/IoMmuDxe/IoMmuBuffer.c > > @@ -367,7 +367,9 @@ IoMmuAllocateBounceBuffer ( > > { > > EFI_STATUS Status; > > UINT32 ReservedMemBitmap; > > + EFI_TPL OldTpl; > > + OldTpl = gBS->RaiseTPL (TPL_NOTIFY); > > ReservedMemBitmap = 0; > > Status = InternalAllocateBuffer ( > > Type, > > @@ -378,6 +380,7 @@ IoMmuAllocateBounceBuffer ( > > ); > > MapInfo->ReservedMemBitmap = ReservedMemBitmap; > > mReservedMemBitmap |= ReservedMemBitmap; > > + gBS->RestoreTPL (OldTpl); > > ASSERT (Status == EFI_SUCCESS); > > It looks as though IoMmuFreeBounceBuffer() should also raise to TPL_NOTIFY > while modifying mReservedMemBitmap, since the modification made in > IoMmuFreeBounceBuffer() is not an atomic operation: > > mReservedMemBitmap &= (UINT32)(~MapInfo->ReservedMemBitmap);
I'd expect modern compilers optimize that to a single instruction, but yes, it's not guaranteed to happen, the compiler can choose to generate a series of load + and + store instructions instead. Let's play safe, I'll send v2. take care, Gerd -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#107058): https://edk2.groups.io/g/devel/message/107058 Mute This Topic: https://groups.io/mt/100233359/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
