From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38909) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aGrCR-00077J-8v for qemu-devel@nongnu.org; Wed, 06 Jan 2016 11:44:40 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aGrCN-0005Yc-Uj for qemu-devel@nongnu.org; Wed, 06 Jan 2016 11:44:39 -0500 Received: from mx1.redhat.com ([209.132.183.28]:54792) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aGrCN-0005YY-Mx for qemu-devel@nongnu.org; Wed, 06 Jan 2016 11:44:35 -0500 Message-ID: <1452098674.29599.106.camel@redhat.com> From: Alex Williamson Date: Wed, 06 Jan 2016 09:44:34 -0700 In-Reply-To: <568C782C.8050001@cn.fujitsu.com> References: <93bca47b31cf1abd661fbe778d415abbb08b680a.1451901804.git.chen.fan.fnst@cn.fujitsu.com> <1452023901.22223.48.camel@redhat.com> <568C782C.8050001@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: Chen Fan , Cao jin , qemu-devel@nongnu.org Cc: mst@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 > > >=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=A0 hw/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=A0 include/hw/vfio/vfio-common.h | 1 + > > > =C2=A0 2 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=A0=C2=A0=C2=A0trace_vfio_pci_hot_reset(vdev->= vbasedev.name, single ? > > > "one" : > > > "multi"); > > > =C2=A0=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=A0=C2=A0vfio_pci_pre_reset(vdev); > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0vdev->vbasedev.needs_reset =3D = false; > > > =C2=A0=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=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=A0=C2=A0tmp->vbasedev.needs_reset =3D fals= e; > > > +=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=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=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=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=A0=C2=A0bool reset_works; > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0bool needs_reset; > > > =C2=A0=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=A0=C2=A0VFIODeviceOps *ops; > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0unsigned int num_irqs; > > > =C2=A0=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=A0=C2=A0The name is also a bit confusing; when I sugg= ested 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=A0=C2=A0This work= s, but > > it's > > perhaps more intrusive than I was thinking.=C2=A0=C2=A0It will need t= o 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. =C2=A0We have a dual port card wit= h devices A and B, both on the same host bus. =C2=A0We start a hot reset on= A and we have the following states: A.bus_in_reset =3D false B.bus_in_reset =3D false Well, that's not accurate. =C2=A0As we complete the hot reset we tag devi= ce B as already being reset: A.bus_in_reset =3D false B.bus_in_reset =3D true That's not accurate either, they're both in the same bus hierarchy, they should not be different. =C2=A0Later hot reset is called on B and we= 're back to: A.bus_in_reset =3D false B.bus_in_reset =3D 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. =C2=A0Imagine=C2=A0pci_bridge_write_config() calls=C2=A0qbus_walk_children() directly instead of calling qbus_reset_all(). =C2=A0The pre_busfn() could set a flag on the PCIBus to indicate the bus is in reset. =C2=A0qdev_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. =C2=A0The modification to vfio is then simply tha= t if we're resetting an AER device and the bus is in reset, we know we can do a hot reset. =C2=A0It 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 =3D 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. =C2=A0vfio_pci_hot_reset() would skip devices for which (VFIOPCIDevice.last_bus_reset_seqid =3D=3D PCIBus.bus_reset_seqid) and for each device reset would set VFIOPCIDevice.last_bus_reset_seqid =3D PCIBus.bus_reset_seqid. That feels like a much more deterministic solution if MST is willing to support it in the PCI specific BusState. =C2=A0Thanks, Alex