Re: [PATCH v4 18/19] spapr: Handle irq backend changes with VFIO PCI devices

[David Gibson]

On Wed, Oct 09, 2019 at 05:08:17PM +1100, David Gibson wrote:
> pseries machine type can have one of two different interrupt controllers in
> use depending on feature negotiation with the guest.  Usually this is
> invisible to devices, because they route to a common set of qemu_irqs which
> in turn dispatch to the correct back end.
> 
> VFIO passthrough devices, however, wire themselves up directly to the KVM
> irqchip for performance, which means they are affected by this change in
> interrupt controller.
> 
> Luckily, there's a notifier chain that will tell VFIO devices to update
> their mappings - we just need to call it whenever the intc backend might
> change.
> 
> In addition, we make sure we set an active intc earlier, because otherwise
> vfio can issue a false warning, because it doesn't think a KVM irqchip is
> in use (which is essentially for good INTx performance).
> 
> Cc: Alex Williamson <address@hidden>
> Cc: Alexey Kardashevskiy <address@hidden>
> 
> Signed-off-by: David Gibson <address@hidden>

For reference, the reason I described this as RFC only in the cover
letter, is that this doesn't work as it stands.

With this patch, when we switch intc we call the intx_routing
notifiers, and vfio_intx_update() in particular.  However that exits
early without calling vfio_intx_enable_kvm() - the bit we actually
need - because the test on pci_intx_route_changed() thinks there's
nothing to do.  The difficulty is that our use case isn't really the
same as the x86 one this notifier path was designed for: they're
changing routing of INTx to global interrupts.  For us the routing to
global interrupts remains the same, but the interrupt controller
handling those global interrupts has changed.

> ---
>  hw/ppc/spapr_irq.c     | 6 ++++++
>  hw/ppc/spapr_pci.c     | 9 +++++++++
>  include/hw/ppc/spapr.h | 1 +
>  3 files changed, 16 insertions(+)
> 
> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> index 473fc8780a..7964e4a1b8 100644
> --- a/hw/ppc/spapr_irq.c
> +++ b/hw/ppc/spapr_irq.c
> @@ -409,6 +409,12 @@ static void set_active_intc(SpaprMachineState *spapr,
>      }
>  
>      spapr->active_intc = new_intc;
> +
> +    /*
> +     * We've changed the interrupt routing at the KVM level, let VFIO
> +     * devices know they need to readjust.
> +     */
> +    spapr_pci_fire_intx_routing_notifiers(spapr);
>  }
>  
>  void spapr_irq_update_active_intc(SpaprMachineState *spapr)
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index cc0e7829b6..3bcf6325d4 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -93,6 +93,15 @@ PCIDevice *spapr_pci_find_dev(SpaprMachineState *spapr, 
> uint64_t buid,
>      return pci_find_device(phb->bus, bus_num, devfn);
>  }
>  
> +void spapr_pci_fire_intx_routing_notifiers(SpaprMachineState *spapr)
> +{
> +    SpaprPhbState *sphb;
> +
> +    QLIST_FOREACH(sphb, &spapr->phbs, list) {
> +        pci_bus_fire_intx_routing_notifier(PCI_HOST_BRIDGE(sphb)->bus);
> +    }
> +}
> +
>  static uint32_t rtas_pci_cfgaddr(uint32_t arg)
>  {
>      /* This handles the encoding of extended config space addresses */
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index d3b4dd7de3..66b68fdd5e 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -805,6 +805,7 @@ void spapr_clear_pending_events(SpaprMachineState *spapr);
>  int spapr_max_server_number(SpaprMachineState *spapr);
>  void spapr_store_hpte(PowerPCCPU *cpu, hwaddr ptex,
>                        uint64_t pte0, uint64_t pte1);
> +void spapr_pci_fire_intx_routing_notifiers(SpaprMachineState *spapr);
>  
>  /* DRC callbacks. */
>  void spapr_core_release(DeviceState *dev);

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