Re: [Qemu-devel] [v15 12/15] vfio: add bus in reset flag

[Chen Fan]

On 01/06/2016 03:58 AM, Alex Williamson wrote:
> On Tue, 2016-01-05 at 09:20 +0800, Cao jin wrote:
> > From: Chen Fan <address@hidden>
> >
> > mark the host bus be in reset. avoid multiple devices trigger the
> > host bus reset many times.
> >
> > Signed-off-by: Chen Fan <address@hidden>
> > ---
> >   hw/vfio/pci.c                 | 6 ++++++
> >   include/hw/vfio/vfio-common.h | 1 +
> >   2 files changed, 7 insertions(+)
> >
> > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> > index ee88db3..aa0d945 100644
> > --- a/hw/vfio/pci.c
> > +++ b/hw/vfio/pci.c
> > @@ -2249,6 +2249,11 @@ static int vfio_pci_hot_reset(VFIOPCIDevice
> > *vdev, bool single)
> >   
> >       trace_vfio_pci_hot_reset(vdev->vbasedev.name, single ? "one" :
> > "multi");
> >   
> > +    if (vdev->vbasedev.bus_in_reset) {
> > +        vdev->vbasedev.bus_in_reset = false;
> > +        return 0;
> > +    }
> > +
> >       vfio_pci_pre_reset(vdev);
> >       vdev->vbasedev.needs_reset = false;
> >   
> > @@ -2312,6 +2317,7 @@ static int vfio_pci_hot_reset(VFIOPCIDevice
> > *vdev, bool single)
> >                   }
> >                   vfio_pci_pre_reset(tmp);
> >                   tmp->vbasedev.needs_reset = false;
> > +                tmp->vbasedev.bus_in_reset = true;
> >                   multi = true;
> >                   break;
> >               }
> > diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-
> > common.h
> > index f037f3c..44b19d7 100644
> > --- a/include/hw/vfio/vfio-common.h
> > +++ b/include/hw/vfio/vfio-common.h
> > @@ -95,6 +95,7 @@ typedef struct VFIODevice {
> >       bool reset_works;
> >       bool needs_reset;
> >       bool no_mmap;
> > +    bool bus_in_reset;
> >       VFIODeviceOps *ops;
> >       unsigned int num_irqs;
> >       unsigned int num_regions;
> I imagine this should be a VFIOPCIDevice field, it has no use in the
> common code.  The name is also a bit confusing; when I suggested a
> bus_in_reset flag, I was referring to a property on the bus itself that
> the existing device_reset could query to switch modes rather than add a
> separate callback as you've done in this series.  This works, but it's
> perhaps more intrusive than I was thinking.  It will need to get
> approval by qdev folks.
maybe I don't get your point. I just think add a bus_in_reset flag in bus
has no much sense. for instance, if assigning device A and B from
different host bus into a same virtual bus. assume all check passed.
then if device A aer occurs. we should reset virtual bus to recover
the device A, we also need to reset the device B and do device B host
bus reset. but here the bus_in_reset just denote the device B not need
to do host bus reset, it's incorrect. right?

> In any case, this bus_in_reset field is tracking whether a device has
> already been reset as part of a hot reset, sort of a more bus-based
> version with opposite polarity of needs_reset.  It doesn't actually
> track the bus reset state at all, it tracks whether we should skip the
> next call to hot reset for that device.  So it should probably be
> something like VFIOPCIDevice.skip_hot_reset (though that's not a great
> name either).
>
> I also wonder if a "hot" reset callback in qbus is really too PCI
> centered, should it just be "bus_reset"?
>
> Finally, it would be great if you could mention in the cover email
> which patches are new or more than superficially modified from the
> previous version so we can more easily focus on the new code to review.
>   Thanks!

Oh I am sorry, thanks for your mention. I will detail the change
from next version.

Thanks,
Chen

> Alex
>
>
> .