From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33530) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aGXkS-0006D8-DG for qemu-devel@nongnu.org; Tue, 05 Jan 2016 14:58:29 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aGXkN-0000Vw-PL for qemu-devel@nongnu.org; Tue, 05 Jan 2016 14:58:28 -0500 Received: from mx1.redhat.com ([209.132.183.28]:56238) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aGXkN-0000VT-Ik for qemu-devel@nongnu.org; Tue, 05 Jan 2016 14:58:23 -0500 Message-ID: <1452023901.22223.48.camel@redhat.com> From: Alex Williamson Date: Tue, 05 Jan 2016 12:58:21 -0700 In-Reply-To: <93bca47b31cf1abd661fbe778d415abbb08b680a.1451901804.git.chen.fan.fnst@cn.fujitsu.com> References: <93bca47b31cf1abd661fbe778d415abbb08b680a.1451901804.git.chen.fan.fnst@cn.fujitsu.com> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [v15 12/15] vfio: add bus in reset flag List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Cao jin , qemu-devel@nongnu.org Cc: chen.fan.fnst@cn.fujitsu.com, mst@redhat.com On Tue, 2016-01-05 at 09:20 +0800, Cao jin wrote: > From: Chen Fan >=20 > mark the host bus be in reset. avoid multiple devices trigger the > host bus reset many times. >=20 > Signed-off-by: Chen Fan > --- > =C2=A0hw/vfio/pci.c=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0| 6 ++++++ > =C2=A0include/hw/vfio/vfio-common.h | 1 + > =C2=A02 files changed, 7 insertions(+) >=20 > 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) > =C2=A0 > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0trace_vfio_pci_hot_reset(vdev->vbasedev.n= ame, single ? "one" : > "multi"); > =C2=A0 > +=C2=A0=C2=A0=C2=A0=C2=A0if (vdev->vbasedev.bus_in_reset) { > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0vdev->vbasedev.bus_in_= reset =3D false; > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0return 0; > +=C2=A0=C2=A0=C2=A0=C2=A0} > + > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0vfio_pci_pre_reset(vdev); > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0vdev->vbasedev.needs_reset =3D false; > =C2=A0 > @@ -2312,6 +2317,7 @@ static int vfio_pci_hot_reset(VFIOPCIDevice > *vdev, bool single) > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0} > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0vfio_pci_pre_reset(tmp); > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0tmp->vbasedev.needs_reset =3D false; > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0tmp->vbasedev.bus_in_reset =3D true; > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0multi =3D true; > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0break; > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0} > 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 { > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0bool reset_works; > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0bool needs_reset; > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0bool no_mmap; > +=C2=A0=C2=A0=C2=A0=C2=A0bool bus_in_reset; > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0VFIODeviceOps *ops; > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0unsigned int num_irqs; > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0unsigned int num_regions; I imagine this should be a VFIOPCIDevice field, it has no use in the common code. =C2=A0The 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. =C2=A0This works, but it= 's perhaps more intrusive than I was thinking. =C2=A0It 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. =C2=A0It 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. =C2=A0So 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. =C2=A0Thanks! Alex