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

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

[Gerd Hoffmann]

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>
---
 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 (#107088): https://edk2.groups.io/g/devel/message/107088
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]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Ard Biesheuvel]

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]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Gerd Hoffmann]

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]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Gerd Hoffmann]

  Hi,

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

Not a straight forward conversion, current code copies around bitmap
snippets, so this logic needs a rewrite to work with
InterlockedCompareExchange32().  Don't have the time to do that now
(leaving tomorrow for my three week summer vacation ...).

take care,
  Gerd



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