[PATCH 0/5] genirq/irqchip: RISC-V PLIC cleanup and optimization

[PATCH 0/5] genirq/irqchip: RISC-V PLIC cleanup and optimization

[Samuel Holland]

This series removes the splinlocks and cpumask operations from the PLIC
driver's hot path. To do that, it first makes the IRQ affinity mask
behavior more consistent between uniprocessor and SMP configurations.
(The Allwinner D1 is a uniprocessor SoC containing a PLIC.)

A further optimization is to take advantage of the fact that multiple
IRQs can be claimed at once. This allows removing the mask operations
for oneshot IRQs -- i.e. the combination of IRQCHIP_ONESHOT_SAFE and
IRQCHIP_EOI_THREADED, which is not currently supported. I will send
this as a separate series, since it makes more invasive changes to the
generic IRQ code.


Samuel Holland (5):
  genirq: GENERIC_IRQ_EFFECTIVE_AFF_MASK depends on SMP
  genirq: Refactor accessors to use irq_data_get_affinity_mask
  genirq: Provide an IRQ affinity mask in non-SMP configs
  irqchip/sifive-plic: Make better use of the effective affinity mask
  irqchip/sifive-plic: Separate the enable and mask operations

 arch/arm/mach-hisi/Kconfig        |  2 +-
 drivers/irqchip/Kconfig           | 15 ++++----
 drivers/irqchip/irq-sifive-plic.c | 61 ++++++++++++++++---------------
 include/linux/irq.h               | 20 ++++++----
 kernel/irq/Kconfig                |  1 +
 5 files changed, 54 insertions(+), 45 deletions(-)

-- 
2.35.1

[PATCH 1/5] genirq: GENERIC_IRQ_EFFECTIVE_AFF_MASK depends on SMP

[Samuel Holland]

An IRQ's effective affinity can only be different from its configured
affinity if there are multiple CPUs. Make it clear that this option is
only meaningful when SMP is enabled. Most of the relevant code in
irqdesc.c is already hidden behind CONFIG_SMP anyway.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---

 arch/arm/mach-hisi/Kconfig |  2 +-
 drivers/irqchip/Kconfig    | 14 +++++++-------
 kernel/irq/Kconfig         |  1 +
 3 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/arch/arm/mach-hisi/Kconfig b/arch/arm/mach-hisi/Kconfig
index 2e980f834a6a..36d71f6797ca 100644
--- a/arch/arm/mach-hisi/Kconfig
+++ b/arch/arm/mach-hisi/Kconfig
@@ -40,7 +40,7 @@ config ARCH_HIP04
 	select HAVE_ARM_ARCH_TIMER
 	select MCPM if SMP
 	select MCPM_QUAD_CLUSTER if SMP
-	select GENERIC_IRQ_EFFECTIVE_AFF_MASK
+	select GENERIC_IRQ_EFFECTIVE_AFF_MASK if SMP
 	help
 	  Support for Hisilicon HiP04 SoC family
 
diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 15edb9a6fcae..bc90500a7573 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -8,7 +8,7 @@ config IRQCHIP
 config ARM_GIC
 	bool
 	select IRQ_DOMAIN_HIERARCHY
-	select GENERIC_IRQ_EFFECTIVE_AFF_MASK
+	select GENERIC_IRQ_EFFECTIVE_AFF_MASK if SMP
 
 config ARM_GIC_PM
 	bool
@@ -34,7 +34,7 @@ config ARM_GIC_V3
 	bool
 	select IRQ_DOMAIN_HIERARCHY
 	select PARTITION_PERCPU
-	select GENERIC_IRQ_EFFECTIVE_AFF_MASK
+	select GENERIC_IRQ_EFFECTIVE_AFF_MASK if SMP
 
 config ARM_GIC_V3_ITS
 	bool
@@ -76,7 +76,7 @@ config ARMADA_370_XP_IRQ
 	bool
 	select GENERIC_IRQ_CHIP
 	select PCI_MSI if PCI
-	select GENERIC_IRQ_EFFECTIVE_AFF_MASK
+	select GENERIC_IRQ_EFFECTIVE_AFF_MASK if SMP
 
 config ALPINE_MSI
 	bool
@@ -112,7 +112,7 @@ config BCM6345_L1_IRQ
 	bool
 	select GENERIC_IRQ_CHIP
 	select IRQ_DOMAIN
-	select GENERIC_IRQ_EFFECTIVE_AFF_MASK
+	select GENERIC_IRQ_EFFECTIVE_AFF_MASK if SMP
 
 config BCM7038_L1_IRQ
 	tristate "Broadcom STB 7038-style L1/L2 interrupt controller driver"
