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;
Date:   Mon, 08 Nov 2021 19:53:58 +0000
Message-ID: <87lf1ye84p.wl-maz@kernel.org>
From:   Marc Zyngier <maz@kernel.org>
To:     Brad Larson <brad@pensando.io>
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>
Subject: Re: [PATCH v3 11/11] arm64: dts: Add Pensando Elba SoC support
In-Reply-To: <CAK9rFnz6X3wR6bMPkVicGoqj+prm5OmY3y3B_8Fc+WGJxcVsXA@mail.gmail.com>
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>
        <CAK9rFnz6X3wR6bMPkVicGoqj+prm5OmY3y3B_8Fc+WGJxcVsXA@mail.gmail.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue)
 FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/27.1
 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Mon, 08 Nov 2021 19:35:14 +0000,
Brad Larson <brad@pensando.io> wrote:
> 
> Hi Mark,

s/k/c/

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

This has nothing to do with following a standard. It has to do with
following the intended use of the architecture. What you have here is
the system architecture equivalent of trusting userspace to build the
kernel page tables. It can work in limited cases. But would you want
to deploy such construct at scale? Probably not.

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

Because you have foreseen all use cases for this HW ahead of time, and
can already tell how SW is going to make use of it? Oh well...

> Propose adding a comment to the dts.
> 
> +                       /*
> +                        * Elba SoC implemented a pre-ITS that happened to
> +                        * be the same implementation as synquacer.
> +                        */

Which contains zero information. What you really want is: "We have
decided to ignore the system architecture, good luck".

	M.

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