Re: [Qemu-devel] [PATCH v2 09/11] vfio: vote a device to do host bus reset

[Chen Fan]

On 03/10/2016 12:37 AM, Alex Williamson wrote:
> On Mon, 7 Mar 2016 11:23:02 +0800
> Cao jin <address@hidden> wrote:
>
> > From: Chen Fan <address@hidden>
> >
> > Signed-off-by: Chen Fan <address@hidden>
> > ---
> >   hw/vfio/pci.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >   hw/vfio/pci.h |  1 +
> >   2 files changed, 58 insertions(+)
> >
> > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> > index 24848c9..8e902d2 100644
> > --- a/hw/vfio/pci.c
> > +++ b/hw/vfio/pci.c
> > @@ -1937,6 +1937,8 @@ static void vfio_check_host_bus_reset(VFIOPCIDevice 
> > *vdev, Error **errp)
> >       /* List all affected devices by bus reset */
> >       devices = &info->devices[0];
> >   
> > +    vdev->single_depend_dev = (info->count == 1);
> > +
> >       /* Verify that we have all the groups required */
> >       for (i = 0; i < info->count; i++) {
> >           PCIHostDeviceAddress host;
> > @@ -2998,6 +3000,49 @@ static void vfio_exitfn(PCIDevice *pdev)
> >       vfio_unregister_bars(vdev);
> >   }
> >   
> > +static VFIOPCIDevice *vfio_pci_get_do_reset_device(VFIOPCIDevice *vdev)
> > +{
> > +    struct vfio_pci_hot_reset_info *info = NULL;
> > +    struct vfio_pci_dependent_device *devices;
> > +    VFIOGroup *group;
> > +    VFIODevice *vbasedev_iter;
> > +    int ret;
> > +
> > +    ret = vfio_get_hot_reset_info(vdev, &info);
> > +    if (ret) {
> > +        error_report("vfio: Cannot enable AER for device %s,"
> > +                     " device does not support hot reset.",
> > +                     vdev->vbasedev.name);
> > +        return NULL;
> Isn't this path called for all devices, not just those trying to
> support AER?
>
> > +    }
> > +
> > +    devices = &info->devices[0];
> > +
> > +    QLIST_FOREACH(group, &vfio_group_list, next) {
> > +        if (group->groupid == devices[0].group_id) {
> > +            break;
> > +        }
> > +    }
> > +
> > +    if (!group) {
> > +        error_report("vfio: Cannot enable AER for device %s, "
> > +                    "depends on group %d which is not owned.",
> > +                    vdev->vbasedev.name, devices[0].group_id);
> > +        return NULL;
> Same here, we're not in an AER specific code path and we haven't even
> tested if the device is trying to support AER.
>
> > +    }
> > +
> > +    QLIST_FOREACH(vbasedev_iter, &group->device_list, next) {
> > +        if (vbasedev_iter->type != VFIO_DEVICE_TYPE_PCI ||
> > +            !(vdev->features & VFIO_FEATURE_ENABLE_AER)) {
> > +            continue;
> > +        }
> > +
> > +        return container_of(vbasedev_iter, VFIOPCIDevice, vbasedev);
> So the reset device is simply the first device in the group list, this
> needs to be documented, but DMA isolation (grouping) and bus isolation
> are separate things and we can have more than one group per bus...
> often an IOMMU group is a single device, so have we really solved
> anything here?  If the next device is on the same bus but in a separate
> group, we're going to hot reset again, right?  Thanks,
You are right, I recall my original idea was not something what like 
this. :)
I tend to favor MST's ideas which said in V1 series patchset:
"If the assumption that vfio functions are combined
in the same way as on the host, then this is not needed:
just reset when function 0 is reset."

so, seems we could implement this in next version.
1. requiring functions on same host bus must were assigned on the same 
virtual
    bus. here we should not have a subordinate bus. because nowadays bridge
    do not support pass through. so we can simply reset the vfio device 
with the
    smallest devfn in a bus reset as long as one device has aer on the bus.

    with this, we don't need to care the groups isolation, because we 
only need the devices
    by a bus hot reset, if the devices all on one virtual bus. reset 
anyone is sufficient.

2. in order to identify the reset is from a normal reset or aer recovery 
reset, we could
    add a flags in vfio device to mark the aer have occurred in 
err_notifier_handle, if
    subsequent reset is coming with the is_bus_rst in parent bus is 
true. we can
    directly do the hot reset.

Thanks,
Chen

> Alex
>
> > +    }
> > +
> > +    return NULL;
> > +}
> > +
> >   static void vfio_pci_reset(DeviceState *dev)
> >   {
> >       PCIDevice *pdev = DO_UPCAST(PCIDevice, qdev, dev);
> > @@ -3005,6 +3050,18 @@ static void vfio_pci_reset(DeviceState *dev)
> >   
> >       trace_vfio_pci_reset(vdev->vbasedev.name);
> >   
> > +    if (pdev->bus->in_hot_reset) {
> > +        VFIOPCIDevice *tmp;
> > +
> > +        tmp = vfio_pci_get_do_reset_device(vdev);
> > +        if (tmp) {
> > +            if (tmp == vdev) {
> > +                vfio_pci_hot_reset(vdev, vdev->single_depend_dev);
> > +            }
> > +            return;
> > +        }
> > +    }
> > +
> >       vfio_pci_pre_reset(vdev);
> >   
> >       if (vdev->resetfn && !vdev->resetfn(vdev)) {
> > diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
> > index aff46c2..32bd31f 100644
> > --- a/hw/vfio/pci.h
> > +++ b/hw/vfio/pci.h
> > @@ -143,6 +143,7 @@ typedef struct VFIOPCIDevice {
> >       bool no_kvm_intx;
> >       bool no_kvm_msi;
> >       bool no_kvm_msix;
> > +    bool single_depend_dev;
> >   } VFIOPCIDevice;
> >   
> >   uint32_t vfio_pci_read_config(PCIDevice *pdev, uint32_t addr, int len);
>
>
> .