Re: [RFC PATCH v2 04/18] iommu/riscv: Add IRQ domain for interrupt remapping

[Nutty.Liu]

On 9/21/2025 4:38 AM, Andrew Jones wrote:
> This is just a skeleton. Until irq-set-affinity functions are
> implemented the IRQ domain doesn't serve any purpose.
>
> Signed-off-by: Andrew Jones<ajones@xxxxxxxxxxxxxxxx>
> ---
>   drivers/iommu/riscv/Makefile   |   2 +-
>   drivers/iommu/riscv/iommu-ir.c | 114 +++++++++++++++++++++++++++++++++
>   drivers/iommu/riscv/iommu.c    |  36 +++++++++++
>   drivers/iommu/riscv/iommu.h    |  12 ++++
>   4 files changed, 163 insertions(+), 1 deletion(-)
>   create mode 100644 drivers/iommu/riscv/iommu-ir.c
>
> diff --git a/drivers/iommu/riscv/Makefile b/drivers/iommu/riscv/Makefile
> index b5929f9f23e6..9c83f877d50f 100644
> --- a/drivers/iommu/riscv/Makefile
> +++ b/drivers/iommu/riscv/Makefile
> @@ -1,3 +1,3 @@
>   # SPDX-License-Identifier: GPL-2.0-only
> -obj-y += iommu.o iommu-platform.o
> +obj-y += iommu.o iommu-ir.o iommu-platform.o
>   obj-$(CONFIG_RISCV_IOMMU_PCI) += iommu-pci.o
> diff --git a/drivers/iommu/riscv/iommu-ir.c b/drivers/iommu/riscv/iommu-ir.c
> new file mode 100644
> index 000000000000..08cf159b587d
> --- /dev/null
> +++ b/drivers/iommu/riscv/iommu-ir.c
> @@ -0,0 +1,114 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * IOMMU Interrupt Remapping
> + *
> + * Copyright © 2025 Ventana Micro Systems Inc.
> + */
> +#include <linux/irqdomain.h>
> +#include <linux/msi.h>
> +
> +#include "iommu.h"
> +
> +static struct irq_chip riscv_iommu_ir_irq_chip = {
> +	.name			= "IOMMU-IR",
> +	.irq_ack		= irq_chip_ack_parent,
> +	.irq_mask		= irq_chip_mask_parent,
> +	.irq_unmask		= irq_chip_unmask_parent,
> +	.irq_set_affinity	= irq_chip_set_affinity_parent,
> +};
> +
> +static int riscv_iommu_ir_irq_domain_alloc_irqs(struct irq_domain *irqdomain,
> +						unsigned int irq_base, unsigned int nr_irqs,
> +						void *arg)
> +{
> +	struct irq_data *data;
> +	int i, ret;
> +
> +	ret = irq_domain_alloc_irqs_parent(irqdomain, irq_base, nr_irqs, arg);
> +	if (ret)
> +		return ret;
> +
> +	for (i = 0; i < nr_irqs; i++) {
> +		data = irq_domain_get_irq_data(irqdomain, irq_base + i);
Nitpick:  Would it be better to check if 'data' is NULL ?

> +		data->chip = &riscv_iommu_ir_irq_chip;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct irq_domain_ops riscv_iommu_ir_irq_domain_ops = {
> +	.alloc = riscv_iommu_ir_irq_domain_alloc_irqs,
> +	.free = irq_domain_free_irqs_parent,
> +};
> +
> +static const struct msi_parent_ops riscv_iommu_ir_msi_parent_ops = {
> +	.prefix			= "IR-",
> +	.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_PCI_MSI_MASK_PARENT,
> +	.chip_flags		= MSI_CHIP_FLAG_SET_ACK,
> +	.init_dev_msi_info	= msi_parent_init_dev_msi_info,
> +};
> +
> +struct irq_domain *riscv_iommu_ir_irq_domain_create(struct riscv_iommu_device *iommu,
> +						    struct device *dev,
> +						    struct riscv_iommu_info *info)
> +{
> +	struct irq_domain *irqparent = dev_get_msi_domain(dev);
> +	struct irq_domain *irqdomain;
> +	struct fwnode_handle *fn;
> +	char *fwname;
> +
> +	fwname = kasprintf(GFP_KERNEL, "IOMMU-IR-%s", dev_name(dev));
> +	if (!fwname)
> +		return NULL;
> +
> +	fn = irq_domain_alloc_named_fwnode(fwname);
> +	kfree(fwname);
> +	if (!fn) {
> +		dev_err(iommu->dev, "Couldn't allocate fwnode\n");
> +		return NULL;
> +	}
> +
> +	irqdomain = irq_domain_create_hierarchy(irqparent, 0, 0, fn,
> +						&riscv_iommu_ir_irq_domain_ops,
> +						info);
> +	if (!irqdomain) {
> +		dev_err(iommu->dev, "Failed to create IOMMU irq domain\n");
> +		irq_domain_free_fwnode(fn);
> +		return NULL;
> +	}
> +
> +	irqdomain->flags |= IRQ_DOMAIN_FLAG_MSI_PARENT;
> +	irqdomain->msi_parent_ops = &riscv_iommu_ir_msi_parent_ops;
> +	irq_domain_update_bus_token(irqdomain, DOMAIN_BUS_MSI_REMAP);
> +
> +	dev_set_msi_domain(dev, irqdomain);
> +
> +	return irqdomain;
> +}
> +
> +void riscv_iommu_ir_irq_domain_remove(struct riscv_iommu_info *info)
> +{
> +	struct fwnode_handle *fn;
> +
> +	if (!info->irqdomain)
> +		return;
> +
> +	fn = info->irqdomain->fwnode;
> +	irq_domain_remove(info->irqdomain);
> +	info->irqdomain = NULL;
> +	irq_domain_free_fwnode(fn);
> +}
> +
> +int riscv_iommu_ir_attach_paging_domain(struct riscv_iommu_domain *domain,
> +					struct device *dev)
> +{
> +	return 0;
> +}
> +
> +void riscv_iommu_ir_free_paging_domain(struct riscv_iommu_domain *domain)
> +{
> +}
> diff --git a/drivers/iommu/riscv/iommu.c b/drivers/iommu/riscv/iommu.c
> index a44c67a848fa..db2acd9dc64b 100644
> --- a/drivers/iommu/riscv/iommu.c
> +++ b/drivers/iommu/riscv/iommu.c
> @@ -17,6 +17,8 @@
>   #include <linux/init.h>
>   #include <linux/iommu.h>
>   #include <linux/iopoll.h>
> +#include <linux/irqchip/riscv-imsic.h>
> +#include <linux/irqdomain.h>
>   #include <linux/kernel.h>
>   #include <linux/pci.h>
>   
> @@ -1026,6 +1028,9 @@ static void riscv_iommu_iodir_update(struct riscv_iommu_device *iommu,
>   
>   		WRITE_ONCE(dc->fsc, new_dc->fsc);
>   		WRITE_ONCE(dc->ta, new_dc->ta & RISCV_IOMMU_PC_TA_PSCID);
> +		WRITE_ONCE(dc->msiptp, new_dc->msiptp);
> +		WRITE_ONCE(dc->msi_addr_mask, new_dc->msi_addr_mask);
> +		WRITE_ONCE(dc->msi_addr_pattern, new_dc->msi_addr_pattern);
Since the MSI page table pointer (msiptp) has been changed,
should all cache entries from this MSI page table need to be invalidated ?

Otherwise,
Reviewed-by: Nutty Liu <nutty.liu@xxxxxxxxxxx>

Thanks,
Nutty