@@ -120,7 +120,7 @@ config BCM7038_L1_IRQ
 	default ARCH_BRCMSTB || BMIPS_GENERIC
 	select GENERIC_IRQ_CHIP
 	select IRQ_DOMAIN
-	select GENERIC_IRQ_EFFECTIVE_AFF_MASK
+	select GENERIC_IRQ_EFFECTIVE_AFF_MASK if SMP
 
 config BCM7120_L2_IRQ
 	tristate "Broadcom STB 7120-style L2 interrupt controller driver"
@@ -179,7 +179,7 @@ config IRQ_MIPS_CPU
 	select GENERIC_IRQ_CHIP
 	select GENERIC_IRQ_IPI if SYS_SUPPORTS_MULTITHREADING
 	select IRQ_DOMAIN
-	select GENERIC_IRQ_EFFECTIVE_AFF_MASK
+	select GENERIC_IRQ_EFFECTIVE_AFF_MASK if SMP
 
 config CLPS711X_IRQCHIP
 	bool
@@ -282,7 +282,7 @@ config VERSATILE_FPGA_IRQ_NR
 config XTENSA_MX
 	bool
 	select IRQ_DOMAIN
-	select GENERIC_IRQ_EFFECTIVE_AFF_MASK
+	select GENERIC_IRQ_EFFECTIVE_AFF_MASK if SMP
 
 config XILINX_INTC
 	bool "Xilinx Interrupt Controller IP"
diff --git a/kernel/irq/Kconfig b/kernel/irq/Kconfig
index 10929eda9825..a2a8df39c2bc 100644
--- a/kernel/irq/Kconfig
+++ b/kernel/irq/Kconfig
@@ -24,6 +24,7 @@ config GENERIC_IRQ_SHOW_LEVEL
 
 # Supports effective affinity mask
 config GENERIC_IRQ_EFFECTIVE_AFF_MASK
+       depends on SMP
        bool
 
 # Support for delayed migration from interrupt context
-- 
2.35.1

[PATCH 2/5] genirq: Refactor accessors to use irq_data_get_affinity_mask

[Samuel Holland]

A couple of functions directly reference the affinity mask. Route them
through irq_data_get_affinity_mask so they will pick up any refactoring
done there.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---

 include/linux/irq.h | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/include/linux/irq.h b/include/linux/irq.h
index f92788ccdba2..48ac33ca3703 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -877,16 +877,16 @@ static inline int irq_data_get_node(struct irq_data *d)
 	return irq_common_data_get_node(d->common);
 }
 
-static inline struct cpumask *irq_get_affinity_mask(int irq)
+static inline struct cpumask *irq_data_get_affinity_mask(struct irq_data *d)
 {
-	struct irq_data *d = irq_get_irq_data(irq);
-
-	return d ? d->common->affinity : NULL;
+	return d->common->affinity;
 }
 
-static inline struct cpumask *irq_data_get_affinity_mask(struct irq_data *d)
+static inline struct cpumask *irq_get_affinity_mask(int irq)
 {
-	return d->common->affinity;
+	struct irq_data *d = irq_get_irq_data(irq);
+
+	return d ? irq_data_get_affinity_mask(d) : NULL;
 }
 
 #ifdef CONFIG_GENERIC_IRQ_EFFECTIVE_AFF_MASK
