Re: [Qemu-devel] [RFC PATCH] vfio: add fixup for broken PCI devices
On Sat, 2012-08-18 at 00:28 +1000, Alexey Kardashevskiy wrote:
> On Fri, Jun 22, 2012 at 6:16 PM, Alexey Kardashevskiy 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
> > >> ---
> > >> 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
Re: [Qemu-devel] [RFC PATCH] vfio: add fixup for broken PCI devices
On Fri, Jun 22, 2012 at 6:16 PM, Alexey Kardashevskiy 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
> >> ---
> >> 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
Re: [Qemu-devel] [RFC PATCH] vfio: add fixup for broken PCI devices
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
>> ---
>> 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
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
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
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
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
> ---
> 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
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 > > > > 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
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 > > 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 signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [RFC PATCH] vfio: add fixup for broken PCI devices
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
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
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
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
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
> ---
> 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 |
