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

[Brad Larson]

Hi Mark,

On Fri, Nov 5, 2021 at 4:35 AM Marc Zyngier <maz@xxxxxxxxxx> wrote:
>
> > > >> +                    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.

The Elba SoC is an embedded chip and not intended as a SBSA-compliant
general platform.  In this implementation the ITS is used to provide
message-based interrupts for our (potentially large set) of hardware
based platform device instances.  Virtualization is not a consideration.
We don't have a SMMU.  Interrupt isolation isn't a practical consideration
for this product.  Propose adding a comment to the dts.

+                       /*
+                        * Elba SoC implemented a pre-ITS that happened to
+                        * be the same implementation as synquacer.
+                        */
                        its: interrupt-controller@820000 {
                                compatible = "arm,gic-v3-its";
                                msi-controller;

Thanks
Brad