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

["Ard Biesheuvel" <ardb@kernel.org>]

On Wed, 19 Jul 2023 at 19:40, Michael Brown <mcb30@ipxe.org> wrote:
>
> On 19/07/2023 17:52, Ard Biesheuvel wrote:
> > On Wed, 19 Jul 2023 at 18:32, Gerd Hoffmann <kraxel@redhat.com> wrote:
> >> On Wed, Jul 19, 2023 at 04:04:28PM +0000, Michael Brown wrote:
> >>> 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,
> >
> > You mean something along the lines of
> >
> >    andl %reg, mReservedMemBitmap(%rip)
> >
> > right?
>
> Even with a single orl/andl instruction, the operation is unlocked.
> It's guaranteed atomic against interrupts (since interrupts always occur
> at instruction boundaries) but it's not guaranteed atomic against
> concurrent accesses to the same global variable from other processors.
>
> (I have no idea if the UEFI model allows APs to call into the IOMMU
> protocol or not, so I don't know if this is a real problem.)
>

No, it's not really a problem. While there is a notion of 'run
function F() on AP #n' in the MpServices protocol, there is no real
SMP support where the same code and data are being used concurrently
on multiple cores in parallel.

> On a quick review of the code, there appear to be other points that also
> modify mReservedMemBitmap (IoMmuAllocateCommonBuffer() and
> IoMmuFreeCommonBuffer()).  I'd guess that these also need to raise to
> TPL_NOTIFY, but I'm not familiar with the code so I don't know if
> there's anything that makes this unnecessary.
>

Thanks for mentioning that - perhaps Gerd could have another look? (I
merged the v2 already)


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