Re: [PATCH 3/4] genirq/msi: Move prepare() call to per-device allocation

Marc Zyngier
2025-05-12T15:55:34Z

On Mon, 12 May 2025 15:24:39 +0100,
Thomas Gleixner <tglx@linutronix.de> wrote:
>
> On Sun, May 11 2025 at 17:35, Marc Zyngier wrote:
> > The current device MSI infrastructure is subtly broken, as it
> > will issue an .msi_prepare() callback into the MSI controller
> > driver every time it needs to allocate an MSI. That's pretty wrong,
> > as the contract between the MSI controller and the core code is that
> > .msi_prepare() is called exactly once per device.
>
> That contract is nowhere written in stone.

It was *definitely* there the first place, and a baked in assumption
since the ITS code was merged. You're welcome to come up with a new
scheme, but the way the HW works requires this prepare phase to take
place once per device.

If we can't have that, maybe we should consider reverting the GICv3/v4
code back to the pre-6.10 scheme that doesn't suffer from this issue.

> There are some MSI controller which get confused about that, but that's
> a problem of said controllers

No. It's an infrastructure problem. This model worked before for a
whole class of HW, until it was mutated into something else.

>
> > diff --git a/include/linux/msi.h b/include/linux/msi.h
> > index 0a44a2cba3105..68a8b2d03eba9 100644
> > --- a/include/linux/msi.h
> > +++ b/include/linux/msi.h
> > @@ -513,12 +513,14 @@ struct msi_domain_info {
> > * @chip: Interrupt chip for this domain
> > * @ops: MSI domain ops
> > * @info: MSI domain info data
> > + * @arg: MSI domain allocation data (arch specific)
>
> arg is a horrible name. Can this please be alloc_info or such?

Because that's the name every single function that takes it as a
parameter uses? But sure, whatever name you want.

>
> > @@ -1025,6 +1026,7 @@ bool msi_create_device_irq_domain(struct device *dev, unsigned int domid,
> > bundle->info.ops = &bundle->ops;
> > bundle->info.data = domain_data;
> > bundle->info.chip_data = chip_data;
> > + bundle->info.alloc_data = &bundle->arg;
> >
> > pops = parent->msi_parent_ops;
> > snprintf(bundle->name, sizeof(bundle->name), "%s%s-%s",
> > @@ -1053,21 +1055,28 @@ bool msi_create_device_irq_domain(struct device *dev, unsigned int domid,
> > msi_lock_descs(dev);
>
> Please work against tip irq/msi which carries the guard() replacement
> for msi_lock_descs(). This patch heavily conflicts with the queued
> changes.
>
> > +static int __populate_alloc_info(struct irq_domain *domain, struct device *dev,
> > + unsigned int nirqs, msi_alloc_info_t *arg)
> > +{
>
> Why does this need double underscores?

Because it doesn't look that out of place in this file?

>
> > + struct msi_domain_info *info = domain->host_data;
> > + int ret = 0;
> > +
> > + /*
> > + * If the caller has provided a template alloc info, use that. Once
> > + * all users of msi_create_irq_domain() have been eliminated, this
> > + * should be the only source of allocation information, and the
> > + * prepare call below should be finally removed.
>
> That's only a matter of decades :)
>
> > + */
> > + if (info->alloc_data)
> > + *arg = *info->alloc_data;
> > + else
> > + ret = msi_domain_prepare_irqs(domain, dev, nirqs, arg);
> > +
> > + return ret;
>
> if (!info->alloc_data)
> return msi_domain_prepare_irqs(domain, dev, nirqs, arg);
>
> *arg = *info->alloc_data;
> return 0;
>
> perhaps?

Sure.

M.

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