[PATCH v2 07/17] irqchip/gic: Atomically update affinity

[PATCH v2 07/17] irqchip/gic: Atomically update affinity

[Marc Zyngier]

The GIC driver uses a RMW sequence to update the affinity, and
relies on the gic_lock_irqsave/gic_unlock_irqrestore sequences
to update it atomically.

But these sequences only expend into anything meaningful if
the BL_SWITCHER option is selected, which almost never happens.

It also turns out that using a RMW and locks is just as silly,
as the GIC distributor supports byte accesses for the GICD_TARGETRn
registers, which when used make the update atomic by definition.

Drop the terminally broken code and replace it by a byte write.

Fixes: 04c8b0f82c7d ("irqchip/gic: Make locking a BL_SWITCHER only feature")
Cc: stable@vger.kernel.org
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 drivers/irqchip/irq-gic.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 00de05abd3c3..c17fabd6741e 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -329,10 +329,8 @@ static int gic_irq_set_vcpu_affinity(struct irq_data *d, void *vcpu)
 static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val,
 			    bool force)
 {
-	void __iomem *reg = gic_dist_base(d) + GIC_DIST_TARGET + (gic_irq(d) & ~3);
-	unsigned int cpu, shift = (gic_irq(d) % 4) * 8;
-	u32 val, mask, bit;
-	unsigned long flags;
+	void __iomem *reg = gic_dist_base(d) + GIC_DIST_TARGET + gic_irq(d);
+	unsigned int cpu;
 
 	if (!force)
 		cpu = cpumask_any_and(mask_val, cpu_online_mask);
@@ -342,13 +340,7 @@ static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val,
 	if (cpu >= NR_GIC_CPU_IF || cpu >= nr_cpu_ids)
 		return -EINVAL;
 
-	gic_lock_irqsave(flags);
-	mask = 0xff << shift;
-	bit = gic_cpu_map[cpu] << shift;
-	val = readl_relaxed(reg) & ~mask;
-	writel_relaxed(val | bit, reg);
-	gic_unlock_irqrestore(flags);
-
+	writeb_relaxed(gic_cpu_map[cpu], reg);
 	irq_data_update_effective_affinity(d, cpumask_of(cpu));
 
 	return IRQ_SET_MASK_OK_DONE;
-- 
2.27.0

Re: [PATCH v2 07/17] irqchip/gic: Atomically update affinity

[Sasha Levin]

Hi

[This is an automated email]

This commit has been processed because it contains a "Fixes:" tag
fixing commit: 04c8b0f82c7d ("irqchip/gic: Make locking a BL_SWITCHER only feature").

The bot has tested the following trees: v5.7.6, v5.4.49, v4.19.130, v4.14.186, v4.9.228.

v5.7.6: Build OK!
v5.4.49: Build OK!
v4.19.130: Build OK!
v4.14.186: Build OK!
v4.9.228: Failed to apply! Possible dependencies:
    0c9e498286ef9 ("irqchip/gic: Report that effective affinity is a single target")


NOTE: The patch will not be queued to stable trees until it is upstream.

How should we proceed with this patch?

-- 
Thanks
Sasha

Re: [PATCH v2 07/17] irqchip/gic: Atomically update affinity

[Sasha Levin]

Hi

[This is an automated email]

This commit has been processed because it contains a "Fixes:" tag
fixing commit: 04c8b0f82c7d ("irqchip/gic: Make locking a BL_SWITCHER only feature").

The bot has tested the following trees: v5.7.6, v5.4.49, v4.19.130, v4.14.186, v4.9.228.

v5.7.6: Build OK!
v5.4.49: Build OK!
v4.19.130: Build OK!
v4.14.186: Build OK!
v4.9.228: Failed to apply! Possible dependencies:
    0c9e498286ef9 ("irqchip/gic: Report that effective affinity is a single target")


NOTE: The patch will not be queued to stable trees until it is upstream.

How should we proceed with this patch?

-- 
Thanks
Sasha

Re: [PATCH v2 07/17] irqchip/gic: Atomically update affinity

[Geert Uytterhoeven]

Hi Marc, Russell,

On Wed, Jun 24, 2020 at 9:59 PM Marc Zyngier <maz@kernel.org> wrote:
> The GIC driver uses a RMW sequence to update the affinity, and
> relies on the gic_lock_irqsave/gic_unlock_irqrestore sequences
> to update it atomically.
>
> But these sequences only expend into anything meaningful if
> the BL_SWITCHER option is selected, which almost never happens.
>
> It also turns out that using a RMW and locks is just as silly,
> as the GIC distributor supports byte accesses for the GICD_TARGETRn
> registers, which when used make the update atomic by definition.
>
> Drop the terminally broken code and replace it by a byte write.
>
> Fixes: 04c8b0f82c7d ("irqchip/gic: Make locking a BL_SWITCHER only feature")
> Cc: stable@vger.kernel.org
> Signed-off-by: Marc Zyngier <maz@kernel.org>

Thanks for your patch, which is now commit 005c34ae4b44f085
("irqchip/gic: Atomically update affinity"), to which I bisected a hard
lock-up during boot on the Renesas EMMA Mobile EV2-based KZM-A9-Dual
board, which has a dual Cortex-A9 with PL390.

Despite the ARM Generic Interrupt Controller Architecture Specification
(both version 1.0 and 2.0) stating that the Interrupt Processor Targets
Registers are byte-accessible, the EMMA Mobile EV2 User's Manual
states that the interrupt registers can be accessed via the APB bus,
in 32-bit units.  Using byte accesses locks up the system.

Unfortunately I only have remote access to the board showing the
issue.  I did check that adding the writeb_relaxed() before the
writel_relaxed() that was used before also causes a lock-up, so the
issue is not an endian mismatch.
Looking at the driver history, these registers have always been
accessed using 32-bit accesses before.  As byte accesses lead
indeed to simpler code, I'm wondering if they had been tried before,
and caused issues before?

Since you said the locking was bogus before, due to the reliance on
the BL_SWITCHER option, I'm not suggesting a plain revert, but I'm
wondering what kind of locking you suggest to use instead?

Thanks for your comments!

> --- a/drivers/irqchip/irq-gic.c
> +++ b/drivers/irqchip/irq-gic.c
> @@ -329,10 +329,8 @@ static int gic_irq_set_vcpu_affinity(struct irq_data *d, void *vcpu)
>  static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val,
>                             bool force)
>  {
> -       void __iomem *reg = gic_dist_base(d) + GIC_DIST_TARGET + (gic_irq(d) & ~3);
> -       unsigned int cpu, shift = (gic_irq(d) % 4) * 8;
> -       u32 val, mask, bit;
> -       unsigned long flags;
> +       void __iomem *reg = gic_dist_base(d) + GIC_DIST_TARGET + gic_irq(d);
> +       unsigned int cpu;
>
>         if (!force)
>                 cpu = cpumask_any_and(mask_val, cpu_online_mask);
> @@ -342,13 +340,7 @@ static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val,
>         if (cpu >= NR_GIC_CPU_IF || cpu >= nr_cpu_ids)
>                 return -EINVAL;
>
> -       gic_lock_irqsave(flags);
> -       mask = 0xff << shift;
> -       bit = gic_cpu_map[cpu] << shift;
> -       val = readl_relaxed(reg) & ~mask;
> -       writel_relaxed(val | bit, reg);
> -       gic_unlock_irqrestore(flags);
> -
> +       writeb_relaxed(gic_cpu_map[cpu], reg);
>         irq_data_update_effective_affinity(d, cpumask_of(cpu));
>
>         return IRQ_SET_MASK_OK_DONE;

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 07/17] irqchip/gic: Atomically update affinity

