Re: [PATCH] virtio_pci: use irq to detect interrupt support
On Thu, Oct 13, 2022 at 6:04 AM Michael S. Tsirkin 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
> Reported-by: Michael Ellerman
> Fixes: 71491c54eafa ("virtio_pci: don't try to use intxif pin is zero")
> Cc: "Angus Chen"
> Signed-off-by: Michael S. Tsirkin
Acked-by: Jason Wang
> ---
>
> 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
Re: [PATCH RESEND] tools/virtio: initialize spinlocks in vring_test.c
On Wed, 12 Oct 2022 08:29:49 +0200, "Ricardo Cañuelo"
wrote:
> The virtio_device vqs_list spinlocks must be initialized before use to
> prevent functions that manipulate the device virtualqueues, such as
> vring_new_virtqueue(), from blocking indefinitely.
>
> Signed-off-by: Ricardo Cañuelo
Reviewed-by: Xuan Zhuo
> ---
> tools/virtio/vringh_test.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/tools/virtio/vringh_test.c b/tools/virtio/vringh_test.c
> index fa87b58bd5fa..98ff808d6f0c 100644
> --- a/tools/virtio/vringh_test.c
> +++ b/tools/virtio/vringh_test.c
> @@ -308,6 +308,7 @@ static int parallel_test(u64 features,
>
> gvdev.vdev.features = features;
> INIT_LIST_HEAD();
> + spin_lock_init(_list_lock);
> gvdev.to_host_fd = to_host[1];
> gvdev.notifies = 0;
>
> @@ -455,6 +456,7 @@ int main(int argc, char *argv[])
> getrange = getrange_iov;
> vdev.features = 0;
> INIT_LIST_HEAD();
> + spin_lock_init(_list_lock);
>
> while (argv[1]) {
> if (strcmp(argv[1], "--indirect") == 0)
> --
> 2.25.1
>
> ___
> Virtualization mailing list
> Virtualization@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio_pci: use irq to detect interrupt support
"Michael S. Tsirkin" 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
> Reported-by: Michael Ellerman
> Fixes: 71491c54eafa ("virtio_pci: don't try to use intxif pin is zero")
> Cc: "Angus Chen"
> Signed-off-by: Michael S. Tsirkin
> ---
>
> 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
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
Re: [PATCH 1/1] virtio_pci: use common helper to configure SR-IOV
On Thu, Oct 13, 2022 at 02:01:04AM +0300, Max Gurtovoy wrote:
>
> On 10/13/2022 12:20 AM, Bjorn Helgaas wrote:
> > On Wed, Oct 12, 2022 at 5:01 AM Max Gurtovoy wrote:
> > >
> > > On 10/12/2022 11:42 AM, Max Gurtovoy wrote:
> > > > On 10/12/2022 8:02 AM, Michael S. Tsirkin wrote:
> > > > > On Thu, Sep 29, 2022 at 02:40:08AM +0300, Max Gurtovoy wrote:
> > > > > > This is instead of re-writing the same logic in virtio driver.
> > > > > >
> > > > > > Signed-off-by: Max Gurtovoy
> > > > > Dropped this as it caused build failures:
> > > > >
> > > > > https://lore.kernel.org/r/202210080424.gSmuYfb0-lkp%40intel.com
> > > > maybe you can re-run it with:
> > > >
> > > > diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
> > > > index 8e98d24917cc..b383326a20e2 100644
> > > > --- a/drivers/virtio/Makefile
> > > > +++ b/drivers/virtio/Makefile
> > > > @@ -5,10 +5,11 @@ obj-$(CONFIG_VIRTIO_PCI_LIB) +=
> > > > virtio_pci_modern_dev.o
> > > > obj-$(CONFIG_VIRTIO_PCI_LIB_LEGACY) += virtio_pci_legacy_dev.o
> > > > obj-$(CONFIG_VIRTIO_MMIO) += virtio_mmio.o
> > > > obj-$(CONFIG_VIRTIO_PCI) += virtio_pci.o
> > > > -virtio_pci-y := virtio_pci_modern.o virtio_pci_common.o
> > > > -virtio_pci-$(CONFIG_VIRTIO_PCI_LEGACY) += virtio_pci_legacy.o
> > > > obj-$(CONFIG_VIRTIO_BALLOON) += virtio_balloon.o
> > > > obj-$(CONFIG_VIRTIO_INPUT) += virtio_input.o
> > > > obj-$(CONFIG_VIRTIO_VDPA) += virtio_vdpa.o
> > > > obj-$(CONFIG_VIRTIO_MEM) += virtio_mem.o
> > > > obj-$(CONFIG_VIRTIO_DMA_SHARED_BUFFER) += virtio_dma_buf.o
> > > > +
> > > > +virtio_pci-$(CONFIG_VIRTIO_PCI) := virtio_pci_modern.o
> > > > virtio_pci_common.o
> > > > +virtio_pci-$(CONFIG_VIRTIO_PCI_LEGACY) += virtio_pci_legacy.o
> > > >
> > > Now I saw that CONFIG_PCI_IOV is not set in the error log so the bellow
> > > should fix it:
> > >
> > > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > > index 060af91bafcd..c519220e8ff8 100644
> > > --- a/include/linux/pci.h
> > > +++ b/include/linux/pci.h
> > > @@ -2228,7 +2228,10 @@ static inline int pci_sriov_set_totalvfs(struct
> > > pci_dev *dev, u16 numvfs)
> > >{ return 0; }
> > >static inline int pci_sriov_get_totalvfs(struct pci_dev *dev)
> > >{ return 0; }
> > > -#define pci_sriov_configure_simple NULL
> > > +static inline int pci_sriov_configure_simple(struct pci_dev *dev, int
> > > nr_virtfn)
> > > +{
> > > + return -ENOSYS;
> > > +}
> > >static inline resource_size_t pci_iov_resource_size(struct pci_dev
> > > *dev, int resno)
> > >{ return 0; }
> > >static inline void pci_vf_drivers_autoprobe(struct pci_dev *dev, bool
> > > probe) { }
> > >
> > > Bjorn,
> > >
> > > WDYT about the above ?
> > >
> > > should I send it to the pci subsystem list ?
> > Yes. I don't apply things that haven't appeared on
> > linux-...@vger.kernel.org.
>
> Sure.
>
> MST,
>
> can you confirm the above fixes the build errors before I sent the v2 ?
Max, please just use the lkp test, it's not hard.
--
MST
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio_pci: use irq to detect interrupt support
On Wed, Oct 12, 2022 at 3:04 PM Michael S. Tsirkin 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, "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
Re: [GIT PULL] virtio: fixes, features
On Wed, Oct 12, 2022 at 11:06:54PM +0200, Arnd Bergmann wrote: > On Wed, Oct 12, 2022, at 7:22 PM, Linus Torvalds wrote: > > > > The NO_IRQ thing is mainly actually defined by a few drivers that just > > never got converted to the proper world order, and even then you can > > see the confusion (ie some drivers use "-1", others use "0", and yet > > others use "((unsigned int)(-1)". > > The last time I looked at removing it for arch/arm/, one problem was > that there were a number of platforms using IRQ 0 as a valid number. > We have converted most of them in the meantime, leaving now only > mach-rpc and mach-footbridge. For the other platforms, we just > renumbered all interrupts to add one, but footbridge apparently > relies on hardcoded ISA interrupts in device drivers. For rpc, > it looks like IRQ 0 (printer) already wouldn't work, and it > looks like there was never a driver referencing it either. Do these two boxes even have pci? > I see that openrisc and parisc also still define NO_IRQ to -1, but at > least openrisc already relies on 0 being the invalid IRQ (from > CONFIG_IRQ_DOMAIN), probably parisc as well. > > Arnd ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[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
Reported-by: Michael Ellerman
Fixes: 71491c54eafa ("virtio_pci: don't try to use intxif pin is zero")
Cc: "Angus Chen"
Signed-off-by: Michael S. Tsirkin
---
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
Re: [PATCH 1/1] virtio_pci: use common helper to configure SR-IOV
On Wed, Oct 12, 2022 at 5:01 AM Max Gurtovoy wrote:
>
>
> On 10/12/2022 11:42 AM, Max Gurtovoy wrote:
> >
> > On 10/12/2022 8:02 AM, Michael S. Tsirkin wrote:
> >> On Thu, Sep 29, 2022 at 02:40:08AM +0300, Max Gurtovoy wrote:
> >>> This is instead of re-writing the same logic in virtio driver.
> >>>
> >>> Signed-off-by: Max Gurtovoy
> >> Dropped this as it caused build failures:
> >>
> >> https://lore.kernel.org/r/202210080424.gSmuYfb0-lkp%40intel.com
> >
> > maybe you can re-run it with:
> >
> > diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
> > index 8e98d24917cc..b383326a20e2 100644
> > --- a/drivers/virtio/Makefile
> > +++ b/drivers/virtio/Makefile
> > @@ -5,10 +5,11 @@ obj-$(CONFIG_VIRTIO_PCI_LIB) += virtio_pci_modern_dev.o
> > obj-$(CONFIG_VIRTIO_PCI_LIB_LEGACY) += virtio_pci_legacy_dev.o
> > obj-$(CONFIG_VIRTIO_MMIO) += virtio_mmio.o
> > obj-$(CONFIG_VIRTIO_PCI) += virtio_pci.o
> > -virtio_pci-y := virtio_pci_modern.o virtio_pci_common.o
> > -virtio_pci-$(CONFIG_VIRTIO_PCI_LEGACY) += virtio_pci_legacy.o
> > obj-$(CONFIG_VIRTIO_BALLOON) += virtio_balloon.o
> > obj-$(CONFIG_VIRTIO_INPUT) += virtio_input.o
> > obj-$(CONFIG_VIRTIO_VDPA) += virtio_vdpa.o
> > obj-$(CONFIG_VIRTIO_MEM) += virtio_mem.o
> > obj-$(CONFIG_VIRTIO_DMA_SHARED_BUFFER) += virtio_dma_buf.o
> > +
> > +virtio_pci-$(CONFIG_VIRTIO_PCI) := virtio_pci_modern.o
> > virtio_pci_common.o
> > +virtio_pci-$(CONFIG_VIRTIO_PCI_LEGACY) += virtio_pci_legacy.o
> >
>
> Now I saw that CONFIG_PCI_IOV is not set in the error log so the bellow
> should fix it:
>
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 060af91bafcd..c519220e8ff8 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -2228,7 +2228,10 @@ static inline int pci_sriov_set_totalvfs(struct
> pci_dev *dev, u16 numvfs)
> { return 0; }
> static inline int pci_sriov_get_totalvfs(struct pci_dev *dev)
> { return 0; }
> -#define pci_sriov_configure_simple NULL
> +static inline int pci_sriov_configure_simple(struct pci_dev *dev, int
> nr_virtfn)
> +{
> + return -ENOSYS;
> +}
> static inline resource_size_t pci_iov_resource_size(struct pci_dev
> *dev, int resno)
> { return 0; }
> static inline void pci_vf_drivers_autoprobe(struct pci_dev *dev, bool
> probe) { }
>
> Bjorn,
>
> WDYT about the above ?
>
> should I send it to the pci subsystem list ?
Yes. I don't apply things that haven't appeared on linux-...@vger.kernel.org.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [GIT PULL] virtio: fixes, features
On Wed, Oct 12, 2022, at 7:22 PM, Linus Torvalds wrote: > > The NO_IRQ thing is mainly actually defined by a few drivers that just > never got converted to the proper world order, and even then you can > see the confusion (ie some drivers use "-1", others use "0", and yet > others use "((unsigned int)(-1)". The last time I looked at removing it for arch/arm/, one problem was that there were a number of platforms using IRQ 0 as a valid number. We have converted most of them in the meantime, leaving now only mach-rpc and mach-footbridge. For the other platforms, we just renumbered all interrupts to add one, but footbridge apparently relies on hardcoded ISA interrupts in device drivers. For rpc, it looks like IRQ 0 (printer) already wouldn't work, and it looks like there was never a driver referencing it either. I see that openrisc and parisc also still define NO_IRQ to -1, but at least openrisc already relies on 0 being the invalid IRQ (from CONFIG_IRQ_DOMAIN), probably parisc as well. Arnd ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [GIT PULL] virtio: fixes, features
On Wed, Oct 12, 2022 at 8:51 AM Michael S. Tsirkin wrote: > > Are you sure? MichaelE is right. This is just bogus historical garbage: > arch/arm/include/asm/irq.h:#ifndef NO_IRQ > arch/arm/include/asm/irq.h:#define NO_IRQ ((unsigned int)(-1)) that I've tried to get rid of for years, but for some reason it just won't die. NO_IRQ should be zero. Or rather, it shouldn't exist at all. It's a bogus thing. You can see just how bogus it is from grepping for it - the users are all completely and utterly confused, and all are entirely historical brokenness. The correct way to check for "no irq" doesn't use NO_IRQ at all, it just does if (dev->irq) ... which is why you will only find a few instances of NO_IRQ in the tree in the first place. The NO_IRQ thing is mainly actually defined by a few drivers that just never got converted to the proper world order, and even then you can see the confusion (ie some drivers use "-1", others use "0", and yet others use "((unsigned int)(-1)". Linus ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [GIT PULL] virtio: fixes, features
On Thu, Oct 13, 2022 at 01:33:59AM +1100, Michael Ellerman wrote:
> Michael Ellerman writes:
> > [ Cc += Bjorn & linux-pci ]
> >
> > "Michael S. Tsirkin" writes:
> >> On Wed, Oct 12, 2022 at 05:21:24PM +1100, Michael Ellerman wrote:
> >>> "Michael S. Tsirkin" writes:
> > ...
> >>> >
> >>> > virtio: fixes, features
> >>> >
> >>> > 9k mtu perf improvements
> >>> > vdpa feature provisioning
> >>> > virtio blk SECURE ERASE support
> >>> >
> >>> > Fixes, cleanups all over the place.
> >>> >
> >>> > Signed-off-by: Michael S. Tsirkin
> >>> >
> >>> >
> >>> > Alvaro Karsz (1):
> >>> > virtio_blk: add SECURE ERASE command support
> >>> >
> >>> > Angus Chen (1):
> >>> > virtio_pci: don't try to use intxif pin is zero
> >>>
> >>> This commit breaks virtio_pci for me 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.
> >>>
> >>> I think this happens because pci_dev->pin is not populated in
> >>> pci_assign_irq().
> >>>
> >>> I would absolutely believe this is bug in our PCI code, but I think it
> >>> may also affect other platforms that use of_irq_parse_and_map_pci().
> >>
> >> How about fixing this in of_irq_parse_and_map_pci then?
> >> Something like the below maybe?
> >>
> >> diff --git a/drivers/pci/of.c b/drivers/pci/of.c
> >> index 196834ed44fe..504c4d75c83f 100644
> >> --- a/drivers/pci/of.c
> >> +++ b/drivers/pci/of.c
> >> @@ -446,6 +446,8 @@ static int of_irq_parse_pci(const struct pci_dev
> >> *pdev, struct of_phandle_args *
> >>if (pin == 0)
> >>return -ENODEV;
> >>
> >> + pdev->pin = pin;
> >> +
> >>/* Local interrupt-map in the device node? Use it! */
> >>if (of_get_property(dn, "interrupt-map", NULL)) {
> >>pin = pci_swizzle_interrupt_pin(pdev, pin);
>
> Backing up a bit. Should the virtio code be looking at pci_dev->pin in
> the first place?
>
> Shouldn't it be checking pci_dev->irq instead?
>
> The original commit talks about irq being 0 and colliding with the timer
> interrupt.
>
> But all (most?) platforms have converged on 0 meaning NO_IRQ since quite
> a fews ago AFAIK.
Are you sure?
arch/arm/include/asm/irq.h:#ifndef NO_IRQ
arch/arm/include/asm/irq.h:#define NO_IRQ ((unsigned int)(-1))
> And the timer irq == 0 is a special case AIUI:
>
> https://lore.kernel.org/all/ca+55afwilp1z+2mzkrfsid1wzq0tqkcn8f2e6nl_avr+m1f...@mail.gmail.com/
>
> cheers
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [GIT PULL] virtio: fixes, features
Michael Ellerman writes:
> [ Cc += Bjorn & linux-pci ]
>
> "Michael S. Tsirkin" writes:
>> On Wed, Oct 12, 2022 at 05:21:24PM +1100, Michael Ellerman wrote:
>>> "Michael S. Tsirkin" writes:
> ...
>>> >
>>> > virtio: fixes, features
>>> >
>>> > 9k mtu perf improvements
>>> > vdpa feature provisioning
>>> > virtio blk SECURE ERASE support
>>> >
>>> > Fixes, cleanups all over the place.
>>> >
>>> > Signed-off-by: Michael S. Tsirkin
>>> >
>>> >
>>> > Alvaro Karsz (1):
>>> > virtio_blk: add SECURE ERASE command support
>>> >
>>> > Angus Chen (1):
>>> > virtio_pci: don't try to use intxif pin is zero
>>>
>>> This commit breaks virtio_pci for me 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.
>>>
>>> I think this happens because pci_dev->pin is not populated in
>>> pci_assign_irq().
>>>
>>> I would absolutely believe this is bug in our PCI code, but I think it
>>> may also affect other platforms that use of_irq_parse_and_map_pci().
>>
>> How about fixing this in of_irq_parse_and_map_pci then?
>> Something like the below maybe?
>>
>> diff --git a/drivers/pci/of.c b/drivers/pci/of.c
>> index 196834ed44fe..504c4d75c83f 100644
>> --- a/drivers/pci/of.c
>> +++ b/drivers/pci/of.c
>> @@ -446,6 +446,8 @@ static int of_irq_parse_pci(const struct pci_dev *pdev,
>> struct of_phandle_args *
>> if (pin == 0)
>> return -ENODEV;
>>
>> +pdev->pin = pin;
>> +
>> /* Local interrupt-map in the device node? Use it! */
>> if (of_get_property(dn, "interrupt-map", NULL)) {
>> pin = pci_swizzle_interrupt_pin(pdev, pin);
Backing up a bit. Should the virtio code be looking at pci_dev->pin in
the first place?
Shouldn't it be checking pci_dev->irq instead?
The original commit talks about irq being 0 and colliding with the timer
interrupt.
But all (most?) platforms have converged on 0 meaning NO_IRQ since quite
a fews ago AFAIK.
And the timer irq == 0 is a special case AIUI:
https://lore.kernel.org/all/ca+55afwilp1z+2mzkrfsid1wzq0tqkcn8f2e6nl_avr+m1f...@mail.gmail.com/
cheers
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio_pci: read interrupt pin directly
"Michael S. Tsirkin" 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().
>
> Work around the issue in virtio for now, and let's try to fix
> all affected pci systems and then we can revert this.
>
> Reported-by: Michael Ellerman
> Fixes: 71491c54eafa ("virtio_pci: don't try to use intxif pin is zero")
> Cc: "Angus Chen"
> Signed-off-by: Michael S. Tsirkin
> ---
> drivers/virtio/virtio_pci_common.c | 10 --
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/virtio/virtio_pci_common.c
> b/drivers/virtio/virtio_pci_common.c
> index 4df77eeb4d16..6155ea4e7e4b 100644
> --- a/drivers/virtio/virtio_pci_common.c
> +++ b/drivers/virtio/virtio_pci_common.c
> @@ -400,6 +400,7 @@ int vp_find_vqs(struct virtio_device *vdev, unsigned int
> nvqs,
> struct irq_affinity *desc)
> {
> int err;
> + u8 pin = 0;
>
> /* Try MSI-X with one vector per queue. */
> err = vp_find_vqs_msix(vdev, nvqs, vqs, callbacks, names, true, ctx,
> desc);
> @@ -409,8 +410,13 @@ 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 pin? If not give up.
> + * NB: It would seem to be better to use pci_dev->pin - unfortunately
> + * not all platforms populate it.
> + */
> + pci_read_config_byte(dev, PCI_INTERRUPT_PIN, );
> + if (!pin)
> return err;
> /* Finally fall back to regular interrupts. */
> return vp_find_vqs_intx(vdev, nvqs, vqs, callbacks, names, ctx);
Needs the delta below in order to compile.
But with that it fixes the issue for me.
Tested-by: Michael Ellerman
cheers
diff --git a/drivers/virtio/virtio_pci_common.c
b/drivers/virtio/virtio_pci_common.c
index 6155ea4e7e4b..cae134e2573f 100644
--- a/drivers/virtio/virtio_pci_common.c
+++ b/drivers/virtio/virtio_pci_common.c
@@ -415,7 +415,7 @@ int vp_find_vqs(struct virtio_device *vdev, unsigned int
nvqs,
* NB: It would seem to be better to use pci_dev->pin - unfortunately
* not all platforms populate it.
*/
- pci_read_config_byte(dev, PCI_INTERRUPT_PIN, );
+ pci_read_config_byte(to_vp_device(vdev)->pci_dev, PCI_INTERRUPT_PIN,
);
if (!pin)
return err;
/* Finally fall back to regular interrupts. */
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [GIT PULL] virtio: fixes, features
[ Cc += Bjorn & linux-pci ]
"Michael S. Tsirkin" writes:
> On Wed, Oct 12, 2022 at 05:21:24PM +1100, Michael Ellerman wrote:
>> "Michael S. Tsirkin" writes:
...
>> >
>> > virtio: fixes, features
>> >
>> > 9k mtu perf improvements
>> > vdpa feature provisioning
>> > virtio blk SECURE ERASE support
>> >
>> > Fixes, cleanups all over the place.
>> >
>> > Signed-off-by: Michael S. Tsirkin
>> >
>> >
>> > Alvaro Karsz (1):
>> > virtio_blk: add SECURE ERASE command support
>> >
>> > Angus Chen (1):
>> > virtio_pci: don't try to use intxif pin is zero
>>
>> This commit breaks virtio_pci for me 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.
>>
>> I think this happens because pci_dev->pin is not populated in
>> pci_assign_irq().
>>
>> I would absolutely believe this is bug in our PCI code, but I think it
>> may also affect other platforms that use of_irq_parse_and_map_pci().
>
> How about fixing this in of_irq_parse_and_map_pci then?
> Something like the below maybe?
>
> diff --git a/drivers/pci/of.c b/drivers/pci/of.c
> index 196834ed44fe..504c4d75c83f 100644
> --- a/drivers/pci/of.c
> +++ b/drivers/pci/of.c
> @@ -446,6 +446,8 @@ static int of_irq_parse_pci(const struct pci_dev *pdev,
> struct of_phandle_args *
> if (pin == 0)
> return -ENODEV;
>
> + pdev->pin = pin;
> +
> /* Local interrupt-map in the device node? Use it! */
> if (of_get_property(dn, "interrupt-map", NULL)) {
> pin = pci_swizzle_interrupt_pin(pdev, pin);
That doesn't fix it in all cases, because there's an early return if
there's a struct device_node associated with the pci_dev, before we even
read the pin.
Also the pci_dev is const, and removing the const would propagate to a
few other places.
The other obvious place to fix it would be in pci_assign_irq(), as
below. That fixes this bug for me, but is otherwise very lightly tested.
cheers
diff --git a/drivers/pci/setup-irq.c b/drivers/pci/setup-irq.c
index cc7d26b015f3..0135413b33af 100644
--- a/drivers/pci/setup-irq.c
+++ b/drivers/pci/setup-irq.c
@@ -22,6 +22,15 @@ void pci_assign_irq(struct pci_dev *dev)
int irq = 0;
struct pci_host_bridge *hbrg = pci_find_host_bridge(dev->bus);
+ /* Make sure dev->pin is populated */
+ pci_read_config_byte(dev, PCI_INTERRUPT_PIN, );
+
+ /* Cope with illegal. */
+ if (pin > 4)
+ pin = 1;
+
+ dev->pin = pin;
+
if (!(hbrg->map_irq)) {
pci_dbg(dev, "runtime IRQ mapping not provided by arch\n");
return;
@@ -34,11 +43,6 @@ void pci_assign_irq(struct pci_dev *dev)
* time the interrupt line passes through a PCI-PCI bridge we must
* apply the swizzle function.
*/
- pci_read_config_byte(dev, PCI_INTERRUPT_PIN, );
- /* Cope with illegal. */
- if (pin > 4)
- pin = 1;
-
if (pin) {
/* Follow the chain of bridges, swizzling as we go. */
if (hbrg->swizzle_irq)
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [GIT PULL] virtio: fixes, features
On Wed, Oct 12, 2022 at 05:21:24PM +1100, Michael Ellerman wrote:
> "Michael S. Tsirkin" writes:
> > The following changes since commit 4fe89d07dcc2804c8b562f6c7896a45643d34b2f:
> >
> > Linux 6.0 (2022-10-02 14:09:07 -0700)
> >
> > are available in the Git repository at:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git
> > tags/for_linus
> >
> > for you to fetch changes up to 71491c54eafa318fdd24a1f26a1c82b28e1ac21d:
> >
> > virtio_pci: don't try to use intxif pin is zero (2022-10-07 20:00:44
> > -0400)
> >
> >
> > virtio: fixes, features
> >
> > 9k mtu perf improvements
> > vdpa feature provisioning
> > virtio blk SECURE ERASE support
> >
> > Fixes, cleanups all over the place.
> >
> > Signed-off-by: Michael S. Tsirkin
> >
> >
> > Alvaro Karsz (1):
> > virtio_blk: add SECURE ERASE command support
> >
> > Angus Chen (1):
> > virtio_pci: don't try to use intxif pin is zero
>
> This commit breaks virtio_pci for me 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.
>
> I think this happens because pci_dev->pin is not populated in
> pci_assign_irq().
>
> I would absolutely believe this is bug in our PCI code, but I think it
> may also affect other platforms that use of_irq_parse_and_map_pci().
>
> cheers
How about fixing this in of_irq_parse_and_map_pci then?
Something like the below maybe?
diff --git a/drivers/pci/of.c b/drivers/pci/of.c
index 196834ed44fe..504c4d75c83f 100644
--- a/drivers/pci/of.c
+++ b/drivers/pci/of.c
@@ -446,6 +446,8 @@ static int of_irq_parse_pci(const struct pci_dev *pdev,
struct of_phandle_args *
if (pin == 0)
return -ENODEV;
+ pdev->pin = pin;
+
/* Local interrupt-map in the device node? Use it! */
if (of_get_property(dn, "interrupt-map", NULL)) {
pin = pci_swizzle_interrupt_pin(pdev, pin);
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH] virtio_pci: read interrupt pin directly
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().
Work around the issue in virtio for now, and let's try to fix
all affected pci systems and then we can revert this.
Reported-by: Michael Ellerman
Fixes: 71491c54eafa ("virtio_pci: don't try to use intxif pin is zero")
Cc: "Angus Chen"
Signed-off-by: Michael S. Tsirkin
---
drivers/virtio/virtio_pci_common.c | 10 --
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/virtio/virtio_pci_common.c
b/drivers/virtio/virtio_pci_common.c
index 4df77eeb4d16..6155ea4e7e4b 100644
--- a/drivers/virtio/virtio_pci_common.c
+++ b/drivers/virtio/virtio_pci_common.c
@@ -400,6 +400,7 @@ int vp_find_vqs(struct virtio_device *vdev, unsigned int
nvqs,
struct irq_affinity *desc)
{
int err;
+ u8 pin = 0;
/* Try MSI-X with one vector per queue. */
err = vp_find_vqs_msix(vdev, nvqs, vqs, callbacks, names, true, ctx,
desc);
@@ -409,8 +410,13 @@ 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 pin? If not give up.
+* NB: It would seem to be better to use pci_dev->pin - unfortunately
+* not all platforms populate it.
+*/
+ pci_read_config_byte(dev, PCI_INTERRUPT_PIN, );
+ if (!pin)
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
[PATCH RESEND] tools/virtio: initialize spinlocks in vring_test.c
The virtio_device vqs_list spinlocks must be initialized before use to
prevent functions that manipulate the device virtualqueues, such as
vring_new_virtqueue(), from blocking indefinitely.
Signed-off-by: Ricardo Cañuelo
---
tools/virtio/vringh_test.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/tools/virtio/vringh_test.c b/tools/virtio/vringh_test.c
index fa87b58bd5fa..98ff808d6f0c 100644
--- a/tools/virtio/vringh_test.c
+++ b/tools/virtio/vringh_test.c
@@ -308,6 +308,7 @@ static int parallel_test(u64 features,
gvdev.vdev.features = features;
INIT_LIST_HEAD();
+ spin_lock_init(_list_lock);
gvdev.to_host_fd = to_host[1];
gvdev.notifies = 0;
@@ -455,6 +456,7 @@ int main(int argc, char *argv[])
getrange = getrange_iov;
vdev.features = 0;
INIT_LIST_HEAD();
+ spin_lock_init(_list_lock);
while (argv[1]) {
if (strcmp(argv[1], "--indirect") == 0)
--
2.25.1
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [GIT PULL] virtio: fixes, features
"Michael S. Tsirkin" writes: > The following changes since commit 4fe89d07dcc2804c8b562f6c7896a45643d34b2f: > > Linux 6.0 (2022-10-02 14:09:07 -0700) > > are available in the Git repository at: > > https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git tags/for_linus > > for you to fetch changes up to 71491c54eafa318fdd24a1f26a1c82b28e1ac21d: > > virtio_pci: don't try to use intxif pin is zero (2022-10-07 20:00:44 -0400) > > > virtio: fixes, features > > 9k mtu perf improvements > vdpa feature provisioning > virtio blk SECURE ERASE support > > Fixes, cleanups all over the place. > > Signed-off-by: Michael S. Tsirkin > > > Alvaro Karsz (1): > virtio_blk: add SECURE ERASE command support > > Angus Chen (1): > virtio_pci: don't try to use intxif pin is zero This commit breaks virtio_pci for me 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. I think this happens because pci_dev->pin is not populated in pci_assign_irq(). I would absolutely believe this is bug in our PCI code, but I think it may also affect other platforms that use of_irq_parse_and_map_pci(). cheers ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
