On Mon, 01 May 2023 09:28:16 +0100, Anup Patel <anup@xxxxxxxxxxxxxx> wrote: > > On Fri, Jan 13, 2023 at 3:40 PM Marc Zyngier <maz@xxxxxxxxxx> wrote: > > > > On Tue, 03 Jan 2023 14:14:05 +0000, > > Anup Patel <apatel@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxx> > > > --- > > > 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.
