Re: [Qemu-devel] [RFC v8.1 10/13] vfio: do hot bus reset when do virtual secondary bus reset

[Alex Williamson]

On Thu, 2015-06-04 at 17:57 +0800, Chen Fan wrote:
> On 05/28/2015 05:33 AM, Alex Williamson wrote:
> > On Wed, 2015-05-27 at 10:46 +0800, Chen Fan wrote:
> >> when do virtual secondary bus reset, the vfio device under
> >> this bus need to do host bus reset to reset the device.
> >> so add this case.
> >>
> >> Signed-off-by: Chen Fan <address@hidden>
> >> ---
> >>   hw/vfio/pci.c | 75 
> >> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>   1 file changed, 75 insertions(+)
> >>
> >> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> >> index a8c5988..b05ccdf 100644
> >> --- a/hw/vfio/pci.c
> >> +++ b/hw/vfio/pci.c
> >> @@ -3803,6 +3803,64 @@ out:
> >>       g_free(info);
> >>   }
> >>   
> >> +/* Check when device reset the affected devices is single or multi,
> >> + * because there was differentiate the hot reset of mulitple in-use
> >> + * devices and hot reset of a single in-use device. */
> >> +static int vfio_pci_affect_devices_is_multi(VFIOPCIDevice *vdev)
> >> +{
> >> +    VFIOGroup *group;
> >> +    struct vfio_pci_hot_reset_info *info = NULL;
> >> +    struct vfio_pci_dependent_device *devices;
> >> +    int ret, i, count = 0;
> >> +
> >> +    ret = vfio_get_hot_reset_info(vdev, &info);
> >> +    if (ret < 0) {
> >> +        goto out;
> >> +    }
> >> +
> >> +    devices = &info->devices[0];
> >> +
> >> +    /* Verify that we have all the groups required */
> >> +    for (i = 0; i < info->count; i++) {
> >> +        PCIHostDeviceAddress host;
> >> +        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;
> >> +        }
> >> +
> >> +        /* Ensure we own the group of the affected device */
> >> +        QLIST_FOREACH(group, &vfio_group_list, next) {
> >> +            if (group->groupid == devices[i].group_id) {
> >> +                break;
> >> +            }
> >> +        }
> >> +
> >> +        if (!group) {
> >> +            ret = -1;
> >> +            goto out;
> >> +        }
> >> +
> >> +        QLIST_FOREACH(vbasedev_iter, &group->device_list, next) {
> >> +            if (vbasedev_iter->type != VFIO_DEVICE_TYPE_PCI) {
> >> +                continue;
> >> +            }
> >> +            count++;
> >> +        }
> >> +    }
> >> +
> >> +    ret = count;
> >> +out:
> >> +    g_free(info);
> >> +    return ret;
> >> +}
> > This basically does almost all the work of the hot reset function just
> > to figure out whether it's a single or multi-reset to call the hot reset
> > function with the right parameter.  Why not just do a hot reset?
> 
> Because the vfio_pci_hot_reset function has a bit odd, if one group
> has two or more function, even though only one function was
> passthroughed to VM, when we need to do bus reset, we should
> call vfio_pci_hot_reset(single), but if multi devices were passed to
> VM, we should call with muliti. also we should update the affected
> devices needs_reset status when do host bus reset. we should not
> do bus reset multi-times, which vfio_pci_hot_reset has done for
> us.

Right, the current function is called via two paths, one for a
per-device reset and one for a system reset.  On a system reset, we
don't mind if we gratuitously reset other assigned or non-assigned
devices.  On a device reset, we can only do a bus reset if there are no
other affected assigned devices.  But still the problem remains that the
above code change duplicates a lot of the hot reset code just to figure
out which way to call the existing function, when it seems like the
easier and more maintainable path is to modify the existing function to
incorporate the functionality we need here.  Thanks,

Alex

> >> +
> >>   static void vfio_pci_machine_done_notify(Notifier *notifier, void 
> >> *unused)
> >>   {
> >>       VFIOGroup *group;
> >> @@ -4039,6 +4097,23 @@ static void vfio_pci_reset(DeviceState *dev)
> >>   
> >>       trace_vfio_pci_reset(vdev->vbasedev.name);
> >>   
> >> +    if (vdev->needs_bus_reset) {
> >> +        vdev->needs_bus_reset = false;
> > A comment would be nice to signal that this secondary test avoids
> > duplicate bus resets.
> >
> >> +        if (vdev->vbasedev.needs_reset) {
> >> +            int ret;
> >> +
> >> +            ret = vfio_pci_affect_devices_is_multi(vdev);
> >> +            if (ret < 0) {
> >> +                error_report("vfio: Cannot reset device 
> >> %04x:%02x:%02x.%x, "
> >> +                             "get hot reset info failed.", 
> >> vdev->host.domain,
> >> +                             vdev->host.bus, vdev->host.slot, 
> >> vdev->host.function);
> >> +            } else {
> >> +                vfio_pci_hot_reset(vdev, ret ? true : false);
> >> +            }
> >> +        }
> >> +        return;
> >> +    }
> >> +
> >>       vfio_pci_pre_reset(vdev);
> >>   
> >>       if (vdev->resetfn && !vdev->resetfn(vdev)) {
> >
> >
> > .
> >
>