[Russell King (Oracle)]

On Thu, Sep 09, 2021 at 05:22:01PM +0200, Geert Uytterhoeven wrote:
> Despite the ARM Generic Interrupt Controller Architecture Specification
> (both version 1.0 and 2.0) stating that the Interrupt Processor Targets
> Registers are byte-accessible, the EMMA Mobile EV2 User's Manual
> states that the interrupt registers can be accessed via the APB bus,
> in 32-bit units.  Using byte accesses locks up the system.

Fun. Seems someone can't read ARMs documentation. Even the old
ARM IHI 0048B.b document I have for the GIC from 2013 states
"In addition, the GICD_IPRIORITYRn, GICD_ITARGETSRn, GICD_CPENDSGIRn,
and GICD_SPENDSGIRn registers support byte accesses."

However, this kind of thing is sadly not uncommon. There's been a
similar issue with the PL011 UART driver as well - some platforms
require 16-bit accesses instead of normal 32-bit accesses.

> Unfortunately I only have remote access to the board showing the
> issue.  I did check that adding the writeb_relaxed() before the
> writel_relaxed() that was used before also causes a lock-up, so the
> issue is not an endian mismatch.
> Looking at the driver history, these registers have always been
> accessed using 32-bit accesses before.  As byte accesses lead
> indeed to simpler code, I'm wondering if they had been tried before,
> and caused issues before?
> 
> Since you said the locking was bogus before, due to the reliance on
> the BL_SWITCHER option, I'm not suggesting a plain revert, but I'm
> wondering what kind of locking you suggest to use instead?

If byte accesses are not going to be workable, then the only
answer _is_ a read-modify-write with working locking.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v2 07/17] irqchip/gic: Atomically update affinity

[Marc Zyngier]

Hi Geert,

