On Thu, Aug 13, 2020 at 09:10:31AM +0100, Paul Durrant wrote: > > -----Original Message----- > > From: Xen-devel <[email protected]> On Behalf Of David > > Woodhouse > > Sent: 11 August 2020 14:25 > > To: Paul Durrant <[email protected]>; [email protected]; > > Roger Pau Monne > > <[email protected]> > > Cc: Eslam Elnikety <[email protected]>; Andrew Cooper > > <[email protected]>; Shan Haitao > > <[email protected]>; Jan Beulich <[email protected]> > > Subject: Re: [Xen-devel] [PATCH v2] x86/hvm: re-work viridian APIC assist > > code > > > > 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. > > > > 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. > > > > The loop appears to be there to handle the case where multiple > devices assigned to a domain have MSIs programmed with the same > dest/vector... which seems like an odd thing for a guest to do but I > guess it is at liberty to do it. Does it matter whether they are > maskable or not?
Such configuration would never work properly, as lapic vectors are edge triggered and thus can't be safely shared between devices? I think the iteration is there in order to get the hvm_pirq_dpci struct that injected that specific vector, so that you can perform the ack if required. Having lapic EOI callbacks should simply this, as you can pass a hvm_pirq_dpci when injecting a vector, and that would be forwarded to the EOI callback, so there should be no need to iterate over the list of hvm_pirq_dpci for a domain. Roger.
