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

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

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 any case, this patch appears to be correct too, so I won't insist.

Thanks,

> ---
>  OvmfPkg/IoMmuDxe/IoMmuBuffer.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/OvmfPkg/IoMmuDxe/IoMmuBuffer.c b/OvmfPkg/IoMmuDxe/IoMmuBuffer.c
> index 103003cae376..6fcf88b50ce1 100644
> --- a/OvmfPkg/IoMmuDxe/IoMmuBuffer.c
> +++ b/OvmfPkg/IoMmuDxe/IoMmuBuffer.c
> @@ -442,7 +442,9 @@ IoMmuAllocateCommonBuffer (
>    )
>  {
>    EFI_STATUS  Status;
> +  EFI_TPL     OldTpl;
>
> +  OldTpl = gBS->RaiseTPL (TPL_NOTIFY);
>    Status = InternalAllocateBuffer (
>               AllocateMaxAddress,
>               MemoryType,
> @@ -453,6 +455,7 @@ IoMmuAllocateCommonBuffer (
>    ASSERT (Status == EFI_SUCCESS);
>
>    mReservedMemBitmap |= *ReservedMemBitmap;
> +  gBS->RestoreTPL (OldTpl);
>
>    if (*ReservedMemBitmap != 0) {
>      *PhysicalAddress -= SIZE_4KB;
> @@ -476,6 +479,8 @@ IoMmuFreeCommonBuffer (
>    IN UINTN                 CommonBufferPages
>    )
>  {
> +  EFI_TPL  OldTpl;
> +
>    if (!mReservedSharedMemSupported) {
>      goto LegacyFreeCommonBuffer;
>    }
> @@ -494,7 +499,10 @@ IoMmuFreeCommonBuffer (
>      mReservedMemBitmap & ((UINT32)(~CommonBufferHeader->ReservedMemBitmap))
>      ));
>
> +  OldTpl              = gBS->RaiseTPL (TPL_NOTIFY);
>    mReservedMemBitmap &= (UINT32)(~CommonBufferHeader->ReservedMemBitmap);
> +  gBS->RestoreTPL (OldTpl);
> +
>    return EFI_SUCCESS;
>
>  LegacyFreeCommonBuffer:
> --
> 2.41.0
>


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