vfio: add fixup for broken PCI devices

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 <[email protected]>

---
 drivers/vfio/pci/vfio_pci.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

drivers/vfio/pci/vfio_pci.c +2 -0

index 1e5315c..6e7c12d 100644
@@ -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 |

Quoted 11 lines · 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.

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.

Quoted 1 line

Quoted 1 line

To eliminate such effect, some quirk should be called. The

proposed

Quoted 1 line

Quoted 1 line

pci_fixup_final does its job well for mentioned NEC PCI USB but

not

Quoted 3 lines

Quoted 1 line

sure if it is 100% correct.

I think we should create a new quirk category... call it

pci_fixup_reset

Quoted 1 line

or something like that, which is responsible for blasting the thing

into

Quoted 2 lines

submission when ownership changes.

Quoted 1 line

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,

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 <[email protected]>

---
 drivers/vfio/pci/vfio_pci.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

drivers/vfio/pci/vfio_pci.c +2 -0

index 1e5315c..6e7c12d 100644
@@ -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 |

Signature · 1 line

1.7.7.3

Quoted 8 lines · 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.

Quoted 5 lines

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 <[email protected]>

Won't current kvm device assignment be affected by this?

If yes need to address that not just vfio.

Quoted 9 lines · Michael S. Tsirkin

Quoted 8 lines · 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.

Quoted 7 lines

Quoted 5 lines

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 <[email protected]>

Won't current kvm device assignment be affected by this?

Would be surprising if not.

Quoted 1 line

If yes need to address that not just vfio.

A reason to solve this at PCI level?

Jan

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 <[email protected]>

---
 drivers/vfio/pci/vfio_pci.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

drivers/vfio/pci/vfio_pci.c +0 -0

index 1e5315c..6e7c12d 100644

Review

Quoted 1 line

(quoted diff) +0 -0

b/drivers/vfio/pci/vfio_pci.c

drivers/vfio/pci/vfio_pci.c static void vfio_pci_disable(struct

Quoted 4 lines

index 1e5315c..6e7c12d 100644

drivers/vfio/pci/vfio_pci.c +0 -0

@@ -88,6 +88,8 @@ static void vfio_pci_disable(struct

vfio_pci_device *vdev)

drivers/vfio/pci/vfio_pci.c static void vfio_pci_disable(struct vfio_pci_device *vdev) · line 95

Quoted 8 lines

drivers/vfio/pci/vfio_pci.c +2 -0

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

Quoted 16 lines · Alex Williamson

Review

Quoted 1 line

(quoted diff) +0 -0

b/drivers/vfio/pci/vfio_pci.c

drivers/vfio/pci/vfio_pci.c static void vfio_pci_disable(struct

Quoted 4 lines

index 1e5315c..6e7c12d 100644

drivers/vfio/pci/vfio_pci.c +0 -0

@@ -88,6 +88,8 @@ static void vfio_pci_disable(struct

vfio_pci_device *vdev)

drivers/vfio/pci/vfio_pci.c static void vfio_pci_disable(struct vfio_pci_device *vdev) · line 95

Quoted 8 lines

drivers/vfio/pci/vfio_pci.c +2 -0

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

Quoted 7 lines

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,

Quoted 29 lines · 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 <[email protected]>

---
 drivers/vfio/pci/vfio_pci.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

drivers/vfio/pci/vfio_pci.c +2 -0

index 1e5315c..6e7c12d 100644
@@ -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);
 

drivers/vfio/pci/vfio_pci.c static void vfio_pci_disable(struct vfio_pci_device *vdev) · line 95

Quoted 1 line

drivers/vfio/pci/vfio_pci.c +0 -0

 	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,

Quoted 18 lines · Alex Williamson

Quoted 17 lines · 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 <[email protected]>

---
 drivers/vfio/pci/vfio_pci.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

Quoted 5 lines

drivers/vfio/pci/vfio_pci.c static void vfio_pci_disable(struct vfio_pci_device

Quoted 5 lines

drivers/vfio/pci/vfio_pci.c +0 -0

index 1e5315c..6e7c12d 100644
@@ -88,6 +88,8 @@ static void vfio_pci_disable(struct vfio_pci_device

*vdev)

Quoted 9 lines

drivers/vfio/pci/vfio_pci.c static void vfio_pci_disable(struct vfio_pci_device *vdev) · line 95

Quoted 8 lines

drivers/vfio/pci/vfio_pci.c +2 -0

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

Quoted 7 lines

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.

From the code, it enables a device, does some tricks in

Quoted 43 lines · Alexey Kardashevskiy

Quoted 18 lines · Alex Williamson

Quoted 17 lines · 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 <[email protected]>

---
 drivers/vfio/pci/vfio_pci.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

Quoted 5 lines

drivers/vfio/pci/vfio_pci.c static void vfio_pci_disable(struct vfio_pci_device

Quoted 5 lines

drivers/vfio/pci/vfio_pci.c +0 -0

index 1e5315c..6e7c12d 100644
@@ -88,6 +88,8 @@ static void vfio_pci_disable(struct vfio_pci_device

*vdev)

Quoted 9 lines

drivers/vfio/pci/vfio_pci.c static void vfio_pci_disable(struct vfio_pci_device *vdev) · line 95

Quoted 8 lines

drivers/vfio/pci/vfio_pci.c +2 -0

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

Quoted 7 lines

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,

Quoted 10 lines

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?