[PATCH v2 5/9] irqchip: Add RISC-V incoming MSI controller driver
Marc Zyngier
maz at kernel.org
Mon May 1 01:44:52 PDT 2023
On Mon, 01 May 2023 09:28:16 +0100,
Anup Patel <anup at brainfault.org> wrote:
>
> On Fri, Jan 13, 2023 at 3:40 PM Marc Zyngier <maz at kernel.org> wrote:
> >
> > On Tue, 03 Jan 2023 14:14:05 +0000,
> > Anup Patel <apatel at ventanamicro.com> wrote:
> > >
> > > The RISC-V advanced interrupt architecture (AIA) specification defines
> > > a new MSI controller for managing MSIs on a RISC-V platform. This new
> > > MSI controller is referred to as incoming message signaled interrupt
> > > controller (IMSIC) which manages MSI on per-HART (or per-CPU) basis.
> > > (For more details refer https://github.com/riscv/riscv-aia)
> >
> > And how about IPIs, which this driver seems to be concerned about?
>
> Okay, I will mention about IPIs in the commit description.
>
> >
> > >
> > > This patch adds an irqchip driver for RISC-V IMSIC found on RISC-V
> > > platforms.
> > >
> > > Signed-off-by: Anup Patel <apatel at ventanamicro.com>
> > > ---
> > > drivers/irqchip/Kconfig | 14 +-
> > > drivers/irqchip/Makefile | 1 +
> > > drivers/irqchip/irq-riscv-imsic.c | 1174 +++++++++++++++++++++++++++
> > > include/linux/irqchip/riscv-imsic.h | 92 +++
> > > 4 files changed, 1280 insertions(+), 1 deletion(-)
> > > create mode 100644 drivers/irqchip/irq-riscv-imsic.c
> > > create mode 100644 include/linux/irqchip/riscv-imsic.h
> > >
> > > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> > > index 9e65345ca3f6..a1315189a595 100644
> > > --- a/drivers/irqchip/Kconfig
> > > +++ b/drivers/irqchip/Kconfig
> > > @@ -29,7 +29,6 @@ config ARM_GIC_V2M
> > >
> > > config GIC_NON_BANKED
> > > bool
> > > -
> > > config ARM_GIC_V3
> > > bool
> > > select IRQ_DOMAIN_HIERARCHY
> > > @@ -548,6 +547,19 @@ config SIFIVE_PLIC
> > > select IRQ_DOMAIN_HIERARCHY
> > > select GENERIC_IRQ_EFFECTIVE_AFF_MASK if SMP
> > >
> > > +config RISCV_IMSIC
> > > + bool
> > > + depends on RISCV
> > > + select IRQ_DOMAIN_HIERARCHY
> > > + select GENERIC_MSI_IRQ_DOMAIN
> > > +
> > > +config RISCV_IMSIC_PCI
> > > + bool
> > > + depends on RISCV_IMSIC
> > > + depends on PCI
> > > + depends on PCI_MSI
> > > + default RISCV_IMSIC
> >
> > This should definitely tell you that this driver needs splitting.
>
> The code under "#ifdef CONFIG_RISCV_IMSIC_PCI" is hardly 40 lines
> so I felt it was too small to deserve its own source file.
It at least needs its own patch.
>
> >
> > > +
> > > config EXYNOS_IRQ_COMBINER
> > > bool "Samsung Exynos IRQ combiner support" if COMPILE_TEST
> > > depends on (ARCH_EXYNOS && ARM) || COMPILE_TEST
> > > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> > > index 87b49a10962c..22c723cc6ec8 100644
> > > --- a/drivers/irqchip/Makefile
> > > +++ b/drivers/irqchip/Makefile
> > > @@ -96,6 +96,7 @@ obj-$(CONFIG_QCOM_MPM) += irq-qcom-mpm.o
> > > obj-$(CONFIG_CSKY_MPINTC) += irq-csky-mpintc.o
> > > obj-$(CONFIG_CSKY_APB_INTC) += irq-csky-apb-intc.o
> > > obj-$(CONFIG_RISCV_INTC) += irq-riscv-intc.o
> > > +obj-$(CONFIG_RISCV_IMSIC) += irq-riscv-imsic.o
> > > obj-$(CONFIG_SIFIVE_PLIC) += irq-sifive-plic.o
> > > obj-$(CONFIG_IMX_IRQSTEER) += irq-imx-irqsteer.o
> > > obj-$(CONFIG_IMX_INTMUX) += irq-imx-intmux.o
> > > diff --git a/drivers/irqchip/irq-riscv-imsic.c b/drivers/irqchip/irq-riscv-imsic.c
> > > new file mode 100644
> > > index 000000000000..4c16b66738d6
> > > --- /dev/null
> > > +++ b/drivers/irqchip/irq-riscv-imsic.c
> > > @@ -0,0 +1,1174 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Copyright (C) 2021 Western Digital Corporation or its affiliates.
> > > + * Copyright (C) 2022 Ventana Micro Systems Inc.
> > > + */
> > > +
> > > +#define pr_fmt(fmt) "riscv-imsic: " fmt
> > > +#include <linux/bitmap.h>
> > > +#include <linux/cpu.h>
> > > +#include <linux/interrupt.h>
> > > +#include <linux/io.h>
> > > +#include <linux/iommu.h>
> > > +#include <linux/irq.h>
> > > +#include <linux/irqchip.h>
> > > +#include <linux/irqchip/chained_irq.h>
> > > +#include <linux/irqchip/riscv-imsic.h>
> > > +#include <linux/irqdomain.h>
> > > +#include <linux/module.h>
> > > +#include <linux/msi.h>
> > > +#include <linux/of.h>
> > > +#include <linux/of_address.h>
> > > +#include <linux/of_irq.h>
> > > +#include <linux/pci.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/spinlock.h>
> > > +#include <linux/smp.h>
> > > +#include <asm/hwcap.h>
> > > +
> > > +#define IMSIC_DISABLE_EIDELIVERY 0
> > > +#define IMSIC_ENABLE_EIDELIVERY 1
> > > +#define IMSIC_DISABLE_EITHRESHOLD 1
> > > +#define IMSIC_ENABLE_EITHRESHOLD 0
> > > +
> > > +#define imsic_csr_write(__c, __v) \
> > > +do { \
> > > + csr_write(CSR_ISELECT, __c); \
> > > + csr_write(CSR_IREG, __v); \
> > > +} while (0)
> > > +
> > > +#define imsic_csr_read(__c) \
> > > +({ \
> > > + unsigned long __v; \
> > > + csr_write(CSR_ISELECT, __c); \
> > > + __v = csr_read(CSR_IREG); \
> > > + __v; \
> > > +})
> > > +
> > > +#define imsic_csr_set(__c, __v) \
> > > +do { \
> > > + csr_write(CSR_ISELECT, __c); \
> > > + csr_set(CSR_IREG, __v); \
> > > +} while (0)
> > > +
> > > +#define imsic_csr_clear(__c, __v) \
> > > +do { \
> > > + csr_write(CSR_ISELECT, __c); \
> > > + csr_clear(CSR_IREG, __v); \
> > > +} while (0)
> > > +
> > > +struct imsic_mmio {
> > > + phys_addr_t pa;
> > > + void __iomem *va;
> > > + unsigned long size;
> > > +};
> > > +
> > > +struct imsic_priv {
> > > + /* Global configuration common for all HARTs */
> > > + struct imsic_global_config global;
> > > +
> > > + /* MMIO regions */
> > > + u32 num_mmios;
> > > + struct imsic_mmio *mmios;
> > > +
> > > + /* Global state of interrupt identities */
> > > + raw_spinlock_t ids_lock;
> > > + unsigned long *ids_used_bimap;
> > > + unsigned long *ids_enabled_bimap;
> > > + unsigned int *ids_target_cpu;
> > > +
> > > + /* Mask for connected CPUs */
> > > + struct cpumask lmask;
> > > +
> > > + /* IPI interrupt identity */
> > > + u32 ipi_id;
> > > + u32 ipi_lsync_id;
> > > +
> > > + /* IRQ domains */
> > > + struct irq_domain *base_domain;
> > > + struct irq_domain *pci_domain;
> > > + struct irq_domain *plat_domain;
> > > +};
> > > +
> > > +struct imsic_handler {
> > > + /* Local configuration for given HART */
> > > + struct imsic_local_config local;
> > > +
> > > + /* Pointer to private context */
> > > + struct imsic_priv *priv;
> > > +};
> > > +
> > > +static bool imsic_init_done;
> > > +
> > > +static int imsic_parent_irq;
> > > +static DEFINE_PER_CPU(struct imsic_handler, imsic_handlers);
> > > +
> > > +const struct imsic_global_config *imsic_get_global_config(void)
> > > +{
> > > + struct imsic_handler *handler = this_cpu_ptr(&imsic_handlers);
> > > +
> > > + if (!handler || !handler->priv)
> > > + return NULL;
> > > +
> > > + return &handler->priv->global;
> > > +}
> > > +EXPORT_SYMBOL_GPL(imsic_get_global_config);
> > > +
> > > +const struct imsic_local_config *imsic_get_local_config(unsigned int cpu)
> > > +{
> > > + struct imsic_handler *handler = per_cpu_ptr(&imsic_handlers, cpu);
> > > +
> > > + if (!handler || !handler->priv)
> > > + return NULL;
> >
> > How can this happen?
>
> These are redundant checks. I will drop.
>
> >
> > > +
> > > + return &handler->local;
> > > +}
> > > +EXPORT_SYMBOL_GPL(imsic_get_local_config);
> >
> > Why are these symbols exported? They have no user, so they shouldn't
> > even exist here. I also seriously doubt there is a valid use case for
> > exposing this information to the rest of the kernel.
>
> The imsic_get_global_config() is used by APLIC driver and KVM RISC-V
> module whereas imsic_get_local_config() is only used by KVM RISC-V.
>
> The KVM RISC-V AIA irqchip patches are available in riscv_kvm_aia_v1
> branch at: https://github.com/avpatel/linux.git. I have not posted KVM RISC-V
> patches due various interdependencies.
Then the symbols can wait, can't they? It'd make more sense if the
KVM-dependent bits were brought together with the KVM patches.
Even better, you'd use some level of abstraction between KVM and the
irqchip code. GIC makes some heavy use of irq_set_vcpu_affinity() as a
private API with KVM, and I'd suggest you look into something similar.
[...]
> > > +#ifdef CONFIG_SMP
> > > +static void __imsic_id_smp_sync(struct imsic_priv *priv)
> > > +{
> > > + struct imsic_handler *handler;
> > > + struct cpumask amask;
> > > + int cpu;
> > > +
> > > + cpumask_and(&amask, &priv->lmask, cpu_online_mask);
> >
> > Can't this race against a CPU going down?
>
> Yes, it can race if a CPU goes down while we are in this function
> but this won't be a problem because the imsic_starting_cpu()
> will unconditionally do imsic_ids_local_sync() when the CPU is
> brought-up again. I will add a multiline comment block explaining
> this.
I'd rather you avoid the race instead of papering over it.
>
> >
> > > + for_each_cpu(cpu, &amask) {
> > > + if (cpu == smp_processor_id())
> > > + continue;
> > > +
> > > + handler = per_cpu_ptr(&imsic_handlers, cpu);
> > > + if (!handler || !handler->priv || !handler->local.msi_va) {
> > > + pr_warn("CPU%d: handler not initialized\n", cpu);
> >
> > How many times are you going to do that? On each failing synchronisation?
>
> My bad for adding these paranoid checks. I remove these checks
> wherever possible.
>
> >
> > > + continue;
> > > + }
> > > +
> > > + writel(handler->priv->ipi_lsync_id, handler->local.msi_va);
> >
> > As I understand it, this is a "behind the scenes" IPI. Why isn't that
> > a *real* IPI?
>
> Yes, that's correct. The ID enable bits are per-CPU accessible only
> via CSRs hence we have a special "behind the scenes" IPI to
> synchronize state of ID enable bits.
My question still stands: why isn't this a *real*, Linux visible IPI?
This sideband signalling makes everything hard to follow, hard to
debug, and screws up accounting.
> > Please split the whole guest stuff out. It is totally unused!
>
> The number of guest IDs is used by KVM RISC-V AIA support which
> is in the pipeline. The KVM RISC-V only need imsic_get_global_config()
> and imsic_get_local_config(). The "nr_guest_ids" is part of the
> IMSIC global config.
And yet it isn't needed for a minimal driver, which what I'd like to
see at first. Shoving the kitchen sink into an initial patch isn't a
great way to get it merged.
M.
--
Without deviation from the norm, progress is not possible.
More information about the linux-riscv
mailing list