On Thu, 09 Sep 2021 16:22:01 +0100,
Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> 
> Hi Marc, Russell,
> 
> On Wed, Jun 24, 2020 at 9:59 PM Marc Zyngier <maz@kernel.org> wrote:
> > The GIC driver uses a RMW sequence to update the affinity, and
> > relies on the gic_lock_irqsave/gic_unlock_irqrestore sequences
> > to update it atomically.
> >
> > But these sequences only expend into anything meaningful if
> > the BL_SWITCHER option is selected, which almost never happens.
> >
> > It also turns out that using a RMW and locks is just as silly,
> > as the GIC distributor supports byte accesses for the GICD_TARGETRn
> > registers, which when used make the update atomic by definition.
> >
> > Drop the terminally broken code and replace it by a byte write.
> >
> > Fixes: 04c8b0f82c7d ("irqchip/gic: Make locking a BL_SWITCHER only feature")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> 
> Thanks for your patch, which is now commit 005c34ae4b44f085
> ("irqchip/gic: Atomically update affinity"), to which I bisected a hard
> lock-up during boot on the Renesas EMMA Mobile EV2-based KZM-A9-Dual
> board, which has a dual Cortex-A9 with PL390.
> 
> Despite the ARM Generic Interrupt Controller Architecture Specification
> (both version 1.0 and 2.0) stating that the Interrupt Processor Targets
> Registers are byte-accessible, the EMMA Mobile EV2 User's Manual
> states that the interrupt registers can be accessed via the APB bus,
> in 32-bit units.  Using byte accesses locks up the system.

Urgh. That is definitely a pretty poor integration. How about the
priority registers? I guess they suffer from the same issue...

> Unfortunately I only have remote access to the board showing the
> issue.  I did check that adding the writeb_relaxed() before the
> writel_relaxed() that was used before also causes a lock-up, so the
> issue is not an endian mismatch.
> Looking at the driver history, these registers have always been
> accessed using 32-bit accesses before.  As byte accesses lead
> indeed to simpler code, I'm wondering if they had been tried before,
> and caused issues before?

Not that I know. A lock was probably fine on a two CPU system. Less so
on a busy 8 CPU machine where interrupts are often migrated. The GIC
architecture makes a point in not requiring locking for most of the
registers that can be accessed concurrently.

> Since you said the locking was bogus before, due to the reliance on
> the BL_SWITCHER option, I'm not suggesting a plain revert, but I'm
> wondering what kind of locking you suggest to use instead?

There isn't much we can do aside from reintroducing the RMW+spinlock
approach, and for real this time. It would have to be handled as a
quirk though, as I'm not keen on reintroducing this for all systems.

I wrote the patchlet below, which is totally untested. Please give it
a go and let me know if it helps.

	M.

diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index d329ec3d64d8..dca40a974b7a 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -107,6 +107,8 @@ static DEFINE_RAW_SPINLOCK(cpu_map_lock);
 
 #endif
 
