Re: [PATCH v15 07/15] PCI: endpoint: pci-ep-msi: Add MSI address/data pair mutable check

[Frank Li <Frank.li@nxp.com>]

On Sat, Mar 01, 2025 at 11:44:50AM +0000, Marc Zyngier wrote:
> On Tue, 11 Feb 2025 19:22:00 +0000,
> Frank Li <Frank.Li@nxp.com> wrote:
> >
> > Some MSI controller change address/data pair when irq_set_affinity().
> > Current PCI endpoint can't support this type MSI controller. So add flag
> > MSI_FLAG_MUTABLE in include/linux/msi.h and check it when allocate
> > doorbell.
> >
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> > change from v14 to v15
> > - none
> >
> > change from  v13 to v14
> > - bring v10 back
> >
> > Change from v9 to v10
> > - new patch
> > ---
> >  drivers/pci/endpoint/pci-ep-msi.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/drivers/pci/endpoint/pci-ep-msi.c b/drivers/pci/endpoint/pci-ep-msi.c
> > index 549b55b864d0e..c0e2d806ee658 100644
> > --- a/drivers/pci/endpoint/pci-ep-msi.c
> > +++ b/drivers/pci/endpoint/pci-ep-msi.c
> > @@ -44,6 +44,14 @@ int pci_epf_alloc_doorbell(struct pci_epf *epf, u16 num_db)
> >
> >  	dev_set_msi_domain(dev, dom);
> >
> > +	if (!irq_domain_is_msi_parent(dom))
> > +		return -EINVAL;
> > +
> > +	if (!irq_domain_is_msi_immutable(dom)) {
> > +		dev_err(dev, "Can't support mutable address/data pair MSI controller\n");
> > +		return -EINVAL;
> > +	}
> > +
> >  	msg = kcalloc(num_db, sizeof(struct pci_epf_doorbell_msg), GFP_KERNEL);
> >  	if (!msg)
> >  		return -ENOMEM;
> >
>
> I really don't think this brings much to the table. These systems are
> not built by picking up random bits that can be put together by hand.
> They are integrated in an SoC, and I can't imagine that the MSI
> controller is picked randomly.
>
> So my conclusion is that this is either *designed* to work, or it
> doesn't exist, and that this code is just dead code.

I think Thomas answer this question. This code is actually safty check MSI
provider's capablity. If MSI provider can't address/pair is not immutable,
there are big risk when irq change affinity.

https://lore.kernel.org/imx/87senwd3mo.ffs@tglx/

"Yes and no. The problem is that the EP implementation is meant to be a
generic library and while GIC-ITS guarantees immutability of the
address/data pair after setup, there are architectures (x86, loongson,
riscv) where the base MSI controller does not and immutability is only
achieved when interrupt remapping is enabled. The latter can be disabled
at boot-time and then the EP implementation becomes a lottery across
affinity changes.

That was my concern about this library implementation and that's why I
asked for a mechanism to ensure that the underlying irqdomain provides a
immutable address/data pair.

So it does not matter for GIC-ITS, but in the larger picture it matters.
"

Let me know if I miss understand your means.

Frank


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