Re: [Qemu-devel] [RFC PATCH] vfio: add fixup for broken PCI devices

[Alex Williamson]

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