+static DEFINE_STATIC_KEY_FALSE(needs_rmw_access);
+
 /*
  * The GIC mapping of CPU interfaces does not necessarily match
  * the logical CPU numbering.  Let's use a mapping as returned
@@ -774,6 +776,25 @@ static int gic_pm_init(struct gic_chip_data *gic)
 #endif
 
 #ifdef CONFIG_SMP
+static void rmw_writeb(u8 bval, void __iomem *addr)
+{
+	static DEFINE_RAW_SPINLOCK(rmw_lock);
+	unsigned long offset = (unsigned long)addr & ~3UL;
+	unsigned long shift = offset * 8;
+	unsigned long flags;
+	u32 val;
+
+	raw_spin_lock_irqsave(&rmw_lock, flags);
+
+	addr -= offset;
+	val = readl_relaxed(addr);
+	val &= ~(0xffUL << shift);
+	val |= (u32)bval << shift;
+	writel_relaxed(val, addr);
+
+	raw_spin_unlock_irqrestore(&rmw_lock, flags);
+}
+
 static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val,
 			    bool force)
 {
@@ -788,7 +809,10 @@ static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val,
 	if (cpu >= NR_GIC_CPU_IF || cpu >= nr_cpu_ids)
 		return -EINVAL;
 
-	writeb_relaxed(gic_cpu_map[cpu], reg);
+	if (static_branch_unlikely(&needs_rmw_access))
+		rmw_writeb(gic_cpu_map[cpu], reg);
+	else
+		writeb_relaxed(gic_cpu_map[cpu], reg);
 	irq_data_update_effective_affinity(d, cpumask_of(cpu));
 
 	return IRQ_SET_MASK_OK_DONE;
@@ -1375,6 +1399,29 @@ static bool gic_check_eoimode(struct device_node *node, void __iomem **base)
 	return true;
 }
 
+static bool gic_enable_rmw_access(void *data)
+{
+	/*
+	 * The EMEV2 class of machines has a broken interconnect, and
+	 * locks up on accesses that are less than 32bit. So far, only
+	 * the affinity setting requires it.
+	 */
+	if (of_machine_is_compatible("renesas,emev2")) {
+		static_branch_enable(&needs_rmw_access);
+		return true;
+	}
+
+	return false;
+}
+
+static const struct gic_quirk gic_quirks[] = {
+	{
+		.desc		= "Implementation with broken byte access",
+		.compatible	= "arm,pl390",
+		.init		= gic_enable_rmw_access,
+	},
+};
+
 static int gic_of_setup(struct gic_chip_data *gic, struct device_node *node)
 {
 	if (!gic || !node)
@@ -1391,6 +1438,8 @@ static int gic_of_setup(struct gic_chip_data *gic, struct device_node *node)
 	if (of_property_read_u32(node, "cpu-offset", &gic->percpu_offset))
 		gic->percpu_offset = 0;
 
+	gic_enable_of_quirks(node, gic_quirks, gic);
+
 	return 0;
 
 error:

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

Re: [PATCH v2 07/17] irqchip/gic: Atomically update affinity

[Geert Uytterhoeven]

Hi Marc,

On Fri, Sep 10, 2021 at 12:23 PM Marc Zyngier <maz@kernel.org> wrote:
> On Thu, 09 Sep 2021 16:22:01 +0100,
> Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Wed, Jun 24, 2020 at 9:59 PM Marc Zyngier <maz@kernel.org> wrote:
> > > The GIC driver uses a RMW sequence to update the affinity, and
> > > relies on the gic_lock_irqsave/gic_unlock_irqrestore sequences
> > > to update it atomically.
> > >
> > > But these sequences only expend into anything meaningful if
> > > the BL_SWITCHER option is selected, which almost never happens.
> > >
> > > It also turns out that using a RMW and locks is just as silly,
> > > as the GIC distributor supports byte accesses for the GICD_TARGETRn
> > > registers, which when used make the update atomic by definition.
> > >
> > > Drop the terminally broken code and replace it by a byte write.
> > >
> > > Fixes: 04c8b0f82c7d ("irqchip/gic: Make locking a BL_SWITCHER only feature")
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Marc Zyngier <maz@kernel.org>
> >
> > Thanks for your patch, which is now commit 005c34ae4b44f085
> > ("irqchip/gic: Atomically update affinity"), to which I bisected a hard
> > lock-up during boot on the Renesas EMMA Mobile EV2-based KZM-A9-Dual
> > board, which has a dual Cortex-A9 with PL390.
> >
> > Despite the ARM Generic Interrupt Controller Architecture Specification
> > (both version 1.0 and 2.0) stating that the Interrupt Processor Targets
> > Registers are byte-accessible, the EMMA Mobile EV2 User's Manual
> > states that the interrupt registers can be accessed via the APB bus,
> > in 32-bit units.  Using byte accesses locks up the system.
>
> Urgh. That is definitely a pretty poor integration. How about the
> priority registers? I guess they suffer from the same issue...

Yes, they do.

> > Unfortunately I only have remote access to the board showing the
> > issue.  I did check that adding the writeb_relaxed() before the
> > writel_relaxed() that was used before also causes a lock-up, so the
> > issue is not an endian mismatch.
> > Looking at the driver history, these registers have always been
> > accessed using 32-bit accesses before.  As byte accesses lead
> > indeed to simpler code, I'm wondering if they had been tried before,
> > and caused issues before?
>
> Not that I know. A lock was probably fine on a two CPU system. Less so
> on a busy 8 CPU machine where interrupts are often migrated. The GIC
> architecture makes a point in not requiring locking for most of the
> registers that can be accessed concurrently.
>
> > Since you said the locking was bogus before, due to the reliance on
> > the BL_SWITCHER option, I'm not suggesting a plain revert, but I'm
> > wondering what kind of locking you suggest to use instead?
>
> There isn't much we can do aside from reintroducing the RMW+spinlock
> approach, and for real this time. It would have to be handled as a
> quirk though, as I'm not keen on reintroducing this for all systems.
>
> I wrote the patchlet below, which is totally untested. Please give it
> a go and let me know if it helps.

Thanks for your quick response!
Your solution works, after making a few small modifications.

> --- a/drivers/irqchip/irq-gic.c
> +++ b/drivers/irqchip/irq-gic.c
> @@ -774,6 +776,25 @@ static int gic_pm_init(struct gic_chip_data *gic)
>  #endif
>
>  #ifdef CONFIG_SMP
> +static void rmw_writeb(u8 bval, void __iomem *addr)
> +{
> +       static DEFINE_RAW_SPINLOCK(rmw_lock);
> +       unsigned long offset = (unsigned long)addr & ~3UL;

Please drop the tilde.

> +       unsigned long shift = offset * 8;

"unsigned int" is sufficient for offset and size.

> +       unsigned long flags;
> +       u32 val;
> +
> +       raw_spin_lock_irqsave(&rmw_lock, flags);
> +
> +       addr -= offset;
> +       val = readl_relaxed(addr);
> +       val &= ~(0xffUL << shift);

No need for the UL suffix.

> +       val |= (u32)bval << shift;

No need for the cast.

> +       writel_relaxed(val, addr);
> +
> +       raw_spin_unlock_irqrestore(&rmw_lock, flags);
> +}
> +
>  static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val,
>                             bool force)
>  {

> @@ -1375,6 +1399,29 @@ static bool gic_check_eoimode(struct device_node *node, void __iomem **base)
>         return true;
>  }
>
> +static bool gic_enable_rmw_access(void *data)
> +{
> +       /*
> +        * The EMEV2 class of machines has a broken interconnect, and
> +        * locks up on accesses that are less than 32bit. So far, only
> +        * the affinity setting requires it.
> +        */
> +       if (of_machine_is_compatible("renesas,emev2")) {
> +               static_branch_enable(&needs_rmw_access);
> +               return true;
> +       }
> +
> +       return false;
> +}
> +
> +static const struct gic_quirk gic_quirks[] = {
> +       {
> +               .desc           = "Implementation with broken byte access",

The output would look better without capitalizing the first word.
I think you can drop the first two words, saving some space:

    GIC: enabling workaround for broken byte access

> +               .compatible     = "arm,pl390",
> +               .init           = gic_enable_rmw_access,
> +       },

Missing "{ /* sentinel */ }".

> +};
> +
>  static int gic_of_setup(struct gic_chip_data *gic, struct device_node *node)
>  {
>         if (!gic || !node)

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 07/17] irqchip/gic: Atomically update affinity

