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

[Alex Williamson <alex.williamson@redhat.com>]

On Wed, 2016-01-06 at 10:13 +0800, Chen Fan wrote:
> 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 <chen.fan.fnst@cn.fujitsu.com>
> > > 
> > > mark the host bus be in reset. avoid multiple devices trigger the
> > > host bus reset many times.
> > > 
> > > Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> > > ---
> > >   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?

Let's take an example of the state of this flag on the device to see
why the name doesn't make sense to me.  We have a dual port card with
devices A and B, both on the same host bus.  We start a hot reset on A
and we have the following states:

A.bus_in_reset = false
B.bus_in_reset = false

Well, that's not accurate.  As we complete the hot reset we tag device
B as already being reset:

A.bus_in_reset = false
B.bus_in_reset = true

That's not accurate either, they're both in the same bus hierarchy,
they should not be different.  Later hot reset is called on B and we're
back to:

A.bus_in_reset = false
B.bus_in_reset = false

So I agree with your algorithm, but the variable is not tracking the
state of the bus being in reset, it's tracking whether to skip the next
reset call.

The separate hot (bus) reset in DeviceClass seems unnecessary too, this
is where I think we could work entirely within the PCI code w/o new
qbus/qdev interfaces.  Imagine pci_bridge_write_config()
calls qbus_walk_children() directly instead of calling
qbus_reset_all().  The pre_busfn() could set a flag on the PCIBus to
indicate the bus is in reset.  qdev_reset_one() could be used as the
post_devfn() and the post_busfn() would call qdev_reset_one() followed
by a clear of the flag.  The modification to vfio is then simply that
if we're resetting an AER device and the bus is in reset, we know we
can do a hot reset.  It simply allows us to test which reset type is
occurring within the existing reset callback rather than adding a new
one.

Going back to my idea of a sequence ID, if we had:

bool PCIBus.bus_in_reset
uint PCIBus.bus_reset_seqid

The pre_busfn would do:

PCIBus.bus_in_reset = true
PCIBus.bus_reset_seqid++

Then we could add:

uint VFIOPCIDevice.last_bus_reset_seqid

vfio_pci_reset() would test (PCIBus.bus_in_reset && VFIOPCIDevice.AER)
to know whether to do a hot reset.  vfio_pci_hot_reset() would skip
devices for which (VFIOPCIDevice.last_bus_reset_seqid ==
PCIBus.bus_reset_seqid) and for each device reset would set
VFIOPCIDevice.last_bus_reset_seqid = PCIBus.bus_reset_seqid.

That feels like a much more deterministic solution if MST is willing to
support it in the PCI specific BusState.  Thanks,

Alex