From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35586) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bMe13-0005Br-HH for qemu-devel@nongnu.org; Mon, 11 Jul 2016 12:25:10 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bMe0x-0005F1-FL for qemu-devel@nongnu.org; Mon, 11 Jul 2016 12:25:04 -0400 Received: from mx1.redhat.com ([209.132.183.28]:38201) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bMe0x-0005Ew-72 for qemu-devel@nongnu.org; Mon, 11 Jul 2016 12:24:59 -0400 Date: Mon, 11 Jul 2016 10:24:56 -0600 From: Alex Williamson Message-ID: <20160711102456.0baa5e26@t450s.home> In-Reply-To: References: <1464315131-25834-1-git-send-email-zhoujie2011@cn.fujitsu.com> <468b752b-a161-902b-d4cc-489dfa18c21e@cn.fujitsu.com> <20160622094236.515549fa@t450s.home> <7746532f-2fad-1304-0df7-7cd25ba761af@cn.fujitsu.com> <20160627095418.659e6e5f@t450s.home> <20160627215808.1531a774@t450s.home> <7912dad0-0e37-603d-fdfe-bb4950b55f28@cn.fujitsu.com> <20160628084052.1e85a730@t450s.home> <689ac38f-96d7-9717-e9c4-d9926272cb86@cn.fujitsu.com> <20160629122242.2ac20254@t450s.home> <99e88b46-8b8b-f7de-700f-8d644a7f005a@cn.fujitsu.com> <20160705110300.029985a6@t450s.home> <24876827-07fc-6f69-6949-7e358720c914@cn.fujitsu.com> <20160707130442.7ac934c8@t450s.home> <3e661554-3c79-9d8f-41b2-c4cdd0fcb95f@cn.fujitsu.com> <20160708113358.2e5ed040@t450s.home> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v8 11/12] vfio: register aer resume notification handler for aer resume List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Zhou Jie Cc: izumi.taku@jp.fujitsu.com, caoj.fnst@cn.fujitsu.com, Chen Fan , qemu-devel@nongnu.org, mst@redhat.com On Sun, 10 Jul 2016 09:28:41 +0800 Zhou Jie wrote: > Hi Alex, > > > The variable clearly isn't visible to the user, so the user can know > > whether the kernel supports this feature, but not whether the feature > > is currently active. Perhaps there's no way to avoid races completely, > > but don't you expect that if we define that certain operations are > > blocked after an error notification that a user may want some way to > > poll for whether the block is active after a reset rather than simply > > calling a blocked interface to probe for it? > Yes, I will use access blocked function, not the variable. I don't understand what this means. > > As we've discussed before, the AER notification needs to be relayed to > > the user without delay, otherwise we only increase the gap where the > > user might consume bogus data. It also only seems reasonable to modify > > the behavior of the interfaces (ie. blocking) if the user is notified, > > which would be through the existing error notifier. We can never > > depend on a specific behavior from the user, we may be dealing with a > > malicious user. > > > > We already disable interrupts in vfio_pci_disable() simply by calling > > the ioctl function directly. > Sorry, I want to know where is vfio_pci_disable invoked. > I can't find it in ioctl function. You have the code, vfio_pci_disable() is invoked when the vfio device file descriptor is released. It's not in the ioctl, it calls the ioctl as the user would to disable all interrupts on the device. > > If we simply disable and re-enable interrupts as you propose, > > how does the user deal with edge triggered > > interrupts that may have occurred during that period? Are they lost? > > Should we instead leave the interrupts enabled but skip > > eventfd_signal() in the interrupts handlers, queuing interrupts for > > re-delivery after the device is resumed? > Yes, they will lost. Is that acceptable? This is part of the problem I have with silently disabling interrupt delivery via the command register across reset. It seems more non-deterministic than properly disabling interrupts and requiring the user to reinitialize them after error. > > Or does it make more sense to > > simply disable the interrupts as done in vfio_pci_disable() and define > > that the user needs to re-establish interrupts before continuing after > > an error event? Thanks, > If user invoked the vfio_pci_disable by ioctl function. I'm in no way suggesting that a user invoke vfio_pci_disable(), I'm just trying to point out that vfio_pci_disable() already does a teardown of interrupts, similar to what seems to be required here. > Yes, user should re-establish interrupts before > continuing after an error event. So if we define that users should re-establish interrupts after an error event, then what's the point of only doing command register masking of the interrupts and requiring the user to both tear-down the interrupts and re-establish them? Thanks, Alex