Re: [PULL 4/8] vfio/pci: Respond to KVM irqchip change notifier

[Eduardo Habkost]

On Tue, Nov 26, 2019 at 05:01:47PM +1100, David Gibson wrote:
> VFIO PCI devices already respond to the pci intx routing notifier, in order
> to update kernel irqchip mappings when routing is updated.  However this
> won't handle the case where the irqchip itself is replaced by a different
> model while retaining the same routing.  This case can happen on
> the pseries machine type due to PAPR feature negotiation.
> 
> To handle that case, add a handler for the irqchip change notifier, which
> does much the same thing as the routing notifier, but is unconditional,
> rather than being a no-op when the routing hasn't changed.
> 
> Cc: Alex Williamson <address@hidden>
> Cc: Alexey Kardashevskiy <address@hidden>
> 
> Signed-off-by: David Gibson <address@hidden>
> Tested-by: Alex Williamson <address@hidden>
> Reviewed-by: Alex Williamson <address@hidden>
> Reviewed-by: Greg Kurz <address@hidden>
> Acked-by: Alex Williamson <address@hidden>
> ---
[...]
> @@ -2973,30 +2981,32 @@ static void vfio_realize(PCIDevice *pdev, Error 
> **errp)
>                                                    vfio_intx_mmap_enable, 
> vdev);
>          pci_device_set_intx_routing_notifier(&vdev->pdev,
>                                               vfio_intx_routing_notifier);
> +        vdev->irqchip_change_notifier.notify = vfio_irqchip_change;
> +        kvm_irqchip_add_change_notifier(&vdev->irqchip_change_notifier);

This code is conditional on
    (vfio_pci_read_config(&vdev->pdev, PCI_INTERRUPT_PIN, 1)).

However:

[...]
> -out_teardown:
> +out_deregister:
>      pci_device_set_intx_routing_notifier(&vdev->pdev, NULL);
> +    kvm_irqchip_remove_change_notifier(&vdev->irqchip_change_notifier);
> +out_teardown:
>      vfio_teardown_msi(vdev);
>      vfio_bars_exit(vdev);
>  error:
> @@ -3064,6 +3076,7 @@ static void vfio_exitfn(PCIDevice *pdev)
>      vfio_unregister_req_notifier(vdev);
>      vfio_unregister_err_notifier(vdev);
>      pci_device_set_intx_routing_notifier(&vdev->pdev, NULL);
> +    kvm_irqchip_remove_change_notifier(&vdev->irqchip_change_notifier);

This is unconditional.  This doesn't look safe, and might be the
cause of the crash reported at
https://bugzilla.redhat.com/show_bug.cgi?id=1782678

-- 
Eduardo