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

[Alex Williamson]

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.

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!

Alex