Re: [PATCH v3 11/11] arm64: dts: Add Pensando Elba SoC support

[Marc Zyngier <maz@xxxxxxxxxx>]

Brad,

On Fri, 05 Nov 2021 00:02:04 +0000,
Brad Larson <brad@xxxxxxxxxxx> wrote:
> 
> Hi Marc,
> 
> On Mon, Oct 25, 2021 at 4:15 AM Marc Zyngier <maz@xxxxxxxxxx> wrote:
> >
> > On 2021-10-25 10:17, Mark Rutland wrote:
> > > Hi,
> > >
> > > On Sun, Oct 24, 2021 at 06:51:56PM -0700, Brad Larson wrote:
> > >> Add Pensando common and Elba SoC specific device nodes
> > >>
> > >> Signed-off-by: Brad Larson <brad@xxxxxxxxxxx>
> > >
> > > [...]
> > >> +            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.


> > >> +                    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.