Brad,

On Fri, 05 Nov 2021 00:02:04 +0000,
Brad Larson [off-list ref] wrote:

Hi Marc,

On Mon, Oct 25, 2021 at 4:15 AM Marc Zyngier [off-list ref] wrote:

quoted

On 2021-10-25 10:17, Mark Rutland wrote:

quoted

Hi,

On Sun, Oct 24, 2021 at 06:51:56PM -0700, Brad Larson wrote:

quoted

Add Pensando common and Elba SoC specific device nodes

Signed-off-by: Brad Larson <redacted>

[...]

quoted

+            gic: interrupt-controller@800000 {
+                    compatible = "arm,gic-v3";
+                    #interrupt-cells = <3>;
+                    #address-cells = <2>;
+                    #size-cells = <2>;
+                    ranges;
+                    interrupt-controller;
+                    reg = <0x0 0x800000 0x0 0x200000>,      /* GICD */
+                          <0x0 0xa00000 0x0 0x200000>;      /* GICR */

This is missing the GICv2 compat regions that the CPUs implement.

Is this what is described as optional in the GIC architecture specification
where a GICv3 system can run restricted GICv2 code?

Yup, that. It is actually implemented by the CPU.

Can you point me in the right direction in the spec and example dts
node if needed.

The Cortex-A72 TRM has everything you need [1]. And since you used the
Synquacer as the model for this, you will see that it has the missing
regions. Alternatively, rk3399.dtsi is a good example.

quoted

quoted

quoted

+                    interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_HIGH>;
+
+                    gic_its: msi-controller@820000 {
+                            compatible = "arm,gic-v3-its";
+                            msi-controller;
+                            #msi-cells = <1>;
+                            reg = <0x0 0x820000 0x0 0x10000>;
+                            socionext,synquacer-pre-its =
+                                                    <0xc00000 0x1000000>;
+                    };
+            };

Is there any shared lineage with Synquacer? The commit message didn't
describe this quirk.

Funny, it looks like there is a sudden outburst of stupid copy/paste
among HW designers. TI did the exact same thing recently.

This totally negates all the advantages of having an ITS and makes
sure that you have all the overhead. Facepalm...

Some background may help explain.  To generate an LPI a peripheral must
write to the GITS_TRANSLATER (a specific address). For the ITS to know
which translations apply to the generated interrupts, it must know which
peripheral performed the write. The ID of the peripheral is known as its
DeviceID, which is often carried along with the write as an AXI sideband
signal.

Yes, I happen to be vaguely familiar with the GIC architecture.

The Elba SoC doesn't carry the DeviceID, so we have to conjure one up
between the peripheral and the ITS.  Instead of telling a peripheral to target
the GITS_TRANSLATER directly, we instead direct it to a specific offset
within a pre-ITS address range (our own IP block).  For writes that land in
that memory range, we derive the DeviceID from (offset >> 2).  The pre-ITS
block then sends (DeviceID, data) to the GITS_TRANSLATER.

The hardware designer came up with the Pre-ITS mechanism in Feb 2018.
When we looked at the upstream kernel later (we developed on 4.14)
we found that not only did it support something similar, it supported the
exact scheme we are using.

And this scheme is totally wrong. It breaks interrupt isolation.

Instead of having a single doorbell and getting the ITS to segregate
between devices itself, you end-up with multiple ones, allowing a
rogue device to impersonate another one by targeting another doorbell.
You can't even use an SMMU to preserve some isolation, because all the
doorbells are in the *same page*. Unmitigated disaster.

At this stage, why did you bother having an ITS at all? You get none
of the security features. Only the excess area, memory allocation,
additional latency and complexity. All you get is a larger INTID
space.

This only shows that the hardware designer didn't understand the ITS
at all. Which seems a common pattern, unfortunately.

	M.

[1] https://developer.arm.com/documentation/100095/0003/Generic-Interrupt-Controller-CPU-Interface/GIC-functional-description/GIC-memory-map?lang=en#way1382452674438__CHDEBJAJ

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