From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:44094) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ScTpw-0007UT-Fv for qemu-devel@nongnu.org; Wed, 06 Jun 2012 23:56:41 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ScTpu-00053b-Es for qemu-devel@nongnu.org; Wed, 06 Jun 2012 23:56:39 -0400 Received: from mx1.redhat.com ([209.132.183.28]:38577) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ScTpu-00053L-6u for qemu-devel@nongnu.org; Wed, 06 Jun 2012 23:56:38 -0400 Message-ID: <1339041383.23475.396.camel@bling.home> From: Alex Williamson Date: Wed, 06 Jun 2012 21:56:23 -0600 In-Reply-To: <1339037557.24838.2.camel@pasglop> References: <4FBF3627.3030504@ozlabs.ru> <1339024663.23475.338.camel@bling.home> <1339037557.24838.2.camel@pasglop> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit Mime-Version: 1.0 Subject: Re: [Qemu-devel] [RFC PATCH] vfio: add fixup for broken PCI devices List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Benjamin Herrenschmidt Cc: Alexey Kardashevskiy , David Gibson , qemu-devel@nongnu.org, kvm@vger.kernel.org, Alex Graf On Thu, 2012-06-07 at 12:52 +1000, Benjamin Herrenschmidt wrote: > On Wed, 2012-06-06 at 17:17 -0600, Alex Williamson wrote: > > > diff --git a/drivers/vfio/pci/vfio_pci.c > > b/drivers/vfio/pci/vfio_pci.c > > > index 1e5315c..6e7c12d 100644 > > > --- a/drivers/vfio/pci/vfio_pci.c > > > +++ b/drivers/vfio/pci/vfio_pci.c > > > @@ -88,6 +88,8 @@ static void vfio_pci_disable(struct > > vfio_pci_device *vdev) > > > { > > > int bar; > > > > > > + pci_fixup_device(pci_fixup_final, vdev->pdev); > > > + > > > pci_disable_device(vdev->pdev); > > > > > > vfio_pci_set_irqs_ioctl(vdev, VFIO_IRQ_SET_DATA_NONE | > > > > Sorry, just taking a look at this again. Do you have any idea what > > fixup it is that makes it work? Calling a fixup at this point seems > > rather odd. I suspect the problem is that vfio is only calling > > pci_load_and_free_saved_state if pci_reset_function reports that it > > worked. kvm device assignment doesn't do that and I'm not sure why I > > did that. If you unconditionally call pci_load_and_free_saved_state a > > bit further down in this function, does it solve the problem? > > No it won't do, you need device-specific "reset" fixup code for devices > where the function reset doesn't do the right thing. > > My suggestion is to add a new quirk category (in addition to > early,late,... add reset) and call that here. > > Then we can do one for the NEC OHCI that properly stops the controller, > among others. I would be -very- surprised if that chip ends up being the > only one causing that sort of trouble. > > Also, some chips will need some "tweaks" after the reset, for example if > we do a full link reset, I know of at least one device that might > randomly fail to properly train the PCIe link, such a quirk is a perfect > spot to add the right fixup. In so far as vfio should only have to call pci_reset_function and device quirks take care of everything else, I agree with you, but that doesn't answer any of my questions. Sure, we may want pre- and post-reset fixup quirks and a pony, but what quirk is actually necessary for this device? Does it fit into the existing pci_dev_specific_reset quirking? Reloading config space isn't a good generic solution, but it might at least shed some light on whether the reset function is doing anything and if a simple config space change fixes it. VFIO needs to do this anyway. Thanks, Alex