@@ -908,7 +908,7 @@ static inline void irq_data_update_effective_affinity(struct irq_data *d,
 static inline
 struct cpumask *irq_data_get_effective_affinity_mask(struct irq_data *d)
 {
-	return d->common->affinity;
+	return irq_data_get_affinity_mask(d);
 }
 #endif
 
-- 
2.35.1

[PATCH 3/5] genirq: Provide an IRQ affinity mask in non-SMP configs

[Samuel Holland]

IRQ affinity masks are not allocated in uniprocessor configurations.
This requires special case non-SMP code in drivers for irqchips which
have per-CPU enable or mask registers.

Since IRQ affinity is always the same in a uniprocessor configuration,
we can still provide the correct affinity mask without allocating one
per IRQ. We can reuse the system-wide cpu_possible_mask.

By returning a real cpumask from irq_data_get_affinity_mask even when
SMP is disabled, irqchip drivers which iterate over that mask will
automatically do the right thing.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---

 include/linux/irq.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/linux/irq.h b/include/linux/irq.h
index 48ac33ca3703..d716e882c020 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -151,7 +151,9 @@ struct irq_common_data {
 #endif
 	void			*handler_data;
 	struct msi_desc		*msi_desc;
+#ifdef CONFIG_SMP
 	cpumask_var_t		affinity;
+#endif
 #ifdef CONFIG_GENERIC_IRQ_EFFECTIVE_AFF_MASK
 	cpumask_var_t		effective_affinity;
 #endif
@@ -879,7 +881,11 @@ static inline int irq_data_get_node(struct irq_data *d)
 
 static inline struct cpumask *irq_data_get_affinity_mask(struct irq_data *d)
 {
+#ifdef CONFIG_SMP
 	return d->common->affinity;
+#else
+	return &__cpu_possible_mask;
+#endif
 }
 
 static inline struct cpumask *irq_get_affinity_mask(int irq)
-- 
2.35.1

Re: [PATCH 3/5] genirq: Provide an IRQ affinity mask in non-SMP configs

[kernel test robot]

Hi Samuel,

I love your patch! Yet something to improve:

[auto build test ERROR on tip/irq/core]
[also build test ERROR on soc/for-next linus/master v5.18-rc6 next-20220506]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Samuel-Holland/genirq-irqchip-RISC-V-PLIC-cleanup-and-optimization/20220509-115510
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git ce4818957fdc5bca57fc2c92b0dfe109d26bcc47
config: mips-randconfig-r024-20220509 (https://download.01.org/0day-ci/archive/20220509/202205091545.QhhCDxVm-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project a385645b470e2d3a1534aae618ea56b31177639f)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install mips cross compiling tool for clang build
        # apt-get install binutils-mips-linux-gnu
        # https://github.com/intel-lab-lkp/linux/commit/4bca5b436c97eb2ee232dd23f262ebad05fa183c
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Samuel-Holland/genirq-irqchip-RISC-V-PLIC-cleanup-and-optimization/20220509-115510
        git checkout 4bca5b436c97eb2ee232dd23f262ebad05fa183c
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=mips SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> kernel/irq/ipi.c:94:30: error: no member named 'affinity' in 'struct irq_common_data'
                   cpumask_copy(data->common->affinity, dest);
                                ~~~~~~~~~~~~  ^
   1 error generated.


vim +94 kernel/irq/ipi.c

d17bf24e695290 Qais Yousef     2015-12-08   13  
d17bf24e695290 Qais Yousef     2015-12-08   14  /**
d17bf24e695290 Qais Yousef     2015-12-08   15   * irq_reserve_ipi() - Setup an IPI to destination cpumask
d17bf24e695290 Qais Yousef     2015-12-08   16   * @domain:	IPI domain
3b35e7e6daef5a Randy Dunlap    2021-08-10   17   * @dest:	cpumask of CPUs which can receive the IPI
d17bf24e695290 Qais Yousef     2015-12-08   18   *
d17bf24e695290 Qais Yousef     2015-12-08   19   * Allocate a virq that can be used to send IPI to any CPU in dest mask.
d17bf24e695290 Qais Yousef     2015-12-08   20   *
3b35e7e6daef5a Randy Dunlap    2021-08-10   21   * Return: Linux IRQ number on success or error code on failure
d17bf24e695290 Qais Yousef     2015-12-08   22   */
7cec18a3906b52 Matt Redfearn   2016-04-25   23  int irq_reserve_ipi(struct irq_domain *domain,
d17bf24e695290 Qais Yousef     2015-12-08   24  			     const struct cpumask *dest)
d17bf24e695290 Qais Yousef     2015-12-08   25  {
d17bf24e695290 Qais Yousef     2015-12-08   26  	unsigned int nr_irqs, offset;
d17bf24e695290 Qais Yousef     2015-12-08   27  	struct irq_data *data;
d17bf24e695290 Qais Yousef     2015-12-08   28  	int virq, i;
d17bf24e695290 Qais Yousef     2015-12-08   29  
d17bf24e695290 Qais Yousef     2015-12-08   30  	if (!domain ||!irq_domain_is_ipi(domain)) {
d17bf24e695290 Qais Yousef     2015-12-08   31  		pr_warn("Reservation on a non IPI domain\n");
7cec18a3906b52 Matt Redfearn   2016-04-25   32  		return -EINVAL;
d17bf24e695290 Qais Yousef     2015-12-08   33  	}
d17bf24e695290 Qais Yousef     2015-12-08   34  
d17bf24e695290 Qais Yousef     2015-12-08   35  	if (!cpumask_subset(dest, cpu_possible_mask)) {
d17bf24e695290 Qais Yousef     2015-12-08   36  		pr_warn("Reservation is not in possible_cpu_mask\n");
7cec18a3906b52 Matt Redfearn   2016-04-25   37  		return -EINVAL;
d17bf24e695290 Qais Yousef     2015-12-08   38  	}
d17bf24e695290 Qais Yousef     2015-12-08   39  
d17bf24e695290 Qais Yousef     2015-12-08   40  	nr_irqs = cpumask_weight(dest);
d17bf24e695290 Qais Yousef     2015-12-08   41  	if (!nr_irqs) {
d17bf24e695290 Qais Yousef     2015-12-08   42  		pr_warn("Reservation for empty destination mask\n");
7cec18a3906b52 Matt Redfearn   2016-04-25   43  		return -EINVAL;
d17bf24e695290 Qais Yousef     2015-12-08   44  	}
d17bf24e695290 Qais Yousef     2015-12-08   45  
d17bf24e695290 Qais Yousef     2015-12-08   46  	if (irq_domain_is_ipi_single(domain)) {
d17bf24e695290 Qais Yousef     2015-12-08   47  		/*
d17bf24e695290 Qais Yousef     2015-12-08   48  		 * If the underlying implementation uses a single HW irq on
d17bf24e695290 Qais Yousef     2015-12-08   49  		 * all cpus then we only need a single Linux irq number for
d17bf24e695290 Qais Yousef     2015-12-08   50  		 * it. We have no restrictions vs. the destination mask. The
d17bf24e695290 Qais Yousef     2015-12-08   51  		 * underlying implementation can deal with holes nicely.
d17bf24e695290 Qais Yousef     2015-12-08   52  		 */
d17bf24e695290 Qais Yousef     2015-12-08   53  		nr_irqs = 1;
d17bf24e695290 Qais Yousef     2015-12-08   54  		offset = 0;
d17bf24e695290 Qais Yousef     2015-12-08   55  	} else {
d17bf24e695290 Qais Yousef     2015-12-08   56  		unsigned int next;
d17bf24e695290 Qais Yousef     2015-12-08   57  
d17bf24e695290 Qais Yousef     2015-12-08   58  		/*
c5f48c0a7aa1a8 Ingo Molnar     2018-12-03   59  		 * The IPI requires a separate HW irq on each CPU. We require
d17bf24e695290 Qais Yousef     2015-12-08   60  		 * that the destination mask is consecutive. If an
d17bf24e695290 Qais Yousef     2015-12-08   61  		 * implementation needs to support holes, it can reserve
d17bf24e695290 Qais Yousef     2015-12-08   62  		 * several IPI ranges.
d17bf24e695290 Qais Yousef     2015-12-08   63  		 */
d17bf24e695290 Qais Yousef     2015-12-08   64  		offset = cpumask_first(dest);
d17bf24e695290 Qais Yousef     2015-12-08   65  		/*
d17bf24e695290 Qais Yousef     2015-12-08   66  		 * Find a hole and if found look for another set bit after the
d17bf24e695290 Qais Yousef     2015-12-08   67  		 * hole. For now we don't support this scenario.
d17bf24e695290 Qais Yousef     2015-12-08   68  		 */
d17bf24e695290 Qais Yousef     2015-12-08   69  		next = cpumask_next_zero(offset, dest);
d17bf24e695290 Qais Yousef     2015-12-08   70  		if (next < nr_cpu_ids)
d17bf24e695290 Qais Yousef     2015-12-08   71  			next = cpumask_next(next, dest);
d17bf24e695290 Qais Yousef     2015-12-08   72  		if (next < nr_cpu_ids) {
d17bf24e695290 Qais Yousef     2015-12-08   73  			pr_warn("Destination mask has holes\n");
7cec18a3906b52 Matt Redfearn   2016-04-25   74  			return -EINVAL;
d17bf24e695290 Qais Yousef     2015-12-08   75  		}
d17bf24e695290 Qais Yousef     2015-12-08   76  	}
d17bf24e695290 Qais Yousef     2015-12-08   77  
06ee6d571f0e35 Thomas Gleixner 2016-07-04   78  	virq = irq_domain_alloc_descs(-1, nr_irqs, 0, NUMA_NO_NODE, NULL);
d17bf24e695290 Qais Yousef     2015-12-08   79  	if (virq <= 0) {
d17bf24e695290 Qais Yousef     2015-12-08   80  		pr_warn("Can't reserve IPI, failed to alloc descs\n");
7cec18a3906b52 Matt Redfearn   2016-04-25   81  		return -ENOMEM;
d17bf24e695290 Qais Yousef     2015-12-08   82  	}
d17bf24e695290 Qais Yousef     2015-12-08   83  
d17bf24e695290 Qais Yousef     2015-12-08   84  	virq = __irq_domain_alloc_irqs(domain, virq, nr_irqs, NUMA_NO_NODE,
eb0dc47ab6810c Vincent Stehle  2016-07-18   85  				       (void *) dest, true, NULL);
d17bf24e695290 Qais Yousef     2015-12-08   86  
d17bf24e695290 Qais Yousef     2015-12-08   87  	if (virq <= 0) {
d17bf24e695290 Qais Yousef     2015-12-08   88  		pr_warn("Can't reserve IPI, failed to alloc hw irqs\n");
d17bf24e695290 Qais Yousef     2015-12-08   89  		goto free_descs;
d17bf24e695290 Qais Yousef     2015-12-08   90  	}
d17bf24e695290 Qais Yousef     2015-12-08   91  
d17bf24e695290 Qais Yousef     2015-12-08   92  	for (i = 0; i < nr_irqs; i++) {
d17bf24e695290 Qais Yousef     2015-12-08   93  		data = irq_get_irq_data(virq + i);
d17bf24e695290 Qais Yousef     2015-12-08  @94  		cpumask_copy(data->common->affinity, dest);
d17bf24e695290 Qais Yousef     2015-12-08   95  		data->common->ipi_offset = offset;
4589f450fb285a Matt Redfearn   2016-04-21   96  		irq_set_status_flags(virq + i, IRQ_NO_BALANCING);
d17bf24e695290 Qais Yousef     2015-12-08   97  	}
d17bf24e695290 Qais Yousef     2015-12-08   98  	return virq;
d17bf24e695290 Qais Yousef     2015-12-08   99  
d17bf24e695290 Qais Yousef     2015-12-08  100  free_descs:
d17bf24e695290 Qais Yousef     2015-12-08  101  	irq_free_descs(virq, nr_irqs);
7cec18a3906b52 Matt Redfearn   2016-04-25  102  	return -EBUSY;
d17bf24e695290 Qais Yousef     2015-12-08  103  }
d17bf24e695290 Qais Yousef     2015-12-08  104  

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH 3/5] genirq: Provide an IRQ affinity mask in non-SMP configs

[Samuel Holland]

On 5/9/22 2:52 AM, kernel test robot wrote:
> Hi Samuel,
> 
> I love your patch! Yet something to improve:
> 
> [auto build test ERROR on tip/irq/core]
> [also build test ERROR on soc/for-next linus/master v5.18-rc6 next-20220506]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Samuel-Holland/genirq-irqchip-RISC-V-PLIC-cleanup-and-optimization/20220509-115510
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git ce4818957fdc5bca57fc2c92b0dfe109d26bcc47
> config: mips-randconfig-r024-20220509 (https://download.01.org/0day-ci/archive/20220509/202205091545.QhhCDxVm-lkp@intel.com/config)
> compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project a385645b470e2d3a1534aae618ea56b31177639f)
> reproduce (this is a W=1 build):
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # install mips cross compiling tool for clang build
>         # apt-get install binutils-mips-linux-gnu
>         # https://github.com/intel-lab-lkp/linux/commit/4bca5b436c97eb2ee232dd23f262ebad05fa183c
>         git remote add linux-review https://github.com/intel-lab-lkp/linux
>         git fetch --no-tags linux-review Samuel-Holland/genirq-irqchip-RISC-V-PLIC-cleanup-and-optimization/20220509-115510
>         git checkout 4bca5b436c97eb2ee232dd23f262ebad05fa183c
>         # save the config file
>         mkdir build_dir && cp config build_dir/.config
>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=mips SHELL=/bin/bash
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> 
> All errors (new ones prefixed by >>):
> 
>>> kernel/irq/ipi.c:94:30: error: no member named 'affinity' in 'struct irq_common_data'
>                    cpumask_copy(data->common->affinity, dest);
>                                 ~~~~~~~~~~~~  ^
>    1 error generated.

I think the fix here is to make GENERIC_IRQ_IPI also depend on SMP. I will add a
patch doing this for v2.

Regards,
Samuel

[PATCH 4/5] irqchip/sifive-plic: Make better use of the effective affinity mask

[Samuel Holland]

The PLIC driver already updates the effective affinity mask in its
.irq_set_affinity callback. Take advantage of that information to only
touch bits (and take spinlocks) for the specific relevant hart contexts.

First, make sure the effective affinity mask is set before IRQ startup.
Since this mask already takes priv->lmask into account, checking that
mask later is no longer needed (and handler->present is equivalent to
the bit being set in priv->lmask).

Then, when (un)masking or changing affinity, only clear/set the enable
bits in the specific old/new context(s). The cpumask operations in
plic_irq_unmask() are not needed because they duplicate the code in
plic_set_affinity().

Signed-off-by: Samuel Holland <samuel@sholland.org>
---

 drivers/irqchip/Kconfig           |  1 +
 drivers/irqchip/irq-sifive-plic.c | 26 ++++++++------------------
 2 files changed, 9 insertions(+), 18 deletions(-)

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index bc90500a7573..56ef2fc6d2eb 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -529,6 +529,7 @@ config SIFIVE_PLIC
 	bool "SiFive Platform-Level Interrupt Controller"
 	depends on RISCV
 	select IRQ_DOMAIN_HIERARCHY
+	select GENERIC_IRQ_EFFECTIVE_AFF_MASK if SMP
 	help
 	   This enables support for the PLIC chip found in SiFive (and
 	   potentially other) RISC-V systems.  The PLIC controls devices
diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
index bb87e4c3b88e..bf7d5bee0c0c 100644
--- a/drivers/irqchip/irq-sifive-plic.c
+++ b/drivers/irqchip/irq-sifive-plic.c
@@ -109,31 +109,18 @@ static inline void plic_irq_toggle(const struct cpumask *mask,
 	for_each_cpu(cpu, mask) {
 		struct plic_handler *handler = per_cpu_ptr(&plic_handlers, cpu);
 
-		if (handler->present &&
-		    cpumask_test_cpu(cpu, &handler->priv->lmask))
-			plic_toggle(handler, d->hwirq, enable);
+		plic_toggle(handler, d->hwirq, enable);
 	}
 }
 
 static void plic_irq_unmask(struct irq_data *d)
 {
-	struct cpumask amask;
-	unsigned int cpu;
-	struct plic_priv *priv = irq_data_get_irq_chip_data(d);
-
-	cpumask_and(&amask, &priv->lmask, cpu_online_mask);
-	cpu = cpumask_any_and(irq_data_get_affinity_mask(d),
-					   &amask);
-	if (WARN_ON_ONCE(cpu >= nr_cpu_ids))
-		return;
-	plic_irq_toggle(cpumask_of(cpu), d, 1);
+	plic_irq_toggle(irq_data_get_effective_affinity_mask(d), d, 1);
 }
 
 static void plic_irq_mask(struct irq_data *d)
 {
-	struct plic_priv *priv = irq_data_get_irq_chip_data(d);
-
-	plic_irq_toggle(&priv->lmask, d, 0);
+	plic_irq_toggle(irq_data_get_effective_affinity_mask(d), d, 0);
 }
 
 #ifdef CONFIG_SMP
@@ -154,11 +141,13 @@ static int plic_set_affinity(struct irq_data *d,
 	if (cpu >= nr_cpu_ids)
 		return -EINVAL;
 
-	plic_irq_toggle(&priv->lmask, d, 0);
-	plic_irq_toggle(cpumask_of(cpu), d, !irqd_irq_masked(d));
+	plic_irq_mask(d);
 
 	irq_data_update_effective_affinity(d, cpumask_of(cpu));
 
+	if (!irqd_irq_masked(d))
+		plic_irq_unmask(d);
+
 	return IRQ_SET_MASK_OK_DONE;
 }
 #endif
@@ -184,6 +173,7 @@ static struct irq_chip plic_chip = {
 #ifdef CONFIG_SMP
 	.irq_set_affinity = plic_set_affinity,
 #endif
+	.flags		= IRQCHIP_AFFINITY_PRE_STARTUP,
 };
 
 static int plic_irqdomain_map(struct irq_domain *d, unsigned int irq,
-- 
2.35.1

[PATCH 5/5] irqchip/sifive-plic: Separate the enable and mask operations

[Samuel Holland]

The PLIC has two per-IRQ checks before sending an IRQ to a hart context.
First, it checks that the IRQ's priority is nonzero. Then, it checks
that the enable bit is set for that combination of IRQ and context.

Currently, the PLIC driver sets both the priority value and the enable
bit in its (un)mask operations. However, modifying the enable bit is
problematic for two reasons:
  1) The enable bits are packed, so changes are not atomic and require
     taking a spinlock.
  2) The following requirememnt from the PLIC spec, which explains the
     racy (un)mask operations in plic_irq_eoi():

       If the completion ID does not match an interrupt source
       that is currently enabled for the target, the completion
       is silently ignored.

Both of these problems are solved by using the priority value to mask
IRQs. Each IRQ has a separate priority register, so writing the priority
value is atomic. And since the enable bit remains set while an IRQ is
masked, the EOI operation works normally. The enable bits are still used
to control the IRQ's affinity.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---

 drivers/irqchip/irq-sifive-plic.c | 53 +++++++++++++++++++------------
 1 file changed, 32 insertions(+), 21 deletions(-)

diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
index bf7d5bee0c0c..53d266a571be 100644
--- a/drivers/irqchip/irq-sifive-plic.c
+++ b/drivers/irqchip/irq-sifive-plic.c
@@ -103,9 +103,7 @@ static inline void plic_irq_toggle(const struct cpumask *mask,
 				   struct irq_data *d, int enable)
 {
 	int cpu;
-	struct plic_priv *priv = irq_data_get_irq_chip_data(d);
 
-	writel(enable, priv->regs + PRIORITY_BASE + d->hwirq * PRIORITY_PER_ID);
 	for_each_cpu(cpu, mask) {
 		struct plic_handler *handler = per_cpu_ptr(&plic_handlers, cpu);
 
@@ -113,16 +111,37 @@ static inline void plic_irq_toggle(const struct cpumask *mask,
 	}
 }
 
-static void plic_irq_unmask(struct irq_data *d)
+static void plic_irq_enable(struct irq_data *d)
 {
 	plic_irq_toggle(irq_data_get_effective_affinity_mask(d), d, 1);
 }
 
-static void plic_irq_mask(struct irq_data *d)
+static void plic_irq_disable(struct irq_data *d)
 {
 	plic_irq_toggle(irq_data_get_effective_affinity_mask(d), d, 0);
 }
 
+static void plic_irq_unmask(struct irq_data *d)
+{
+	struct plic_priv *priv = irq_data_get_irq_chip_data(d);
+
+	writel(1, priv->regs + PRIORITY_BASE + d->hwirq * PRIORITY_PER_ID);
+}
+
+static void plic_irq_mask(struct irq_data *d)
+{
+	struct plic_priv *priv = irq_data_get_irq_chip_data(d);
+
+	writel(0, priv->regs + PRIORITY_BASE + d->hwirq * PRIORITY_PER_ID);
+}
+
+static void plic_irq_eoi(struct irq_data *d)
+{
+	struct plic_handler *handler = this_cpu_ptr(&plic_handlers);
+
+	writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM);
+}
+
 #ifdef CONFIG_SMP
 static int plic_set_affinity(struct irq_data *d,
 			     const struct cpumask *mask_val, bool force)
@@ -141,32 +160,21 @@ static int plic_set_affinity(struct irq_data *d,
 	if (cpu >= nr_cpu_ids)
 		return -EINVAL;
 
-	plic_irq_mask(d);
+	plic_irq_disable(d);
 
 	irq_data_update_effective_affinity(d, cpumask_of(cpu));
 
-	if (!irqd_irq_masked(d))
-		plic_irq_unmask(d);
+	if (!irqd_irq_disabled(d))
+		plic_irq_enable(d);
 
 	return IRQ_SET_MASK_OK_DONE;
 }
 #endif
 
-static void plic_irq_eoi(struct irq_data *d)
-{
-	struct plic_handler *handler = this_cpu_ptr(&plic_handlers);
-
-	if (irqd_irq_masked(d)) {
-		plic_irq_unmask(d);
-		writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM);
-		plic_irq_mask(d);
-	} else {
-		writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM);
-	}
-}
-
 static struct irq_chip plic_chip = {
 	.name		= "SiFive PLIC",
+	.irq_enable	= plic_irq_enable,
+	.irq_disable	= plic_irq_disable,
 	.irq_mask	= plic_irq_mask,
 	.irq_unmask	= plic_irq_unmask,
 	.irq_eoi	= plic_irq_eoi,
@@ -372,8 +380,11 @@ static int __init plic_init(struct device_node *node,
 			i * CONTEXT_ENABLE_SIZE;
 		handler->priv = priv;
 done:
-		for (hwirq = 1; hwirq <= nr_irqs; hwirq++)
+		for (hwirq = 1; hwirq <= nr_irqs; hwirq++) {
 			plic_toggle(handler, hwirq, 0);
+			writel(1, priv->regs + PRIORITY_BASE +
+				  hwirq * PRIORITY_PER_ID);
+		}
 		nr_handlers++;
 	}
 
-- 
2.35.1

Re: [PATCH 5/5] irqchip/sifive-plic: Separate the enable and mask operations

[Marc Zyngier]

On Mon, 09 May 2022 04:43:33 +0100,
Samuel Holland <samuel@sholland.org> wrote:
> 
> The PLIC has two per-IRQ checks before sending an IRQ to a hart context.
> First, it checks that the IRQ's priority is nonzero. Then, it checks
> that the enable bit is set for that combination of IRQ and context.
> 
> Currently, the PLIC driver sets both the priority value and the enable
> bit in its (un)mask operations. However, modifying the enable bit is
> problematic for two reasons:
>   1) The enable bits are packed, so changes are not atomic and require
>      taking a spinlock.
>   2) The following requirememnt from the PLIC spec, which explains the
>      racy (un)mask operations in plic_irq_eoi():
> 
>        If the completion ID does not match an interrupt source
>        that is currently enabled for the target, the completion
>        is silently ignored.
> 
> Both of these problems are solved by using the priority value to mask
> IRQs. Each IRQ has a separate priority register, so writing the priority
> value is atomic. And since the enable bit remains set while an IRQ is
> masked, the EOI operation works normally. The enable bits are still used
> to control the IRQ's affinity.

