linux-kernel - Re: [PATCH 1/2] cxl/pci: Add generic MSI-X/MSI irq support

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]
Date:   Sun, 23 Oct 2022 19:08:57 -0700
From:   Dan Williams <dan.j.williams@...el.com>
To:     Ira Weiny <ira.weiny@...el.com>,
        Dan Williams <dan.j.williams@...el.com>
CC:     Davidlohr Bueso <dave@...olabs.net>, <Jonathan.Cameron@...wei.com>,
        <dave.jiang@...el.com>, <alison.schofield@...el.com>,
        <bwidawsk@...nel.org>, <vishal.l.verma@...el.com>,
        <a.manzanares@...sung.com>, <linux-cxl@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/2] cxl/pci: Add generic MSI-X/MSI irq support

Ira Weiny wrote:
> On Sat, Oct 22, 2022 at 03:05:45PM -0700, Dan Williams wrote:
> > Davidlohr Bueso wrote:
> > > Introduce a generic irq table for CXL components/features that can have
> > > standard irq support - DOE requires dynamic vector sizing and is not
> > > considered here. For now the table is empty.
> > > 
> > > Create an infrastructure to query the max vectors required for the CXL
> > > device. Upon successful allocation, users can plug in their respective isr
> > > at any point thereafter, which is supported by a new cxlds->has_irq flag,
> > > for example, if the irq setup is not done in the PCI driver, such as
> > > the case of the CXL-PMU.
> > > 
> > > Reviewed-by: Dave Jiang <dave.jiang@...el.com>
> > > Signed-off-by: Davidlohr Bueso <dave@...olabs.net>
> > > ---
> > >  drivers/cxl/cxlmem.h |  3 ++
> > >  drivers/cxl/pci.c    | 72 ++++++++++++++++++++++++++++++++++++++++++++
> > >  2 files changed, 75 insertions(+)
> > > 
> > > diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> > > index 88e3a8e54b6a..72b69b003302 100644
> > > --- a/drivers/cxl/cxlmem.h
> > > +++ b/drivers/cxl/cxlmem.h
> > > @@ -211,6 +211,7 @@ struct cxl_endpoint_dvsec_info {
> > >   * @info: Cached DVSEC information about the device.
> > >   * @serial: PCIe Device Serial Number
> > >   * @doe_mbs: PCI DOE mailbox array
> > > + * @has_irq: PCIe MSI-X/MSI support
> > >   * @mbox_send: @dev specific transport for transmitting mailbox commands
> > >   *
> > >   * See section 8.2.9.5.2 Capacity Configuration and Label Storage for
> > > @@ -247,6 +248,8 @@ struct cxl_dev_state {
> > >  
> > >  	struct xarray doe_mbs;
> > >  
> > > +	bool has_irq;
> > > +
> > >  	int (*mbox_send)(struct cxl_dev_state *cxlds, struct cxl_mbox_cmd *cmd);
> > >  };
> > >  
> > > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> > > index faeb5d9d7a7a..9c3e95ebaa26 100644
> > > --- a/drivers/cxl/pci.c
> > > +++ b/drivers/cxl/pci.c
> > > @@ -428,6 +428,73 @@ static void devm_cxl_pci_create_doe(struct cxl_dev_state *cxlds)
> > >  	}
> > >  }
> > >  
> > > +/**
> > > + * struct cxl_irq_cap - CXL feature that is capable of receiving MSI-X/MSI irqs.
> > > + *
> > > + * @name: Name of the device/component generating this interrupt.
> > > + * @get_max_msgnum: Get the feature's largest interrupt message number.  If the
> > > + *		    feature does not have the Interrupt Supported bit set, then
> > > + *		    return -1.
> > > + */
> > > +struct cxl_irq_cap {
> > > +	const char *name;
> > > +	int (*get_max_msgnum)(struct cxl_dev_state *cxlds);
> > 
> > Why is this a callback, why not just have the features populate their
> > irq numbers?
> 
> I think we have decided to forgo the callback but I'm not sure what you mean by
> 'populate their irq numbers'?
> 
> > 
> > > +};
> > > +
> > > +static const struct cxl_irq_cap cxl_irq_cap_table[] = {
> > > +	NULL
> > > +};
> > > +
> > > +static void cxl_pci_free_irq_vectors(void *data)
> > > +{
> > > +	pci_free_irq_vectors(data);
> > > +}
> > > +
> > > +/*
> > > + * Attempt to allocate the largest amount of necessary vectors.
> > > + *
> > > + * Returns 0 upon a successful allocation of *all* vectors, or a
> > > + * negative value otherwise.
> > > + */
> > > +static int cxl_pci_alloc_irq_vectors(struct cxl_dev_state *cxlds)
> > > +{
> > > +	struct device *dev = cxlds->dev;
> > > +	struct pci_dev *pdev = to_pci_dev(dev);
> > > +	int rc, i, vectors = -1;
> > > +
> > > +	for (i = 0; i < ARRAY_SIZE(cxl_irq_cap_table); i++) {
> > > +		int irq;
> > > +
> > > +		if (!cxl_irq_cap_table[i].get_max_msgnum)
> > > +			continue;
> > > +
> > > +		irq = cxl_irq_cap_table[i].get_max_msgnum(cxlds);
> > > +		vectors = max_t(int, irq, vectors);
> > > +	}
> > 
> > Forgive me if I have missed something, I only look at interrupt enable
> > code once every few years, and the APIs are always a bit different, but
> > is this not too early to read the message number? The number is not
> > stable until either MSI or MSI-X has been selected below at
> > pci_alloc_irq_vectors() time?
>  
> Well I keep getting wrapped around the axle on this one too.
> 
> This all started back when Jonathan originally attempted to allocate the
> maximum number of vectors a device _could_ allocate.  But it was recommended that
> we determine the max number first then allocate that number.
> 
> This seems like a chicken and egg issue.  How is the number not stable before
> calling pci_alloc_irq_vectors() when you need the max msg number in that call?

Are we talking about the same thing? I am talking about the value in the
"Interrupt Message Number" field. That depends on whether MSI or MSI-X
gets enabled. The number of vectors the device can support is static.

Since CXL is such an a la carte spec I think this is situation to just
specify a large number of amx vectors to pci_alloc_irq_vectors() and
then find out after the fact if all of the interrupt generators that
today's cxl_pci knows about in the device each got their own vector.