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 Date: Fri, 05 Nov 2021 11:35:46 +0000 [thread overview] Message-ID: <87zgqi96nh.wl-maz@kernel.org> (raw) In-Reply-To: <CAK9rFnxH=DtC9vEJJCXkGEWjvg2E8vHPUcRy9rJpo34Furtmvw@mail.gmail.com> Brad, On Fri, 05 Nov 2021 00:02:04 +0000, Brad Larson <brad@pensando.io> wrote: > > Hi Marc, > > On Mon, Oct 25, 2021 at 4:15 AM Marc Zyngier <maz@kernel.org> 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@pensando.io> > > > > > > [...] > > >> + 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.
WARNING: multiple messages have this Message-ID (diff)
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 Date: Fri, 05 Nov 2021 11:35:46 +0000 [thread overview] Message-ID: <87zgqi96nh.wl-maz@kernel.org> (raw) In-Reply-To: <CAK9rFnxH=DtC9vEJJCXkGEWjvg2E8vHPUcRy9rJpo34Furtmvw@mail.gmail.com> Brad, On Fri, 05 Nov 2021 00:02:04 +0000, Brad Larson <brad@pensando.io> wrote: > > Hi Marc, > > On Mon, Oct 25, 2021 at 4:15 AM Marc Zyngier <maz@kernel.org> 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@pensando.io> > > > > > > [...] > > >> + 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. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2021-11-05 11:35 UTC|newest] Thread overview: 90+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-10-25 1:51 [PATCH v3 00/11] Support Pensando Elba SoC Brad Larson 2021-10-25 1:51 ` Brad Larson 2021-10-25 1:51 ` [PATCH v3 01/11] dt-bindings: arm: pensando: add Pensando boards Brad Larson 2021-10-25 1:51 ` Brad Larson 2021-10-27 21:37 ` Rob Herring 2021-10-27 21:37 ` Rob Herring 2021-10-25 1:51 ` [PATCH v3 02/11] dt-bindings: Add vendor prefix for Pensando Systems Brad Larson 2021-10-25 1:51 ` Brad Larson 2021-10-27 21:38 ` Rob Herring 2021-10-27 21:38 ` Rob Herring 2021-11-05 0:16 ` Brad Larson 2021-11-05 0:16 ` Brad Larson 2021-10-25 1:51 ` [PATCH v3 03/11] dt-bindings: mmc: Add Pensando Elba SoC binding Brad Larson 2021-10-25 1:51 ` Brad Larson 2021-10-25 12:54 ` Rob Herring 2021-10-25 12:54 ` Rob Herring 2021-11-05 0:13 ` Brad Larson 2021-11-05 0:13 ` Brad Larson 2021-10-26 18:10 ` Rob Herring 2021-10-26 18:10 ` Rob Herring 2021-11-17 1:21 ` Brad Larson 2021-11-17 1:21 ` Brad Larson 2021-11-17 1:27 ` Brad Larson 2021-11-17 1:27 ` Brad Larson 2021-10-25 1:51 ` [PATCH v3 04/11] dt-bindings: spi: Add compatible for Pensando Elba SoC Brad Larson 2021-10-25 1:51 ` Brad Larson 2021-10-27 21:38 ` Rob Herring 2021-10-27 21:38 ` Rob Herring 2021-10-28 7:26 ` Serge Semin 2021-10-28 7:26 ` Serge Semin 2021-11-15 22:05 ` Brad Larson 2021-11-15 22:05 ` Brad Larson 2021-10-25 1:51 ` [PATCH v3 05/11] spi: dw: Add Pensando Elba SoC SPI Controller bindings Brad Larson 2021-10-25 1:51 ` Brad Larson 2021-10-27 21:42 ` Rob Herring 2021-10-27 21:42 ` Rob Herring 2021-10-28 7:49 ` Serge Semin 2021-10-28 7:49 ` Serge Semin 2021-10-28 7:52 ` Serge Semin 2021-10-28 7:52 ` Serge Semin 2021-11-15 22:24 ` Brad Larson 2021-11-15 22:24 ` Brad Larson 2021-11-16 11:29 ` Serge Semin 2021-11-16 11:29 ` Serge Semin 2021-11-16 23:11 ` Brad Larson 2021-11-16 23:11 ` Brad Larson 2021-11-17 8:19 ` Serge Semin 2021-11-17 8:19 ` Serge Semin 2021-11-17 21:35 ` Brad Larson 2021-11-17 21:35 ` Brad Larson 2021-10-25 1:51 ` [PATCH v3 06/11] MAINTAINERS: Add entry for PENSANDO Brad Larson 2021-10-25 1:51 ` Brad Larson 2021-10-25 1:51 ` [PATCH v3 07/11] arm64: Add config for Pensando SoC platforms Brad Larson 2021-10-25 1:51 ` Brad Larson 2021-10-25 1:51 ` [PATCH v3 08/11] spi: cadence-quadspi: Add compatible for Pensando Elba SoC Brad Larson 2021-10-25 1:51 ` Brad Larson 2021-10-25 1:51 ` [PATCH v3 09/11] mmc: sdhci-cadence: Add Pensando Elba SoC support Brad Larson 2021-10-25 1:51 ` Brad Larson 2021-10-25 1:51 ` [PATCH v3 10/11] spi: dw: Add support for Pensando Elba SoC Brad Larson 2021-10-25 1:51 ` Brad Larson 2021-10-28 9:11 ` Serge Semin 2021-10-28 9:11 ` Serge Semin 2021-10-31 13:19 ` Andy Shevchenko 2021-10-31 13:19 ` Andy Shevchenko 2021-10-25 1:51 ` [PATCH v3 11/11] arm64: dts: Add Pensando Elba SoC support Brad Larson 2021-10-25 1:51 ` Brad Larson 2021-10-25 9:17 ` Mark Rutland 2021-10-25 9:17 ` Mark Rutland 2021-10-25 11:15 ` Marc Zyngier 2021-10-25 11:15 ` Marc Zyngier 2021-11-05 0:02 ` Brad Larson 2021-11-05 0:02 ` Brad Larson 2021-11-05 11:35 ` Marc Zyngier [this message] 2021-11-05 11:35 ` Marc Zyngier 2021-11-08 19:35 ` Brad Larson 2021-11-08 19:35 ` Brad Larson 2021-11-08 19:53 ` Marc Zyngier 2021-11-08 19:53 ` Marc Zyngier 2021-11-08 20:01 ` Brad Larson 2021-11-08 20:01 ` Brad Larson 2021-11-04 22:53 ` Brad Larson 2021-11-04 22:53 ` Brad Larson 2021-11-08 10:25 ` Mark Rutland 2021-11-08 10:25 ` Mark Rutland 2021-11-08 19:02 ` Brad Larson 2021-11-08 19:02 ` Brad Larson 2021-10-27 21:37 ` Rob Herring 2021-10-27 21:37 ` Rob Herring 2021-11-11 2:08 ` Brad Larson 2021-11-11 2:08 ` Brad Larson
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=87zgqi96nh.wl-maz@kernel.org \ --to=maz@kernel.org \ --cc=adrian.hunter@intel.com \ --cc=arnd@arndb.de \ --cc=bgolaszewski@baylibre.com \ --cc=brad@pensando.io \ --cc=broonie@kernel.org \ --cc=devicetree@vger.kernel.org \ --cc=fancer.lancer@gmail.com \ --cc=linus.walleij@linaro.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-gpio@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mmc@vger.kernel.org \ --cc=linux-spi@vger.kernel.org \ --cc=mark.rutland@arm.com \ --cc=olof@lixom.net \ --cc=ulf.hansson@linaro.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.