Re: [PATCH v2 1/2] PCI/MSI: Fix the confusing IRQ sysfs ABI for MSI-X

On Mon, Aug 23, 2021 at 10:30 PM Marc Zyngier [off-list ref] wrote:

On Sat, 21 Aug 2021 23:14:35 +0100,
Barry Song [off-list ref] wrote:

quoted

On Sat, Aug 21, 2021 at 10:42 PM Marc Zyngier [off-list ref] wrote:

quoted

Hi Bjorn,

On Sat, 21 Aug 2021 00:33:28 +0100,
Bjorn Helgaas [off-list ref] wrote:

quoted

[...]

quoted

quoted

quoted

    In msix_setup_entries(), we get nvecs msi_entry structs, and we
    get a saved .default_irq in each one?

That's a key point.

Old-school PCI/MSI is represented by a single interrupt, and you
*could* somehow make it relatively easy for drivers that only
understand INTx to migrate to MSI if you replaced whatever is held in
dev->irq (which should only represent the INTx mapping) with the MSI
interrupt number. Which I guess is what the MSI code is doing.

This is the 21st century, and nobody should ever rely on such horror,
but I'm sure we do have such drivers in the tree. Boo.

However, this *cannot* hold true for Multi-MSI, nor MSI-X, because
there is a plurality of interrupts. Even worse, for MSI-X, there is
zero guarantee that the allocated interrupts will be in a contiguous
space.

Given that, what is dev->irq good for? "Absolutely Nothing! (say it
again!)".

The only thing is that dev->irq is an sysfs ABI to userspace. Due to
the inconsistency between legacy PCI INTx, MSI, MSI-X, this ABI
should have been absolutely broken nowadays.  This is actually what
the patchset was originally aiming at to fix.

I do not think we should expose more of a broken abstraction to
userspace. We will have to carry on exposing the first MSI in this
field forever, but it doesn't mean we should have to do it for MSI-X.

quoted

One more question from me is that does dev->irq actually hold any
valid hardware INTx information while hardware is using MSI-X? At
least in my hardware, sysfs ABI for PCI is all "0".

That's probably because nothing actually configured the interrupt, or
that there is no INTx implementation. I have that on systems with
pretty dodgy (or incomplete) firmware.

quoted

root@ubuntu:/sys/devices/pci0000:7c/0000:7c:00.0/0000:7d:00.3# cat irq
0

root@ubuntu:/sys/devices/pci0000:7c/0000:7c:00.0/0000:7d:00.3# ls -l msi_irqs/*
-r--r--r-- 1 root root 4096 Aug 21 22:04 msi_irqs/499
-r--r--r-- 1 root root 4096 Aug 21 22:04 msi_irqs/500
-r--r--r-- 1 root root 4096 Aug 21 22:04 msi_irqs/501
...
root@ubuntu:/sys/devices/pci0000:7c/0000:7c:00.0/0000:7d:00.3# cat msi_irqs/499
msix

Not quite sure how it is going on different hardware platforms.

My D05 does that as well, and it doesn't expose any INTx support.

quoted

quoted

MSI-X is not something you can "accidentally" use. You have to
actively embrace it. In all honesty, this patch tries to move in the
wrong direction. If anything, we should kill this hack altogether and
fix the (handful of?) drivers that rely on it. That'd actually be a
good way to find whether they are still worth keeping in the tree. And
if it breaks too many of them, then at least we'll know where we
stand.

I'd be tempted to leave the below patch simmer in -next for a few
weeks and see if how many people shout:

This looks like a more proper direction to go.
but here i am wondering how sysfs ABI document should follow the below change
doc is patch 2/2:
https://lore.kernel.org/lkml/20210820223744.8439-3-21cnbao@gmail.com/ (local)

On the other hand, my feeling is that nobody should depend on sysfs
irq entry nowadays.

Too late. It is there, and we need to preserve it. I just don't think
feeding it more erroneous information is the right thing to do.

My patch was only dealing with the kernel side of things, not the
userspace ABI. That ABI should be carried on unchanged.

it seems this isn't true. your patch is also changing userspace ABI as
long as you change pci_dev->irq
which will be shown in sysfs irq entry.

if we don't want to change the behaviour of any existing ABI, it seems
the only thing we can do here
to document it well in ABI doc. i actually doubt anyone has really
understood what the irq entry
is really showing.

quoted

For example, userspace irqbalance is actually using
/sys/devices/.../msi_irqs/ So probably we should set this ABI
invisible when devices are using MSI or MSI-X?

Can it actually be made optional? I don't believe we can.

Thanks,

        M.

--
Without deviation from the norm, progress is not possible.

Thanks
barry