* Re: [PATCH] virtio_pci: use irq to detect interrupt support
2022-10-12 22:04 [PATCH] virtio_pci: use irq to detect interrupt support Michael S. Tsirkin
@ 2022-10-12 22:27 ` Linus Torvalds
2022-10-13 1:34 ` Michael Ellerman
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Linus Torvalds @ 2022-10-12 22:27 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Michael Ellerman, virtualization, linux-kernel, Angus Chen
On Wed, Oct 12, 2022 at 3:04 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> This seems to happen because pci_dev->pin is not populated in
> pci_assign_irq().
Yeah, I have to say, I wonder why it was looking at pci_dev->pin in
the first place.
I was curious, as this is fairly odd.
Of course, I only did a fairly strange 'grep' for this thing, so I
might have missed some other use:
git grep -e '->pin\>' $(git grep -l 'struct pci_dev')
and the above will possibly find other uses of 'pin' as a member than
the 'struct pci_dev', so I'm not going to claim the above is
exhaustive, but looking at the results, I do see how ACPI has somewhat
similar logic, and acpi_pci_irq_enable() does this:
...
pin = dev->pin;
if (!pin) {
dev_dbg(&dev->dev, "No interrupt pin configured\n");
return 0;
}
...
but in the end that seems to be because it's then later using the pin
to do the actual PCI IRQ routing table lookup, and then it uses that
value to actually look up the IRQ number:
dev->irq = rc;
dev->irq_managed = 1;
and in fact all this code also has a "have I already looked this up"
and then it doesn't do anything (but somewhat illogically, it does
that *after* having tested for that "!pin" condition - I think it
would make more sense to go "oh, I already have this interrupt mapped,
I shouldn't care about the pin", but I suspect it doesn't matter in
practice).
And I really think that that is basically the only time you should use
that 'pci_dev->pin' thing: it basically exists not for "does this
device have an IRQ", but for "what is the routing of this irq on this
device".
There's also some testing of dev->pin in drives/pci/pci.c and
drivers/pci/probe.c, but it seems to be very similar: it's actually
doing the irq lookup, and the pin swizzling that goes along with it.
IOW, I think that yes, this patch makes sense, and virtio_pci was
doing something odd. That virtio_pci driver at no point actually cares
about the PCI pin, will not do any PCI irq routing lookup with it, it
will just do
err = request_irq(vp_dev->pci_dev->irq, [...]
using the pci_dev->irq that has already been looked up.
So the patch makes sense to me. If there is some problem with
pci_dev->irq, then it's up to request_irq() to complain about it (ie
we have things like IRQ_NOTCONNECTED etc, which is a more modern-day
version of the old NO_IRQ thing). Again, not something that
virtio_pci should check for itself, I think.
But I don't really know the virtio code. I can only say that "check
the pin" pattern seems entirely wrong and should only be done by code
that does actual irq routing, and "just check the irq" is what
everybody else does.
Linus
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] virtio_pci: use irq to detect interrupt support
2022-10-12 22:04 [PATCH] virtio_pci: use irq to detect interrupt support Michael S. Tsirkin
2022-10-12 22:27 ` Linus Torvalds
@ 2022-10-13 1:34 ` Michael Ellerman
2022-10-13 2:29 ` Jason Wang
[not found] ` <TY2PR06MB3424171328BEE476FCB9942785249@TY2PR06MB3424.apcprd06.prod.outlook.com>
3 siblings, 0 replies; 5+ messages in thread
From: Michael Ellerman @ 2022-10-13 1:34 UTC (permalink / raw)
To: Michael S. Tsirkin, linux-kernel
Cc: virtualization, Linus Torvalds, Angus Chen
"Michael S. Tsirkin" <mst@redhat.com> writes:
> commit 71491c54eafa ("virtio_pci: don't try to use intxif pin is zero")
> breaks virtio_pci on powerpc, when running as a qemu guest.
>
> vp_find_vqs() bails out because pci_dev->pin == 0.
>
> But pci_dev->irq is populated correctly, so vp_find_vqs_intx() would
> succeed if we called it - which is what the code used to do.
>
> This seems to happen because pci_dev->pin is not populated in
> pci_assign_irq().
>
> Which is absolutely a bug in the relevant PCI code, but it
> may also affect other platforms that use of_irq_parse_and_map_pci().
>
> However Linus said:
> The correct way to check for "no irq" doesn't use NO_IRQ at all, it just does
> if (dev->irq) ...
> so let's just check irq and be done with it.
>
> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> Reported-by: Michael Ellerman <mpe@ellerman.id.au>
> Fixes: 71491c54eafa ("virtio_pci: don't try to use intxif pin is zero")
> Cc: "Angus Chen" <angus.chen@jaguarmicro.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>
> Build tested only - very late here. Angus any chance you could
> help test this? Thanks!
This works for me on powerpc.
Tested-by: Michael Ellerman <mpe@ellerman.id.au>
cheers
> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> index 4df77eeb4d16..a6c86f916dbd 100644
> --- a/drivers/virtio/virtio_pci_common.c
> +++ b/drivers/virtio/virtio_pci_common.c
> @@ -409,8 +409,8 @@ int vp_find_vqs(struct virtio_device *vdev, unsigned int nvqs,
> err = vp_find_vqs_msix(vdev, nvqs, vqs, callbacks, names, false, ctx, desc);
> if (!err)
> return 0;
> - /* Is there an interrupt pin? If not give up. */
> - if (!(to_vp_device(vdev)->pci_dev->pin))
> + /* Is there an interrupt? If not give up. */
> + if (!(to_vp_device(vdev)->pci_dev->irq))
> return err;
> /* Finally fall back to regular interrupts. */
> return vp_find_vqs_intx(vdev, nvqs, vqs, callbacks, names, ctx);
> --
> MST
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] virtio_pci: use irq to detect interrupt support
2022-10-12 22:04 [PATCH] virtio_pci: use irq to detect interrupt support Michael S. Tsirkin
2022-10-12 22:27 ` Linus Torvalds
2022-10-13 1:34 ` Michael Ellerman
@ 2022-10-13 2:29 ` Jason Wang
[not found] ` <TY2PR06MB3424171328BEE476FCB9942785249@TY2PR06MB3424.apcprd06.prod.outlook.com>
3 siblings, 0 replies; 5+ messages in thread
From: Jason Wang @ 2022-10-13 2:29 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Michael Ellerman, virtualization, Linus Torvalds, linux-kernel,
Angus Chen
On Thu, Oct 13, 2022 at 6:04 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> commit 71491c54eafa ("virtio_pci: don't try to use intxif pin is zero")
> breaks virtio_pci on powerpc, when running as a qemu guest.
>
> vp_find_vqs() bails out because pci_dev->pin == 0.
>
> But pci_dev->irq is populated correctly, so vp_find_vqs_intx() would
> succeed if we called it - which is what the code used to do.
>
> This seems to happen because pci_dev->pin is not populated in
> pci_assign_irq().
>
> Which is absolutely a bug in the relevant PCI code, but it
> may also affect other platforms that use of_irq_parse_and_map_pci().
>
> However Linus said:
> The correct way to check for "no irq" doesn't use NO_IRQ at all, it just does
> if (dev->irq) ...
> so let's just check irq and be done with it.
>
> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> Reported-by: Michael Ellerman <mpe@ellerman.id.au>
> Fixes: 71491c54eafa ("virtio_pci: don't try to use intxif pin is zero")
> Cc: "Angus Chen" <angus.chen@jaguarmicro.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Acked-by: Jason Wang <jasowang@redhat.com>
> ---
>
> Build tested only - very late here. Angus any chance you could
> help test this? Thanks!
>
> drivers/virtio/virtio_pci_common.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> index 4df77eeb4d16..a6c86f916dbd 100644
> --- a/drivers/virtio/virtio_pci_common.c
> +++ b/drivers/virtio/virtio_pci_common.c
> @@ -409,8 +409,8 @@ int vp_find_vqs(struct virtio_device *vdev, unsigned int nvqs,
> err = vp_find_vqs_msix(vdev, nvqs, vqs, callbacks, names, false, ctx, desc);
> if (!err)
> return 0;
> - /* Is there an interrupt pin? If not give up. */
> - if (!(to_vp_device(vdev)->pci_dev->pin))
> + /* Is there an interrupt? If not give up. */
> + if (!(to_vp_device(vdev)->pci_dev->irq))
> return err;
> /* Finally fall back to regular interrupts. */
> return vp_find_vqs_intx(vdev, nvqs, vqs, callbacks, names, ctx);
> --
> MST
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 5+ messages in thread[parent not found: <TY2PR06MB3424171328BEE476FCB9942785249@TY2PR06MB3424.apcprd06.prod.outlook.com>]
* Re: [PATCH] virtio_pci: use irq to detect interrupt support
[not found] ` <TY2PR06MB3424171328BEE476FCB9942785249@TY2PR06MB3424.apcprd06.prod.outlook.com>
@ 2022-10-14 5:32 ` Michael S. Tsirkin
0 siblings, 0 replies; 5+ messages in thread
From: Michael S. Tsirkin @ 2022-10-14 5:32 UTC (permalink / raw)
To: Angus Chen
Cc: Michael Ellerman, Linus Torvalds, linux-kernel@vger.kernel.org,
virtualization@lists.linux-foundation.org
On Fri, Oct 14, 2022 at 12:09:20AM +0000, Angus Chen wrote:
> Hi Mst
>
> > -----Original Message-----
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Thursday, October 13, 2022 6:04 AM
> > To: linux-kernel@vger.kernel.org
> > Cc: Linus Torvalds <torvalds@linux-foundation.org>; Michael Ellerman
> > <mpe@ellerman.id.au>; Angus Chen <angus.chen@jaguarmicro.com>; Jason
> > Wang <jasowang@redhat.com>; virtualization@lists.linux-foundation.org
> > Subject: [PATCH] virtio_pci: use irq to detect interrupt support
> >
> > commit 71491c54eafa ("virtio_pci: don't try to use intxif pin is zero")
> > breaks virtio_pci on powerpc, when running as a qemu guest.
> >
> > vp_find_vqs() bails out because pci_dev->pin == 0.
> >
> > But pci_dev->irq is populated correctly, so vp_find_vqs_intx() would
> > succeed if we called it - which is what the code used to do.
> >
> > This seems to happen because pci_dev->pin is not populated in
> > pci_assign_irq().
> >
> > Which is absolutely a bug in the relevant PCI code, but it
> > may also affect other platforms that use of_irq_parse_and_map_pci().
> >
> > However Linus said:
> > The correct way to check for "no irq" doesn't use NO_IRQ at all, it just does
> > if (dev->irq) ...
> > so let's just check irq and be done with it.
> >
> > Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> > Reported-by: Michael Ellerman <mpe@ellerman.id.au>
> > Fixes: 71491c54eafa ("virtio_pci: don't try to use intxif pin is zero")
> > Cc: "Angus Chen" <angus.chen@jaguarmicro.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >
> > Build tested only - very late here. Angus any chance you could
> > help test this? Thanks!
> I tested the patch ，it work well on x86。Inlcude legacy and modern driver.
> And I tested it on arm server also，and find some problem because of 0.9.5 limitation.
> "platform bug: legacy virtio-pci must not be used with RAM above 0x%llxGB\n",
> But the error is not effected by our patch.with or without our patch ,it print the same.
Yes that's a limitation of 0.9.5 - just make a smaller VM to test
legacy.
> And I test modern dirver,it work well also.
> Sorry for the late reply.
Great, thanks a lot!
> >
> > drivers/virtio/virtio_pci_common.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/virtio/virtio_pci_common.c
> > b/drivers/virtio/virtio_pci_common.c
> > index 4df77eeb4d16..a6c86f916dbd 100644
> > --- a/drivers/virtio/virtio_pci_common.c
> > +++ b/drivers/virtio/virtio_pci_common.c
> > @@ -409,8 +409,8 @@ int vp_find_vqs(struct virtio_device *vdev, unsigned int
> > nvqs,
> > err = vp_find_vqs_msix(vdev, nvqs, vqs, callbacks, names, false, ctx, desc);
> > if (!err)
> > return 0;
> > - /* Is there an interrupt pin? If not give up. */
> > - if (!(to_vp_device(vdev)->pci_dev->pin))
> > + /* Is there an interrupt? If not give up. */
> > + if (!(to_vp_device(vdev)->pci_dev->irq))
> > return err;
> > /* Finally fall back to regular interrupts. */
> > return vp_find_vqs_intx(vdev, nvqs, vqs, callbacks, names, ctx);
> > --
> > MST
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 5+ messages in thread