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;
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?
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?
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
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.
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:
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
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
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 accessIndeed, 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.
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 accessIndeed, 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
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 accessIndeed, 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
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 accessIndeed, 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
linux-stable-mirror@lists.linaro.org
-
Geert Uytterhoeven -
Magnus Damm -
Marc Zyngier -
Russell King (Oracle) -
Sasha Levin