On Tue, Dec 17, 2024 at 11:56:18PM +0100, Thomas Gleixner wrote:
> On Wed, Dec 11 2024 at 15:57, Frank Li wrote:
> > +static int pci_epf_msi_prepare(struct irq_domain *domain, struct device *dev,
> > + int nvec, msi_alloc_info_t *arg)
> > +{
> > + struct pci_epf *epf = to_pci_epf(dev);
> > + struct msi_domain_info *msi_info;
> > + struct pci_epc *epc = epf->epc;
> > +
> > + memset(arg, 0, sizeof(*arg));
> > + arg->scratchpad[0].ul = of_msi_map_id(epc->dev.parent, NULL,
> > + (epf->func_no << 8) | epf->vfunc_no);
> > +
> > + /*
> > + * @domain->msi_domain_info->hwsize contains the size of the device
> > + * domain, but vector allocation happens one by one.
> > + */
> > + msi_info = msi_get_domain_info(domain);
> > + if (msi_info->hwsize > nvec)
> > + nvec = msi_info->hwsize;
> > +
> > + /* Allocate at least 32 MSIs, and always as a power of 2 */
> > + nvec = max_t(int, 32, roundup_pow_of_two(nvec));
> > +
> > + msi_info = msi_get_domain_info(domain->parent);
> > + return msi_info->ops->msi_prepare(domain->parent, dev, nvec, arg);
>
> While I was trying to make sense of the change log of patch [1/9] I
> looked at this function to understand why this needs an override.
>
> This is a copy of its_msi_prepare() except for the scratchpad[0].ul
> part. But that's a GIC-V3 implementation specific detail, which has
> absolutely no business in code which claims to be a generic library for
> PCI endpoints.
>
> Worse you created a GIC-V3 only PCI endpoint library under the
> assumption that the underlying ITS/MSI implementation is immutable. Of
> course there is no safety net either to validate that the underlying
> parent domain is actually GIC-V3-ITS. That's wrong in every aspect.
>
> So let's take a step back and analyze what is actually required to make
> this a proper generic library.
>
> The endpoint function device needs its own device ID which is required
> to set up a device specific translation in the interrupt remapping unit.
>
> Now you decided that this is bound to a DT mapping, which is odd to
> begin with. What's DT specific about this? The cirumstance that your
> hardware is DT based and the endpoint controller ID map needs to be
> retrieved from there? How is this generic in any way? How is this
> supposed to work with ACPI enumerated hardware? Not to ask the question
> how this should work with non GIC-V3-ITS based hardware.
>
> That's all but generic, it's an ad hoc hack to support your particular
> setup implemented by layering violations.
>
> In fact the mapping ID is composed by the parent mapping ID and the
> function numbers, right?
>
> The general PCIe convention here is:
>
> domain:bus:slot.func
>
> That's well defined and if you look at real devices then lspci shows:
>
> 0000:3d:00.1 Ethernet controller: Ethernet Connection for 10GBASE-T
> 0000:3d:06.0 Ethernet controller: Ethernet Virtual Function
> 0000:...
> 0000:3d:06.7 Ethernet controller: Ethernet Virtual Function
> 0000:3d:07.0 Ethernet controller: Ethernet Virtual Function
> 0000:...
> 0000:3d:07.7 Ethernet controller: Ethernet Virtual Function
>
> In PCI address representation:
>
> domain:bus:slot:function
>
> which is usually condensed into a single word based on the range limits
> of function, device and bus:
>
> function: bit 0-2 (max. 8)
> device: bit 3-7 (max. 32)
> bus: bit 8-15 (max. 256)
> domain: bit 16-31 (mostly theoretical)
>
> Endpoint devices should follow exactly the same scheme, no?
Can't reuse BDF beasue Bus because device have not any means for EP side.
such as PCI EP controller have 8 physical functions. called EP.1 EP.2 ....
EP.n may connect PCI host 1, which bus number is 1. so BDF
0000.01.00.1: sampe EP devices
but if it connect another PCI bus 3, so BDF is
0000.03.00.1 sampe EP devices.
At EP side, it don't care connect to RC bus1 or bus3.
EP.1 use MSI irq 3
EP.2 use MSI ire 4
regardless EP connect bus 1 or bus 3, door bell always trigger EP side
irq 3 for EP.1, irq 4 for EP.2
So only function and vfunction should be used for as request id.
I worry about some hardware (I have not met yet) direct use BDF from RC
side because it is simple hardware implementation for dual role pci
controller.
>
> Now looking at your ID retrieval:
>
> > + arg->scratchpad[0].ul = of_msi_map_id(epc->dev.parent, NULL,
> > + (epf->func_no << 8) | epf->vfunc_no);
>
> I really have to ask why this is making up its own representation
> instead of simply using the standard PCI B/D/F conventions?
See above!
Addtional difference vfunc need difference doorbell msi domain.
I have not sure how virtual function to map to BDF. If it map D, we just
simple mask B. I want reserve it to avoid ABI change in future if vfunc
support since my hardware don't support EP's side vfunc yet.
Anyway, let me implement what your below suggestion then fine tune this.
>
> Whatever the reason is, fact is that the actual interrupt domain support
> needs to be done differently. There is no way that the endpoint library
> makes assumption about the underlying interrupt domain and copies a
> function just because. This has to be completely agnostic, no if, no
> but.
>
> So the consequence is that the underlying MSI parent domains needs to
> know about the endpoint requirements, which is how all MSI variants are
> modeled, i.e. with a MSI domain bus.
>
> That also solves the problem of immutable MSI messages without any
> further magic. Interrupt domains, which do not provide them, won't
> provide the endpoint MSI domain bus and therefore the lookup of the
> parent MSI domain for the endpoint fails.
>
> The uncompilable mockup below should give you a hint.
Thank for your example, let me try to implement it.
Frank
>
> Thanks,
>
> tglx
> ---
> drivers/irqchip/irq-gic-v3-its-msi-parent.c | 50 ++++++++++++++++++++--------
> drivers/irqchip/irq-msi-lib.c | 5 ++
> drivers/irqchip/irq-msi-lib.h | 12 +++++-
> include/linux/irqdomain_defs.h | 2 +
> 4 files changed, 51 insertions(+), 18 deletions(-)
>
> --- a/drivers/irqchip/irq-gic-v3-its-msi-parent.c
> +++ b/drivers/irqchip/irq-gic-v3-its-msi-parent.c
> @@ -126,20 +126,9 @@ int __weak iort_pmsi_get_dev_id(struct d
> return -1;
> }
>
> -static int its_pmsi_prepare(struct irq_domain *domain, struct device *dev,
> - int nvec, msi_alloc_info_t *info)
> +static int __its_pmsi_prepare(struct irq_domain *domain, struct device *dev,
> + int nvec, msi_alloc_info_t *info, u32 dev_id)
> {
> - struct msi_domain_info *msi_info;
> - u32 dev_id;
> - int ret;
> -
> - if (dev->of_node)
> - ret = of_pmsi_get_dev_id(domain->parent, dev, &dev_id);
> - else
> - ret = iort_pmsi_get_dev_id(dev, &dev_id);
> - if (ret)
> - return ret;
> -
> /* ITS specific DeviceID, as the core ITS ignores dev. */
> info->scratchpad[0].ul = dev_id;
>
> @@ -159,6 +148,36 @@ static int its_pmsi_prepare(struct irq_d
> dev, nvec, info);
> }
>
> +static int its_pci_ep_msi_prepare(struct irq_domain *domain, struct device *dev,
> + int nvec, msi_alloc_info_t *info)
> +{
> + u32 dev_id = dev_get_pci_ep_id(dev);
> + struct msi_domain_info *msi_info;
> + int ret = -ENOTSUPP;
> +
> + if (dev->of_node)
> + ret = do_magic_ep_id_map();
> + if (ret)
> + return ret;
> + return __its_pmsi_prepare(domain, dev, nvec, info, dev_id);
> +}
> +
> +static int its_pmsi_prepare(struct irq_domain *domain, struct device *dev,
> + int nvec, msi_alloc_info_t *info)
> +{
> + struct msi_domain_info *msi_info;
> + u32 dev_id;
> + int ret;
> +
> + if (dev->of_node)
> + ret = of_pmsi_get_dev_id(domain->parent, dev, &dev_id);
> + else
> + ret = iort_pmsi_get_dev_id(dev, &dev_id);
> + if (ret)
> + return ret;
> + return __its_pmsi_prepare(domain, dev, nvec, info, dev_id);
> +}
> +
> static bool its_init_dev_msi_info(struct device *dev, struct irq_domain *domain,
> struct irq_domain *real_parent, struct msi_domain_info *info)
> {
> @@ -183,6 +202,9 @@ static bool its_init_dev_msi_info(struct
> */
> info->ops->msi_prepare = its_pci_msi_prepare;
> break;
> + case DOMAIN_BUS_PCI_DEVICE_EP_MSI:
> + info->ops->msi_prepare = its_pci_ep_msi_prepare;
> + break;
> case DOMAIN_BUS_DEVICE_MSI:
> case DOMAIN_BUS_WIRED_TO_MSI:
> /*
> @@ -204,7 +226,7 @@ const struct msi_parent_ops gic_v3_its_m
> .supported_flags = ITS_MSI_FLAGS_SUPPORTED,
> .required_flags = ITS_MSI_FLAGS_REQUIRED,
> .bus_select_token = DOMAIN_BUS_NEXUS,
> - .bus_select_mask = MATCH_PCI_MSI | MATCH_PLATFORM_MSI,
> + .bus_select_mask = MATCH_PCI_MSI | MATCH_PLATFORM_PCI_EP_MSI | MATCH_PLATFORM_MSI,
> .prefix = "ITS-",
> .init_dev_msi_info = its_init_dev_msi_info,
> };
> --- a/drivers/irqchip/irq-msi-lib.c
> +++ b/drivers/irqchip/irq-msi-lib.c
> @@ -55,8 +55,11 @@ bool msi_lib_init_dev_msi_info(struct de
> case DOMAIN_BUS_PCI_DEVICE_MSIX:
> if (WARN_ON_ONCE(!IS_ENABLED(CONFIG_PCI_MSI)))
> return false;
> -
> break;
> + case DOMAIN_BUS_DEVICE_PCI_EP_MSI:
> + if (WARN_ON_ONCE(!IS_ENABLED(CONFIG_PCI_ENDPOINT)))
> + return false;
> + fallthrough;
> case DOMAIN_BUS_DEVICE_MSI:
> /*
> * Per device MSI should never have any MSI feature bits
> --- a/drivers/irqchip/irq-msi-lib.h
> +++ b/drivers/irqchip/irq-msi-lib.h
> @@ -10,12 +10,18 @@
> #include <linux/msi.h>
>
> #ifdef CONFIG_PCI_MSI
> -#define MATCH_PCI_MSI BIT(DOMAIN_BUS_PCI_MSI)
> +#define MATCH_PCI_MSI BIT(DOMAIN_BUS_PCI_MSI)
> #else
> -#define MATCH_PCI_MSI (0)
> +#define MATCH_PCI_MSI (0)
> #endif
>
> -#define MATCH_PLATFORM_MSI BIT(DOMAIN_BUS_PLATFORM_MSI)
> +#ifdef CONFIG_PCI_ENDPOINT
> +#define MATCH_PLATFORM_PCI_EP_MSI BIT(DOMAIN_BUS_PLATFORM_PCI_EP_MSI)
> +#else
> +#define MATCH_PLATFORM_PCI_EP_MSI (0)
> +#endif
> +
> +#define MATCH_PLATFORM_MSI BIT(DOMAIN_BUS_PLATFORM_MSI)
>
> int msi_lib_irq_domain_select(struct irq_domain *d, struct irq_fwspec *fwspec,
> enum irq_domain_bus_token bus_token);
> --- a/include/linux/irqdomain_defs.h
> +++ b/include/linux/irqdomain_defs.h
> @@ -15,6 +15,7 @@ enum irq_domain_bus_token {
> DOMAIN_BUS_GENERIC_MSI,
> DOMAIN_BUS_PCI_MSI,
> DOMAIN_BUS_PLATFORM_MSI,
> + DOMAIN_BUS_PLATFORM_PCI_EP_MSI,
> DOMAIN_BUS_NEXUS,
> DOMAIN_BUS_IPI,
> DOMAIN_BUS_FSL_MC_MSI,
> @@ -27,6 +28,7 @@ enum irq_domain_bus_token {
> DOMAIN_BUS_AMDVI,
> DOMAIN_BUS_DEVICE_MSI,
> DOMAIN_BUS_WIRED_TO_MSI,
> + DOMAIN_BUS_DEVICE_PCI_EP_MSI,
> };
>
> #endif /* _LINUX_IRQDOMAIN_DEFS_H */
