Message-ID: <86ttwwh24b.wl-maz@kernel.org>
Date:   Mon, 01 May 2023 09:44:52 +0100
From:   Marc Zyngier <maz@...nel.org>
To:     Anup Patel <anup@...infault.org>
Cc:     Anup Patel <apatel@...tanamicro.com>,
        Palmer Dabbelt <palmer@...belt.com>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Atish Patra <atishp@...shpatra.org>,
        Alistair Francis <Alistair.Francis@....com>,
        linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org,
        devicetree@...r.kernel.org
Subject: Re: [PATCH v2 5/9] irqchip: Add RISC-V incoming MSI controller driver

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

Powered by blists - more mailing lists