| Message ID | 4FBF3627.3030504@ozlabs.ru |
|---|---|
| State | New |
| Headers | show |
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 |
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
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.
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
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
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.
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
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.
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
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.
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.
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?
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
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 |
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(-)