[Magnus Damm]

Hi Geert, Mark, RMK, everyone,

Thanks for your efforts. Let me just chime in with a few details and a question.

On Fri, Sep 10, 2021 at 10:19 PM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> On Fri, Sep 10, 2021 at 12:23 PM Marc Zyngier <maz@kernel.org> wrote:
> > On Thu, 09 Sep 2021 16:22:01 +0100,
> > Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>     GIC: enabling workaround for broken byte access

Indeed, byte access is unsupported according to the EMEV2 documentation.

The EMEV2 documentation R19UH0036EJ0600 Chapter 7 Interrupt Control on
page 97 says:
"Interrupt registers can be accessed via the APB bus, in 32-bit units"
"For details about register functions, see ARM Generic Interrupt
Controller Architecture Specification Architecture version 1.0"
The file  "R19UH0036EJ0600_1Chip.pdf" is the 6th edition version
published in 2010 and is not marked as confidential.

From my basic research, "ARM Generic Interrupt Controller Architecture
Specification Architecture version 1.0" is documented in ARM IHI 0048A
from 2008 (Non-Confidential) which contains:
"All GIC registers are 32-bit wide." and "All registers support 32-bit
word access..."
"In addition, the following registers support byte accesses:"
"ICDIPR"
"ICDIPTR"

So the GICv1 documentation says byte access is partially supported
however EMEV2 documentation says 32-bit access is required.

> > +               .compatible     = "arm,pl390",
> > +               .init           = gic_enable_rmw_access,
> > +       },

May I ask about a clarification about the EMEV2 DTS and DT binding
documentation in:
arch/arm/boot/dts/emev2.dts
Documentation/devicetree/bindings/interrupt-controller/arm,gic.yaml

On EMEV2 the DT compatible string currently seems to be the rather
generic "arm,pl390". In the DT binding documentation GICv1 is listed
in an example as "arm,cortex-a9-gic". Is there any reason for not
using the GICv1 compatible string (and 32-bit access) for EMEV2? Just
curious.

Cheers,

/ magnus

Re: [PATCH v2 07/17] irqchip/gic: Atomically update affinity

[Marc Zyngier]

Hi Magnus,

On Sat, 11 Sep 2021 03:49:20 +0100,
Magnus Damm <magnus.damm@gmail.com> wrote:
> 
> Hi Geert, Mark, RMK, everyone,
> 
> Thanks for your efforts. Let me just chime in with a few details and a question.
> 
> On Fri, Sep 10, 2021 at 10:19 PM Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
> > On Fri, Sep 10, 2021 at 12:23 PM Marc Zyngier <maz@kernel.org> wrote:
> > > On Thu, 09 Sep 2021 16:22:01 +0100,
> > > Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> >     GIC: enabling workaround for broken byte access
> 
> Indeed, byte access is unsupported according to the EMEV2 documentation.
> 
> The EMEV2 documentation R19UH0036EJ0600 Chapter 7 Interrupt Control on
> page 97 says:
> "Interrupt registers can be accessed via the APB bus, in 32-bit units"
> "For details about register functions, see ARM Generic Interrupt
> Controller Architecture Specification Architecture version 1.0"
> The file  "R19UH0036EJ0600_1Chip.pdf" is the 6th edition version
> published in 2010 and is not marked as confidential.

This is as bad as it gets. Do you know if any other Renesas platform
is affected by the same issue?

> 
> From my basic research, "ARM Generic Interrupt Controller Architecture
> Specification Architecture version 1.0" is documented in ARM IHI 0048A
> from 2008 (Non-Confidential) which contains:
> "All GIC registers are 32-bit wide." and "All registers support 32-bit
> word access..."
> "In addition, the following registers support byte accesses:"
> "ICDIPR"

Renamed to GICD_IPRIORITYRn in IHI0048B.

> "ICDIPTR"

Renamed to GICD_ITARGETRn in IHI0048B.

See IHI0048B_b ("B.1 Alternative register names" and specifically
table B-1) for the translation table between GICv1 and GICv2 names.

