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

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

[Alexey Kardashevskiy]

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 |
-- 
1.7.7.3

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

[Benjamin Herrenschmidt]

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.

I think we should create a new quirk category... call it pci_fixup_reset
or something like that, which is responsible for blasting the thing into
submission when ownership changes.

We'll need these for more than just USB I suspect.

Cheers,
Ben.

> 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 |

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

[Alex Williamson]

On Fri, 2012-05-25 at 18:28 +1000, Benjamin Herrenschmidt 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.
> 
> I think we should create a new quirk category... call it pci_fixup_reset
> or something like that, which is responsible for blasting the thing into
> submission when ownership changes.
> 
> We'll need these for more than just USB I suspect.

We already have pci_dev_specific_reset() called from pci_dev_reset().
Does this device support any of the standard reset mechanisms?  It would
be nice to know what within the final fixups keeps this device working.
Thanks,

Alex

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

[Benjamin Herrenschmidt]

On Fri, 2012-05-25 at 06:24 -0600, Alex Williamson wrote:
> > > 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.
> > 
> > I think we should create a new quirk category... call it
> pci_fixup_reset
> > or something like that, which is responsible for blasting the thing
> into
> > submission when ownership changes.
> > 
> > We'll need these for more than just USB I suspect.
> 
> We already have pci_dev_specific_reset() called from pci_dev_reset().
> Does this device support any of the standard reset mechanisms?  It
> would
> be nice to know what within the final fixups keeps this device
> working.
> Thanks, 

Well, HW is HW ... it's going to be broken one way or another. Reset is
no exception, and we already have a way to deal with that sort of
breakage via the quirks. They are already divided in several categories
(early, normal, final, ...), I suggest we just add one for reset. No
point re-inventing a callback mechanism when we already have one.

Cheers,
Ben.

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

[Michael S. Tsirkin]

On Fri, May 25, 2012 at 05:35:03PM +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>

Won't current kvm device assignment be affected by this?
If yes need to address that not just vfio.

> ---
>  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 |
> -- 
> 1.7.7.3

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

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 929 bytes --]

On 2012-05-28 14:44, Michael S. Tsirkin wrote:
> On Fri, May 25, 2012 at 05:35:03PM +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>
> 
> Won't current kvm device assignment be affected by this?

Would be surprising if not.

> If yes need to address that not just vfio.

A reason to solve this at PCI level?

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

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

[Michael S. Tsirkin]

On Mon, May 28, 2012 at 02:48:10PM +0200, Jan Kiszka wrote:
> On 2012-05-28 14:44, Michael S. Tsirkin wrote:
> > On Fri, May 25, 2012 at 05:35:03PM +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>
> > 
> > Won't current kvm device assignment be affected by this?
> 
> Would be surprising if not.
> 
> > If yes need to address that not just vfio.
> 
> A reason to solve this at PCI level?
> 
> Jan
> 

Sure, and I think this is what Benjamin Herren was suggesting.
I just wanted to add another reason.

-- 
MST

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

[Alex Williamson]

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,

Alex

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

[Benjamin Herrenschmidt]

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.

Cheers,
Ben.

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

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

[Benjamin Herrenschmidt]

On Wed, 2012-06-06 at 21:56 -0600, Alex Williamson wrote:
> 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, 

Knowing those NEC OHCI critters I suspect it just continues DMA'ing as
usual and just ignores the reset... not sure what the actual setup is
though, those are PCI 2.x devices behind a PCI-E to PCI-X bridge, so I'm
not even sure they support a semi-decent reset.

We might want to even tell the bridge to hard reset what's behind it in
that case (everything is one isolation group behind that bridge anyway)
but that has its own problems (CSR unreliability, need for extra delays,
need to reconfigure the whole config space etc....)

Cheers,
Ben.

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

[Alexey Kardashevskiy]

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.



-- 
Alexey

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

[Alexey Kardashevskiy]

[-- Attachment #1: Type: text/plain, Size: 3117 bytes --]

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?


-- 
Alexey

[-- Attachment #2: Type: text/html, Size: 3933 bytes --]

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

[Alex Williamson]

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