Message-ID: <CAAhSdy3ACa-447MrLs=5GLjSSWKiKyR585RFWP_ycM3RZ+RTQg@mail.gmail.com>
Date:   Tue, 1 Mar 2022 23:10:55 +0530
From:   Anup Patel <anup@...infault.org>
To:     Marc Zyngier <maz@...nel.org>
Cc:     Anup Patel <apatel@...tanamicro.com>,
        Palmer Dabbelt <palmer@...belt.com>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Daniel Lezcano <daniel.lezcano@...aro.org>,
        Atish Patra <atishp@...shpatra.org>,
        Alistair Francis <Alistair.Francis@....com>,
        linux-riscv <linux-riscv@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org List" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 3/6] RISC-V: Treat IPIs as normal Linux IRQs

On Tue, Mar 1, 2022 at 8:07 PM Marc Zyngier <maz@...nel.org> wrote:
>
> On Tue, 01 Mar 2022 04:27:19 +0000,
> Anup Patel <apatel@...tanamicro.com> wrote:
> >
> > Currently, the RISC-V kernel provides arch specific hooks (i.e.
> > struct riscv_ipi_ops) to register IPI handling methods. The stats
> > gathering of IPIs is also arch specific in the RISC-V kernel.
> >
> > Other architectures (such as ARM, ARM64, and MIPS) have moved away
> > from custom arch specific IPI handling methods. Currently, these
> > architectures have Linux irqchip drivers providing a range of Linux
> > IRQ numbers to be used as IPIs and IPI triggering is done using
> > generic IPI APIs. This approach allows architectures to treat IPIs
> > as normal Linux IRQs and IPI stats gathering is done by the generic
> > Linux IRQ subsystem.
> >
> > We extend the RISC-V IPI handling as-per above approach so that arch
> > specific IPI handling methods (struct riscv_ipi_ops) can be removed
> > and the IPI handling is done through the Linux IRQ subsystem.
> >
> > Signed-off-by: Anup Patel <apatel@...tanamicro.com>
> > ---
> >  arch/riscv/Kconfig                |   1 +
> >  arch/riscv/include/asm/ipi-mux.h  |  43 ++++++
> >  arch/riscv/include/asm/sbi.h      |   2 +
> >  arch/riscv/include/asm/smp.h      |  35 +++--
> >  arch/riscv/kernel/Makefile        |   1 +
> >  arch/riscv/kernel/cpu-hotplug.c   |   3 +-
> >  arch/riscv/kernel/ipi-mux.c       | 222 ++++++++++++++++++++++++++++++
> >  arch/riscv/kernel/irq.c           |   3 +-
> >  arch/riscv/kernel/sbi.c           |  13 +-
> >  arch/riscv/kernel/smp.c           | 153 ++++++++++----------
> >  arch/riscv/kernel/smpboot.c       |   5 +-
> >  drivers/clocksource/timer-clint.c |   8 +-
> >  drivers/irqchip/irq-riscv-intc.c  |  55 ++++----
> >  13 files changed, 405 insertions(+), 139 deletions(-)
>
> This patch is huge and touches way too many things to be reviewed as
> such. Please split arch-specific stuff from clocksource and irqchip,
> going through a transition phase.

Sure, I will split this patch into smaller patches.