> So the GICv1 documentation says byte access is partially supported
> however EMEV2 documentation says 32-bit access is required.

Which is definitely an integration bug. Both set of registers *must*
support byte accesses. This isn't optional and left to the
appreciation of the integrator. This breaks the programming model
badly, and prevents standard software from running unmodified.

One of the few things the GIC architecture got right is the absence of
locking requirements, as all the registers can be accessed
concurrently by multiple CPUs as long as they operate on distinct
interrupts. This is why the enable and pending registers have both set
and clear accessors, that the priority and target registers are byte
accessible, and that everything else happens in CPU-private registers
(the CPU interface).

This requirement has been there from day-1. Even the good old DIC (the
GIC's ancestor) that was included with the 11MP-Core says: "All
Interrupt Distributor Registers are byte accessible.", which is more
than actually necessary for the GIC. See DDI 0360F for details. And
yes, SW written for the GIC does work on the DIC.

> 
> > > +               .compatible     = "arm,pl390",
> > > +               .init           = gic_enable_rmw_access,
> > > +       },
> 
> May I ask about a clarification about the EMEV2 DTS and DT binding
> documentation in:
> arch/arm/boot/dts/emev2.dts
> Documentation/devicetree/bindings/interrupt-controller/arm,gic.yaml
> 
> On EMEV2 the DT compatible string currently seems to be the rather
> generic "arm,pl390". In the DT binding documentation GICv1 is listed
> in an example as "arm,cortex-a9-gic". Is there any reason for not
> using the GICv1 compatible string (and 32-bit access) for EMEV2? Just
> curious.

GICv1 is an architecture specification. PL390 is an implementation of
GICv1. The so called "Cortex-A9 GIC" doesn't really exist. It is
simply the amalgamation of the CPU interface implemented by the A9
(with the prototype of the GICv2 virtualisation extensions) with a
distributor (usually a PL390, but not necessarily). All of them
require that the priority and target registers are byte accessible.

As for changing the compatibility string, I don't see the point. This
will break existing setups, and doesn't change the core of the
issue. As far as I can see, the EMEV2 DT is correct in the sense that
it describes the actual implementation of the GIC used.

Thanks,

	M.

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

Re: [PATCH v2 07/17] irqchip/gic: Atomically update affinity

[Magnus Damm]

Hi Marc,

On Sun, Sep 12, 2021 at 4:32 AM Marc Zyngier <maz@kernel.org> wrote:
> On Sat, 11 Sep 2021 03:49:20 +0100,
> Magnus Damm <magnus.damm@gmail.com> wrote:
> > On Fri, Sep 10, 2021 at 10:19 PM Geert Uytterhoeven
> > <geert@linux-m68k.org> wrote:
> > > On Fri, Sep 10, 2021 at 12:23 PM Marc Zyngier <maz@kernel.org> wrote:
> > > > On Thu, 09 Sep 2021 16:22:01 +0100,
> > > > Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > >     GIC: enabling workaround for broken byte access
> >
> > Indeed, byte access is unsupported according to the EMEV2 documentation.
> >
> > The EMEV2 documentation R19UH0036EJ0600 Chapter 7 Interrupt Control on
> > page 97 says:
> > "Interrupt registers can be accessed via the APB bus, in 32-bit units"
> > "For details about register functions, see ARM Generic Interrupt
> > Controller Architecture Specification Architecture version 1.0"
> > The file  "R19UH0036EJ0600_1Chip.pdf" is the 6th edition version
> > published in 2010 and is not marked as confidential.
>
> This is as bad as it gets. Do you know if any other Renesas platform
> is affected by the same issue?

Next time we have a beer together I would be happy to show you some
legacy interrupt controller code. =)

EMEV2 and the Emma Mobile product line came from the NEC Electronics
side that got merged into Renesas Electronics in 2010. Historically
NEC Electronics mainly used MIPS I've been told, and the Emma Mobile
SoCs were one of the earlier Cortex-A9 adopters. That might have
something to do with the rather loose interpretation of the spec.

Renesas SoCs from a similar era:
AP4 (sh7372) AP4EVB (Cortex-A8 + INTCA/INTCS)
R-Mobile A1 (r8a7740) Armadillo-800-EVA (Cortex-A9 + INTCA/INTCS)
R-Car M1A (r8a7778) Bock-W (Cortex-A9 + GIC)
R-Car H1 (r8a7779) Marzen (4 x Cortex-A9 + GIC)
Emma Mobile EMEV2 KZM9D (2 x Cortex-A9 + GIC)
SH-Mobile AG5 (sh73a0) KZM9G (2 x Cortex-A9 + GIC)

The INTCA/INTCS interrupt controllers came from the SH architecture
but were phased out once SMP became the norm. I've got the majority of
the boards above hooked up for remote access if anyone wants to test
something.

