| Message ID | 20230905104721.52199-1-lpieralisi@kernel.org |
|---|---|
| Headers | show |
| Series | irqchip/gic-v3: Enable non-coherent GIC designs probing | expand |
On Tue, Sep 05, 2023 at 12:17:51PM +0100, Robin Murphy wrote: > On 05/09/2023 11:47 am, Lorenzo Pieralisi wrote: > > The GIC v3 specifications allow redistributors and ITSes interconnect > > ports used to access memory to be wired up in a way that makes the > > respective initiators/memory observers non-coherent. > > > > Add the standard dma-noncoherent property to the GICv3 bindings to > > allow firmware to describe the redistributors/ITSes components and > > interconnect ports behaviour in system designs where the redistributors > > and ITSes are not coherent with the CPU. > > > > Signed-off-by: Lorenzo Pieralisi <lpieralisi@kernel.org> > > Cc: Rob Herring <robh@kernel.org> > > --- > > .../bindings/interrupt-controller/arm,gic-v3.yaml | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.yaml b/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.yaml > > index 39e64c7f6360..0a81ae4519a6 100644 > > --- a/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.yaml > > +++ b/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.yaml > > @@ -106,6 +106,10 @@ properties: > > $ref: /schemas/types.yaml#/definitions/uint32 > > maximum: 4096 > > + dma-noncoherent: > > + description: | > > + Present if the GIC redistributors are not cache coherent with the CPU. > > I wonder if it's worth being a bit more specific here, e.g. "if the GIC > {redistributors,ITS} permit programming cacheable inner-shareable memory > attributes, but are connected to a non-coherent downstream interconnect." In my opinion it is and I wanted to elaborate on what I wrote but then I thought that this is a standard DT property, I wasn't sure whether we really need to explain what it is there for. We are using the property to plug a hole so I agree with you, we should be as clear as possible in the property definition but I will rely on Rob/Marc's opinion, I don't know what's the DT policy for this. > That might help clarify why the negative property, which could seem a bit > backwards at first glance, and that it's not so important in the cases where > the GIC itself is fundamentally non-coherent anyway (which *is* > software-discoverable). Is it ? Again, see above, are we defining "dma-noncoherent" to fix a bug or to fix the specs ? The shareability bits are writeable and even a fundamentally non-coherent GIC design could allow writing them, AFAIU. I would avoid putting ourselves into a corner where we can't use this property because the binding itself is too strict on what it is solving. > Otherwise, this is the same approach that I like and have previously lobbied > for, so obviously I approve :) > > (plus I do think it's the right shape to be able to slot an equivalent field > into ACPI MADT entries without *too* much bother) We are in agreement, let's see what others think. Thanks, Lorenzo > > Thanks, > Robin. > > > + > > msi-controller: > > description: > > Only present if the Message Based Interrupt functionality is > > @@ -193,6 +197,10 @@ patternProperties: > > compatible: > > const: arm,gic-v3-its > > + dma-noncoherent: > > + description: | > > + Present if the GIC ITS is not cache coherent with the CPU. > > + > > msi-controller: true > > "#msi-cells": > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Tue, Sep 05, 2023 at 12:34:58PM +0100, Marc Zyngier wrote: > On Tue, 05 Sep 2023 11:47:21 +0100, > Lorenzo Pieralisi <lpieralisi@kernel.org> wrote: > > > > The GIC architecture specification defines a set of registers > > for redistributors and ITSes that control the sharebility and > > cacheability attributes of redistributors/ITSes initiator ports > > on the interconnect (GICR_[V]PROPBASER, GICR_[V]PENDBASER, > > GITS_BASER<n>). > > > > Architecturally the GIC provides a means to drive shareability > > and cacheability attributes signals and related IWB/OWB/ISH barriers > > but it is not mandatory for designs to wire up the corresponding > > interconnect signals that control the cacheability/shareability > > of transactions. > > > > Redistributors and ITSes interconnect ports can be connected to > > non-coherent interconnects that are not able to manage the > > shareability/cacheability attributes; this implicitly makes > > the redistributors and ITSes non-coherent observers. > > > > So far, the GIC driver on probe executes a write to "probe" for > > the redistributors and ITSes registers shareability bitfields > > by writing a value (ie InnerShareable - the shareability domain the > > CPUs are in) and check it back to detect whether the value sticks or > > not; this hinges on a GIC programming model behaviour that predates the > > current specifications, that just define shareability bits as writeable > > but do not guarantee that writing certain shareability values > > enable the expected behaviour for the redistributors/ITSes > > memory interconnect ports. > > > > To enable non-coherent GIC designs, introduce the "dma-noncoherent" > > device tree property to allow firmware to describe redistributors and > > ITSes as non-coherent observers on the memory interconnect and use the > > property to force the shareability attributes to be programmed into the > > redistributors and ITSes registers. > > > > Signed-off-by: Lorenzo Pieralisi <lpieralisi@kernel.org> > > Cc: Robin Murphy <robin.murphy@arm.com> > > Cc: Mark Rutland <mark.rutland@arm.com> > > Cc: Marc Zyngier <maz@kernel.org> > > --- > > drivers/irqchip/irq-gic-v3-its.c | 19 +++++++++++++++---- > > 1 file changed, 15 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c > > index e0c2b10d154d..758ea3092305 100644 > > --- a/drivers/irqchip/irq-gic-v3-its.c > > +++ b/drivers/irqchip/irq-gic-v3-its.c > > @@ -5056,7 +5056,8 @@ static int __init its_compute_its_list_map(struct resource *res, > > } > > > > static int __init its_probe_one(struct resource *res, > > - struct fwnode_handle *handle, int numa_node) > > + struct fwnode_handle *handle, int numa_node, > > + bool non_coherent) > > { > > struct its_node *its; > > void __iomem *its_base; > > @@ -5148,7 +5149,7 @@ static int __init its_probe_one(struct resource *res, > > gits_write_cbaser(baser, its->base + GITS_CBASER); > > tmp = gits_read_cbaser(its->base + GITS_CBASER); > > > > - if (its->flags & ITS_FLAGS_FORCE_NON_SHAREABLE) > > + if (its->flags & ITS_FLAGS_FORCE_NON_SHAREABLE || non_coherent) > > tmp &= ~GITS_CBASER_SHAREABILITY_MASK; > > Please use the non_coherent attribute to set the flag, instead of > using it as some sideband signalling. Not having this information > stored in the its_node structure makes it harder to debug. > > We have an over-engineered quirk framework, and it should be put to a > good use. > > > > > if ((tmp ^ baser) & GITS_CBASER_SHAREABILITY_MASK) { > > @@ -5356,11 +5357,19 @@ static const struct of_device_id its_device_id[] = { > > {}, > > }; > > > > +static void of_check_rdists_coherent(struct device_node *node) > > +{ > > + if (of_property_read_bool(node, "dma-noncoherent")) > > + gic_rdists->flags |= RDIST_FLAGS_FORCE_NON_SHAREABLE; > > +} > > + > > static int __init its_of_probe(struct device_node *node) > > { > > struct device_node *np; > > struct resource res; > > > > + of_check_rdists_coherent(node); > > It really feels that the flag should instead be communicated by the > base GIC driver, as it readily communicates the whole rdists structure > already. > > > + > > /* > > * Make sure *all* the ITS are reset before we probe any, as > > * they may be sharing memory. If any of the ITS fails to > > @@ -5396,7 +5405,8 @@ static int __init its_of_probe(struct device_node *node) > > continue; > > } > > > > - its_probe_one(&res, &np->fwnode, of_node_to_nid(np)); > > + its_probe_one(&res, &np->fwnode, of_node_to_nid(np), > > + of_property_read_bool(np, "dma-noncoherent")); > > } > > return 0; > > } > > @@ -5533,7 +5543,8 @@ static int __init gic_acpi_parse_madt_its(union acpi_subtable_headers *header, > > } > > > > err = its_probe_one(&res, dom_handle, > > - acpi_get_its_numa_node(its_entry->translation_id)); > > + acpi_get_its_numa_node(its_entry->translation_id), > > + false); > > I came up with the following alternative approach, which is as usual > completely untested. It is entirely based on the quirk infrastructure, > and doesn't touch the ACPI path at all. > > Thanks, > > M. > > diff --git a/drivers/irqchip/irq-gic-common.h b/drivers/irqchip/irq-gic-common.h > index 3db4592cda1c..00641e88aa38 100644 > --- a/drivers/irqchip/irq-gic-common.h > +++ b/drivers/irqchip/irq-gic-common.h > @@ -29,4 +29,8 @@ void gic_enable_quirks(u32 iidr, const struct gic_quirk *quirks, > void gic_enable_of_quirks(const struct device_node *np, > const struct gic_quirk *quirks, void *data); > > +#define RDIST_FLAGS_PROPBASE_NEEDS_FLUSHING (1 << 0) > +#define RDIST_FLAGS_RD_TABLES_PREALLOCATED (1 << 1) > +#define RDIST_FLAGS_FORCE_NON_SHAREABLE (1 << 2) > + > #endif /* _IRQ_GIC_COMMON_H */ > diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c > index e0c2b10d154d..6edf59af757b 100644 > --- a/drivers/irqchip/irq-gic-v3-its.c > +++ b/drivers/irqchip/irq-gic-v3-its.c > @@ -44,10 +44,6 @@ > #define ITS_FLAGS_WORKAROUND_CAVIUM_23144 (1ULL << 2) > #define ITS_FLAGS_FORCE_NON_SHAREABLE (1ULL << 3) > > -#define RDIST_FLAGS_PROPBASE_NEEDS_FLUSHING (1 << 0) > -#define RDIST_FLAGS_RD_TABLES_PREALLOCATED (1 << 1) > -#define RDIST_FLAGS_FORCE_NON_SHAREABLE (1 << 2) > - > #define RD_LOCAL_LPI_ENABLED BIT(0) > #define RD_LOCAL_PENDTABLE_PREALLOCATED BIT(1) > #define RD_LOCAL_MEMRESERVE_DONE BIT(2) > @@ -4754,6 +4750,14 @@ static bool __maybe_unused its_enable_rk3588001(void *data) > return true; > } > > +static bool its_set_non_coherent(void *data) > +{ > + struct its_node *its = data; > + > + its->flags |= ITS_FLAGS_FORCE_NON_SHAREABLE; > + return true; > +} > + > static const struct gic_quirk its_quirks[] = { > #ifdef CONFIG_CAVIUM_ERRATUM_22375 > { > @@ -4808,6 +4812,11 @@ static const struct gic_quirk its_quirks[] = { > .init = its_enable_rk3588001, > }, > #endif > + { > + .desc = "ITS: non-coherent attribute", > + .property = "dma-noncoherent", > + .init = its_set_non_coherent, > + }, For the records, that requires adding a gic_enable_of_quirks() call for the ITS DT node that at the moment is not needed, something like this: -- >8 -- diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c index 25a12b46ce35..3ae3cb9aadd9 100644 --- a/drivers/irqchip/irq-gic-v3-its.c +++ b/drivers/irqchip/irq-gic-v3-its.c @@ -4826,6 +4826,10 @@ static void its_enable_quirks(struct its_node *its) u32 iidr = readl_relaxed(its->base + GITS_IIDR); gic_enable_quirks(iidr, its_quirks, its); + + if (is_of_node(its->fwnode_handle)) + gic_enable_of_quirks(to_of_node(its->fwnode_handle), + its_quirks, its); } static int its_save_disable(void) > { > } > }; > diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c > index eedfa8e9f077..7f518c0ae723 100644 > --- a/drivers/irqchip/irq-gic-v3.c > +++ b/drivers/irqchip/irq-gic-v3.c > @@ -1857,6 +1857,14 @@ static bool gic_enable_quirk_arm64_2941627(void *data) > return true; > } > > +static bool rd_set_non_coherent(void *data) > +{ > + struct gic_chip_data *d = data; > + > + d->rdists.flags |= RDIST_FLAGS_FORCE_NON_SHAREABLE; > + return true; > +} > + > static const struct gic_quirk gic_quirks[] = { > { > .desc = "GICv3: Qualcomm MSM8996 broken firmware", > @@ -1923,6 +1931,11 @@ static const struct gic_quirk gic_quirks[] = { > .mask = 0xff0f0fff, > .init = gic_enable_quirk_arm64_2941627, > }, > + { > + .desc = "GICv3: non-coherent attribute", > + .property = "dma-noncoherent", > + .init = rd_set_non_coherent, > + }, > { > } > }; > > -- > 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
On 2023-09-06 10:41, Lorenzo Pieralisi wrote: > This series is v2 of a previous version[1]. > > v1 -> v2: > - Updated DT bindings as per feedback > - Updated patch[2] to use GIC quirks infrastructure > > [1] > https://lore.kernel.org/all/20230905104721.52199-1-lpieralisi@kernel.org > > Original cover letter > --- > The GICv3 architecture specifications provide a means for the > system programmer to set the shareability and cacheability > attributes the GIC components (redistributors and ITSes) use > to drive memory transactions. > > Albeit the architecture give control over shareability/cacheability > memory transactions attributes (and barriers), it is allowed to > connect the GIC interconnect ports to non-coherent memory ports > on the interconnect, basically tying off shareability/cacheability > "wires" and de-facto making the redistributors and ITSes non-coherent > memory observers. > > This series aims at starting a discussion over a possible solution > to this problem, by adding to the GIC device tree bindings the > standard dma-noncoherent property. The GIC driver uses the property > to force the redistributors and ITSes shareability attributes to > non-shareable, which consequently forces the driver to use CMOs > on GIC memory tables. > > On ARM DT DMA is default non-coherent, so the GIC driver can't rely > on the generic DT dma-coherent/non-coherent property management layer > (of_dma_is_coherent()) which would default all GIC designs in the field > as non-coherent; it has to rely on ad-hoc dma-noncoherent property > handling. > > When a consistent approach is agreed upon for DT an equivalent binding > will > be put forward for ACPI based systems. What is the plan for this last point? I'd like to see at least a proposal before taking this series in. M.
On Wed, Sep 06, 2023 at 10:52:01AM +0100, Marc Zyngier wrote: > On 2023-09-06 10:41, Lorenzo Pieralisi wrote: > > This series is v2 of a previous version[1]. > > > > v1 -> v2: > > - Updated DT bindings as per feedback > > - Updated patch[2] to use GIC quirks infrastructure > > > > [1] > > https://lore.kernel.org/all/20230905104721.52199-1-lpieralisi@kernel.org > > > > Original cover letter > > --- > > The GICv3 architecture specifications provide a means for the > > system programmer to set the shareability and cacheability > > attributes the GIC components (redistributors and ITSes) use > > to drive memory transactions. > > > > Albeit the architecture give control over shareability/cacheability > > memory transactions attributes (and barriers), it is allowed to > > connect the GIC interconnect ports to non-coherent memory ports > > on the interconnect, basically tying off shareability/cacheability > > "wires" and de-facto making the redistributors and ITSes non-coherent > > memory observers. > > > > This series aims at starting a discussion over a possible solution > > to this problem, by adding to the GIC device tree bindings the > > standard dma-noncoherent property. The GIC driver uses the property > > to force the redistributors and ITSes shareability attributes to > > non-shareable, which consequently forces the driver to use CMOs > > on GIC memory tables. > > > > On ARM DT DMA is default non-coherent, so the GIC driver can't rely > > on the generic DT dma-coherent/non-coherent property management layer > > (of_dma_is_coherent()) which would default all GIC designs in the field > > as non-coherent; it has to rely on ad-hoc dma-noncoherent property > > handling. > > > > When a consistent approach is agreed upon for DT an equivalent binding > > will > > be put forward for ACPI based systems. > > What is the plan for this last point? I'd like to see at least > a proposal before taking this series in. Absolutely, I am starting a thread on related MADT changes, should not take too long. Lorenzo
On Wed, Sep 06, 2023 at 01:23:30PM +0200, Lorenzo Pieralisi wrote: > On Wed, Sep 06, 2023 at 10:52:01AM +0100, Marc Zyngier wrote: > > On 2023-09-06 10:41, Lorenzo Pieralisi wrote: > > > This series is v2 of a previous version[1]. > > > > > > v1 -> v2: > > > - Updated DT bindings as per feedback > > > - Updated patch[2] to use GIC quirks infrastructure > > > > > > [1] > > > https://lore.kernel.org/all/20230905104721.52199-1-lpieralisi@kernel.org > > > > > > Original cover letter > > > --- > > > The GICv3 architecture specifications provide a means for the > > > system programmer to set the shareability and cacheability > > > attributes the GIC components (redistributors and ITSes) use > > > to drive memory transactions. > > > > > > Albeit the architecture give control over shareability/cacheability > > > memory transactions attributes (and barriers), it is allowed to > > > connect the GIC interconnect ports to non-coherent memory ports > > > on the interconnect, basically tying off shareability/cacheability > > > "wires" and de-facto making the redistributors and ITSes non-coherent > > > memory observers. > > > > > > This series aims at starting a discussion over a possible solution > > > to this problem, by adding to the GIC device tree bindings the > > > standard dma-noncoherent property. The GIC driver uses the property > > > to force the redistributors and ITSes shareability attributes to > > > non-shareable, which consequently forces the driver to use CMOs > > > on GIC memory tables. > > > > > > On ARM DT DMA is default non-coherent, so the GIC driver can't rely > > > on the generic DT dma-coherent/non-coherent property management layer > > > (of_dma_is_coherent()) which would default all GIC designs in the field > > > as non-coherent; it has to rely on ad-hoc dma-noncoherent property > > > handling. > > > > > > When a consistent approach is agreed upon for DT an equivalent binding > > > will > > > be put forward for ACPI based systems. > > > > What is the plan for this last point? I'd like to see at least > > a proposal before taking this series in. > > Absolutely, I am starting a thread on related MADT changes, should not > take too long. Quick update, bindings filed, I will code against it but we should not merge anything till it is approved (could be missing v6.7 timeline). https://bugzilla.tianocore.org/show_bug.cgi?id=4557 Lorenzo
The GICv3 architecture specifications provide a means for the system programmer to set the shareability and cacheability attributes the GIC components (redistributors and ITSes) use to drive memory transactions. Albeit the architecture give control over shareability/cacheability memory transactions attributes (and barriers), it is allowed to connect the GIC interconnect ports to non-coherent memory ports on the interconnect, basically tying off shareability/cacheability "wires" and de-facto making the redistributors and ITSes non-coherent memory observers. This series aims at starting a discussion over a possible solution to this problem, by adding to the GIC device tree bindings the standard dma-noncoherent property. The GIC driver uses the property to force the redistributors and ITSes shareability attributes to non-shareable, which consequently forces the driver to use CMOs on GIC memory tables. On ARM DT DMA is default non-coherent, so the GIC driver can't rely on the generic DT dma-coherent/non-coherent property management layer (of_dma_is_coherent()) which would default all GIC designs in the field as non-coherent; it has to rely on ad-hoc dma-noncoherent property handling. When a consistent approach is agreed upon for DT an equivalent binding will be put forward for ACPI based systems. Lorenzo Pieralisi (2): dt-bindings: interrupt-controller: arm,gic-v3: Add dma-noncoherent property irqchip/gic-v3: Enable non-coherent redistributors/ITSes probing .../interrupt-controller/arm,gic-v3.yaml | 8 ++++++++ drivers/irqchip/irq-gic-v3-its.c | 19 +++++++++++++++---- 2 files changed, 23 insertions(+), 4 deletions(-)