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

[Alex Williamson <alex.williamson@redhat.com>]

On Sat, 2012-08-18 at 00:28 +1000, Alexey Kardashevskiy wrote:
> On Fri, Jun 22, 2012 at 6:16 PM, Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> 
> > On 07/06/12 09:17, Alex Williamson wrote:
> > > On Fri, 2012-05-25 at 17:35 +1000, Alexey Kardashevskiy wrote:
> > >> Some adapters (like NEC PCI USB controller) do not flush their config
> > >> on a sioftware reset and remember DMA config, etc.
> > >>
> > >> If we use such an adapter with QEMU, then crash QEMU (stop it with
> > >> ctrl-A ctrl-X), and try to use it in QEMU again, it may start working
> > >> immediately with previous config when pci_enable_device() is called
> > >> on that PCI function.
> > >>
> > >> To eliminate such effect, some quirk should be called. The proposed
> > >> pci_fixup_final does its job well for mentioned NEC PCI USB but not
> > >> sure if it is 100% correct.
> > >>
> > >> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> > >> ---
> > >>  drivers/vfio/pci/vfio_pci.c |    2 ++
> > >>  1 files changed, 2 insertions(+), 0 deletions(-)
> > >>
> > >> 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?  Thanks,
> >
> >
> > Checked again.
> >
> > Seems to be a false alarm, cannot reproduce the bad behavior anymore,
> > looks like it was caused by
> > another issue which Alex fixed.
> >
> > So although the problem may arise again, there is nothing urgent to do at
> > the moment.
> >
> >
> While doing cleanups in my SPAPR IOMMU driver for VFIO,
> I found that I have not unmapped all the pages on module_exit.
> Heh. My bad. So I implemented that and got a lot of strange accidental
> crashes of the host kernel when I tried to debug
> "USB Controller: NEC Corporation USB (rev 43)".
> I applied the quoted patch and it has gone.
> 
> You asked what fixup exactly does but honestly I do not know.
> From the code, it enables a device, does some tricks in
> quirk_usb_handoff_ohci()/quirk_usb_disable_ehci() and
> disables the device back. The comments of these quirks
> say that they basically disable interrupts and do shutdown/reset
> via OHCI/EHCI specific registers as pci_disable_device() does
> not do the job like it does not reset the device's DMA config so when
> it gets enabled back, it continues DMA transfer to/from addresses
> which were actual at the moment of QEMU shutdown/group release.
> 
> So do we add a new class of quirks?

Sounds like a device specific (or maybe class specific) reset to me.  I
don't think we have any business directly calling PCI quirks from VFIO
code.  Let's make pci_reset_function do what it takes to settle the
device.  Thanks,

Alex