> > From my basic research, "ARM Generic Interrupt Controller Architecture
> > Specification Architecture version 1.0" is documented in ARM IHI 0048A
> > from 2008 (Non-Confidential) which contains:
> > "All GIC registers are 32-bit wide." and "All registers support 32-bit
> > word access..."
> > "In addition, the following registers support byte accesses:"
> > "ICDIPR"
>
> Renamed to GICD_IPRIORITYRn in IHI0048B.
>
> > "ICDIPTR"
>
> Renamed to GICD_ITARGETRn in IHI0048B.
>
> See IHI0048B_b ("B.1 Alternative register names" and specifically
> table B-1) for the translation table between GICv1 and GICv2 names.

Thanks.

> > So the GICv1 documentation says byte access is partially supported
> > however EMEV2 documentation says 32-bit access is required.
>
> Which is definitely an integration bug. Both set of registers *must*
> support byte accesses. This isn't optional and left to the
> appreciation of the integrator. This breaks the programming model
> badly, and prevents standard software from running unmodified.

This reminds me that on SH we used to fix up I/O access alignment for
certain on-chip devices by trapping. The fast path worked well and the
special case worked but was slow.

> One of the few things the GIC architecture got right is the absence of
> locking requirements, as all the registers can be accessed
> concurrently by multiple CPUs as long as they operate on distinct
> interrupts. This is why the enable and pending registers have both set
> and clear accessors, that the priority and target registers are byte
> accessible, and that everything else happens in CPU-private registers
> (the CPU interface).

Yeah the GIC is quite nice IMO. The legacy INTC hardware often had
separate registers for setting and clearing, however priority probably
required read-modify-write. SMP wasn't an issue. =)

> This requirement has been there from day-1. Even the good old DIC (the
> GIC's ancestor) that was included with the 11MP-Core says: "All
> Interrupt Distributor Registers are byte accessible.", which is more
> than actually necessary for the GIC. See DDI 0360F for details. And
> yes, SW written for the GIC does work on the DIC.

Interesting. For some not so big reason this makes me think of Monty Python. =)

> >
> > > > +               .compatible     = "arm,pl390",
> > > > +               .init           = gic_enable_rmw_access,
> > > > +       },
> >
> > May I ask about a clarification about the EMEV2 DTS and DT binding
> > documentation in:
> > arch/arm/boot/dts/emev2.dts
> > Documentation/devicetree/bindings/interrupt-controller/arm,gic.yaml
> >
> > On EMEV2 the DT compatible string currently seems to be the rather
> > generic "arm,pl390". In the DT binding documentation GICv1 is listed
> > in an example as "arm,cortex-a9-gic". Is there any reason for not
> > using the GICv1 compatible string (and 32-bit access) for EMEV2? Just
> > curious.
>
> GICv1 is an architecture specification. PL390 is an implementation of
> GICv1. The so called "Cortex-A9 GIC" doesn't really exist. It is
> simply the amalgamation of the CPU interface implemented by the A9
> (with the prototype of the GICv2 virtualisation extensions) with a
> distributor (usually a PL390, but not necessarily). All of them
> require that the priority and target registers are byte accessible.

Makes sense.

> As for changing the compatibility string, I don't see the point. This
> will break existing setups, and doesn't change the core of the
> issue. As far as I can see, the EMEV2 DT is correct in the sense that
> it describes the actual implementation of the GIC used.

I'm all for not breaking existing setups, but EMEV2 is a pretty rare
case. So my line of thinking was that instead of punishing all GIC
platforms with this EMEV2-specific workaround it is completely fine
from my side to to special case it in DT if it makes the rest of the
code any cleaner. But it is really up to you guys.

Thanks,

/ magnus

Re: [PATCH v2 07/17] irqchip/gic: Atomically update affinity

[Geert Uytterhoeven]

Hi Magnus,

On Sun, Sep 12, 2021 at 7:40 AM Magnus Damm <magnus.damm@gmail.com> wrote:
> On Sun, Sep 12, 2021 at 4:32 AM Marc Zyngier <maz@kernel.org> wrote:
> > On Sat, 11 Sep 2021 03:49:20 +0100,
> > Magnus Damm <magnus.damm@gmail.com> wrote:
> > > On Fri, Sep 10, 2021 at 10:19 PM Geert Uytterhoeven
> > > <geert@linux-m68k.org> wrote:
> > > > On Fri, Sep 10, 2021 at 12:23 PM Marc Zyngier <maz@kernel.org> wrote:
> > > > > On Thu, 09 Sep 2021 16:22:01 +0100,
> > > > > Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > >     GIC: enabling workaround for broken byte access
> > >
> > > Indeed, byte access is unsupported according to the EMEV2 documentation.
> > >
> > > The EMEV2 documentation R19UH0036EJ0600 Chapter 7 Interrupt Control on
> > > page 97 says:
> > > "Interrupt registers can be accessed via the APB bus, in 32-bit units"
> > > "For details about register functions, see ARM Generic Interrupt
> > > Controller Architecture Specification Architecture version 1.0"
> > > The file  "R19UH0036EJ0600_1Chip.pdf" is the 6th edition version
> > > published in 2010 and is not marked as confidential.
> >
> > This is as bad as it gets. Do you know if any other Renesas platform
> > is affected by the same issue?
>
> Next time we have a beer together I would be happy to show you some
> legacy interrupt controller code. =)
>
> EMEV2 and the Emma Mobile product line came from the NEC Electronics
> side that got merged into Renesas Electronics in 2010. Historically
> NEC Electronics mainly used MIPS I've been told, and the Emma Mobile
> SoCs were one of the earlier Cortex-A9 adopters. That might have
> something to do with the rather loose interpretation of the spec.

