Re: [Qemu-devel] [RFC v7 04/11] vfio: add check host bus reset is support or not

[Alex Williamson]

On Tue, 2015-05-19 at 12:42 +0800, Chen Fan wrote:
> when machine is done, we should check the all vfio devices
> whether support host bus reset, then when need virtual secondary
> bus reset, we should reset host bus.
> 
> Signed-off-by: Chen Fan <address@hidden>
> ---
>  hw/vfio/pci.c | 83 
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 83 insertions(+)
> 
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 43869e9..ff639db 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -168,6 +168,7 @@ typedef struct VFIOPCIDevice {
>      bool req_enabled;
>      bool has_flr;
>      bool has_pm_reset;
> +    bool has_bus_reset;
>      bool rom_read_failed;
>  } VFIOPCIDevice;
>  
> @@ -3533,6 +3534,82 @@ static void vfio_pci_host_needs_bus_reset(Notifier *n, 
> void *opaque)
>      vbasedev->needs_bus_reset = true;
>  }
>  

This function really needs some comments

> +static void vfio_check_host_bus_reset(VFIODevice *vbasedev)
> +{
> +    VFIOPCIDevice *vdev = container_of(vbasedev, VFIOPCIDevice, vbasedev);
> +    struct vfio_pci_hot_reset_info *info = NULL;
> +    struct vfio_pci_dependent_device *devices;
> +    VFIOGroup *group;
> +    int ret, i;
> +
> +    ret = vfio_get_hot_reset_info(vdev, &info);
> +    if (ret < 0) {
> +        goto out;
> +    }
> +
> +    devices = &info->devices[0];

So we have the list of host devices affected by a bus reset...

> +
> +    /* Verify that we have all the groups required */
> +    for (i = 0; i < info->count; i++) {
> +        PCIHostDeviceAddress host;
> +        VFIOPCIDevice *tmp;
> +        VFIODevice *vbasedev_iter;
> +
> +        host.domain = devices[i].segment;
> +        host.bus = devices[i].bus;
> +        host.slot = PCI_SLOT(devices[i].devfn);
> +        host.function = PCI_FUNC(devices[i].devfn);
> +

Skip the current device

> +        if (vfio_pci_host_match(&host, &vdev->host)) {
> +            continue;
> +        }
> +

Make sure we own the group for the device

> +        QLIST_FOREACH(group, &vfio_group_list, next) {
> +            if (group->groupid == devices[i].group_id) {
> +                break;
> +            }
> +        }
> +
> +        if (!group) {
> +            goto out;
> +        }
> +

Search the group device list for the device, if found make sure it's on
the same virtual bus as the target device...  This shouldn't be limited
to the group device list, it should be done for all vfio-pci devices.
For instance, a dual-port card could have 2 separate physical functions
that are each isolated and appear in different groups.  Obviously a bus
reset will affect both.  Anything affected by the reset needs to be on
(or below) the bus of the target device.  That also exposes another
issue that I don't see addressed here; a physical bus reset will cascade
to all subordinate buses, but your notifier only works via access to the
bus reset bit in the control register.  Do we also need to make sure the
reset function for PCI bridges triggers the notify?

Also, this assumes that the host and guest PCI configuration is static,
which is not necessarily true for either.  So I think that caching the
bus reset capability is wrong.

> +        QLIST_FOREACH(vbasedev_iter, &group->device_list, next) {
> +            if (vbasedev_iter->type != VFIO_DEVICE_TYPE_PCI) {
> +                continue;
> +            }
> +            tmp = container_of(vbasedev_iter, VFIOPCIDevice, vbasedev);
> +            if (vfio_pci_host_match(&host, &tmp->host)) {
> +                if (PCI_BUS(vdev->pdev.bus) !=
> +                    PCI_BUS(tmp->pdev.bus)) {
> +                    goto out;
> +                }
> +            }
> +        }
> +    }
> +
> +    vdev->has_bus_reset = true;
> +
> +out:
> +    g_free(info);
> +}
> +
> +static void vfio_pci_machine_done_notify(Notifier *notifier, void *unused)
> +{
> +    VFIOGroup *group;
> +    VFIODevice *vbasedev;
> +
> +    QLIST_FOREACH(group, &vfio_group_list, next) {
> +         QLIST_FOREACH(vbasedev, &group->device_list, next) {
> +             vfio_check_host_bus_reset(vbasedev);
> +         }
> +    }
> +}
> +
> +static Notifier machine_notifier = {
> +    .notify = vfio_pci_machine_done_notify,
> +};
> +
>  static int vfio_initfn(PCIDevice *pdev)
>  {
>      VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
> @@ -3821,6 +3898,12 @@ static const TypeInfo vfio_pci_dev_info = {
>  static void register_vfio_pci_dev_type(void)
>  {
>      type_register_static(&vfio_pci_dev_info);
> +
> +    /*
> +     * Register notifier when machine init is done, since we need
> +     * check the configration manner after all vfio device are inited.
> +     */
> +    qemu_add_machine_init_done_notifier(&machine_notifier);
>  }
>  
>  type_init(register_vfio_pci_dev_type)