From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57606) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bKTkc-0002t8-5I for qemu-devel@nongnu.org; Tue, 05 Jul 2016 13:03:15 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bKTkV-0007rw-Ih for qemu-devel@nongnu.org; Tue, 05 Jul 2016 13:03:08 -0400 Received: from mx1.redhat.com ([209.132.183.28]:48473) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bKTkV-0007rC-AO for qemu-devel@nongnu.org; Tue, 05 Jul 2016 13:03:03 -0400 Date: Tue, 5 Jul 2016 11:03:00 -0600 From: Alex Williamson Message-ID: <20160705110300.029985a6@t450s.home> In-Reply-To: References: <1464315131-25834-1-git-send-email-zhoujie2011@cn.fujitsu.com> <20160620211306.66a6b249@t450s.home> <576935FC.1080503@easystack.cn> <20160621084443.330f932d@t450s.home> <20160621215626.71c99582@t450s.home> <113474d2-8408-db49-e7ef-8c6b736af866@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> 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, 5 Jul 2016 09:36:27 +0800 Zhou Jie wrote: > ping Due to weekend and holiday in my country, there were zero regular working hours between your emails. > On 2016/7/3 12:00, Zhou Jie wrote: > > Hi Alex, > > > > On 2016/6/30 9:45, Zhou Jie wrote: > >> Hi Alex, > >> > >> On 2016/6/30 2:22, Alex Williamson wrote: > >>> On Wed, 29 Jun 2016 16:54:05 +0800 > >>> Zhou Jie wrote: > >>> > >>>> Hi Alex, > >>>> > >>>>> And yet we have struct pci_dev.broken_intx_masking and we test for > >>>>> working DisINTx via pci_intx_mask_supported() rather than simply > >>>>> looking for a PCIe device. Some devices are broken and some simply > >>>>> don't follow the spec, so you're going to need to deal with that or > >>>>> exclude those devices. > >>>> For those devices I have no way to disable the INTx. > >>> > >>> disable_irq()? Clearly vfio-pci already manages these types of devices > >>> and can disable INTx. This is why I keep suggesting that maybe tearing > >>> the interrupt setup down completely is a more complete and reliable > >>> approach than masking in the command register. Unless we're going to > >>> exclude such devices from supporting this mode (which I don't condone), > >>> we must deal with them. > >> Thank you for tell me that. > >> Yes, I can use disable_irq to disable the pci device irq. > >> But should I enable the irq after reset? > >> I will dig into it. > > > > I will alter the VFIO driver like following. > > During err occurs and resume: > > 1. Make config space read only. > > 2. Disable INTx/MSI Interrupt. > > 3. Do nothing for bar regions. > > > > The following code will be modified. > > 1. vfio_pci_ioctl > > add a flag in vfio_device_info for workable_state support > > return workable_state in "struct vfio_pci_device" when user get info Seems like two flags are required, one to indicate the presence of this feature and another to indicate the state. I'd prefer something like access_blocked. > > 2. vfio_pci_ioctl > > During err occurs and resume: > > if (cmd == VFIO_DEVICE_SET_IRQS || VFIO_DEVICE_RESET > > || VFIO_DEVICE_GET_PCI_HOT_RESET_INFO || VFIO_DEVICE_PCI_HOT_RESET) > > block for workable_state clearing > > 3. vfio_pci_write > > During err occurs and resume: > > ignore and return 0 This is contradictory to your comment "Do nothing for bar regions". ISTR that returning 0 for read/write calls is an easy way to break users since we've return neither the desired bytes nor an error code. > > 4. vfio_pci_aer_err_detected > > Set workable_state to false in "struct vfio_pci_device" > > Disable INTx: > > If Disable INTx is support > > disable by PCI_COMMAND > > else > > disable by disable_irq function > > Disable MSI: > > disable by clearing the "Bus Master Enable" bit of PCI_COMMAND I've suggested repeatedly to properly teardown these interrupts. I disagree with your proposed approach here. If the device is intended to be in a state that requires re-initialization then the interrupt setup should be part of that. > > 5. vfio_pci_aer_resume > > Set workable_state to true in "struct vfio_pci_device" > > About INTx: > > According to the value of "vdev->ctx[0].masked" > > to decide whether to enable INTx > > About MSI: > > After reset the "Bus Master Enable" bit is default to 0. > > So user should process this after reset. Again, I think this is error prone, teardown the interrupts and define that the device state, including interrupts, needs to be reinitialized after error. Why are you not incorporating this feedback? Thanks, Alex