Re: [RFC 5/5] spapr: Work around spurious warnings from vfio INTx initialization

[Greg Kurz]

On Thu, 17 Oct 2019 10:43:11 +0200
Cédric Le Goater <address@hidden> wrote:

> On 17/10/2019 07:42, David Gibson 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.
> > 
> > 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 <address@hidden>
> > Cc: Alexey Kardashevskiy <address@hidden>
> > 
> > Signed-off-by: David Gibson <address@hidden>
> > ---
> >  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 45544b8976..bb91c61fa0 100644
> > --- a/hw/ppc/spapr_irq.c
> > +++ b/hw/ppc/spapr_irq.c
> > @@ -345,6 +345,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);
> 
> This will call the de/activate hooks of the irq chip very early 
> before reset and CAS, and won't call them at reset if the intc
> pointers are the same (xive only for instance). It breaks very 
> obviously the emulated XIVE device which won't reset the OS CAM 
> line with the CPU id values and CPU notification will be broken.
> 

Yes. The problem is that we now have a new path:

spapr_irq_init()
 spapr_irq_update_active_intc()
  set_active_intc()
   spapr_xive_activate()

and we go there before the CPUs are realized:

(gdb) p cpus
$6 = {tqh_first = 0x0, tqh_circ = {tql_next = 0x0, 
    tql_prev = 0x100f203a8 <cpus>}}

spapr_xive_activate() thus doesn't reset the OS CAM line.

When the initial machine reset happens later, CPUs are
present but we don't go down the activate path since XIVE
is already active.

Commenting away the following check in set_active_intc() is enough
to fix:

    if (new_intc == spapr->active_intc) {
        /* Nothing to do */
        return;
    }

but this also reveals that handling the OS CAM line reset in
spapr_xive_activate() only may be a bit fragile since it obviously
requires CPUs to be present... Maybe this should also be done by
the CPUs on their realize path ?

> We have to find something else.
> 
> 
> >  }
> >  
> >  int spapr_irq_claim(SpaprMachineState *spapr, int irq, bool lsi, Error 
> > **errp)
> > @@ -500,7 +508,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)) {
> 
> This change can go in another patch.
> 
> C. 
> 
> >          new_intc = SPAPR_INTC(spapr->xive);
> >      } else {
> >          new_intc = SPAPR_INTC(spapr->ics);
> > 
>