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

[Cédric Le Goater]

On 20/11/2019 05:17, David Gibson wrote:
> 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. 

Yes. this was replaced by :

  http://patchwork.ozlabs.org/cover/1181522/

> Does that mean that my 5/5 patch should be good now?

We should.

C.