From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33761) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bMzsZ-0008SN-6l for qemu-devel@nongnu.org; Tue, 12 Jul 2016 11:45:48 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bMzsX-0000Rc-98 for qemu-devel@nongnu.org; Tue, 12 Jul 2016 11:45:47 -0400 Received: from mx1.redhat.com ([209.132.183.28]:33768) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bMzsW-0000RQ-Os for qemu-devel@nongnu.org; Tue, 12 Jul 2016 11:45:45 -0400 Date: Tue, 12 Jul 2016 09:45:43 -0600 From: Alex Williamson Message-ID: <20160712094543.422ce7f7@t450s.home> In-Reply-To: References: <1464315131-25834-1-git-send-email-zhoujie2011@cn.fujitsu.com> <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> <20160711102456.0baa5e26@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 Tue, 12 Jul 2016 09:42:21 +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. > I will use workable state support flag > to let user know whether the kenerl support block feature. > And make configure space writing and ioctl function blocked. And what of my suggestion that a user may desire to poll the state of the device? > > 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. > What is "properly disabling interrupts"? We've discussed this, it's tearing down the eventfds and the irq handlers using the same mechanism as used in vfio_pci_disable(). > User don't know what vfio driver has done. > What's the difference of "disabling interrupt delivery via > the command register" and "doing a teardown of interrupts"? > The interrupts will still lost silently. A user does know what the vfio driver has done if you define the behavior that on an AER error reported event, as signaled to the user via the error notification interrupt, vfio-pci will teardown device interrupts to an uninitialized state. The difference between the command register approach you suggest and the teardown I suggest is that the command register is simply masking interrupt deliver while the teardown approach returns the device to an uninitialized interrupt state. Take a look at the device state when a bus reset occurs, what state is saved and restored and what is left at a default PCI value. The command register is saved and restored, so any manipulation we do of it is racing the host kernel AER handling and bus reset. What about MSI and MSI-X? Looks to me like those are left at the PCI default initialization state, so now after an AER error we have irq handlers and eventfds configured, while in fact the device has been de-programmed. To handle that we're expecting users to teardown the interrupt state and re-establish it? Again, why not just teardown the interrupt state ourselves? I dont' see the value in simply masking the command register, especially when it doesn't handle the no-DisINTx case. > >>> 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, > Take "Intel(R) 10GbE PCI Express Virtual Function" as an example. > In drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c > static const struct pci_error_handlers ixgbevf_err_handler = { > .error_detected = ixgbevf_io_error_detected, > .slot_reset = ixgbevf_io_slot_reset, > .resume = ixgbevf_io_resume, > }; > User tear-down the interrupts in ixgbevf_io_error_detected function. > And up the interrupts in ixgbevf_io_resume. > > Guest OS driver will do both tear-down the interrupts > and re-establish them. > Because it don't know what host vfio driver has done. > > I disable the interrupts to pretend them interfere with device reset. We cannot depend on the behavior of any given driver and the fact that the guest driver may teardown interrupts anyway is not a justification that vfio shouldn't be doing this to make the device state presented to the user consistent. Thanks, Alex