Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20211025015156.33133-1-brad@pensando.io> <20211025015156.33133-12-brad@pensando.io>
 <20211025091731.GA2001@C02TD0UTHF1T.local> <a20805de16e1196c2ed46dd949473c9a@kernel.org>
 <CAK9rFnxH=DtC9vEJJCXkGEWjvg2E8vHPUcRy9rJpo34Furtmvw@mail.gmail.com> <87zgqi96nh.wl-maz@kernel.org>
In-Reply-To: <87zgqi96nh.wl-maz@kernel.org>
From:   Brad Larson <brad@pensando.io>
Date:   Mon, 8 Nov 2021 11:35:14 -0800
Message-ID: <CAK9rFnz6X3wR6bMPkVicGoqj+prm5OmY3y3B_8Fc+WGJxcVsXA@mail.gmail.com>
Subject: Re: [PATCH v3 11/11] arm64: dts: Add Pensando Elba SoC support
To:     Marc Zyngier <maz@kernel.org>
Cc:     Mark Rutland <mark.rutland@arm.com>,
        Linux ARM <linux-arm-kernel@lists.infradead.org>,
        Arnd Bergmann <arnd@arndb.de>,
        Linus Walleij <linus.walleij@linaro.org>,
        Bartosz Golaszewski <bgolaszewski@baylibre.com>,
        Mark Brown <broonie@kernel.org>,
        Serge Semin <fancer.lancer@gmail.com>,
        Adrian Hunter <adrian.hunter@intel.com>,
        Ulf Hansson <ulf.hansson@linaro.org>,
        Olof Johansson <olof@lixom.net>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
        linux-spi <linux-spi@vger.kernel.org>,
        linux-mmc <linux-mmc@vger.kernel.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Mark,

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