From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58296) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aGdgx-0007mB-PY for qemu-devel@nongnu.org; Tue, 05 Jan 2016 21:19:16 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aGdgu-00033l-KZ for qemu-devel@nongnu.org; Tue, 05 Jan 2016 21:19:15 -0500 Received: from [59.151.112.132] (port=6761 helo=heian.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aGdgt-000339-VT for qemu-devel@nongnu.org; Tue, 05 Jan 2016 21:19:12 -0500 Message-ID: <568C782C.8050001@cn.fujitsu.com> Date: Wed, 6 Jan 2016 10:13:00 +0800 From: Chen Fan MIME-Version: 1.0 References: <93bca47b31cf1abd661fbe778d415abbb08b680a.1451901804.git.chen.fan.fnst@cn.fujitsu.com> <1452023901.22223.48.camel@redhat.com> In-Reply-To: <1452023901.22223.48.camel@redhat.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit 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: Alex Williamson , Cao jin , qemu-devel@nongnu.org Cc: mst@redhat.com 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 >> >> mark the host bus be in reset. avoid multiple devices trigger the >> host bus reset many times. >> >> Signed-off-by: Chen Fan >> --- >> 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 > > > . >