[edk2-devel] [RFC/RFT PATCH] OvmfPkg/IoMmuDxe: don't rely on TPLs for re-entrancy

[Ard Biesheuvel]

On Thu, 24 Aug 2023 at 10:06, Gerd Hoffmann <kraxel at redhat.com> wrote:
>
> On Wed, Aug 23, 2023 at 07:10:52PM +0100, Pedro Falcato wrote:
> > On Wed, Aug 23, 2023 at 4:12 PM Ard Biesheuvel <ardb at kernel.org> wrote:
> > >
> > > On Wed, 23 Aug 2023 at 13:08, Gerd Hoffmann <kraxel at redhat.com> wrote:
> > > >
> > > > Hmm, QE reports back it slows down the boot alot.  No boot hangs yet
> > > > with 12 test runs so far, which isn't that much for a reproduce rate
> > > > below 20% ...
> > > >
> > > > https://bugzilla.redhat.com//show_bug.cgi?id=2211060#c28
> > > >
> > > > So I guess we go with the TPL version for the coming stable tag and
> > > > leave any improvements for later ...
> > >
> > > Yeah, this was not going to make the stable tag in any case.
> > >
> > > The boot speed regression seems odd, though - this is effectively UP
> > > code so there shouldn't be any contention, the only thing this patch
> > > does is ensure that the critical section is restarted if it was
> > > interrupted
>
> QE reported back boot times without this patch are ~20-30 seconds,
> with this patch it can be more than 3 minutes.
>
> > FWIW: Given completely correct logic, straightforward logic, a lock
> > cmpxchg is much slower(3-4x) than an a non-lock cmpxchg, which itself
> > is around 2x as slow as a regular relaxed load + store.
> > See https://gist.github.com/heatd/49c9be23ccb1f4ad8dfeac231da2647a for
> > a nice fun test benchmark.
>
> Also note that IoMmuDxe is only used in case memory encryption is
> enabled (I/O uses unencrypted bounce buffers so the host can virtio
> emulation can read/write stuff there).  Maybe that affects performance
> too.
>
> Cc'ing the AMD people for comments on that.
>
> > HOWEVER, given this is likely in IO paths, I would *really* not expect
> > this to make a difference, right? Even virtio drivers will themselves
> > trap with a VMEXIT whenever you touch a hardware register...
>
> It's only a single VMEXIT per I/O request (ring the doorbell after
> adding a request to the ring), but still, this is heavy enough that
> the cmpxchg difference should be in the noise.
>
> > Gerd, could you folks get a perf kvm (perf-kvm(1)) recording out of
> > that OVMF build? Assuming you can get that thing to work, that is,
> > personally it mysteriously stopped working 6 years ago for me :)
>
> I can try hack IoMmuDxe so it is used unconditionally and try reproduce
> locally (without sev-capable hardware), but most likely not this week.
>

I have tried the patch below, and I don't see any slowdowns with or
without the patch, running both DEBUG and RELEASE builds under
ordinary KVM/nested-virt. Note that the change in the first hunk will
cause the ASSERT()s removed in the other hunks to trigger so the code
is definitely being exercised.


diff --git a/OvmfPkg/IoMmuDxe/IoMmuDxe.c b/OvmfPkg/IoMmuDxe/IoMmuDxe.c
index aab6d8b90687..c1082b733d3a 100644
--- a/OvmfPkg/IoMmuDxe/IoMmuDxe.c
+++ b/OvmfPkg/IoMmuDxe/IoMmuDxe.c
@@ -25,7 +25,7 @@ IoMmuDxeEntryPoint (
   // When SEV or TDX is enabled, install IoMmu protocol otherwise install the
   // placeholder protocol so that other dependent module can run.
   //
-  if (MemEncryptSevIsEnabled () || MemEncryptTdxIsEnabled ()) {
+  if (TRUE || MemEncryptSevIsEnabled () || MemEncryptTdxIsEnabled ()) {
     Status = InstallIoMmuProtocol ();
   } else {
     Handle = NULL;
diff --git a/OvmfPkg/IoMmuDxe/IoMmuBuffer.c b/OvmfPkg/IoMmuDxe/IoMmuBuffer.c
index 2764c35044ac..27fe068362ff 100644
--- a/OvmfPkg/IoMmuDxe/IoMmuBuffer.c
+++ b/OvmfPkg/IoMmuDxe/IoMmuBuffer.c
@@ -184,7 +184,7 @@ IoMmuInitReservedSharedMem (
                    );
         ASSERT (!EFI_ERROR (Status));
       } else {
-        ASSERT (FALSE);
+        //ASSERT (FALSE);
       }
     }

@@ -233,7 +233,7 @@ IoMmuReleaseReservedSharedMem (
                    );
         ASSERT (!EFI_ERROR (Status));
       } else {
-        ASSERT (FALSE);
+        //ASSERT (FALSE);
       }
     }
   }


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#108002): https://edk2.groups.io/g/devel/message/108002
Mute This Topic: https://groups.io/mt/100256049/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-