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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87lf4pq2mf.wl-maz@kernel.org>
Date:   Wed, 25 Aug 2021 10:45:28 +0100
From:   Marc Zyngier <maz@...nel.org>
To:     Bjorn Helgaas <helgaas@...nel.org>
Cc:     Barry Song <21cnbao@...il.com>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        Jonathan Corbet <corbet@....net>, Jonathan.Cameron@...wei.com,
        bilbao@...edu, Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        leon@...nel.org, LKML <linux-kernel@...r.kernel.org>,
        linux-pci@...r.kernel.org, Linuxarm <linuxarm@...wei.com>,
        luzmaximilian@...il.com, mchehab+huawei@...nel.org,
        schnelle@...ux.ibm.com, Barry Song <song.bao.hua@...ilicon.com>,
        Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH v2 1/2] PCI/MSI: Fix the confusing IRQ sysfs ABI for MSI-X

On Tue, 24 Aug 2021 20:34:38 +0100,
Bjorn Helgaas <helgaas@...nel.org> wrote:
> 
> On Tue, Aug 24, 2021 at 10:46:59AM +1200, Barry Song wrote:
> > On Mon, Aug 23, 2021 at 11:28 PM Marc Zyngier <maz@...nel.org> wrote:
> > >
> > > On Mon, 23 Aug 2021 12:03:08 +0100,
> > > Barry Song <21cnbao@...il.com> wrote:
> 
> > +static ssize_t irq_show(struct device *dev,
> > +                                        struct device_attribute *attr,
> > +                                        char *buf)
> > +{
> > +       struct pci_dev *pdev = to_pci_dev(dev);
> > +#ifdef CONFIG_PCI_MSI
> > +       struct msi_desc *desc = first_pci_msi_entry(pdev);
> > +
> > +       /* for MSI, return the 1st IRQ in IRQ vector */
> > +       if (desc && !desc->msi_attrib.is_msix)
> > +               return sysfs_emit(buf, "%u\n", desc->irq);
> > +#endif
> > +
> > +       return sysfs_emit(buf, "%u\n", pdev->irq);
> > +}
> > +static DEVICE_ATTR_RO(irq);
> 
> Makes sense to me.  And with Marc's patch maybe we could get rid of
> default_irq, which also seems nice.
> 
> > > > 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.
> > >
> > > Given that we can't prove that it is actually the case, I believe this
> > > is the only option.
> > 
> > we have to document the ABI like below though it seems quite annoying.
> > 
> > 1. for devices which don't support MSI and MSI-X, show legacy INTx
> > 2. for devices which support MSI
> >     a. if CONFIG_PCI_MSI is not enabled,  show legacy INTx
> >     b. if CONFIG_PCI_MSI is enabled and devices are using MSI at this
> > moment, show 1st IRQ in the vector
> >     c. if CONFIG_PCI_MSI is enabled, but we shutdown its MSI before
> > the users call sysfs entry,
> >         so at this moment, devices are not using MSI,  show legacy INTx
> > 3. for devices which support MSI-X, no matter if it is using MSI-X,
> >     show legacy INTx
> > 4. In Addition, INTx might be broken due to incomplete firmware or
> > hardware design for MSI and MSI-X cases
> > 
> > To be honest, it sounds like a disaster :-) but if this is what we
> > have to do, I'd like to try it in v3.
> 
> It doesn't seem necessary to me to get into the gory details of
> CONFIG_PCI_MSI -- if that's not enabled, drivers can't use MSI anyway.
> 
> I don't understand 3.  If a device supports both MSI and MSI-X and a
> driver enables MSI, msi_capability_init() writes dev->irq, so it looks
> like "irq" should contain the first MSI vector.
> 
> I don't understand 4, either.  Is the possibility of broken hardware
> or firmware something we need to document?  
> 
> What about something like this?
> 
>   If a driver has enabled MSI (not MSI-X), "irq" contains the IRQ of
>   the first MSI vector.  Otherwise "irq" contains the IRQ of the
>   legacy INTx interrupt.
> 

I think that pretty much nails it. CONFIG_MSI is not something that
userspace can (nor should) discover anyway.

For (4), you may want to add that

  "irq" being set to 0 indicates that the device isn't capable of
  generating legacy INTx interrupts.

Thanks,

	M.

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