Re: [Qemu-devel] [PATCH v13 09/13] add check reset mechanism when hotplug vfio device

[Cao jin]

On 11/12/2015 07:51 PM, Michael S. Tsirkin wrote:
> On Wed, Nov 11, 2015 at 06:34:27PM +0800, Cao jin wrote:
> > From: Chen Fan <address@hidden>
> >
> > Since we support multi-function hotplug. the function 0 indicate
> > the closure of the slot, so we have the chance to do the check.
> >
> > Signed-off-by: Chen Fan <address@hidden>
> > ---
> >   hw/pci/pci.c             | 29 +++++++++++++++++++++++++++++
> >   hw/vfio/pci.c            | 19 +++++++++++++++++++
> >   hw/vfio/pci.h            |  2 ++
> >   include/hw/pci/pci_bus.h |  5 +++++
> >   4 files changed, 55 insertions(+)
> >
> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > index 168b9cc..f6ca6ef 100644
> > --- a/hw/pci/pci.c
> > +++ b/hw/pci/pci.c
> > @@ -81,6 +81,7 @@ static void pci_bus_realize(BusState *qbus, Error **errp)
> >       PCIBus *bus = PCI_BUS(qbus);
> >
> >       vmstate_register(NULL, -1, &vmstate_pcibus, bus);
> > +    notifier_with_return_list_init(&bus->hotplug_notifiers);
> >   }
> >
> >   static void pci_bus_unrealize(BusState *qbus, Error **errp)
> > @@ -1835,6 +1836,22 @@ PCIDevice *pci_find_device(PCIBus *bus, int bus_num, 
> > uint8_t devfn)
> >       return bus->devices[devfn];
> >   }
> >
> > +void pci_bus_add_hotplug_notifier(PCIBus *bus, NotifierWithReturn *notify)
> > +{
> > +    notifier_with_return_list_add(&bus->hotplug_notifiers, notify);
> > +}
> > +
> > +void pci_bus_remove_hotplug_notifier(NotifierWithReturn *notifier)
> > +{
> > +    notifier_with_return_remove(notifier);
> > +}
> > +
> > +static int pci_bus_hotplug_notifier(PCIBus *bus, void *opaque)
> > +{
> > +    return notifier_with_return_list_notify(&bus->hotplug_notifiers,
> > +                                            opaque);
> > +}
> > +
> >   static void pci_qdev_realize(DeviceState *qdev, Error **errp)
> >   {
> >       PCIDevice *pci_dev = (PCIDevice *)qdev;
> > @@ -1877,6 +1894,18 @@ static void pci_qdev_realize(DeviceState *qdev, Error 
> > **errp)
> >           pci_qdev_unrealize(DEVICE(pci_dev), NULL);
> >           return;
> >       }
> > +
> > +    /*
> > +     *  If the function is func 0, indicate the closure of the slot.
> > +     *  signal the callback.
> > +     */
> > +    if (DEVICE(pci_dev)->hotplugged &&
> > +        pci_get_function_0(pci_dev) == pci_dev &&
> > +        pci_bus_hotplug_notifier(bus, pci_dev)) {
> > +        error_setg(errp, "failed to hotplug function 0");
> > +        pci_qdev_unrealize(DEVICE(pci_dev), NULL);
> > +        return;
> > +    }
>
> I don't understand why this is required in pci core.
> PCI Device is already constructed anyway.
> Just do the checks and call unrealize in vfio.

Because when do multi-function hotplug, the function 0 on the pcie bus 
probably is not a vfio device. so we should trigger the check from pci 
core.

> I also don't see why you are tying this to hotplug.
> I would check each function as it's added.
> But that's a vfio thing, if both you and Alex think
> it's a good idea, fine by me.

The device is  initialized one by one no matter it is cold plugged or 
hot plugged, but for the vfio with aer that need to get depended devices 
required by bus reset, so need to make sure the reset depended devices 
are assigned to qemu, in vfio, there is a machine done callback to check 
the bus reset for boot up, so it also should be done in hotplug。

it looks little complicated, Alex, any idea?

> >   }
> >
> >   static void pci_default_realize(PCIDevice *dev, Error **errp)
> > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> > index 31ffd44..e619998 100644
> > --- a/hw/vfio/pci.c
> > +++ b/hw/vfio/pci.c
> > @@ -1990,6 +1990,19 @@ static int vfio_check_devices_host_bus_reset(void)
> >       return 0;
> >   }
> >
> > +static int vfio_check_bus_reset(NotifierWithReturn *n, void *opaque)
> > +{
> > +    VFIOPCIDevice *vdev = container_of(n, VFIOPCIDevice, hotplug_notifier);
> > +    PCIDevice *pci_dev = PCI_DEVICE(vdev);
> > +    PCIDevice *pci_func0 = opaque;
> > +
> > +    if (pci_get_function_0(pci_dev) != pci_func0) {
> > +        return 0;
> > +    }
> > +
> > +    return vfio_check_host_bus_reset(vdev);
> > +}
> > +
> >   static int vfio_setup_aer(VFIOPCIDevice *vdev, uint8_t cap_ver,
> >                             int pos, uint16_t size)
> >   {
> > @@ -2044,6 +2057,9 @@ static int vfio_setup_aer(VFIOPCIDevice *vdev, uint8_t 
> > cap_ver,
> >           return ret;
> >       }
> >
> > +    vdev->hotplug_notifier.notify = vfio_check_bus_reset;
> > +    pci_bus_add_hotplug_notifier(pdev->bus, &vdev->hotplug_notifier);
> > +
> >       return 0;
> >
> >   error:
> > @@ -2919,6 +2935,9 @@ 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);
> > +    if (vdev->features & VFIO_FEATURE_ENABLE_AER) {
> > +        pci_bus_remove_hotplug_notifier(&vdev->hotplug_notifier);
> > +    }
> >       vfio_disable_interrupts(vdev);
> >       if (vdev->intx.mmap_timer) {
> >           timer_free(vdev->intx.mmap_timer);
> > diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
> > index 59ae194..b385f07 100644
> > --- a/hw/vfio/pci.h
> > +++ b/hw/vfio/pci.h
> > @@ -142,6 +142,8 @@ typedef struct VFIOPCIDevice {
> >       bool no_kvm_intx;
> >       bool no_kvm_msi;
> >       bool no_kvm_msix;
> > +
> > +    NotifierWithReturn hotplug_notifier;
> >   } VFIOPCIDevice;
> >
> >   uint32_t vfio_pci_read_config(PCIDevice *pdev, uint32_t addr, int len);
> > diff --git a/include/hw/pci/pci_bus.h b/include/hw/pci/pci_bus.h
> > index 403fec6..7812fa9 100644
> > --- a/include/hw/pci/pci_bus.h
> > +++ b/include/hw/pci/pci_bus.h
> > @@ -39,8 +39,13 @@ struct PCIBus {
> >          Keep a count of the number of devices with raised IRQs.  */
> >       int nirq;
> >       int *irq_count;
> > +
> > +    NotifierWithReturnList hotplug_notifiers;
> >   };
> >
> > +void pci_bus_add_hotplug_notifier(PCIBus *bus, NotifierWithReturn *notify);
> > +void pci_bus_remove_hotplug_notifier(NotifierWithReturn *notify);
> > +
> >   typedef struct PCIBridgeWindows PCIBridgeWindows;
> >
> >   /*
> > --
> > 1.9.3
> .

--
Yours Sincerely,

Cao Jin