This is pretty neat.

My only concern is around whether implementations do when changing
priority of enabled interrupts. The PLIC architecture is conveniently
silent on the subject, but that's certainly something that can result
in total crap with the ARM GICs, for example, because an
implementation is free to apply this priority change on an already
pending interrupt, or not. But the kernel really wants the interrupt
to be masked once it tells the HW to do so.

Could anyone please check the RTL for some common implementations?

A way to avoid the above trouble (should it exist) would be to
disable the interrupt when changing the priority, and then reenable
it. You'd still get the simpler EOI, which is what you really want.

Thanks,

	M.

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

Re: [PATCH 5/5] irqchip/sifive-plic: Separate the enable and mask operations

[Samuel Holland]

On 5/10/22 5:27 AM, Marc Zyngier wrote:
> On Mon, 09 May 2022 04:43:33 +0100,
> Samuel Holland <samuel@sholland.org> wrote:
>>
>> The PLIC has two per-IRQ checks before sending an IRQ to a hart context.
>> First, it checks that the IRQ's priority is nonzero. Then, it checks
>> that the enable bit is set for that combination of IRQ and context.
>>
>> Currently, the PLIC driver sets both the priority value and the enable
>> bit in its (un)mask operations. However, modifying the enable bit is
>> problematic for two reasons:
>>   1) The enable bits are packed, so changes are not atomic and require
>>      taking a spinlock.
>>   2) The following requirememnt from the PLIC spec, which explains the
>>      racy (un)mask operations in plic_irq_eoi():
>>
>>        If the completion ID does not match an interrupt source
>>        that is currently enabled for the target, the completion
>>        is silently ignored.
>>
>> Both of these problems are solved by using the priority value to mask
>> IRQs. Each IRQ has a separate priority register, so writing the priority
>> value is atomic. And since the enable bit remains set while an IRQ is
>> masked, the EOI operation works normally. The enable bits are still used
>> to control the IRQ's affinity.
> 
> This is pretty neat.
> 
> My only concern is around whether implementations do when changing
> priority of enabled interrupts. The PLIC architecture is conveniently
> silent on the subject, but that's certainly something that can result
> in total crap with the ARM GICs, for example, because an
> implementation is free to apply this priority change on an already
> pending interrupt, or not. But the kernel really wants the interrupt
> to be masked once it tells the HW to do so.

