Re: [RFC PATCH 0/9] CXL: Read and clear event logs

[Davidlohr Bueso]

On Mon, 22 Aug 2022, Ira Weiny wrote:

> Generally it seems ok but I have questions below.
>
> > One thing I have not
> > considered in this is the DOE stuff.
>
> I think this is the harder thing to support because of needing to allow both
> the PCI layer and the CXL layer to create irqs.  Potentially at different
> times.

I agree.

> > -/* Register Block Identifier (RBI) */
> > -enum cxl_regloc_type {
> > -	CXL_REGLOC_RBI_EMPTY = 0,
> > -	CXL_REGLOC_RBI_COMPONENT,
> > -	CXL_REGLOC_RBI_VIRT,
> > -	CXL_REGLOC_RBI_MEMDEV,
> > -	CXL_REGLOC_RBI_TYPES
> > -};
>
> Why move this?

That was sloppy of me, sorry. I wanted to reuse struct cxlds forward declaration,
no idea why that diff formed.

> > -
> >  static inline resource_size_t cxl_regmap_to_base(struct pci_dev *pdev,
> > 						 struct cxl_register_map *map)
> >  {
> > @@ -75,4 +66,44 @@ int devm_cxl_port_enumerate_dports(struct cxl_port *port);
> >  struct cxl_dev_state;
> >  int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm);
> >  void read_cdat_data(struct cxl_port *port);
> > +
> > +#define CXL_IRQ_CAPABILITY_TABLE				\
> > +	C(ISOLATION, "isolation", NULL),			\
> > +	C(PMU, "pmu_overflow", NULL), /* per pmu instance */	\
> > +	C(MBOX, "mailbox", NULL), /* primary-only */		\
> > +	C(EVENT, "event", NULL),
>
> This is defining get_max_msgnum to NULL right?

Yes. So untl there are any users everything's a nop.

> > +
> > +#undef C
> > +#define C(a, b, c) CXL_IRQ_CAPABILITY_##a
> > +enum  { CXL_IRQ_CAPABILITY_TABLE };
> > +#undef C
> > +#define C(a, b, c) { b, c }
> > +/**
> > + * struct cxl_irq_cap - CXL feature that is capable of receiving MSI/MSI-X irqs.
> > + *
> > + * @name: Name of the device generating this interrupt.
> > + * @get_max_msgnum: Get the feature's largest interrupt message number. In cases
> > + *                  where there is only one instance it also indicates which
> > + *                  MSI/MSI-X vector is used for the interrupt message generated
> > + *                  in association with the feature. 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);
> > +};
> > +
> > +static const
> > +struct cxl_irq_cap cxl_irq_cap_table[] = { CXL_IRQ_CAPABILITY_TABLE };
> > +#undef C
>
> Why all this macro magic?

A nifty trick Dan likes, it avoids duplicating the fields (enums + the table).

> > +
> > +/* Register Block Identifier (RBI) */
> > +enum cxl_regloc_type {
> > +	CXL_REGLOC_RBI_EMPTY = 0,
> > +	CXL_REGLOC_RBI_COMPONENT,
> > +	CXL_REGLOC_RBI_VIRT,
> > +	CXL_REGLOC_RBI_MEMDEV,
> > +	CXL_REGLOC_RBI_TYPES
> > +};
> > +
> >  #endif /* __CXL_PCI_H__ */
> > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> > index faeb5d9d7a7a..c0fe78e0559b 100644
> > --- a/drivers/cxl/pci.c
> > +++ b/drivers/cxl/pci.c
> > @@ -387,6 +387,52 @@ static int cxl_setup_regs(struct pci_dev *pdev, enum cxl_regloc_type type,
> > 	return rc;
> >  }
> >
> > +static void cxl_pci_free_irq_vectors(void *data)
> > +{
> > +	pci_free_irq_vectors(data);
> > +}
> > +
> > +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);
> > +	}
> > +
> > +	if (vectors == -1)
> > +		return -EINVAL; /* no irq support whatsoever */
> > +
> > +	vectors++;
>
> This is pretty much what earlier versions of the DOE code did with the
> exception of only have 1 get_max_msgnum() calls defined (for DOE).  But there
> was a lot of debate about how to share vectors with the PCI layer.  And
> eventually we got rid of it.  I'm still trying to figure it out.  Sorry for
> being slow.

That makes sense, thanks for the explanation. And no not slow, it is _I_
that needs to go re-read the DOE stuff with more attention. But while I
knew this was the hardest part, all I really wanted was a basic irq
support to add to the bg cmd handling series.

> Perhaps we do this for this series.  However, won't we have an issue if we want
> to support switch events?

If possible, could you elaborate more on this?

Thanks,
Davidlohr