Re: [Xen-devel] [PATCH v2] x86/hvm: re-work viridian APIC assist code

From: "Roger Pau Monné" <roger.pau@citrix.com>
To: David Woodhouse <dwmw2@infradead.org>
Cc: Paul Durrant <paul.durrant@citrix.com>,
	<xen-devel@lists.xenproject.org>,
	 Eslam Elnikety <elnikety@amazon.de>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Shan Haitao <haitao.shan@intel.com>,
	Jan Beulich <jbeulich@suse.com>
Subject: Re: [Xen-devel] [PATCH v2] x86/hvm: re-work viridian APIC assist code
Date: Wed, 12 Aug 2020 15:43:48 +0200	[thread overview]
Message-ID: <20200812134348.GB975@Air-de-Roger> (raw)
In-Reply-To: <7547be305e3ede9edb897e2be898fe54e0b4065c.camel@infradead.org>

Sorry for not replying earlier, wanted to finish something first.

On Tue, Aug 11, 2020 at 02:25:16PM +0100, David Woodhouse wrote:
> Resending this straw man patch at Roger's request, to restart discussion.
> 
> Redux: In order to cope with the relatively rare case of unmaskable
> legacy MSIs, each vlapic EOI takes a domain-global spinlock just to
> iterate over all IRQs and determine that there's actually nothing to
> do.
> 
> In my testing, I observe that this drops Windows performance on passed-
> through NVMe from 2.2M IOPS down to about 1.0M IOPS.
> 
> I have a variant of this patch which just has a single per-domain "I
> attached legacy unmaskable MSIs" flag, which is never cleared. The
> patch below is per-vector (but Roger points out it should be per-vCPU
> per-vector). I don't know that we really care enough to do more than
> the single per-domain flag, which in real life would never happen
> anyway unless you have crappy hardware, at which point you get back to
> today's status quo.

I've just posted a series [0] that should allow setting of EOI
callbacks for lapic vectors, which I think could be relevant for the
use-case here. Note the series itself doesn't change the current
behavior much, as it will still register a callback for each injected
MSI, regardless of whether it's maskable or non-maskable.

I think you could easily make a patch on top of my series that
prevents registering the EOI callback for maskable MSIs, and it would
avoid having to add a per-domain flag or anything like that.

> My main concern is that this code is fairly sparsely documented and I'm
> only 99% sure that this code path really *is* only for unmaskable MSIs,
> and doesn't have some other esoteric use. A second opinion on that
> would be particularly welcome.

That's also my reading, maskable MSIs will have ack_type set to
ACKTYPE_NONE according to irq_acktype, which will then prevent
do_IRQ_guest from setting pirq->masked and thus turn desc_guest_eoi
into a noop.

I assume this is because maskable MSIs can always be masked by Xen if
required, so there's no reason to hold them pending in the lapic. OTOH
Xen has no way to prevent non-maskable MSIs except from keeping the
vector ISR bit set.

[0] https://lore.kernel.org/xen-devel/20200812124709.4165-1-roger.pau@citrix.com/T/#mb300899b0d95d5b6e78ca8caea21482d91b1cea8

Roger.