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

[David Gibson]

On Thu, Oct 17, 2019 at 03:42:06PM +0200, Cédric Le Goater wrote:
> On 17/10/2019 10:43, Cédric Le Goater 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.
> > 
> > We have to find something else.
> 
> Here is a possible fix for the (re)setting of the OS CAM line. 
> 
> Removing spapr_xive_set_tctx_os_cam() is a good thing but this solution
> shows some issues in our modeling of hot-plugged CPUS with a reset() 
> being called at realize().

Ok, I've applied the patch below now.  Does that mean that my 5/5
patch should be good now?

-- 
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

signature.asc
Description: PGP signature