On Tue, Dec 03, 2024 at 02:53:45PM +0100, Thomas Gleixner wrote:
> On Thu, Nov 14 2024 at 17:18, Andrew Jones wrote:
> > @@ -96,9 +96,8 @@ static int imsic_irq_set_affinity(struct irq_data *d, const struct cpumask *mask
> > bool force)
> > {
> > struct imsic_vector *old_vec, *new_vec;
> > - struct irq_data *pd = d->parent_data;
> >
> > - old_vec = irq_data_get_irq_chip_data(pd);
> > + old_vec = irq_data_get_irq_chip_data(d);
> > if (WARN_ON(!old_vec))
> > return -ENOENT;
> >
> > @@ -116,13 +115,13 @@ static int imsic_irq_set_affinity(struct irq_data *d, const struct cpumask *mask
> > return -ENOSPC;
> >
> > /* Point device to the new vector */
> > - imsic_msi_update_msg(d, new_vec);
> > + imsic_msi_update_msg(irq_get_irq_data(d->irq), new_vec);
>
> This looks more than fishy. See below.
>
> > @@ -245,7 +247,7 @@ static bool imsic_init_dev_msi_info(struct device *dev,
> > if (WARN_ON_ONCE(domain != real_parent))
> > return false;
> > #ifdef CONFIG_SMP
> > - info->chip->irq_set_affinity = imsic_irq_set_affinity;
> > + info->chip->irq_set_affinity = irq_chip_set_affinity_parent;
>
> This should use msi_domain_set_affinity(), which does the right thing:
>
> 1) It invokes the irq_set_affinity() callback of the parent domain
>
> 2) It composes the message via the hierarchy
>
> 3) It writes the message with the msi_write_msg() callback of the top
> level domain
>
> Sorry, I missed that when reviewing the original IMSIC MSI support.
>
> The whole IMSIC MSI support can be moved over to MSI LIB which makes all
> of this indirection go away and your intermediate domain will just fit
> in.
>
> Uncompiled patch below. If that works, it needs to be split up properly.
Thanks Thomas. I gave your patch below a go, but we now fail to have an
msi domain set up when probing devices which go through aplic_msi_setup(),
resulting in an immediate NULL deference in
msi_create_device_irq_domain(). I'll look closer tomorrow.
Thanks,
drew
>
> Note, this removes the setup of the irq_retrigger callback, but that's
> fine because on hierarchical domains irq_chip_retrigger_hierarchy() is
> invoked anyway. See try_retrigger().
>
> Thanks,
>
> tglx
> ---
> drivers/irqchip/Kconfig | 1
> drivers/irqchip/irq-gic-v2m.c | 1
> drivers/irqchip/irq-imx-mu-msi.c | 1
> drivers/irqchip/irq-msi-lib.c | 11 +-
> drivers/irqchip/irq-mvebu-gicp.c | 1
> drivers/irqchip/irq-mvebu-odmi.c | 1
> drivers/irqchip/irq-mvebu-sei.c | 1
> drivers/irqchip/irq-riscv-imsic-platform.c | 131 +----------------------------
> include/linux/msi.h | 11 ++
> 9 files changed, 32 insertions(+), 127 deletions(-)
>
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -587,6 +587,7 @@ config RISCV_IMSIC
> select IRQ_DOMAIN_HIERARCHY
> select GENERIC_IRQ_MATRIX_ALLOCATOR
> select GENERIC_MSI_IRQ
> + select IRQ_MSI_LIB
>
> config RISCV_IMSIC_PCI
> bool
> --- a/drivers/irqchip/irq-gic-v2m.c
> +++ b/drivers/irqchip/irq-gic-v2m.c
> @@ -255,6 +255,7 @@ static void __init gicv2m_teardown(void)
> static struct msi_parent_ops gicv2m_msi_parent_ops = {
> .supported_flags = GICV2M_MSI_FLAGS_SUPPORTED,
> .required_flags = GICV2M_MSI_FLAGS_REQUIRED,
> + .chip_flags = MSI_CHIP_FLAG_SET_EOI | MSI_CHIP_FLAG_SET_ACK,
> .bus_select_token = DOMAIN_BUS_NEXUS,
> .bus_select_mask = MATCH_PCI_MSI | MATCH_PLATFORM_MSI,
> .prefix = "GICv2m-",
> --- a/drivers/irqchip/irq-imx-mu-msi.c
> +++ b/drivers/irqchip/irq-imx-mu-msi.c
> @@ -214,6 +214,7 @@ static void imx_mu_msi_irq_handler(struc
> static const struct msi_parent_ops imx_mu_msi_parent_ops = {
> .supported_flags = IMX_MU_MSI_FLAGS_SUPPORTED,
> .required_flags = IMX_MU_MSI_FLAGS_REQUIRED,
> + .chip_flags = MSI_CHIP_FLAG_SET_EOI | MSI_CHIP_FLAG_SET_ACK,
> .bus_select_token = DOMAIN_BUS_NEXUS,
> .bus_select_mask = MATCH_PLATFORM_MSI,
> .prefix = "MU-MSI-",
> --- a/drivers/irqchip/irq-msi-lib.c
> +++ b/drivers/irqchip/irq-msi-lib.c
> @@ -28,6 +28,7 @@ bool msi_lib_init_dev_msi_info(struct de
> struct msi_domain_info *info)
> {
> const struct msi_parent_ops *pops = real_parent->msi_parent_ops;
> + struct irq_chip *chip = info->chip;
> u32 required_flags;
>
> /* Parent ops available? */
> @@ -92,10 +93,10 @@ bool msi_lib_init_dev_msi_info(struct de
> info->flags |= required_flags;
>
> /* Chip updates for all child bus types */
> - if (!info->chip->irq_eoi)
> - info->chip->irq_eoi = irq_chip_eoi_parent;
> - if (!info->chip->irq_ack)
> - info->chip->irq_ack = irq_chip_ack_parent;
> + if (!chip->irq_eoi && (pops->chip_flags & MSI_CHIP_FLAG_SET_EOI))
> + chip->irq_eoi = irq_chip_eoi_parent;
> + if (!chip->irq_ack && (pops->chip_flags & MSI_CHIP_FLAG_SET_ACK))
> + chip->irq_ack = irq_chip_ack_parent;
>
> /*
> * The device MSI domain can never have a set affinity callback. It
> @@ -105,7 +106,7 @@ bool msi_lib_init_dev_msi_info(struct de
> * device MSI domain aside of mask/unmask which is provided e.g. by
> * PCI/MSI device domains.
> */
> - info->chip->irq_set_affinity = msi_domain_set_affinity;
> + chip->irq_set_affinity = msi_domain_set_affinity;
> return true;
> }
> EXPORT_SYMBOL_GPL(msi_lib_init_dev_msi_info);
> --- a/drivers/irqchip/irq-mvebu-gicp.c
> +++ b/drivers/irqchip/irq-mvebu-gicp.c
> @@ -161,6 +161,7 @@ static const struct irq_domain_ops gicp_
> static const struct msi_parent_ops gicp_msi_parent_ops = {
> .supported_flags = GICP_MSI_FLAGS_SUPPORTED,
> .required_flags = GICP_MSI_FLAGS_REQUIRED,
> + .chip_flags = MSI_CHIP_FLAG_SET_EOI | MSI_CHIP_FLAG_SET_ACK,
> .bus_select_token = DOMAIN_BUS_GENERIC_MSI,
> .bus_select_mask = MATCH_PLATFORM_MSI,
> .prefix = "GICP-",
> --- a/drivers/irqchip/irq-mvebu-odmi.c
> +++ b/drivers/irqchip/irq-mvebu-odmi.c
> @@ -157,6 +157,7 @@ static const struct irq_domain_ops odmi_
> static const struct msi_parent_ops odmi_msi_parent_ops = {
> .supported_flags = ODMI_MSI_FLAGS_SUPPORTED,
> .required_flags = ODMI_MSI_FLAGS_REQUIRED,
> + .chip_flags = MSI_CHIP_FLAG_SET_EOI | MSI_CHIP_FLAG_SET_ACK,
> .bus_select_token = DOMAIN_BUS_GENERIC_MSI,
> .bus_select_mask = MATCH_PLATFORM_MSI,
> .prefix = "ODMI-",
> --- a/drivers/irqchip/irq-mvebu-sei.c
> +++ b/drivers/irqchip/irq-mvebu-sei.c
> @@ -356,6 +356,7 @@ static void mvebu_sei_reset(struct mvebu
> static const struct msi_parent_ops sei_msi_parent_ops = {
> .supported_flags = SEI_MSI_FLAGS_SUPPORTED,
> .required_flags = SEI_MSI_FLAGS_REQUIRED,
> + .chip_flags = MSI_CHIP_FLAG_SET_EOI | MSI_CHIP_FLAG_SET_ACK,
> .bus_select_mask = MATCH_PLATFORM_MSI,
> .bus_select_token = DOMAIN_BUS_GENERIC_MSI,
> .prefix = "SEI-",
> --- a/drivers/irqchip/irq-riscv-imsic-platform.c
> +++ b/drivers/irqchip/irq-riscv-imsic-platform.c
> @@ -21,6 +21,7 @@
> #include <linux/smp.h>
>
> #include "irq-riscv-imsic-state.h"
> +#include "irq-msi-lib.h"
>
> static bool imsic_cpu_page_phys(unsigned int cpu, unsigned int guest_index,
> phys_addr_t *out_msi_pa)
> @@ -84,19 +85,10 @@ static void imsic_irq_compose_msg(struct
> }
>
> #ifdef CONFIG_SMP
> -static void imsic_msi_update_msg(struct irq_data *d, struct imsic_vector *vec)
> -{
> - struct msi_msg msg = { };
> -
> - imsic_irq_compose_vector_msg(vec, &msg);
> - irq_data_get_irq_chip(d)->irq_write_msi_msg(d, &msg);
> -}
> -
> static int imsic_irq_set_affinity(struct irq_data *d, const struct cpumask *mask_val,
> bool force)
> {
> struct imsic_vector *old_vec, *new_vec;
> - struct irq_data *pd = d->parent_data;
>
> old_vec = irq_data_get_irq_chip_data(pd);
> if (WARN_ON(!old_vec))
> @@ -115,14 +107,11 @@ static int imsic_irq_set_affinity(struct
> if (!new_vec)
> return -ENOSPC;
>
> - /* Point device to the new vector */
> - imsic_msi_update_msg(d, new_vec);
> -
> /* Update irq descriptors with the new vector */
> - pd->chip_data = new_vec;
> + d->chip_data = new_vec;
>
> /* Update effective affinity of parent irq data */
> - irq_data_update_effective_affinity(pd, cpumask_of(new_vec->cpu));
> + irq_data_update_effective_affinity(d, cpumask_of(new_vec->cpu));
>
> /* Move state of the old vector to the new vector */
> imsic_vector_move(old_vec, new_vec);
> @@ -137,6 +126,9 @@ static struct irq_chip imsic_irq_base_ch
> .irq_unmask = imsic_irq_unmask,
> .irq_retrigger = imsic_irq_retrigger,
> .irq_compose_msi_msg = imsic_irq_compose_msg,
> +#ifdef CONFIG_SMP
> + .irq_set_affinity = imsic_irq_set_affinity,
> +#endif
> .flags = IRQCHIP_SKIP_SET_WAKE |
> IRQCHIP_MASK_ON_SUSPEND,
> };
> @@ -172,22 +164,6 @@ static void imsic_irq_domain_free(struct
> irq_domain_free_irqs_parent(domain, virq, nr_irqs);
> }
>
> -static int imsic_irq_domain_select(struct irq_domain *domain, struct irq_fwspec *fwspec,
> - enum irq_domain_bus_token bus_token)
> -{
> - const struct msi_parent_ops *ops = domain->msi_parent_ops;
> - u32 busmask = BIT(bus_token);
> -
> - if (fwspec->fwnode != domain->fwnode || fwspec->param_count != 0)
> - return 0;
> -
> - /* Handle pure domain searches */
> - if (bus_token == ops->bus_select_token)
> - return 1;
> -
> - return !!(ops->bus_select_mask & busmask);
> -}
> -
> #ifdef CONFIG_GENERIC_IRQ_DEBUGFS
> static void imsic_irq_debug_show(struct seq_file *m, struct irq_domain *d,
> struct irq_data *irqd, int ind)
> @@ -210,104 +186,15 @@ static const struct irq_domain_ops imsic
> #endif
> };
>
> -#ifdef CONFIG_RISCV_IMSIC_PCI
> -
> -static void imsic_pci_mask_irq(struct irq_data *d)
> -{
> - pci_msi_mask_irq(d);
> - irq_chip_mask_parent(d);
> -}
> -
> -static void imsic_pci_unmask_irq(struct irq_data *d)
> -{
> - irq_chip_unmask_parent(d);
> - pci_msi_unmask_irq(d);
> -}
> -
> -#define MATCH_PCI_MSI BIT(DOMAIN_BUS_PCI_MSI)
> -
> -#else
> -
> -#define MATCH_PCI_MSI 0
> -
> -#endif
> -
> -static bool imsic_init_dev_msi_info(struct device *dev,
> - struct irq_domain *domain,
> - struct irq_domain *real_parent,
> - struct msi_domain_info *info)
> -{
> - const struct msi_parent_ops *pops = real_parent->msi_parent_ops;
> -
> - /* MSI parent domain specific settings */
> - switch (real_parent->bus_token) {
> - case DOMAIN_BUS_NEXUS:
> - if (WARN_ON_ONCE(domain != real_parent))
> - return false;
> -#ifdef CONFIG_SMP
> - info->chip->irq_set_affinity = imsic_irq_set_affinity;
> -#endif
> - break;
> - default:
> - WARN_ON_ONCE(1);
> - return false;
> - }
> -
> - /* Is the target supported? */
> - switch (info->bus_token) {
> -#ifdef CONFIG_RISCV_IMSIC_PCI
> - case DOMAIN_BUS_PCI_DEVICE_MSI:
> - case DOMAIN_BUS_PCI_DEVICE_MSIX:
> - info->chip->irq_mask = imsic_pci_mask_irq;
> - info->chip->irq_unmask = imsic_pci_unmask_irq;
> - break;
> -#endif
> - case DOMAIN_BUS_DEVICE_MSI:
> - /*
> - * Per-device MSI should never have any MSI feature bits
> - * set. It's sole purpose is to create a dumb interrupt
> - * chip which has a device specific irq_write_msi_msg()
> - * callback.
> - */
> - if (WARN_ON_ONCE(info->flags))
> - return false;
> -
> - /* Core managed MSI descriptors */
> - info->flags |= MSI_FLAG_ALLOC_SIMPLE_MSI_DESCS |
> - MSI_FLAG_FREE_MSI_DESCS;
> - break;
> - case DOMAIN_BUS_WIRED_TO_MSI:
> - break;
> - default:
> - WARN_ON_ONCE(1);
> - return false;
> - }
> -
> - /* Use hierarchial chip operations re-trigger */
> - info->chip->irq_retrigger = irq_chip_retrigger_hierarchy;
> -
> - /*
> - * Mask out the domain specific MSI feature flags which are not
> - * supported by the real parent.
> - */
> - info->flags &= pops->supported_flags;
> -
> - /* Enforce the required flags */
> - info->flags |= pops->required_flags;
> -
> - return true;
> -}
> -
> -#define MATCH_PLATFORM_MSI BIT(DOMAIN_BUS_PLATFORM_MSI)
> -
> static const struct msi_parent_ops imsic_msi_parent_ops = {
> .supported_flags = MSI_GENERIC_FLAGS_MASK |
> MSI_FLAG_PCI_MSIX,
> .required_flags = MSI_FLAG_USE_DEF_DOM_OPS |
> - MSI_FLAG_USE_DEF_CHIP_OPS,
> + MSI_FLAG_USE_DEF_CHIP_OPS |
> + MSI_FLAG_PCI_MSI_MASK_PARENT,
> .bus_select_token = DOMAIN_BUS_NEXUS,
> .bus_select_mask = MATCH_PCI_MSI | MATCH_PLATFORM_MSI,
> - .init_dev_msi_info = imsic_init_dev_msi_info,
> + .init_dev_msi_info = msi_lib_init_dev_msi_info,
> };
>
> int imsic_irqdomain_init(void)
> --- a/include/linux/msi.h
> +++ b/include/linux/msi.h
> @@ -558,11 +558,21 @@ enum {
> MSI_FLAG_NO_AFFINITY = (1 << 21),
> };
>
> +/*
> + * Flags for msi_parent_ops::chip_flags
> + */
> +enum {
> + MSI_CHIP_FLAG_SET_EOI = (1 << 0),
> + MSI_CHIP_FLAG_SET_ACK = (1 << 1),
> +};
> +
> /**
> * struct msi_parent_ops - MSI parent domain callbacks and configuration info
> *
> * @supported_flags: Required: The supported MSI flags of the parent domain
> * @required_flags: Optional: The required MSI flags of the parent MSI domain
> + * @chip_flags: Optional: Select MSI chip callbacks to update with defaults
> + * in msi_lib_init_dev_msi_info().
> * @bus_select_token: Optional: The bus token of the real parent domain for
> * irq_domain::select()
> * @bus_select_mask: Optional: A mask of supported BUS_DOMAINs for
> @@ -575,6 +585,7 @@ enum {
> struct msi_parent_ops {
> u32 supported_flags;
> u32 required_flags;
> + u32 chip_flags;
> u32 bus_select_token;
> u32 bus_select_mask;
> const char *prefix;