Indeed.  I used to work on products using EMMA1 and EMMA2, and they
were MIPS-based (vr4120A for EMMA2, IIRC).  Later variants (EMMA2H
and EMMA3?) did include a small ARM core for standby control.

> Renesas SoCs from a similar era:
> AP4 (sh7372) AP4EVB (Cortex-A8 + INTCA/INTCS)

This is no longer supported upstream (and not affected, as no GIC).

> R-Mobile A1 (r8a7740) Armadillo-800-EVA (Cortex-A9 + INTCA/INTCS)

R-Mobile A1 has GIC (PL390), too, and is not affected.

> R-Car M1A (r8a7778) Bock-W (Cortex-A9 + GIC)
> R-Car H1 (r8a7779) Marzen (4 x Cortex-A9 + GIC)
> Emma Mobile EMEV2 KZM9D (2 x Cortex-A9 + GIC)
> SH-Mobile AG5 (sh73a0) KZM9G (2 x Cortex-A9 + GIC)

All of these (except for EMEV2) are fine, too.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 07/17] irqchip/gic: Atomically update affinity

[Magnus Damm]

Hi Geert, everyone,

On Mon, Sep 13, 2021 at 5:05 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Magnus,
>
> On Sun, Sep 12, 2021 at 7:40 AM Magnus Damm <magnus.damm@gmail.com> wrote:
> > On Sun, Sep 12, 2021 at 4:32 AM Marc Zyngier <maz@kernel.org> wrote:
> > > On Sat, 11 Sep 2021 03:49:20 +0100,
> > > Magnus Damm <magnus.damm@gmail.com> wrote:
> > > > On Fri, Sep 10, 2021 at 10:19 PM Geert Uytterhoeven
> > > > <geert@linux-m68k.org> wrote:
> > > > > On Fri, Sep 10, 2021 at 12:23 PM Marc Zyngier <maz@kernel.org> wrote:
> > > > > > On Thu, 09 Sep 2021 16:22:01 +0100,
> > > > > > Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > >     GIC: enabling workaround for broken byte access
> > > >
> > > > Indeed, byte access is unsupported according to the EMEV2 documentation.
> > > >
> > > > The EMEV2 documentation R19UH0036EJ0600 Chapter 7 Interrupt Control on
> > > > page 97 says:
> > > > "Interrupt registers can be accessed via the APB bus, in 32-bit units"
> > > > "For details about register functions, see ARM Generic Interrupt
> > > > Controller Architecture Specification Architecture version 1.0"
> > > > The file  "R19UH0036EJ0600_1Chip.pdf" is the 6th edition version
> > > > published in 2010 and is not marked as confidential.
> > >
> > > This is as bad as it gets. Do you know if any other Renesas platform
> > > is affected by the same issue?
> >
> > Next time we have a beer together I would be happy to show you some
> > legacy interrupt controller code. =)
> >
> > EMEV2 and the Emma Mobile product line came from the NEC Electronics
> > side that got merged into Renesas Electronics in 2010. Historically
> > NEC Electronics mainly used MIPS I've been told, and the Emma Mobile
> > SoCs were one of the earlier Cortex-A9 adopters. That might have
> > something to do with the rather loose interpretation of the spec.
>
> Indeed.  I used to work on products using EMMA1 and EMMA2, and they
> were MIPS-based (vr4120A for EMMA2, IIRC).  Later variants (EMMA2H
> and EMMA3?) did include a small ARM core for standby control.

Thanks for sharing some more background!

> > Renesas SoCs from a similar era:
> > AP4 (sh7372) AP4EVB (Cortex-A8 + INTCA/INTCS)
>
> This is no longer supported upstream (and not affected, as no GIC).

Right. I might mix it up with the AP4.5 chip that I used for SMP
prototyping back then. It had 4 x CA9 and obviously a GIC.

> > R-Mobile A1 (r8a7740) Armadillo-800-EVA (Cortex-A9 + INTCA/INTCS)
>
> R-Mobile A1 has GIC (PL390), too, and is not affected.
>
> > R-Car M1A (r8a7778) Bock-W (Cortex-A9 + GIC)
> > R-Car H1 (r8a7779) Marzen (4 x Cortex-A9 + GIC)
> > Emma Mobile EMEV2 KZM9D (2 x Cortex-A9 + GIC)
> > SH-Mobile AG5 (sh73a0) KZM9G (2 x Cortex-A9 + GIC)
>
> All of these (except for EMEV2) are fine, too.

Thanks for checking!

Cheers,

/ magnus