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

["Michael Brown" <mcb30@ipxe.org>]

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.)

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.

Sorry not to be more help.

Thanks,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107060): https://edk2.groups.io/g/devel/message/107060
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]
-=-=-=-=-=-=-=-=-=-=-=-