Re: [edk2-devel] [PATCH 1/1] OvmfPkg/IoMmuDxe: add locking to IoMmuAllocateCommonBuffer

["Gerd Hoffmann" <kraxel@redhat.com>]

On Thu, Jul 20, 2023 at 10:34:31AM +0200, Ard Biesheuvel wrote:
> On Thu, 20 Jul 2023 at 10:24, Gerd Hoffmann <kraxel@redhat.com> wrote:
> >
> > IoMmuAllocateCommonBuffer has the very same allocation pattern
> > IoMmuAllocateBounceBuffer uses, so the fix added by commit a52044a9e602
> > ("OvmfPkg/IoMmuDxe: add locking to IoMmuAllocateBounceBuffer") is needed
> > here too.
> >
> > Reported-by: Michael Brown <mcb30@ipxe.org>
> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> 
> Thanks.
> 
> After pondering this a bit longer, I wonder whether we should simply
> use InterlockedCompareExchange32() instead, rather than play with the
> TPL levels. The only thing we are protecting here are concurrent
> modifications of mReservedMemBitmap, right?

In IoMmuFree{Bounce,Common}Buffer() yes.

In the allocation code paths no.  The InternalAllocateBuffer() called
searches mReservedMemBitmap for a unused + big enough buffer, returns
what it has found without actually reserving it.  Setting the bit is
done by the caller.  Thats why both InternalAllocateBuffer() call and
"mReservedMemBitmap |= bit" runs with TPL raised.

Not sure why InternalAllocateBuffer() doesn't update mReservedMemBitmap
itself.  That would be needed to protect the allocation code paths with
InterlockedCompareExchange32() too.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107092): https://edk2.groups.io/g/devel/message/107092
Mute This Topic: https://groups.io/mt/100251949/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-