linux-kernel - Re: [PATCH 3/3] KVM: x86: Decouple APICv activation state from apicv_inhibit

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z6FVaLOsPqmAPNWu@google.com>
Date: Mon, 3 Feb 2025 15:46:48 -0800
From: Sean Christopherson <seanjc@...gle.com>
To: Paolo Bonzini <pbonzini@...hat.com>
Cc: "Naveen N Rao (AMD)" <naveen@...nel.org>, kvm@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Suravee Suthikulpanit <suravee.suthikulpanit@....com>, Vasant Hegde <vasant.hegde@....com>, 
	Maxim Levitsky <mlevitsk@...hat.com>, Vitaly Kuznetsov <vkuznets@...hat.com>
Subject: Re: [PATCH 3/3] KVM: x86: Decouple APICv activation state from apicv_inhibit_reasons

On Mon, Feb 03, 2025, Paolo Bonzini wrote:
> On 2/3/25 19:45, Sean Christopherson wrote:
> > Unless there's a very, very good reason to support a use case that generates
> > ExtInts during boot, but _only_ during boot, and otherwise doesn't have any APICv
> > ihibits, I'm leaning towards making SVM's IRQ window inhibit sticky, i.e. never
> > clear it.
> 
> BIOS tends to use PIT, so that may be too much.  With respect to Naveen's report
> of contention on apicv_update_lock, I would go with the sticky-bit idea but apply
> it to APICV_INHIBIT_REASON_PIT_REINJ.

That won't work, at least not with yet more changes, because KVM creates the
in-kernel PIT with reinjection enabled by default.  The stick-bit idea is that
if a bit is set and can never be cleared, then there's no need to track new
updates.  Since userspace needs to explicitly disable reinjection, the inhibit
can't be sticky.

I assume We could fudge around that easily enough by deferring the inhibit until
a vCPU is created (or run?), but piggybacking PIT_REINJ won't help the userspace
I/O APIC case.

> I don't love adding another inhibit reason but, together, these two should
> remove the contention on apicv_update_lock.  Another idea could be to move
> IRQWIN to per-vCPU reason but Maxim tells me that it's not so easy.

Oh, yeah, that reminds me of the other reason I would vote for a sticky flag:
if inhibition really is toggling rapidly, performance is going to be quite bad
because inhibiting APICv requires (a) zapping APIC SPTEs and (b) serializing
writers if multiple vCPUs trigger the 0=>1 transition.

And there's some amount of serialization even if there's only a single writer,
as KVM kicks all vCPUs to toggle APICv (and again to flush TLBs, if necessary).

Hmm, something doesn't add up.  Naveen's changelog says:

  KVM additionally inhibits AVIC for requesting a IRQ window every time it has
  to inject external interrupts resulting in a barrage of inhibits being set and
  cleared. This shows significant performance degradation compared to AVIC being
  disabled, due to high contention on apicv_update_lock.

But if this is a "real world" use case where the only source of ExtInt is the
PIT, and kernels typically only wire up the PIT to the BSP, why is there
contention on apicv_update_lock?  APICv isn't actually being toggled, so readers
blocking writers to handle KVM_REQ_APICV_UPDATE shouldn't be a problem.

Naveen, do you know why there's a contention on apicv_update_lock?  Are multiple
vCPUs actually trying to inject ExtInt?