>
> More below.
>
> >  create mode 100644 arch/riscv/include/asm/ipi-mux.h
> >  create mode 100644 arch/riscv/kernel/ipi-mux.c
> >
> > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > index 5adcbd9b5e88..167681d6d3f8 100644
> > --- a/arch/riscv/Kconfig
> > +++ b/arch/riscv/Kconfig
> > @@ -54,6 +54,7 @@ config RISCV
> >       select GENERIC_GETTIMEOFDAY if HAVE_GENERIC_VDSO
> >       select GENERIC_IDLE_POLL_SETUP
> >       select GENERIC_IOREMAP if MMU
> > +     select GENERIC_IRQ_IPI
> >       select GENERIC_IRQ_MULTI_HANDLER
> >       select GENERIC_IRQ_SHOW
> >       select GENERIC_IRQ_SHOW_LEVEL
> > diff --git a/arch/riscv/include/asm/ipi-mux.h b/arch/riscv/include/asm/ipi-mux.h
> > new file mode 100644
> > index 000000000000..988e2bba372a
> > --- /dev/null
> > +++ b/arch/riscv/include/asm/ipi-mux.h
> > @@ -0,0 +1,43 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * Copyright (c) 2022 Ventana Micro Systems Inc.
> > + */
> > +
> > +#ifndef _ASM_RISCV_IPI_MUX_H
> > +#define _ASM_RISCV_IPI_MUX_H
> > +
> > +struct cpumask;
> > +
> > +#ifdef CONFIG_SMP
> > +
> > +/* Handle muxed IPIs */
> > +void riscv_ipi_mux_handle_irq(void);
> > +
> > +/* Create irq_domain for muxed IPIs */
> > +struct irq_domain *riscv_ipi_mux_create(bool use_soft_irq,
> > +                     void (*clear_ipi)(void),
> > +                     void (*send_ipi)(const struct cpumask *mask));
> > +
> > +/* Destroy irq_domain for muxed IPIs */
> > +void riscv_ipi_mux_destroy(struct irq_domain *d);
> > +
> > +#else
> > +
> > +static inline void riscv_ipi_mux_handle_irq(void)
> > +{
> > +}
> > +
> > +static inline struct irq_domain *riscv_ipi_mux_create(bool use_soft_irq,
> > +                     void (*clear_ipi)(void),
> > +                     void (*send_ipi)(const struct cpumask *mask))
> > +{
> > +     return NULL;
> > +}
> > +
> > +static inline void riscv_ipi_mux_destroy(struct irq_domain *d)
> > +{
> > +}
> > +
> > +#endif
> > +
> > +#endif /* _ASM_RISCV_IPI_MUX_H */
> > diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h
> > index 06133b4f8e20..edd2c3135b4a 100644
> > --- a/arch/riscv/include/asm/sbi.h
> > +++ b/arch/riscv/include/asm/sbi.h
> > @@ -133,6 +133,7 @@ struct sbiret {
> >  };
> >
> >  void sbi_init(void);
> > +void sbi_ipi_init(void);
> >  struct sbiret sbi_ecall(int ext, int fid, unsigned long arg0,
> >                       unsigned long arg1, unsigned long arg2,
> >                       unsigned long arg3, unsigned long arg4,
> > @@ -202,6 +203,7 @@ static inline unsigned long sbi_mk_version(unsigned long major,
> >  int sbi_err_map_linux_errno(int err);
> >  #else /* CONFIG_RISCV_SBI */
> >  static inline int sbi_remote_fence_i(const struct cpumask *cpu_mask) { return -1; }
> > +static inline void sbi_ipi_init(void) { }
> >  static inline void sbi_init(void) {}
> >  #endif /* CONFIG_RISCV_SBI */
> >  #endif /* _ASM_RISCV_SBI_H */
> > diff --git a/arch/riscv/include/asm/smp.h b/arch/riscv/include/asm/smp.h
> > index 23170c933d73..178fe4ada592 100644
> > --- a/arch/riscv/include/asm/smp.h
> > +++ b/arch/riscv/include/asm/smp.h
> > @@ -15,11 +15,6 @@
> >  struct seq_file;
> >  extern unsigned long boot_cpu_hartid;
> >
> > -struct riscv_ipi_ops {
> > -     void (*ipi_inject)(const struct cpumask *target);
> > -     void (*ipi_clear)(void);
> > -};
> > -
> >  #ifdef CONFIG_SMP
> >  /*
> >   * Mapping between linux logical cpu index and hartid.
> > @@ -33,9 +28,6 @@ void show_ipi_stats(struct seq_file *p, int prec);
> >  /* SMP initialization hook for setup_arch */
> >  void __init setup_smp(void);
> >
> > -/* Called from C code, this handles an IPI. */
> > -void handle_IPI(struct pt_regs *regs);
> > -
> >  /* Hook for the generic smp_call_function_many() routine. */
> >  void arch_send_call_function_ipi_mask(struct cpumask *mask);
> >
> > @@ -44,11 +36,17 @@ void arch_send_call_function_single_ipi(int cpu);
> >
> >  int riscv_hartid_to_cpuid(int hartid);
> >
> > -/* Set custom IPI operations */
> > -void riscv_set_ipi_ops(const struct riscv_ipi_ops *ops);
> > +/* Enable IPI for CPU hotplug */
> > +void riscv_ipi_enable(void);
> > +
> > +/* Disable IPI for CPU hotplug */
> > +void riscv_ipi_disable(void);
> >
> > -/* Clear IPI for current CPU */
> > -void riscv_clear_ipi(void);
> > +/* Check if IPI interrupt numbers are available */
> > +bool riscv_ipi_have_virq_range(void);
> > +
> > +/* Set the IPI interrupt numbers for arch (called by irqchip drivers) */
> > +void riscv_ipi_set_virq_range(int virq, int nr_irqs);
> >
> >  /* Secondary hart entry */
> >  asmlinkage void smp_callin(void);
> > @@ -82,11 +80,20 @@ static inline unsigned long cpuid_to_hartid_map(int cpu)
> >       return boot_cpu_hartid;
> >  }
> >
> > -static inline void riscv_set_ipi_ops(const struct riscv_ipi_ops *ops)
> > +static inline void riscv_ipi_enable(void)
> >  {
> >  }
> >
> > -static inline void riscv_clear_ipi(void)
> > +static inline void riscv_ipi_disable(void)
> > +{
> > +}
> > +
> > +static inline bool riscv_ipi_have_virq_range(void)
> > +{
> > +     return false;
> > +}
> > +
> > +static inline void riscv_ipi_set_virq_range(int virq, int nr)
> >  {
> >  }
> >
> > diff --git a/arch/riscv/kernel/Makefile b/arch/riscv/kernel/Makefile
> > index ffc87e76b1dd..5047723e5527 100644
> > --- a/arch/riscv/kernel/Makefile
> > +++ b/arch/riscv/kernel/Makefile
> > @@ -42,6 +42,7 @@ obj-$(CONFIG_RISCV_M_MODE)  += traps_misaligned.o
> >  obj-$(CONFIG_FPU)            += fpu.o
> >  obj-$(CONFIG_SMP)            += smpboot.o
> >  obj-$(CONFIG_SMP)            += smp.o
> > +obj-$(CONFIG_SMP)            += ipi-mux.o
> >  obj-$(CONFIG_SMP)            += cpu_ops.o
> >
> >  obj-$(CONFIG_RISCV_BOOT_SPINWAIT) += cpu_ops_spinwait.o
> > diff --git a/arch/riscv/kernel/cpu-hotplug.c b/arch/riscv/kernel/cpu-hotplug.c
> > index f7a832e3a1d1..39235cf50652 100644
> > --- a/arch/riscv/kernel/cpu-hotplug.c
> > +++ b/arch/riscv/kernel/cpu-hotplug.c
> > @@ -13,7 +13,7 @@
> >  #include <asm/irq.h>
> >  #include <asm/cpu_ops.h>
> >  #include <asm/numa.h>
> > -#include <asm/sbi.h>
> > +#include <asm/smp.h>
> >
> >  bool cpu_has_hotplug(unsigned int cpu)
> >  {
> > @@ -43,6 +43,7 @@ int __cpu_disable(void)
> >       remove_cpu_topology(cpu);
> >       numa_remove_cpu(cpu);
> >       set_cpu_online(cpu, false);
> > +     riscv_ipi_disable();
> >       irq_migrate_all_off_this_cpu();
> >
> >       return ret;
> > diff --git a/arch/riscv/kernel/ipi-mux.c b/arch/riscv/kernel/ipi-mux.c
> > new file mode 100644
> > index 000000000000..3a0405f0e0de
> > --- /dev/null
> > +++ b/arch/riscv/kernel/ipi-mux.c
> > @@ -0,0 +1,222 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Multiplex several IPIs over a single HW IPI.
> > + *
> > + * Copyright (c) 2022 Ventana Micro Systems Inc.
> > + */
> > +
> > +#define pr_fmt(fmt) "riscv-ipi-mux: " fmt
> > +#include <linux/cpu.h>
> > +#include <linux/cpumask.h>
> > +#include <linux/init.h>
> > +#include <linux/irq.h>
> > +#include <linux/irqchip.h>
> > +#include <linux/irqchip/chained_irq.h>
> > +#include <linux/irqdomain.h>
> > +#include <linux/smp.h>
> > +#include <asm/ipi-mux.h>
> > +
> > +struct ipi_mux {
> > +     struct irq_domain *domain;
> > +     int parent_virq;
> > +     void (*clear_ipi)(void);
> > +     void (*send_ipi)(const struct cpumask *mask);
> > +};
>
> Why do you need this in the arch code? It really looks like something
> that is irqchip specific (single IPI signal on which actual IPIs are
> overlayed). It is also something that other irqchips are already
> implementing, so there is potential for consolidation.

I agree we can share the IPI muxing among irqchip drivers.

I was not sure where to place this IPI muxing so I made it
RISC-V specific initially.

Can we place a simplified IPI muxing (with no RISC-V specific
stuff) under drivers/irqchip or kernel/irq ??

>
> > +
> > +static struct ipi_mux ipi_mux_priv;
> > +static DEFINE_PER_CPU(unsigned long, ipi_mux_bits);
> > +
> > +static void ipi_mux_dummy(struct irq_data *d)
> > +{
> > +}
> > +
> > +static void ipi_mux_send_mask(struct irq_data *d, const struct cpumask *mask)
> > +{
> > +     int cpu;
> > +
> > +     /* Barrier before doing atomic bit update to IPI bits */
> > +     smp_mb__before_atomic();
> > +
> > +     for_each_cpu(cpu, mask)
> > +             set_bit(d->hwirq, per_cpu_ptr(&ipi_mux_bits, cpu));
> > +
> > +     /* Barrier after doing atomic bit update to IPI bits */
> > +     smp_mb__after_atomic();
> > +
> > +     if (ipi_mux_priv.send_ipi)
> > +             ipi_mux_priv.send_ipi(mask);
> > +}
> > +
> > +static struct irq_chip ipi_mux_chip = {
> > +     .name           = "RISC-V IPI Mux",
> > +     .irq_mask       = ipi_mux_dummy,
> > +     .irq_unmask     = ipi_mux_dummy,
> > +     .ipi_send_mask  = ipi_mux_send_mask,
> > +};
> > +
> > +static int ipi_mux_domain_map(struct irq_domain *d, unsigned int irq,
> > +                           irq_hw_number_t hwirq)
> > +{
> > +     irq_set_percpu_devid(irq);
> > +     irq_domain_set_info(d, irq, hwirq, &ipi_mux_chip, d->host_data,
> > +                         handle_percpu_devid_irq, NULL, NULL);
> > +
> > +     return 0;
> > +}
> > +
> > +static int ipi_mux_domain_alloc(struct irq_domain *d, unsigned int virq,
> > +                             unsigned int nr_irqs, void *arg)
> > +{
> > +     int i, ret;
> > +     irq_hw_number_t hwirq;
> > +     unsigned int type = IRQ_TYPE_NONE;
> > +     struct irq_fwspec *fwspec = arg;
> > +
> > +     ret = irq_domain_translate_onecell(d, fwspec, &hwirq, &type);
> > +     if (ret)
> > +             return ret;
> > +
> > +     for (i = 0; i < nr_irqs; i++) {
> > +             ret = ipi_mux_domain_map(d, virq + i, hwirq + i);
> > +             if (ret)
> > +                     return ret;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct irq_domain_ops ipi_mux_domain_ops = {
> > +     .translate      = irq_domain_translate_onecell,
> > +     .alloc          = ipi_mux_domain_alloc,
> > +     .free           = irq_domain_free_irqs_top,
> > +};
> > +
> > +void riscv_ipi_mux_handle_irq(void)
> > +{
> > +     int err;
> > +     unsigned long irqs, *bits = this_cpu_ptr(&ipi_mux_bits);
> > +     irq_hw_number_t hwirq;
> > +
> > +     while (true) {
> > +             if (ipi_mux_priv.clear_ipi)
> > +                     ipi_mux_priv.clear_ipi();
> > +
> > +             /* Order bit clearing and data access. */
> > +             mb();
> > +
> > +             irqs = xchg(bits, 0);
> > +             if (!irqs)
> > +                     break;
> > +
> > +             for_each_set_bit(hwirq, &irqs, BITS_PER_LONG) {
> > +                     err = generic_handle_domain_irq(ipi_mux_priv.domain,
> > +                                                     hwirq);
> > +                     if (unlikely(err))
> > +                             pr_warn_ratelimited(
> > +                                     "can't find mapping for hwirq %lu\n",
> > +                                     hwirq);
> > +             }
> > +     }
> > +}
> > +
> > +static void ipi_mux_handle_irq(struct irq_desc *desc)
> > +{
> > +     struct irq_chip *chip = irq_desc_get_chip(desc);
> > +
> > +     chained_irq_enter(chip, desc);
> > +     riscv_ipi_mux_handle_irq();
> > +     chained_irq_exit(chip, desc);
> > +}
> > +
> > +static int ipi_mux_dying_cpu(unsigned int cpu)
> > +{
> > +     if (ipi_mux_priv.parent_virq)
> > +             disable_percpu_irq(ipi_mux_priv.parent_virq);
> > +     return 0;
> > +}
> > +
> > +static int ipi_mux_starting_cpu(unsigned int cpu)
> > +{
> > +     if (ipi_mux_priv.parent_virq)
> > +             enable_percpu_irq(ipi_mux_priv.parent_virq,
> > +                     irq_get_trigger_type(ipi_mux_priv.parent_virq));
> > +     return 0;
> > +}
> > +
> > +struct irq_domain *riscv_ipi_mux_create(bool use_soft_irq,
> > +                     void (*clear_ipi)(void),
> > +                     void (*send_ipi)(const struct cpumask *mask))
> > +{
>
> There really shouldn't be a need for such a registration interface
> anyway (the current idiom is to allocate IPIs in the root irqchip, and
> pass them to the arch code).
>
> Why can't you model it after the existing architectures?

I ended up with a lot of duplicate code between SBI IPI driver and
SiFive CLINT driver so I factored out the IPI muxing as separate
sources. We also have RISC-V AIA drivers using the same IPI muxing.

If we simplify the IPI muxing and move it out of arch/riscv then
changes in this patch are straight forward to review.

>
> > +     int virq, parent_virq = 0;
> > +     struct irq_domain *domain;
> > +     struct irq_fwspec ipi;
> > +
> > +     if (ipi_mux_priv.domain || riscv_ipi_have_virq_range())
> > +             return NULL;
> > +
> > +     if (use_soft_irq) {
> > +             domain = irq_find_matching_fwnode(riscv_get_intc_hwnode(),
> > +                                               DOMAIN_BUS_ANY);
> > +             if (!domain) {
> > +                     pr_err("unable to find INTC IRQ domain\n");
> > +                     return NULL;
> > +             }
> > +
> > +             parent_virq = irq_create_mapping(domain, RV_IRQ_SOFT);
> > +             if (!parent_virq) {
> > +                     pr_err("unable to create INTC IRQ mapping\n");
> > +                     return NULL;
> > +             }
> > +     }
> > +
> > +     domain = irq_domain_add_linear(NULL, BITS_PER_LONG,
> > +                                    &ipi_mux_domain_ops, NULL);
> > +     if (!domain) {
> > +             pr_err("unable to add IPI Mux domain\n");
> > +             goto fail_dispose_mapping;
> > +     }
> > +
> > +     ipi.fwnode = domain->fwnode;
> > +     ipi.param_count = 1;
> > +     ipi.param[0] = 0;
> > +     virq = __irq_domain_alloc_irqs(domain, -1, BITS_PER_LONG,
> > +                                    NUMA_NO_NODE, &ipi, false, NULL);
> > +     if (virq <= 0) {
> > +             pr_err("unable to alloc IRQs from IPI Mux domain\n");
> > +             goto fail_domain_remove;
> > +     }
> > +
> > +     ipi_mux_priv.domain = domain;
> > +     ipi_mux_priv.parent_virq = parent_virq;
> > +     ipi_mux_priv.clear_ipi = clear_ipi;
> > +     ipi_mux_priv.send_ipi = send_ipi;
> > +
> > +     if (parent_virq)
> > +             irq_set_chained_handler(parent_virq, ipi_mux_handle_irq);
> > +
> > +     cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
> > +                       "irqchip/riscv/ipi-mux:starting",
> > +                       ipi_mux_starting_cpu, ipi_mux_dying_cpu);
> > +
> > +     riscv_ipi_set_virq_range(virq, BITS_PER_LONG);
> > +
> > +     return ipi_mux_priv.domain;
> > +
> > +fail_domain_remove:
> > +     irq_domain_remove(domain);
> > +fail_dispose_mapping:
> > +     if (parent_virq)
> > +             irq_dispose_mapping(parent_virq);
> > +     return NULL;
> > +}
> > +
> > +void riscv_ipi_mux_destroy(struct irq_domain *d)
> > +{
> > +     if (!d || ipi_mux_priv.domain != d)
> > +             return;
> > +
> > +     irq_domain_remove(ipi_mux_priv.domain);
> > +     if (ipi_mux_priv.parent_virq)
> > +             irq_dispose_mapping(ipi_mux_priv.parent_virq);
> > +     memset(&ipi_mux_priv, 0, sizeof(ipi_mux_priv));
> > +}
>
> Err... Under which circumstances do you destroy such an irqdomain?
> Once all the CPUs have been removed from the machine?

In case, there was a failure after creating an IPI muxing domain then
this function allows us to destroy the IPI muxing domain.

>
> I really think you should aim for a simpler abstraction. The arch code
> should deal with the interrupts themselves, and leave the
> *implementation* of the IPI to the driver code, potentially shared
> across irqchips. This would avoid creating more new interfaces that
> are specific to one architecture or another.

I agree.

>
>         M.
>
> --
> Without deviation from the norm, progress is not possible.

Regards,
Anup

Powered by blists - more mailing lists