I think this can be expected based on this comment from the PLIC spec, which is
the only place using the word "mask":

"The PLIC will mask all PLIC interrupts of a priority less than or equal to
threshold. For example, a`threshold` value of zero permits all interrupts with
non-zero priority."

> Could anyone please check the RTL for some common implementations?

The C9xx PLIC explicitly checks for priority 0 in each gateway:

https://github.com/T-head-Semi/openc906/blob/main/C906_RTL_FACTORY/gen_rtl/plic/rtl/plic_int_kid.v#L152

and also continuously updates the IRQ outputs based on comparing the arbitrated
highest priority interrupt with the target's threshold:

https://github.com/T-head-Semi/openc906/blob/main/C906_RTL_FACTORY/gen_rtl/plic/rtl/plic_arb_ctrl.v#L295


Other implementations appear to do something similar, continuously computing the
highest priority interrupt, and comparing that to the target threshold:

https://github.com/chipsalliance/rocket-chip/blob/master/src/main/scala/devices/tilelink/Plic.scala#L187
https://github.com/lowRISC/rv_plic/blob/master/rtl/rv_plic_target.sv#L92
https://github.com/lowRISC/opentitan/blob/master/hw/ip_templates/rv_plic/rtl/rv_plic_target.sv.tpl#L58
https://github.com/pulp-platform/rv_plic/blob/master/rtl/rv_plic_target.sv#L92
https://github.com/RoaLogic/plic/blob/master/rtl/verilog/core/plic_target.sv#L126
https://github.com/qemu/qemu/blob/master/hw/intc/sifive_plic.c#L96

Those are the implementations I could find. Others may know about non-public
implementations.

Regards,
Samuel

> A way to avoid the above trouble (should it exist) would be to
> disable the interrupt when changing the priority, and then reenable
> it. You'd still get the simpler EOI, which is what you really want.
> 
> Thanks,
> 
> 	M.
>