Re: [PATCH v4 19/19] spapr: Work around spurious warnings from vfio INTx initialization

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3431 bytes --]

On Wed, Oct 09, 2019 at 10:37:38AM +0200, Greg Kurz wrote:
> On Wed,  9 Oct 2019 17:08:18 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > Traditional PCI INTx for vfio devices can only perform well if using
> > an in-kernel irqchip.  Therefore, vfio_intx_update() issues a warning
> > if an in kernel irqchip is not available.
> 
> Can you elaborate on what doesn't "perform well" without an
> in-kernel irqchip ?

Not really, no, but Alex Williamson tells me it is soo.

> Is it a matter of performance or is it
> actually broken or something else ?

I believe it's a matter of performance, but such a big one that it
makes using it without kernel irqchip infeasible in many cases.

> What is the impact on -machine accel=kvm,kernel-irqchip=off which
> always spit this warning ?

It should still spit that warning.

> > We usually do have an in-kernel irqchip available for pseries machines
> > on POWER hosts.  However, because the platform allows feature
> > negotiation of what interrupt controller model to use, we don't
> > currently initialize it until machine reset.  vfio_intx_update() is
> > called (first) from vfio_realize() before that, so it can issue a
> > spurious warning, even if we will have an in kernel irqchip by the
> > time we need it.
> > 
> > To workaround this, make a call to spapr_irq_update_active_intc() from
> > spapr_irq_init() which is called at machine realize time, before the
> > vfio realize.  This call will be pretty much obsoleted by the later
> > call at reset time, but it serves to suppress the spurious warning
> > from VFIO.
> > 
> > Cc: Alex Williamson <alex.williamson@redhat.com>
> > Cc: Alexey Kardashevskiy <aik@ozlabs.ru>
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  hw/ppc/spapr_irq.c | 11 ++++++++++-
> >  1 file changed, 10 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> > index 7964e4a1b8..3aeb523f3e 100644
> > --- a/hw/ppc/spapr_irq.c
> > +++ b/hw/ppc/spapr_irq.c
> > @@ -274,6 +274,14 @@ void spapr_irq_init(SpaprMachineState *spapr, Error **errp)
> >  
> >      spapr->qirqs = qemu_allocate_irqs(spapr_set_irq, spapr,
> >                                        smc->nr_xirqs + SPAPR_XIRQ_BASE);
> > +
> > +    /*
> > +     * Mostly we don't actually need this until reset, except that not
> > +     * having this set up can cause VFIO devices to issue a
> > +     * false-positive warning during realize(), because they don't yet
> > +     * have an in-kernel irq chip.
> > +     */
> > +    spapr_irq_update_active_intc(spapr);
> >  }
> >  
> >  int spapr_irq_claim(SpaprMachineState *spapr, int irq, bool lsi, Error **errp)
> > @@ -429,7 +437,8 @@ void spapr_irq_update_active_intc(SpaprMachineState *spapr)
> >           * this.
> >           */
> >          new_intc = SPAPR_INTC(spapr->xive);
> > -    } else if (spapr_ovec_test(spapr->ov5_cas, OV5_XIVE_EXPLOIT)) {
> > +    } else if (spapr->ov5_cas
> > +               && spapr_ovec_test(spapr->ov5_cas, OV5_XIVE_EXPLOIT)) {
> >          new_intc = SPAPR_INTC(spapr->xive);
> >      } else {
> >          new_intc = SPAPR_INTC(spapr->ics);
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]