From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36178) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bw9QL-00071S-Cu for qemu-devel@nongnu.org; Mon, 17 Oct 2016 11:01:58 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bw9QI-0004BS-BD for qemu-devel@nongnu.org; Mon, 17 Oct 2016 11:01:57 -0400 Received: from mx1.redhat.com ([209.132.183.28]:57612) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1bw9QI-0004BK-2x for qemu-devel@nongnu.org; Mon, 17 Oct 2016 11:01:54 -0400 Date: Mon, 17 Oct 2016 09:01:51 -0600 From: Alex Williamson Message-ID: <20161017090151.6778cc28@t450s.home> In-Reply-To: <58049264.5000002@cn.fujitsu.com> References: <1476443819-10347-1-git-send-email-caoj.fnst@cn.fujitsu.com> <20161014095030.1c7c93c7@t450s.home> <58049264.5000002@cn.fujitsu.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] vfio: fix duplicate function call List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Cao jin Cc: qemu-devel@nongnu.org On Mon, 17 Oct 2016 16:57:08 +0800 Cao jin wrote: > Hi, > > On 10/14/2016 11:50 PM, Alex Williamson wrote: > > On Fri, 14 Oct 2016 19:16:59 +0800 > > Cao jin wrote: > > > >> When vfio device is reset(encounter FLR, or bus reset), if need to do > >> bus reset(vfio_pci_hot_reset_one is called), vfio_pci_pre_reset & > >> vfio_pci_post_reset will be called twice. > >> > >> Signed-off-by: Cao jin > >> --- > >> Also has a little question on vfio_pci_reset. it will be called when encounter > >> bus reset, or FLR. The reset method's priority in this function now is: > >> > >> 1. If has "device specific reset function", then do it > >> 2. If has FLR, then do it. > >> 3. If it can do bus reset(only 1 affected device), then do it > >> 4. If has pm_reset, then do it > >> > >> The question is: why pm reset has low priority than bus reset(if it does > >> can do a bus reset)? why bus reset is not the last choice? In PCI driver > >> of kernel, pls see __pci_dev_reset, we can see, if device support pm reset, > >> it won't do bus reset. > > > > The PCI spec doesn't really define what sort of reset is done with a PM > > reset. My thinking was that if a device advertises an FLR capability > > then the hardware has made a concerted effort to have a per function > > reset mechanism available. NoSoftRst- is not terribly common and it's > > not entirely clear to me that the hardware has made a conscious effort > > to provide this for the purposes of per function reset mechanism. > > Therefore I've opt'd to prioritize a bus reset over a PM reset. > > > > I still have a question about vfio_pci_reset. I checked commit message > in f16f39c3, if I understand right, couldn't we put > > /* See if we can do our own bus reset */ > if (!vfio_pci_hot_reset_one(vdev)) { > goto post_reset; > } > > in the 1st priority? Because if there is 1 affected device, then it will > do bus reset which is the best reset we can do; if there are more than 1 > affected devices, after this patch, vfio_pci_hot_reset_one will do > nothing, and then try other reset methods. It's possible, yes, but that disregards that the hardware has gone to the trouble to implement a proper function level reset. As I explained, I de-prioritize PM reset, specifically because I'm not sure if hardware designers are necessarily intending it for the purpose of a device reset. For FLR this is the entire purpose of the interface. We also have a fair bit of experience with the current priority scheme and I would not take it lightly to change without some compelling evidence to prove that a new priority scheme is better than the existing. There do also exist devices which do not behave properly with a secondary bus reset, see drivers/pci/quirks.c:quirk_no_bus_reset() in the kernel tree. It's possible more devices like this exist, but we don't see them because they implement FLR. A bus reset may result in a more complete device reset, but it's also more disruptive to the system. Thanks, Alex