From: "Ard Biesheuvel" <ardb@kernel.org>
To: Gerd Hoffmann <kraxel@redhat.com>
Cc: Pedro Falcato <pedro.falcato@gmail.com>,
devel@edk2.groups.io, Jiewen Yao <jiewen.yao@intel.com>,
Michael Brown <mcb30@ipxe.org>,
Tom Lendacky <thomas.lendacky@amd.com>,
Michael Roth <michael.roth@amd.com>
Subject: Re: [edk2-devel] [RFC/RFT PATCH] OvmfPkg/IoMmuDxe: don't rely on TPLs for re-entrancy
Date: Thu, 24 Aug 2023 12:31:32 +0200 [thread overview]
Message-ID: <CAMj1kXE-B4_T+kAMpmS0ZX2K6Jb08kn+2dMm1KgYeCm8sE_dPQ@mail.gmail.com> (raw)
In-Reply-To: <vgcfdomhc7iqee2a75sgh6lynh5zv2xazsd7ouydybupq5btct@szcf66ar5etf>
On Thu, 24 Aug 2023 at 10:06, Gerd Hoffmann <kraxel@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@kernel.org> wrote:
> > >
> > > On Wed, 23 Aug 2023 at 13:08, Gerd Hoffmann <kraxel@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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
next prev parent reply other threads:[~2023-08-24 10:31 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-07-20 13:45 [edk2-devel] [RFC/RFT PATCH] OvmfPkg/IoMmuDxe: don't rely on TPLs for re-entrancy Ard Biesheuvel
2023-07-21 22:55 ` Pedro Falcato
2023-07-24 14:21 ` Ard Biesheuvel
2023-07-24 17:08 ` Pedro Falcato
2023-08-22 6:25 ` Gerd Hoffmann
2023-08-22 7:57 ` Ard Biesheuvel
2023-08-23 11:07 ` Gerd Hoffmann
2023-08-23 15:11 ` Ard Biesheuvel
2023-08-23 17:18 ` Gerd Hoffmann
2023-08-23 18:10 ` Pedro Falcato
2023-08-24 8:06 ` Gerd Hoffmann
2023-08-24 10:31 ` Ard Biesheuvel [this message]
2023-08-28 9:16 ` Gerd Hoffmann
2023-08-28 11:13 ` Ard Biesheuvel
2023-08-31 16:01 ` Ard Biesheuvel
2023-09-04 11:45 ` Gerd Hoffmann
2023-09-04 12:06 ` Ard Biesheuvel
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAMj1kXE-B4_T+kAMpmS0ZX2K6Jb08kn+2dMm1KgYeCm8sE_dPQ@mail.gmail.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox