[PATCH 0/2] irqchip/gic-v3: Enable non-coherent GIC designs probing

[PATCH 0/2] irqchip/gic-v3: Enable non-coherent GIC designs probing

[Lorenzo Pieralisi]

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(-)

-- 
2.34.1

[PATCH 1/2] dt-bindings: interrupt-controller: arm,gic-v3: Add dma-noncoherent property

[Lorenzo Pieralisi]

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.
+
   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":
-- 
2.34.1

Re: [PATCH 1/2] dt-bindings: interrupt-controller: arm,gic-v3: Add dma-noncoherent property

[Robin Murphy]

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

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)

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":

Re: [PATCH 1/2] dt-bindings: interrupt-controller: arm,gic-v3: Add dma-noncoherent property

[Lorenzo Pieralisi]

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

Re: [PATCH 1/2] dt-bindings: interrupt-controller: arm,gic-v3: Add dma-noncoherent property

[Robin Murphy]

On 05/09/2023 1:22 pm, Lorenzo Pieralisi wrote:
> 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 mean the case on GIC-500 and earlier where the register bits could be 
hard-wired. I'm not sure a GIC implementation which didn't even *try* to 
honour the programmed attributes in what it emits would be considered 
valid; it certainly couldn't be considered sensible :/

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

Really I'm just getting at the fact that if you do have a legacy GIC 
with hard-wired attributes then whatever DT says is most likely 
irrelevant anyway (unless the integrator has done something utterly 
bonkers and tied off the interconnect input to *different* attributes, 
but I would consider that beyond the bounds of fair reasoning...)

Cheers,
Robin.

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

Re: [PATCH 1/2] dt-bindings: interrupt-controller: arm,gic-v3: Add dma-noncoherent property

[Rob Herring]

On Tue, Sep 05, 2023 at 12:47:20PM +0200, 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: |

Don't need the '|' if no formatting to preserve.

> +      Present if the GIC redistributors are not cache coherent with the CPU.
> +
>    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":
> -- 
> 2.34.1
> 

[PATCH 2/2] irqchip/gic-v3: Enable non-coherent redistributors/ITSes probing

[Lorenzo Pieralisi]

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;
 
 	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);
+
 	/*
 	 * 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);
 	if (!err)
 		return 0;
 
-- 
2.34.1

Re: [PATCH 2/2] irqchip/gic-v3: Enable non-coherent redistributors/ITSes probing

[Marc Zyngier]

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,
+	},
 	{
 	}
 };
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.

Re: [PATCH 2/2] irqchip/gic-v3: Enable non-coherent redistributors/ITSes probing

[Robin Murphy]

On 05/09/2023 12:34 pm, 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.

FWIW I think I agree that looks a bit easier to follow overall, and 
especially since we already have the SoC-based Rockchip variant of this 
using a quirk, having two different ways of carrying the same underlying 
information through certain paths does seem a bit icky.

Cheers,
Robin.

> 
> 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,
> +	},
>   	{
>   	}
>   };
> 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,
> +	},
>   	{
>   	}
>   };
> 

Re: [PATCH 2/2] irqchip/gic-v3: Enable non-coherent redistributors/ITSes probing

[Lorenzo Pieralisi]

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.

That makes sense, I will do - I was wondering though whether this
is a quirk or a binding that describes the architecture because
the architecture can't provide that information.

That's the reason why I have not implemented it as a quirk but
obviously that's exactly the feedback I need.

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

Yes I had the same feeling while writing the code but the patch
below (if we treat this as quirk) is already much cleaner.

> > +
> >  	/*
> >  	 * 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.

The patch below makes sense and it is much cleaner - provided we
consider this a quirk, which I am not sure it is, that's the only
question I have.

Thanks,
Lorenzo

> 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,
> +	},
>  	{
>  	}
>  };
> 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

Re: [PATCH 2/2] irqchip/gic-v3: Enable non-coherent redistributors/ITSes probing

[Marc Zyngier]

On Tue, 05 Sep 2023 13:30:47 +0100,
Lorenzo Pieralisi <lpieralisi@kernel.org> wrote:
> 
> On Tue, Sep 05, 2023 at 12:34:58PM +0100, Marc Zyngier wrote:
> > 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.
> 
> The patch below makes sense and it is much cleaner - provided we
> consider this a quirk, which I am not sure it is, that's the only
> question I have.

Come on, the whole architecture is a bag of sick hacks. And given that
it makes the change slightly less ugly, I'd rather have that.

	M.

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

Re: [PATCH 2/2] irqchip/gic-v3: Enable non-coherent redistributors/ITSes probing

[Lorenzo Pieralisi]

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

Re: [PATCH 2/2] irqchip/gic-v3: Enable non-coherent redistributors/ITSes probing

[Marc Zyngier]

On Tue, 05 Sep 2023 15:24:51 +0100,
Lorenzo Pieralisi <lpieralisi@kernel.org> wrote:
> 
> On Tue, Sep 05, 2023 at 12:34:58PM +0100, Marc Zyngier wrote:

> > @@ -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);
>  }
>  

Ah, well spotted. Yes, please fold this in.

Thanks,

	M.

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

Re: [PATCH 2/2] irqchip/gic-v3: Enable non-coherent redistributors/ITSes probing

[Fang Xiang]

On Tue, Sep 05, 2023 at 04:24:51PM +0200, Lorenzo Pieralisi wrote:
> On Tue, Sep 05, 2023 at 12:34:58PM +0100, Marc Zyngier wrote:
> > 
> > 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,
> > +	},
> >  	{
> >  	}
> >  };

I have tested this patch above on my deivce and it works well. Looking forward the official release.

Re: [PATCH 2/2] irqchip/gic-v3: Enable non-coherent redistributors/ITSes probing

[Lorenzo Pieralisi]

On Tue, Sep 05, 2023 at 12:34:58PM +0100, Marc Zyngier wrote:

[...]

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

Writing the ACPI bits. We can't use the quirks framework for ACPI (we
don't have "properties" and I don't think we want to attach any to the
fwnode_handle) that's why I generalized its_probe_one() above with an
extra param, that would have simplified ACPI parsing:

- we alloc struct its_node in its_probe_one() but at that stage
  ACPI parsing was already done. If we have to parse the MADT(ITS) again
  just to scan for non-coherent we then have to match the MADT entries
  to the *current* struct its_node* we are handling (MADT parsing
  callbacks don't even take a param - we have to resort to global
  variables - definitely doable but it is a bit ugly).

I will post the resulting code (or something else if I find anything
nicer) but I thought I would mention the reasoning behind the additional
bool flag above.

Lorenzo

> 
> 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,
> +	},
>  	{
>  	}
>  };
> 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

Re: [PATCH 2/2] irqchip/gic-v3: Enable non-coherent redistributors/ITSes probing

[Robin Murphy]

On 03/10/2023 3:43 pm, Lorenzo Pieralisi wrote:
> On Tue, Sep 05, 2023 at 12:34:58PM +0100, Marc Zyngier wrote:
> 
> [...]
> 
>>>   	 * 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.
> 
> Writing the ACPI bits. We can't use the quirks framework for ACPI (we
> don't have "properties" and I don't think we want to attach any to the
> fwnode_handle) that's why I generalized its_probe_one() above with an
> extra param, that would have simplified ACPI parsing:
> 
> - we alloc struct its_node in its_probe_one() but at that stage
>    ACPI parsing was already done. If we have to parse the MADT(ITS) again
>    just to scan for non-coherent we then have to match the MADT entries
>    to the *current* struct its_node* we are handling (MADT parsing
>    callbacks don't even take a param - we have to resort to global
>    variables - definitely doable but it is a bit ugly).

How about a compromise of passing a whole MADT flags field into 
its_probe_one() (where its_of_probe() can just pass 0), to pass through 
to its_enable_quirks() to then match against an madt_flags field in the 
gic_quirk? gic_acpi_init() could then do something similar for the 
redistributor quirk, although I guess it would then need to distinguish 
GICC and GICR-based quirks cases since the respective flags are in 
different formats.

Thanks,
Robin.

Re: [PATCH 2/2] irqchip/gic-v3: Enable non-coherent redistributors/ITSes probing

[Marc Zyngier]

On Tue, 03 Oct 2023 15:43:40 +0100,
Lorenzo Pieralisi <lpieralisi@kernel.org> wrote:
> 
> On Tue, Sep 05, 2023 at 12:34:58PM +0100, Marc Zyngier wrote:
> 
> [...]
> 
> > >  	 * 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.
> 
> Writing the ACPI bits. We can't use the quirks framework for ACPI (we
> don't have "properties" and I don't think we want to attach any to the
> fwnode_handle) that's why I generalized its_probe_one() above with an
> extra param, that would have simplified ACPI parsing:
> 
> - we alloc struct its_node in its_probe_one() but at that stage
>   ACPI parsing was already done. If we have to parse the MADT(ITS) again
>   just to scan for non-coherent we then have to match the MADT entries
>   to the *current* struct its_node* we are handling (MADT parsing
>   callbacks don't even take a param - we have to resort to global
>   variables - definitely doable but it is a bit ugly).

Well, a more acceptable approach would be for its_probe_one() to take
an allocated and possibly pre-populated its_node structure (crucially,
with the quirk flags set), which itself results in a bunch of low
hanging cleanups, see the patch below.

I have boot tested it in a DT guest, so it is obviously perfect.

	M.

From 978f654d4459adf0b8f3f8e896ca37035b3b114c Mon Sep 17 00:00:00 2001
From: Marc Zyngier <maz@kernel.org>
Date: Tue, 3 Oct 2023 17:35:27 +0100
Subject: [PATCH] irqchip/gic-v3-its: Split allocation from initialisation of
 its_node

In order to pave the way for more fancy quirk handling without making
more of a mess of this terrible driver, split the allocation of the
ITS descriptor (its_node) from the actual probing.

This will allow firmware-specific hooks to be added between these
two points.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 drivers/irqchip/irq-gic-v3-its.c | 151 +++++++++++++++++++------------
 1 file changed, 91 insertions(+), 60 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index e0c2b10d154d..bf21383b714e 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -4952,7 +4952,7 @@ static void __init __iomem *its_map_one(struct resource *res, int *err)
 	return NULL;
 }
 
-static int its_init_domain(struct fwnode_handle *handle, struct its_node *its)
+static int its_init_domain(struct its_node *its)
 {
 	struct irq_domain *inner_domain;
 	struct msi_domain_info *info;
@@ -4966,7 +4966,7 @@ static int its_init_domain(struct fwnode_handle *handle, struct its_node *its)
 
 	inner_domain = irq_domain_create_hierarchy(its_parent,
 						   its->msi_domain_flags, 0,
-						   handle, &its_domain_ops,
+						   its->fwnode_handle, &its_domain_ops,
 						   info);
 	if (!inner_domain) {
 		kfree(info);
@@ -5017,8 +5017,7 @@ static int its_init_vpe_domain(void)
 	return 0;
 }
 
-static int __init its_compute_its_list_map(struct resource *res,
-					   void __iomem *its_base)
+static int __init its_compute_its_list_map(struct its_node *its)
 {
 	int its_number;
 	u32 ctlr;
@@ -5032,15 +5031,15 @@ static int __init its_compute_its_list_map(struct resource *res,
 	its_number = find_first_zero_bit(&its_list_map, GICv4_ITS_LIST_MAX);
 	if (its_number >= GICv4_ITS_LIST_MAX) {
 		pr_err("ITS@%pa: No ITSList entry available!\n",
-		       &res->start);
+		       &its->phys_base);
 		return -EINVAL;
 	}
 
-	ctlr = readl_relaxed(its_base + GITS_CTLR);
+	ctlr = readl_relaxed(its->base + GITS_CTLR);
 	ctlr &= ~GITS_CTLR_ITS_NUMBER;
 	ctlr |= its_number << GITS_CTLR_ITS_NUMBER_SHIFT;
-	writel_relaxed(ctlr, its_base + GITS_CTLR);
-	ctlr = readl_relaxed(its_base + GITS_CTLR);
+	writel_relaxed(ctlr, its->base + GITS_CTLR);
+	ctlr = readl_relaxed(its->base + GITS_CTLR);
 	if ((ctlr & GITS_CTLR_ITS_NUMBER) != (its_number << GITS_CTLR_ITS_NUMBER_SHIFT)) {
 		its_number = ctlr & GITS_CTLR_ITS_NUMBER;
 		its_number >>= GITS_CTLR_ITS_NUMBER_SHIFT;
@@ -5048,75 +5047,50 @@ static int __init its_compute_its_list_map(struct resource *res,
 
 	if (test_and_set_bit(its_number, &its_list_map)) {
 		pr_err("ITS@%pa: Duplicate ITSList entry %d\n",
-		       &res->start, its_number);
+		       &its->phys_base, its_number);
 		return -EINVAL;
 	}
 
 	return its_number;
 }
 
-static int __init its_probe_one(struct resource *res,
-				struct fwnode_handle *handle, int numa_node)
+static int __init its_probe_one(struct its_node *its)
 {
-	struct its_node *its;
-	void __iomem *its_base;
-	u64 baser, tmp, typer;
+	u64 baser, tmp;
 	struct page *page;
 	u32 ctlr;
 	int err;
 
-	its_base = its_map_one(res, &err);
-	if (!its_base)
-		return err;
-
-	pr_info("ITS %pR\n", res);
-
-	its = kzalloc(sizeof(*its), GFP_KERNEL);
-	if (!its) {
-		err = -ENOMEM;
-		goto out_unmap;
-	}
-
-	raw_spin_lock_init(&its->lock);
-	mutex_init(&its->dev_alloc_lock);
-	INIT_LIST_HEAD(&its->entry);
-	INIT_LIST_HEAD(&its->its_device_list);
-	typer = gic_read_typer(its_base + GITS_TYPER);
-	its->typer = typer;
-	its->base = its_base;
-	its->phys_base = res->start;
 	if (is_v4(its)) {
-		if (!(typer & GITS_TYPER_VMOVP)) {
-			err = its_compute_its_list_map(res, its_base);
+		if (!(its->typer & GITS_TYPER_VMOVP)) {
+			err = its_compute_its_list_map(its);
 			if (err < 0)
-				goto out_free_its;
+				goto out;
 
 			its->list_nr = err;
 
 			pr_info("ITS@%pa: Using ITS number %d\n",
-				&res->start, err);
+				&its->phys_base, err);
 		} else {
-			pr_info("ITS@%pa: Single VMOVP capable\n", &res->start);
+			pr_info("ITS@%pa: Single VMOVP capable\n", &its->phys_base);
 		}
 
 		if (is_v4_1(its)) {
-			u32 svpet = FIELD_GET(GITS_TYPER_SVPET, typer);
+			u32 svpet = FIELD_GET(GITS_TYPER_SVPET, its->typer);
 
-			its->sgir_base = ioremap(res->start + SZ_128K, SZ_64K);
+			its->sgir_base = ioremap(its->phys_base + SZ_128K, SZ_64K);
 			if (!its->sgir_base) {
 				err = -ENOMEM;
-				goto out_free_its;
+				goto out;
 			}
 
-			its->mpidr = readl_relaxed(its_base + GITS_MPIDR);
+			its->mpidr = readl_relaxed(its->base + GITS_MPIDR);
 
 			pr_info("ITS@%pa: Using GICv4.1 mode %08x %08x\n",
-				&res->start, its->mpidr, svpet);
+				&its->phys_base, its->mpidr, svpet);
 		}
 	}
 
-	its->numa_node = numa_node;
-
 	page = alloc_pages_node(its->numa_node, GFP_KERNEL | __GFP_ZERO,
 				get_order(ITS_CMD_QUEUE_SZ));
 	if (!page) {
@@ -5125,12 +5099,9 @@ static int __init its_probe_one(struct resource *res,
 	}
 	its->cmd_base = (void *)page_address(page);
 	its->cmd_write = its->cmd_base;
-	its->fwnode_handle = handle;
 	its->get_msi_base = its_irq_get_msi_base;
 	its->msi_domain_flags = IRQ_DOMAIN_FLAG_ISOLATED_MSI;
 
-	its_enable_quirks(its);
-
 	err = its_alloc_tables(its);
 	if (err)
 		goto out_free_cmd;
@@ -5174,7 +5145,7 @@ static int __init its_probe_one(struct resource *res,
 		ctlr |= GITS_CTLR_ImDe;
 	writel_relaxed(ctlr, its->base + GITS_CTLR);
 
-	err = its_init_domain(handle, its);
+	err = its_init_domain(its);
 	if (err)
 		goto out_free_tables;
 
@@ -5191,11 +5162,8 @@ static int __init its_probe_one(struct resource *res,
 out_unmap_sgir:
 	if (its->sgir_base)
 		iounmap(its->sgir_base);
-out_free_its:
-	kfree(its);
-out_unmap:
-	iounmap(its_base);
-	pr_err("ITS@%pa: failed probing (%d)\n", &res->start, err);
+out:
+	pr_err("ITS@%pa: failed probing (%d)\n", &its->phys_base, err);
 	return err;
 }
 
@@ -5356,10 +5324,53 @@ static const struct of_device_id its_device_id[] = {
 	{},
 };
 
+static struct its_node __init *its_node_init(struct resource *res,
+					     struct fwnode_handle *handle, int numa_node)
+{
+	void __iomem *its_base;
+	struct its_node *its;
+	int err;
+
+	its_base = its_map_one(res, &err);
+	if (!its_base)
+		return NULL;
+
+	pr_info("ITS %pR\n", res);
+
+	its = kzalloc(sizeof(*its), GFP_KERNEL);
+	if (!its)
+		goto out_unmap;
+
+	raw_spin_lock_init(&its->lock);
+	mutex_init(&its->dev_alloc_lock);
+	INIT_LIST_HEAD(&its->entry);
+	INIT_LIST_HEAD(&its->its_device_list);
+
+	its->typer = gic_read_typer(its_base + GITS_TYPER);
+	its->base = its_base;
+	its->phys_base = res->start;
+
+	its->numa_node = numa_node;
+	its->fwnode_handle = handle;
+
+	return its;
+
+out_unmap:
+	iounmap(its_base);
+	return NULL;
+}
+
+static void its_node_destroy(struct its_node *its)
+{
+	iounmap(its->base);
+	kfree(its);
+}
+
 static int __init its_of_probe(struct device_node *node)
 {
 	struct device_node *np;
 	struct resource res;
+	int err;
 
 	/*
 	 * Make sure *all* the ITS are reset before we probe any, as
@@ -5369,8 +5380,6 @@ static int __init its_of_probe(struct device_node *node)
 	 */
 	for (np = of_find_matching_node(node, its_device_id); np;
 	     np = of_find_matching_node(np, its_device_id)) {
-		int err;
-
 		if (!of_device_is_available(np) ||
 		    !of_property_read_bool(np, "msi-controller") ||
 		    of_address_to_resource(np, 0, &res))
@@ -5383,6 +5392,8 @@ static int __init its_of_probe(struct device_node *node)
 
 	for (np = of_find_matching_node(node, its_device_id); np;
 	     np = of_find_matching_node(np, its_device_id)) {
+		struct its_node *its;
+
 		if (!of_device_is_available(np))
 			continue;
 		if (!of_property_read_bool(np, "msi-controller")) {
@@ -5396,7 +5407,17 @@ static int __init its_of_probe(struct device_node *node)
 			continue;
 		}
 
-		its_probe_one(&res, &np->fwnode, of_node_to_nid(np));
+
+		its = its_node_init(&res, &np->fwnode, of_node_to_nid(np));
+		if (!its)
+			return -ENOMEM;
+
+		its_enable_quirks(its);
+		err = its_probe_one(its);
+		if (err)  {
+			its_node_destroy(its);
+			return err;
+		}
 	}
 	return 0;
 }
@@ -5508,6 +5529,7 @@ static int __init gic_acpi_parse_madt_its(union acpi_subtable_headers *header,
 {
 	struct acpi_madt_generic_translator *its_entry;
 	struct fwnode_handle *dom_handle;
+	struct its_node *its;
 	struct resource res;
 	int err;
 
@@ -5532,11 +5554,20 @@ static int __init gic_acpi_parse_madt_its(union acpi_subtable_headers *header,
 		goto dom_err;
 	}
 
-	err = its_probe_one(&res, dom_handle,
-			acpi_get_its_numa_node(its_entry->translation_id));
+	its = its_node_init(&res, dom_handle,
+			    acpi_get_its_numa_node(its_entry->translation_id));
+	if (!its) {
+		err = -ENOMEM;
+		goto node_err;
+	}
+
+	/* Stick ACPI quirk handling here */
+
+	err = its_probe_one(its);
 	if (!err)
 		return 0;
 
+node_err:
 	iort_deregister_domain_token(its_entry->translation_id);
 dom_err:
 	irq_domain_free_fwnode(dom_handle);
-- 
2.34.1

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

Re: [PATCH 2/2] irqchip/gic-v3: Enable non-coherent redistributors/ITSes probing

[Lorenzo Pieralisi]

On Tue, Oct 03, 2023 at 05:44:27PM +0100, Marc Zyngier wrote:
> On Tue, 03 Oct 2023 15:43:40 +0100,
> Lorenzo Pieralisi <lpieralisi@kernel.org> wrote:
> > 
> > On Tue, Sep 05, 2023 at 12:34:58PM +0100, Marc Zyngier wrote:
> > 
> > [...]
> > 
> > > >  	 * 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.
> > 
> > Writing the ACPI bits. We can't use the quirks framework for ACPI (we
> > don't have "properties" and I don't think we want to attach any to the
> > fwnode_handle) that's why I generalized its_probe_one() above with an
> > extra param, that would have simplified ACPI parsing:
> > 
> > - we alloc struct its_node in its_probe_one() but at that stage
> >   ACPI parsing was already done. If we have to parse the MADT(ITS) again
> >   just to scan for non-coherent we then have to match the MADT entries
> >   to the *current* struct its_node* we are handling (MADT parsing
> >   callbacks don't even take a param - we have to resort to global
> >   variables - definitely doable but it is a bit ugly).
> 
> Well, a more acceptable approach would be for its_probe_one() to take
> an allocated and possibly pre-populated its_node structure (crucially,
> with the quirk flags set), which itself results in a bunch of low
> hanging cleanups, see the patch below.
> 
> I have boot tested it in a DT guest, so it is obviously perfect.

Thanks Marc I will test it and add the ACPI parsing bits.

Lorenzo

> 
> 	M.
> 
> From 978f654d4459adf0b8f3f8e896ca37035b3b114c Mon Sep 17 00:00:00 2001
> From: Marc Zyngier <maz@kernel.org>
> Date: Tue, 3 Oct 2023 17:35:27 +0100
> Subject: [PATCH] irqchip/gic-v3-its: Split allocation from initialisation of
>  its_node
> 
> In order to pave the way for more fancy quirk handling without making
> more of a mess of this terrible driver, split the allocation of the
> ITS descriptor (its_node) from the actual probing.
> 
> This will allow firmware-specific hooks to be added between these
> two points.
> 
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  drivers/irqchip/irq-gic-v3-its.c | 151 +++++++++++++++++++------------
>  1 file changed, 91 insertions(+), 60 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index e0c2b10d154d..bf21383b714e 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -4952,7 +4952,7 @@ static void __init __iomem *its_map_one(struct resource *res, int *err)
>  	return NULL;
>  }
>  
> -static int its_init_domain(struct fwnode_handle *handle, struct its_node *its)
> +static int its_init_domain(struct its_node *its)
>  {
>  	struct irq_domain *inner_domain;
>  	struct msi_domain_info *info;
> @@ -4966,7 +4966,7 @@ static int its_init_domain(struct fwnode_handle *handle, struct its_node *its)
>  
>  	inner_domain = irq_domain_create_hierarchy(its_parent,
>  						   its->msi_domain_flags, 0,
> -						   handle, &its_domain_ops,
> +						   its->fwnode_handle, &its_domain_ops,
>  						   info);
>  	if (!inner_domain) {
>  		kfree(info);
> @@ -5017,8 +5017,7 @@ static int its_init_vpe_domain(void)
>  	return 0;
>  }
>  
> -static int __init its_compute_its_list_map(struct resource *res,
> -					   void __iomem *its_base)
> +static int __init its_compute_its_list_map(struct its_node *its)
>  {
>  	int its_number;
>  	u32 ctlr;
> @@ -5032,15 +5031,15 @@ static int __init its_compute_its_list_map(struct resource *res,
>  	its_number = find_first_zero_bit(&its_list_map, GICv4_ITS_LIST_MAX);
>  	if (its_number >= GICv4_ITS_LIST_MAX) {
>  		pr_err("ITS@%pa: No ITSList entry available!\n",
> -		       &res->start);
> +		       &its->phys_base);
>  		return -EINVAL;
>  	}
>  
> -	ctlr = readl_relaxed(its_base + GITS_CTLR);
> +	ctlr = readl_relaxed(its->base + GITS_CTLR);
>  	ctlr &= ~GITS_CTLR_ITS_NUMBER;
>  	ctlr |= its_number << GITS_CTLR_ITS_NUMBER_SHIFT;
> -	writel_relaxed(ctlr, its_base + GITS_CTLR);
> -	ctlr = readl_relaxed(its_base + GITS_CTLR);
> +	writel_relaxed(ctlr, its->base + GITS_CTLR);
> +	ctlr = readl_relaxed(its->base + GITS_CTLR);
>  	if ((ctlr & GITS_CTLR_ITS_NUMBER) != (its_number << GITS_CTLR_ITS_NUMBER_SHIFT)) {
>  		its_number = ctlr & GITS_CTLR_ITS_NUMBER;
>  		its_number >>= GITS_CTLR_ITS_NUMBER_SHIFT;
> @@ -5048,75 +5047,50 @@ static int __init its_compute_its_list_map(struct resource *res,
>  
>  	if (test_and_set_bit(its_number, &its_list_map)) {
>  		pr_err("ITS@%pa: Duplicate ITSList entry %d\n",
> -		       &res->start, its_number);
> +		       &its->phys_base, its_number);
>  		return -EINVAL;
>  	}
>  
>  	return its_number;
>  }
>  
> -static int __init its_probe_one(struct resource *res,
> -				struct fwnode_handle *handle, int numa_node)
> +static int __init its_probe_one(struct its_node *its)
>  {
> -	struct its_node *its;
> -	void __iomem *its_base;
> -	u64 baser, tmp, typer;
> +	u64 baser, tmp;
>  	struct page *page;
>  	u32 ctlr;
>  	int err;
>  
> -	its_base = its_map_one(res, &err);
> -	if (!its_base)
> -		return err;
> -
> -	pr_info("ITS %pR\n", res);
> -
> -	its = kzalloc(sizeof(*its), GFP_KERNEL);
> -	if (!its) {
> -		err = -ENOMEM;
> -		goto out_unmap;
> -	}
> -
> -	raw_spin_lock_init(&its->lock);
> -	mutex_init(&its->dev_alloc_lock);
> -	INIT_LIST_HEAD(&its->entry);
> -	INIT_LIST_HEAD(&its->its_device_list);
> -	typer = gic_read_typer(its_base + GITS_TYPER);
> -	its->typer = typer;
> -	its->base = its_base;
> -	its->phys_base = res->start;
>  	if (is_v4(its)) {
> -		if (!(typer & GITS_TYPER_VMOVP)) {
> -			err = its_compute_its_list_map(res, its_base);
> +		if (!(its->typer & GITS_TYPER_VMOVP)) {
> +			err = its_compute_its_list_map(its);
>  			if (err < 0)
> -				goto out_free_its;
> +				goto out;
>  
>  			its->list_nr = err;
>  
>  			pr_info("ITS@%pa: Using ITS number %d\n",
> -				&res->start, err);
> +				&its->phys_base, err);
>  		} else {
> -			pr_info("ITS@%pa: Single VMOVP capable\n", &res->start);
> +			pr_info("ITS@%pa: Single VMOVP capable\n", &its->phys_base);
>  		}
>  
>  		if (is_v4_1(its)) {
> -			u32 svpet = FIELD_GET(GITS_TYPER_SVPET, typer);
> +			u32 svpet = FIELD_GET(GITS_TYPER_SVPET, its->typer);
>  
> -			its->sgir_base = ioremap(res->start + SZ_128K, SZ_64K);
> +			its->sgir_base = ioremap(its->phys_base + SZ_128K, SZ_64K);
>  			if (!its->sgir_base) {
>  				err = -ENOMEM;
> -				goto out_free_its;
> +				goto out;
>  			}
>  
> -			its->mpidr = readl_relaxed(its_base + GITS_MPIDR);
> +			its->mpidr = readl_relaxed(its->base + GITS_MPIDR);
>  
>  			pr_info("ITS@%pa: Using GICv4.1 mode %08x %08x\n",
> -				&res->start, its->mpidr, svpet);
> +				&its->phys_base, its->mpidr, svpet);
>  		}
>  	}
>  
> -	its->numa_node = numa_node;
> -
>  	page = alloc_pages_node(its->numa_node, GFP_KERNEL | __GFP_ZERO,
>  				get_order(ITS_CMD_QUEUE_SZ));
>  	if (!page) {
> @@ -5125,12 +5099,9 @@ static int __init its_probe_one(struct resource *res,
>  	}
>  	its->cmd_base = (void *)page_address(page);
>  	its->cmd_write = its->cmd_base;
> -	its->fwnode_handle = handle;
>  	its->get_msi_base = its_irq_get_msi_base;
>  	its->msi_domain_flags = IRQ_DOMAIN_FLAG_ISOLATED_MSI;
>  
> -	its_enable_quirks(its);
> -
>  	err = its_alloc_tables(its);
>  	if (err)
>  		goto out_free_cmd;
> @@ -5174,7 +5145,7 @@ static int __init its_probe_one(struct resource *res,
>  		ctlr |= GITS_CTLR_ImDe;
>  	writel_relaxed(ctlr, its->base + GITS_CTLR);
>  
> -	err = its_init_domain(handle, its);
> +	err = its_init_domain(its);
>  	if (err)
>  		goto out_free_tables;
>  
> @@ -5191,11 +5162,8 @@ static int __init its_probe_one(struct resource *res,
>  out_unmap_sgir:
>  	if (its->sgir_base)
>  		iounmap(its->sgir_base);
> -out_free_its:
> -	kfree(its);
> -out_unmap:
> -	iounmap(its_base);
> -	pr_err("ITS@%pa: failed probing (%d)\n", &res->start, err);
> +out:
> +	pr_err("ITS@%pa: failed probing (%d)\n", &its->phys_base, err);
>  	return err;
>  }
>  
> @@ -5356,10 +5324,53 @@ static const struct of_device_id its_device_id[] = {
>  	{},
>  };
>  
> +static struct its_node __init *its_node_init(struct resource *res,
> +					     struct fwnode_handle *handle, int numa_node)
> +{
> +	void __iomem *its_base;
> +	struct its_node *its;
> +	int err;
> +
> +	its_base = its_map_one(res, &err);
> +	if (!its_base)
> +		return NULL;
> +
> +	pr_info("ITS %pR\n", res);
> +
> +	its = kzalloc(sizeof(*its), GFP_KERNEL);
> +	if (!its)
> +		goto out_unmap;
> +
> +	raw_spin_lock_init(&its->lock);
> +	mutex_init(&its->dev_alloc_lock);
> +	INIT_LIST_HEAD(&its->entry);
> +	INIT_LIST_HEAD(&its->its_device_list);
> +
> +	its->typer = gic_read_typer(its_base + GITS_TYPER);
> +	its->base = its_base;
> +	its->phys_base = res->start;
> +
> +	its->numa_node = numa_node;
> +	its->fwnode_handle = handle;
> +
> +	return its;
> +
> +out_unmap:
> +	iounmap(its_base);
> +	return NULL;
> +}
> +
> +static void its_node_destroy(struct its_node *its)
> +{
> +	iounmap(its->base);
> +	kfree(its);
> +}
> +
>  static int __init its_of_probe(struct device_node *node)
>  {
>  	struct device_node *np;
>  	struct resource res;
> +	int err;
>  
>  	/*
>  	 * Make sure *all* the ITS are reset before we probe any, as
> @@ -5369,8 +5380,6 @@ static int __init its_of_probe(struct device_node *node)
>  	 */
>  	for (np = of_find_matching_node(node, its_device_id); np;
>  	     np = of_find_matching_node(np, its_device_id)) {
> -		int err;
> -
>  		if (!of_device_is_available(np) ||
>  		    !of_property_read_bool(np, "msi-controller") ||
>  		    of_address_to_resource(np, 0, &res))
> @@ -5383,6 +5392,8 @@ static int __init its_of_probe(struct device_node *node)
>  
>  	for (np = of_find_matching_node(node, its_device_id); np;
>  	     np = of_find_matching_node(np, its_device_id)) {
> +		struct its_node *its;
> +
>  		if (!of_device_is_available(np))
>  			continue;
>  		if (!of_property_read_bool(np, "msi-controller")) {
> @@ -5396,7 +5407,17 @@ static int __init its_of_probe(struct device_node *node)
>  			continue;
>  		}
>  
> -		its_probe_one(&res, &np->fwnode, of_node_to_nid(np));
> +
> +		its = its_node_init(&res, &np->fwnode, of_node_to_nid(np));
> +		if (!its)
> +			return -ENOMEM;
> +
> +		its_enable_quirks(its);
> +		err = its_probe_one(its);
> +		if (err)  {
> +			its_node_destroy(its);
> +			return err;
> +		}
>  	}
>  	return 0;
>  }
> @@ -5508,6 +5529,7 @@ static int __init gic_acpi_parse_madt_its(union acpi_subtable_headers *header,
>  {
>  	struct acpi_madt_generic_translator *its_entry;
>  	struct fwnode_handle *dom_handle;
> +	struct its_node *its;
>  	struct resource res;
>  	int err;
>  
> @@ -5532,11 +5554,20 @@ static int __init gic_acpi_parse_madt_its(union acpi_subtable_headers *header,
>  		goto dom_err;
>  	}
>  
> -	err = its_probe_one(&res, dom_handle,
> -			acpi_get_its_numa_node(its_entry->translation_id));
> +	its = its_node_init(&res, dom_handle,
> +			    acpi_get_its_numa_node(its_entry->translation_id));
> +	if (!its) {
> +		err = -ENOMEM;
> +		goto node_err;
> +	}
> +
> +	/* Stick ACPI quirk handling here */
> +
> +	err = its_probe_one(its);
>  	if (!err)
>  		return 0;
>  
> +node_err:
>  	iort_deregister_domain_token(its_entry->translation_id);
>  dom_err:
>  	irq_domain_free_fwnode(dom_handle);
> -- 
> 2.34.1
> 
> -- 
> Without deviation from the norm, progress is not possible.

Re: [PATCH 2/2] irqchip/gic-v3: Enable non-coherent redistributors/ITSes probing

[Lorenzo Pieralisi]

On Tue, Oct 03, 2023 at 05:44:27PM +0100, Marc Zyngier wrote:
> On Tue, 03 Oct 2023 15:43:40 +0100,
> Lorenzo Pieralisi <lpieralisi@kernel.org> wrote:
> > 
> > On Tue, Sep 05, 2023 at 12:34:58PM +0100, Marc Zyngier wrote:
> > 
> > [...]
> > 
> > > >  	 * 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.
> > 
> > Writing the ACPI bits. We can't use the quirks framework for ACPI (we
> > don't have "properties" and I don't think we want to attach any to the
> > fwnode_handle) that's why I generalized its_probe_one() above with an
> > extra param, that would have simplified ACPI parsing:
> > 
> > - we alloc struct its_node in its_probe_one() but at that stage
> >   ACPI parsing was already done. If we have to parse the MADT(ITS) again
> >   just to scan for non-coherent we then have to match the MADT entries
> >   to the *current* struct its_node* we are handling (MADT parsing
> >   callbacks don't even take a param - we have to resort to global
> >   variables - definitely doable but it is a bit ugly).
> 
> Well, a more acceptable approach would be for its_probe_one() to take
> an allocated and possibly pre-populated its_node structure (crucially,
> with the quirk flags set), which itself results in a bunch of low
> hanging cleanups, see the patch below.
> 
> I have boot tested it in a DT guest, so it is obviously perfect.

If you don't mind I will post it together with the resulting series.

I have just removed:

/* Stick ACPI quirk handling here */

point taken :)

Thanks,
Lorenzo

> 	M.
> 
> From 978f654d4459adf0b8f3f8e896ca37035b3b114c Mon Sep 17 00:00:00 2001
> From: Marc Zyngier <maz@kernel.org>
> Date: Tue, 3 Oct 2023 17:35:27 +0100
> Subject: [PATCH] irqchip/gic-v3-its: Split allocation from initialisation of
>  its_node
> 
> In order to pave the way for more fancy quirk handling without making
> more of a mess of this terrible driver, split the allocation of the
> ITS descriptor (its_node) from the actual probing.
> 
> This will allow firmware-specific hooks to be added between these
> two points.
> 
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  drivers/irqchip/irq-gic-v3-its.c | 151 +++++++++++++++++++------------
>  1 file changed, 91 insertions(+), 60 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index e0c2b10d154d..bf21383b714e 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -4952,7 +4952,7 @@ static void __init __iomem *its_map_one(struct resource *res, int *err)
>  	return NULL;
>  }
>  
> -static int its_init_domain(struct fwnode_handle *handle, struct its_node *its)
> +static int its_init_domain(struct its_node *its)
>  {
>  	struct irq_domain *inner_domain;
>  	struct msi_domain_info *info;
> @@ -4966,7 +4966,7 @@ static int its_init_domain(struct fwnode_handle *handle, struct its_node *its)
>  
>  	inner_domain = irq_domain_create_hierarchy(its_parent,
>  						   its->msi_domain_flags, 0,
> -						   handle, &its_domain_ops,
> +						   its->fwnode_handle, &its_domain_ops,
>  						   info);
>  	if (!inner_domain) {
>  		kfree(info);
> @@ -5017,8 +5017,7 @@ static int its_init_vpe_domain(void)
>  	return 0;
>  }
>  
> -static int __init its_compute_its_list_map(struct resource *res,
> -					   void __iomem *its_base)
> +static int __init its_compute_its_list_map(struct its_node *its)
>  {
>  	int its_number;
>  	u32 ctlr;
> @@ -5032,15 +5031,15 @@ static int __init its_compute_its_list_map(struct resource *res,
>  	its_number = find_first_zero_bit(&its_list_map, GICv4_ITS_LIST_MAX);
>  	if (its_number >= GICv4_ITS_LIST_MAX) {
>  		pr_err("ITS@%pa: No ITSList entry available!\n",
> -		       &res->start);
> +		       &its->phys_base);
>  		return -EINVAL;
>  	}
>  
> -	ctlr = readl_relaxed(its_base + GITS_CTLR);
> +	ctlr = readl_relaxed(its->base + GITS_CTLR);
>  	ctlr &= ~GITS_CTLR_ITS_NUMBER;
>  	ctlr |= its_number << GITS_CTLR_ITS_NUMBER_SHIFT;
> -	writel_relaxed(ctlr, its_base + GITS_CTLR);
> -	ctlr = readl_relaxed(its_base + GITS_CTLR);
> +	writel_relaxed(ctlr, its->base + GITS_CTLR);
> +	ctlr = readl_relaxed(its->base + GITS_CTLR);
>  	if ((ctlr & GITS_CTLR_ITS_NUMBER) != (its_number << GITS_CTLR_ITS_NUMBER_SHIFT)) {
>  		its_number = ctlr & GITS_CTLR_ITS_NUMBER;
>  		its_number >>= GITS_CTLR_ITS_NUMBER_SHIFT;
> @@ -5048,75 +5047,50 @@ static int __init its_compute_its_list_map(struct resource *res,
>  
>  	if (test_and_set_bit(its_number, &its_list_map)) {
>  		pr_err("ITS@%pa: Duplicate ITSList entry %d\n",
> -		       &res->start, its_number);
> +		       &its->phys_base, its_number);
>  		return -EINVAL;
>  	}
>  
>  	return its_number;
>  }
>  
> -static int __init its_probe_one(struct resource *res,
> -				struct fwnode_handle *handle, int numa_node)
> +static int __init its_probe_one(struct its_node *its)
>  {
> -	struct its_node *its;
> -	void __iomem *its_base;
> -	u64 baser, tmp, typer;
> +	u64 baser, tmp;
>  	struct page *page;
>  	u32 ctlr;
>  	int err;
>  
> -	its_base = its_map_one(res, &err);
> -	if (!its_base)
> -		return err;
> -
> -	pr_info("ITS %pR\n", res);
> -
> -	its = kzalloc(sizeof(*its), GFP_KERNEL);
> -	if (!its) {
> -		err = -ENOMEM;
> -		goto out_unmap;
> -	}
> -
> -	raw_spin_lock_init(&its->lock);
> -	mutex_init(&its->dev_alloc_lock);
> -	INIT_LIST_HEAD(&its->entry);
> -	INIT_LIST_HEAD(&its->its_device_list);
> -	typer = gic_read_typer(its_base + GITS_TYPER);
> -	its->typer = typer;
> -	its->base = its_base;
> -	its->phys_base = res->start;
>  	if (is_v4(its)) {
> -		if (!(typer & GITS_TYPER_VMOVP)) {
> -			err = its_compute_its_list_map(res, its_base);
> +		if (!(its->typer & GITS_TYPER_VMOVP)) {
> +			err = its_compute_its_list_map(its);
>  			if (err < 0)
> -				goto out_free_its;
> +				goto out;
>  
>  			its->list_nr = err;
>  
>  			pr_info("ITS@%pa: Using ITS number %d\n",
> -				&res->start, err);
> +				&its->phys_base, err);
>  		} else {
> -			pr_info("ITS@%pa: Single VMOVP capable\n", &res->start);
> +			pr_info("ITS@%pa: Single VMOVP capable\n", &its->phys_base);
>  		}
>  
>  		if (is_v4_1(its)) {
> -			u32 svpet = FIELD_GET(GITS_TYPER_SVPET, typer);
> +			u32 svpet = FIELD_GET(GITS_TYPER_SVPET, its->typer);
>  
> -			its->sgir_base = ioremap(res->start + SZ_128K, SZ_64K);
> +			its->sgir_base = ioremap(its->phys_base + SZ_128K, SZ_64K);
>  			if (!its->sgir_base) {
>  				err = -ENOMEM;
> -				goto out_free_its;
> +				goto out;
>  			}
>  
> -			its->mpidr = readl_relaxed(its_base + GITS_MPIDR);
> +			its->mpidr = readl_relaxed(its->base + GITS_MPIDR);
>  
>  			pr_info("ITS@%pa: Using GICv4.1 mode %08x %08x\n",
> -				&res->start, its->mpidr, svpet);
> +				&its->phys_base, its->mpidr, svpet);
>  		}
>  	}
>  
> -	its->numa_node = numa_node;
> -
>  	page = alloc_pages_node(its->numa_node, GFP_KERNEL | __GFP_ZERO,
>  				get_order(ITS_CMD_QUEUE_SZ));
>  	if (!page) {
> @@ -5125,12 +5099,9 @@ static int __init its_probe_one(struct resource *res,
>  	}
>  	its->cmd_base = (void *)page_address(page);
>  	its->cmd_write = its->cmd_base;
> -	its->fwnode_handle = handle;
>  	its->get_msi_base = its_irq_get_msi_base;
>  	its->msi_domain_flags = IRQ_DOMAIN_FLAG_ISOLATED_MSI;
>  
> -	its_enable_quirks(its);
> -
>  	err = its_alloc_tables(its);
>  	if (err)
>  		goto out_free_cmd;
> @@ -5174,7 +5145,7 @@ static int __init its_probe_one(struct resource *res,
>  		ctlr |= GITS_CTLR_ImDe;
>  	writel_relaxed(ctlr, its->base + GITS_CTLR);
>  
> -	err = its_init_domain(handle, its);
> +	err = its_init_domain(its);
>  	if (err)
>  		goto out_free_tables;
>  
> @@ -5191,11 +5162,8 @@ static int __init its_probe_one(struct resource *res,
>  out_unmap_sgir:
>  	if (its->sgir_base)
>  		iounmap(its->sgir_base);
> -out_free_its:
> -	kfree(its);
> -out_unmap:
> -	iounmap(its_base);
> -	pr_err("ITS@%pa: failed probing (%d)\n", &res->start, err);
> +out:
> +	pr_err("ITS@%pa: failed probing (%d)\n", &its->phys_base, err);
>  	return err;
>  }
>  
> @@ -5356,10 +5324,53 @@ static const struct of_device_id its_device_id[] = {
>  	{},
>  };
>  
> +static struct its_node __init *its_node_init(struct resource *res,
> +					     struct fwnode_handle *handle, int numa_node)
> +{
> +	void __iomem *its_base;
> +	struct its_node *its;
> +	int err;
> +
> +	its_base = its_map_one(res, &err);
> +	if (!its_base)
> +		return NULL;
> +
> +	pr_info("ITS %pR\n", res);
> +
> +	its = kzalloc(sizeof(*its), GFP_KERNEL);
> +	if (!its)
> +		goto out_unmap;
> +
> +	raw_spin_lock_init(&its->lock);
> +	mutex_init(&its->dev_alloc_lock);
> +	INIT_LIST_HEAD(&its->entry);
> +	INIT_LIST_HEAD(&its->its_device_list);
> +
> +	its->typer = gic_read_typer(its_base + GITS_TYPER);
> +	its->base = its_base;
> +	its->phys_base = res->start;
> +
> +	its->numa_node = numa_node;
> +	its->fwnode_handle = handle;
> +
> +	return its;
> +
> +out_unmap:
> +	iounmap(its_base);
> +	return NULL;
> +}
> +
> +static void its_node_destroy(struct its_node *its)
> +{
> +	iounmap(its->base);
> +	kfree(its);
> +}
> +
>  static int __init its_of_probe(struct device_node *node)
>  {
>  	struct device_node *np;
>  	struct resource res;
> +	int err;
>  
>  	/*
>  	 * Make sure *all* the ITS are reset before we probe any, as
> @@ -5369,8 +5380,6 @@ static int __init its_of_probe(struct device_node *node)
>  	 */
>  	for (np = of_find_matching_node(node, its_device_id); np;
>  	     np = of_find_matching_node(np, its_device_id)) {
> -		int err;
> -
>  		if (!of_device_is_available(np) ||
>  		    !of_property_read_bool(np, "msi-controller") ||
>  		    of_address_to_resource(np, 0, &res))
> @@ -5383,6 +5392,8 @@ static int __init its_of_probe(struct device_node *node)
>  
>  	for (np = of_find_matching_node(node, its_device_id); np;
>  	     np = of_find_matching_node(np, its_device_id)) {
> +		struct its_node *its;
> +
>  		if (!of_device_is_available(np))
>  			continue;
>  		if (!of_property_read_bool(np, "msi-controller")) {
> @@ -5396,7 +5407,17 @@ static int __init its_of_probe(struct device_node *node)
>  			continue;
>  		}
>  
> -		its_probe_one(&res, &np->fwnode, of_node_to_nid(np));
> +
> +		its = its_node_init(&res, &np->fwnode, of_node_to_nid(np));
> +		if (!its)
> +			return -ENOMEM;
> +
> +		its_enable_quirks(its);
> +		err = its_probe_one(its);
> +		if (err)  {
> +			its_node_destroy(its);
> +			return err;
> +		}
>  	}
>  	return 0;
>  }
> @@ -5508,6 +5529,7 @@ static int __init gic_acpi_parse_madt_its(union acpi_subtable_headers *header,
>  {
>  	struct acpi_madt_generic_translator *its_entry;
>  	struct fwnode_handle *dom_handle;
> +	struct its_node *its;
>  	struct resource res;
>  	int err;
>  
> @@ -5532,11 +5554,20 @@ static int __init gic_acpi_parse_madt_its(union acpi_subtable_headers *header,
>  		goto dom_err;
>  	}
>  
> -	err = its_probe_one(&res, dom_handle,
> -			acpi_get_its_numa_node(its_entry->translation_id));
> +	its = its_node_init(&res, dom_handle,
> +			    acpi_get_its_numa_node(its_entry->translation_id));
> +	if (!its) {
> +		err = -ENOMEM;
> +		goto node_err;
> +	}
> +
> +	/* Stick ACPI quirk handling here */
> +
> +	err = its_probe_one(its);
>  	if (!err)
>  		return 0;
>  
> +node_err:
>  	iort_deregister_domain_token(its_entry->translation_id);
>  dom_err:
>  	irq_domain_free_fwnode(dom_handle);
> -- 
> 2.34.1
> 
> -- 
> Without deviation from the norm, progress is not possible.

[PATCH v2 0/2] irqchip/gic-v3: Enable non-coherent GIC designs probing

[Lorenzo Pieralisi]

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.

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      | 12 +++++++++++
 drivers/irqchip/irq-gic-common.h              |  4 ++++
 drivers/irqchip/irq-gic-v3-its.c              | 21 +++++++++++++++----
 drivers/irqchip/irq-gic-v3.c                  | 13 ++++++++++++
 4 files changed, 46 insertions(+), 4 deletions(-)

-- 
2.34.1

[PATCH v2 1/2] dt-bindings: interrupt-controller: arm,gic-v3: Add dma-noncoherent property

[Lorenzo Pieralisi]

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    | 12 ++++++++++++
 1 file changed, 12 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..c9bc9aad93f1 100644
--- a/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.yaml
+++ b/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.yaml
@@ -106,6 +106,12 @@ properties:
     $ref: /schemas/types.yaml#/definitions/uint32
     maximum: 4096
 
+  dma-noncoherent:
+    description:
+      Present if the GIC redistributors permit programming shareability
+      and cacheability attributes but are connected to a non-coherent
+      downstream interconnect.
+
   msi-controller:
     description:
       Only present if the Message Based Interrupt functionality is
@@ -193,6 +199,12 @@ patternProperties:
       compatible:
         const: arm,gic-v3-its
 
+      dma-noncoherent:
+        description:
+          Present if the GIC ITS permits programming shareability and
+          cacheability attributes but are connected to a non-coherent
+          downstream interconnect.
+
       msi-controller: true
 
       "#msi-cells":
-- 
2.34.1

Re: [PATCH v2 1/2] dt-bindings: interrupt-controller: arm,gic-v3: Add dma-noncoherent property

[Rob Herring]

On Wed, 06 Sep 2023 11:41:38 +0200, 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    | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v2 1/2] dt-bindings: interrupt-controller: arm,gic-v3: Add dma-noncoherent property

[Lorenzo Pieralisi]

On Wed, Sep 06, 2023 at 11:41:38AM +0200, 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    | 12 ++++++++++++
>  1 file changed, 12 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..c9bc9aad93f1 100644
> --- a/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.yaml
> +++ b/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.yaml
> @@ -106,6 +106,12 @@ properties:
>      $ref: /schemas/types.yaml#/definitions/uint32
>      maximum: 4096
>  
> +  dma-noncoherent:
> +    description:
> +      Present if the GIC redistributors permit programming shareability
> +      and cacheability attributes but are connected to a non-coherent
> +      downstream interconnect.
> +
>    msi-controller:
>      description:
>        Only present if the Message Based Interrupt functionality is
> @@ -193,6 +199,12 @@ patternProperties:
>        compatible:
>          const: arm,gic-v3-its
>  
> +      dma-noncoherent:
> +        description:
> +          Present if the GIC ITS permits programming shareability and
> +          cacheability attributes but are connected to a non-coherent

s/are/is

Sorry, I will update the patch accordingly.

Lorenzo

> +          downstream interconnect.
> +
>        msi-controller: true
>  
>        "#msi-cells":
> -- 
> 2.34.1
> 

[PATCH v2 2/2] irqchip/gic-v3: Enable non-coherent redistributors/ITSes probing

[Lorenzo Pieralisi]

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 through the GIC quirks mechanism.

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-common.h |  4 ++++
 drivers/irqchip/irq-gic-v3-its.c | 21 +++++++++++++++++----
 drivers/irqchip/irq-gic-v3.c     | 13 +++++++++++++
 3 files changed, 34 insertions(+), 4 deletions(-)

diff --git a/drivers/irqchip/irq-gic-common.h b/drivers/irqchip/irq-gic-common.h
index 3db4592cda1c..c4de216874db 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..adde347dc890 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,
+	},
 	{
 	}
 };
@@ -4817,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..f59ac9586b7b 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,
+	},
 	{
 	}
 };
-- 
2.34.1

Re: [PATCH v2 0/2] irqchip/gic-v3: Enable non-coherent GIC designs probing

[Marc Zyngier]

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.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v2 0/2] irqchip/gic-v3: Enable non-coherent GIC designs probing

[Lorenzo Pieralisi]

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

Re: [PATCH v2 0/2] irqchip/gic-v3: Enable non-coherent GIC designs probing

[Lorenzo Pieralisi]

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

[PATCH v3 0/5] irqchip/gic-v3: Enable non-coherent GIC designs probing

[Lorenzo Pieralisi]

This series is v3 of previous series:

v2: https://lore.kernel.org/all/20230906094139.16032-1-lpieralisi@kernel.org
v1: https://lore.kernel.org/all/20230905104721.52199-1-lpieralisi@kernel.org

v2 -> v3:
	- Added ACPICA temporary changes and ACPI changes to implement
	  ECR https://bugzilla.tianocore.org/show_bug.cgi?id=4557
	- ACPI changes are for testing purposes - subject to ECR code
	  first approval

v1 -> v2:
	- Updated DT bindings as per feedback
	- Updated patch[2] to use GIC quirks infrastructure

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.

Lorenzo Pieralisi (4):
  dt-bindings: interrupt-controller: arm,gic-v3: Add dma-noncoherent
    property
  irqchip/gic-v3: Enable non-coherent redistributors/ITSes DT probing
  ACPICA: Add new MADT GICC/GICR/ITS flags handling [code first]
  irqchip/gic-v3: Enable non-coherent redistributors/ITSes ACPI probing

Marc Zyngier (1):
  irqchip/gic-v3-its: Split allocation from initialisation of its_node

 .../interrupt-controller/arm,gic-v3.yaml      |  12 ++
 drivers/acpi/processor_core.c                 |  21 +++
 drivers/irqchip/irq-gic-common.h              |  12 ++
 drivers/irqchip/irq-gic-v3-its.c              | 174 +++++++++++-------
 drivers/irqchip/irq-gic-v3.c                  |  22 +++
 include/acpi/actbl2.h                         |  11 +-
 include/linux/acpi.h                          |   3 +
 7 files changed, 189 insertions(+), 66 deletions(-)

-- 
2.34.1

[PATCH v3 1/5] dt-bindings: interrupt-controller: arm,gic-v3: Add dma-noncoherent property

[Lorenzo Pieralisi]

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.

Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Lorenzo Pieralisi <lpieralisi@kernel.org>
Cc: Rob Herring <robh@kernel.org>
---
 .../bindings/interrupt-controller/arm,gic-v3.yaml    | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.yaml b/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.yaml
index 2bc38479a41e..0f4a062c9d6f 100644
--- a/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.yaml
+++ b/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.yaml
@@ -106,6 +106,12 @@ properties:
     $ref: /schemas/types.yaml#/definitions/uint32
     maximum: 4096
 
+  dma-noncoherent:
+    description:
+      Present if the GIC redistributors permit programming shareability
+      and cacheability attributes but are connected to a non-coherent
+      downstream interconnect.
+
   msi-controller:
     description:
       Only present if the Message Based Interrupt functionality is
@@ -193,6 +199,12 @@ patternProperties:
       compatible:
         const: arm,gic-v3-its
 
+      dma-noncoherent:
+        description:
+          Present if the GIC ITS permits programming shareability and
+          cacheability attributes but is connected to a non-coherent
+          downstream interconnect.
+
       msi-controller: true
 
       "#msi-cells":
-- 
2.34.1

[PATCH v3 2/5] irqchip/gic-v3: Enable non-coherent redistributors/ITSes DT probing

[Lorenzo Pieralisi]

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 through the GIC quirks mechanism.

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-common.h |  4 ++++
 drivers/irqchip/irq-gic-v3-its.c | 21 +++++++++++++++++----
 drivers/irqchip/irq-gic-v3.c     | 13 +++++++++++++
 3 files changed, 34 insertions(+), 4 deletions(-)

diff --git a/drivers/irqchip/irq-gic-common.h b/drivers/irqchip/irq-gic-common.h
index 3db4592cda1c..f407cce9ecaa 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..adde347dc890 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,
+	},
 	{
 	}
 };
@@ -4817,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..f59ac9586b7b 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,
+	},
 	{
 	}
 };
-- 
2.34.1

[PATCH v3 3/5] irqchip/gic-v3-its: Split allocation from initialisation of its_node

[Lorenzo Pieralisi]

From: Marc Zyngier <maz@kernel.org>

In order to pave the way for more fancy quirk handling without making
more of a mess of this terrible driver, split the allocation of the
ITS descriptor (its_node) from the actual probing.

This will allow firmware-specific hooks to be added between these
two points.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 drivers/irqchip/irq-gic-v3-its.c | 149 ++++++++++++++++++-------------
 1 file changed, 89 insertions(+), 60 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index adde347dc890..75a2dd550625 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -4965,7 +4965,7 @@ static void __init __iomem *its_map_one(struct resource *res, int *err)
 	return NULL;
 }
 
-static int its_init_domain(struct fwnode_handle *handle, struct its_node *its)
+static int its_init_domain(struct its_node *its)
 {
 	struct irq_domain *inner_domain;
 	struct msi_domain_info *info;
@@ -4979,7 +4979,7 @@ static int its_init_domain(struct fwnode_handle *handle, struct its_node *its)
 
 	inner_domain = irq_domain_create_hierarchy(its_parent,
 						   its->msi_domain_flags, 0,
-						   handle, &its_domain_ops,
+						   its->fwnode_handle, &its_domain_ops,
 						   info);
 	if (!inner_domain) {
 		kfree(info);
@@ -5030,8 +5030,7 @@ static int its_init_vpe_domain(void)
 	return 0;
 }
 
-static int __init its_compute_its_list_map(struct resource *res,
-					   void __iomem *its_base)
+static int __init its_compute_its_list_map(struct its_node *its)
 {
 	int its_number;
 	u32 ctlr;
@@ -5045,15 +5044,15 @@ static int __init its_compute_its_list_map(struct resource *res,
 	its_number = find_first_zero_bit(&its_list_map, GICv4_ITS_LIST_MAX);
 	if (its_number >= GICv4_ITS_LIST_MAX) {
 		pr_err("ITS@%pa: No ITSList entry available!\n",
-		       &res->start);
+		       &its->phys_base);
 		return -EINVAL;
 	}
 
-	ctlr = readl_relaxed(its_base + GITS_CTLR);
+	ctlr = readl_relaxed(its->base + GITS_CTLR);
 	ctlr &= ~GITS_CTLR_ITS_NUMBER;
 	ctlr |= its_number << GITS_CTLR_ITS_NUMBER_SHIFT;
-	writel_relaxed(ctlr, its_base + GITS_CTLR);
-	ctlr = readl_relaxed(its_base + GITS_CTLR);
+	writel_relaxed(ctlr, its->base + GITS_CTLR);
+	ctlr = readl_relaxed(its->base + GITS_CTLR);
 	if ((ctlr & GITS_CTLR_ITS_NUMBER) != (its_number << GITS_CTLR_ITS_NUMBER_SHIFT)) {
 		its_number = ctlr & GITS_CTLR_ITS_NUMBER;
 		its_number >>= GITS_CTLR_ITS_NUMBER_SHIFT;
@@ -5061,75 +5060,50 @@ static int __init its_compute_its_list_map(struct resource *res,
 
 	if (test_and_set_bit(its_number, &its_list_map)) {
 		pr_err("ITS@%pa: Duplicate ITSList entry %d\n",
-		       &res->start, its_number);
+		       &its->phys_base, its_number);
 		return -EINVAL;
 	}
 
 	return its_number;
 }
 
-static int __init its_probe_one(struct resource *res,
-				struct fwnode_handle *handle, int numa_node)
+static int __init its_probe_one(struct its_node *its)
 {
-	struct its_node *its;
-	void __iomem *its_base;
-	u64 baser, tmp, typer;
+	u64 baser, tmp;
 	struct page *page;
 	u32 ctlr;
 	int err;
 
-	its_base = its_map_one(res, &err);
-	if (!its_base)
-		return err;
-
-	pr_info("ITS %pR\n", res);
-
-	its = kzalloc(sizeof(*its), GFP_KERNEL);
-	if (!its) {
-		err = -ENOMEM;
-		goto out_unmap;
-	}
-
-	raw_spin_lock_init(&its->lock);
-	mutex_init(&its->dev_alloc_lock);
-	INIT_LIST_HEAD(&its->entry);
-	INIT_LIST_HEAD(&its->its_device_list);
-	typer = gic_read_typer(its_base + GITS_TYPER);
-	its->typer = typer;
-	its->base = its_base;
-	its->phys_base = res->start;
 	if (is_v4(its)) {
-		if (!(typer & GITS_TYPER_VMOVP)) {
-			err = its_compute_its_list_map(res, its_base);
+		if (!(its->typer & GITS_TYPER_VMOVP)) {
+			err = its_compute_its_list_map(its);
 			if (err < 0)
-				goto out_free_its;
+				goto out;
 
 			its->list_nr = err;
 
 			pr_info("ITS@%pa: Using ITS number %d\n",
-				&res->start, err);
+				&its->phys_base, err);
 		} else {
-			pr_info("ITS@%pa: Single VMOVP capable\n", &res->start);
+			pr_info("ITS@%pa: Single VMOVP capable\n", &its->phys_base);
 		}
 
 		if (is_v4_1(its)) {
-			u32 svpet = FIELD_GET(GITS_TYPER_SVPET, typer);
+			u32 svpet = FIELD_GET(GITS_TYPER_SVPET, its->typer);
 
-			its->sgir_base = ioremap(res->start + SZ_128K, SZ_64K);
+			its->sgir_base = ioremap(its->phys_base + SZ_128K, SZ_64K);
 			if (!its->sgir_base) {
 				err = -ENOMEM;
-				goto out_free_its;
+				goto out;
 			}
 
-			its->mpidr = readl_relaxed(its_base + GITS_MPIDR);
+			its->mpidr = readl_relaxed(its->base + GITS_MPIDR);
 
 			pr_info("ITS@%pa: Using GICv4.1 mode %08x %08x\n",
-				&res->start, its->mpidr, svpet);
+				&its->phys_base, its->mpidr, svpet);
 		}
 	}
 
-	its->numa_node = numa_node;
-
 	page = alloc_pages_node(its->numa_node, GFP_KERNEL | __GFP_ZERO,
 				get_order(ITS_CMD_QUEUE_SZ));
 	if (!page) {
@@ -5138,12 +5112,9 @@ static int __init its_probe_one(struct resource *res,
 	}
 	its->cmd_base = (void *)page_address(page);
 	its->cmd_write = its->cmd_base;
-	its->fwnode_handle = handle;
 	its->get_msi_base = its_irq_get_msi_base;
 	its->msi_domain_flags = IRQ_DOMAIN_FLAG_ISOLATED_MSI;
 
-	its_enable_quirks(its);
-
 	err = its_alloc_tables(its);
 	if (err)
 		goto out_free_cmd;
@@ -5187,7 +5158,7 @@ static int __init its_probe_one(struct resource *res,
 		ctlr |= GITS_CTLR_ImDe;
 	writel_relaxed(ctlr, its->base + GITS_CTLR);
 
-	err = its_init_domain(handle, its);
+	err = its_init_domain(its);
 	if (err)
 		goto out_free_tables;
 
@@ -5204,11 +5175,8 @@ static int __init its_probe_one(struct resource *res,
 out_unmap_sgir:
 	if (its->sgir_base)
 		iounmap(its->sgir_base);
-out_free_its:
-	kfree(its);
-out_unmap:
-	iounmap(its_base);
-	pr_err("ITS@%pa: failed probing (%d)\n", &res->start, err);
+out:
+	pr_err("ITS@%pa: failed probing (%d)\n", &its->phys_base, err);
 	return err;
 }
 
@@ -5369,10 +5337,53 @@ static const struct of_device_id its_device_id[] = {
 	{},
 };
 
+static struct its_node __init *its_node_init(struct resource *res,
+					     struct fwnode_handle *handle, int numa_node)
+{
+	void __iomem *its_base;
+	struct its_node *its;
+	int err;
+
+	its_base = its_map_one(res, &err);
+	if (!its_base)
+		return NULL;
+
+	pr_info("ITS %pR\n", res);
+
+	its = kzalloc(sizeof(*its), GFP_KERNEL);
+	if (!its)
+		goto out_unmap;
+
+	raw_spin_lock_init(&its->lock);
+	mutex_init(&its->dev_alloc_lock);
+	INIT_LIST_HEAD(&its->entry);
+	INIT_LIST_HEAD(&its->its_device_list);
+
+	its->typer = gic_read_typer(its_base + GITS_TYPER);
+	its->base = its_base;
+	its->phys_base = res->start;
+
+	its->numa_node = numa_node;
+	its->fwnode_handle = handle;
+
+	return its;
+
+out_unmap:
+	iounmap(its_base);
+	return NULL;
+}
+
+static void its_node_destroy(struct its_node *its)
+{
+	iounmap(its->base);
+	kfree(its);
+}
+
 static int __init its_of_probe(struct device_node *node)
 {
 	struct device_node *np;
 	struct resource res;
+	int err;
 
 	/*
 	 * Make sure *all* the ITS are reset before we probe any, as
@@ -5382,8 +5393,6 @@ static int __init its_of_probe(struct device_node *node)
 	 */
 	for (np = of_find_matching_node(node, its_device_id); np;
 	     np = of_find_matching_node(np, its_device_id)) {
-		int err;
-
 		if (!of_device_is_available(np) ||
 		    !of_property_read_bool(np, "msi-controller") ||
 		    of_address_to_resource(np, 0, &res))
@@ -5396,6 +5405,8 @@ static int __init its_of_probe(struct device_node *node)
 
 	for (np = of_find_matching_node(node, its_device_id); np;
 	     np = of_find_matching_node(np, its_device_id)) {
+		struct its_node *its;
+
 		if (!of_device_is_available(np))
 			continue;
 		if (!of_property_read_bool(np, "msi-controller")) {
@@ -5409,7 +5420,17 @@ static int __init its_of_probe(struct device_node *node)
 			continue;
 		}
 
-		its_probe_one(&res, &np->fwnode, of_node_to_nid(np));
+
+		its = its_node_init(&res, &np->fwnode, of_node_to_nid(np));
+		if (!its)
+			return -ENOMEM;
+
+		its_enable_quirks(its);
+		err = its_probe_one(its);
+		if (err)  {
+			its_node_destroy(its);
+			return err;
+		}
 	}
 	return 0;
 }
@@ -5521,6 +5542,7 @@ static int __init gic_acpi_parse_madt_its(union acpi_subtable_headers *header,
 {
 	struct acpi_madt_generic_translator *its_entry;
 	struct fwnode_handle *dom_handle;
+	struct its_node *its;
 	struct resource res;
 	int err;
 
@@ -5545,11 +5567,18 @@ static int __init gic_acpi_parse_madt_its(union acpi_subtable_headers *header,
 		goto dom_err;
 	}
 
-	err = its_probe_one(&res, dom_handle,
-			acpi_get_its_numa_node(its_entry->translation_id));
+	its = its_node_init(&res, dom_handle,
+			    acpi_get_its_numa_node(its_entry->translation_id));
+	if (!its) {
+		err = -ENOMEM;
+		goto node_err;
+	}
+
+	err = its_probe_one(its);
 	if (!err)
 		return 0;
 
+node_err:
 	iort_deregister_domain_token(its_entry->translation_id);
 dom_err:
 	irq_domain_free_fwnode(dom_handle);
-- 
2.34.1

Re: [PATCH v3 3/5] irqchip/gic-v3-its: Split allocation from initialisation of its_node

[Dominic Rath]

Hi,

On Fri, Oct 06, 2023 at 02:59:27PM +0200, Lorenzo Pieralisi wrote:
> From: Marc Zyngier <maz@kernel.org>
> 
> In order to pave the way for more fancy quirk handling without making
> more of a mess of this terrible driver, split the allocation of the
> ITS descriptor (its_node) from the actual probing.

it seems that this change breaks MSI-X (MSI?) reception on at least
the TI AM64x, probably most/all of TI's recent devices (K3).

These devices rely on a quirk CONFIG_SOCIONEXT_SYNQUACER_PREITS that
uses an address from the dts specified e.g. as

  socionext,synquacer-pre-its = <0x1000000 0x400000>;

to configure a MSI base address that differs from the ARM default.

With this change, the quirk still sets its->get_msi_base and clears
IRQ_DOMAIN_FLAG_ISOLATED_MSI from its->msi_domain_flags during
its_of_probe, but both get overwritten again during its_probe_one
with the defaults.

Previously the defaults would be set first and then the quirks were
applied.

I have no idea whether TI's use of this quirk was "correct", but it did
work, and since 6.6-rc6 MSI-X has been broken for us.

Best Regards,
Dominic

Re: [PATCH v3 3/5] irqchip/gic-v3-its: Split allocation from initialisation of its_node

[Marc Zyngier]

On Tue, 24 Oct 2023 09:48:31 +0100,
Dominic Rath <dominic.rath@ibv-augsburg.net> wrote:
> 
> Hi,
> 
> On Fri, Oct 06, 2023 at 02:59:27PM +0200, Lorenzo Pieralisi wrote:
> > From: Marc Zyngier <maz@kernel.org>
> > 
> > In order to pave the way for more fancy quirk handling without making
> > more of a mess of this terrible driver, split the allocation of the
> > ITS descriptor (its_node) from the actual probing.
> 
> it seems that this change breaks MSI-X (MSI?) reception on at least
> the TI AM64x, probably most/all of TI's recent devices (K3).
> 
> These devices rely on a quirk CONFIG_SOCIONEXT_SYNQUACER_PREITS that
> uses an address from the dts specified e.g. as
> 
>   socionext,synquacer-pre-its = <0x1000000 0x400000>;
> 
> to configure a MSI base address that differs from the ARM default.

<rant>
Why on Earth are people using a property that is specific to another
implementation? Not only the HW is braindead, but this is now tied to
whatever additional implementation quirks we find...

This is just dumb.
</rant>

> With this change, the quirk still sets its->get_msi_base and clears
> IRQ_DOMAIN_FLAG_ISOLATED_MSI from its->msi_domain_flags during
> its_of_probe, but both get overwritten again during its_probe_one
> with the defaults.
> 
> Previously the defaults would be set first and then the quirks were
> applied.

Yeah, that's clearly a regression, and I've confirmed it on my
Synquacer (which means the TI folks have accurately copied a dumb
idea). Can you please give the patch below a go on your system and
confirm asap whether it works for you?

> I have no idea whether TI's use of this quirk was "correct", but it did
> work, and since 6.6-rc6 MSI-X has been broken for us.

Just as for bad SW, the worse HW ideas get replicated. Then I write
bad SW for it.

Thanks,

	M.

From b5571a69f09733ecfa0c944cc48baced6590d024 Mon Sep 17 00:00:00 2001
From: Marc Zyngier <maz@kernel.org>
Date: Tue, 24 Oct 2023 11:07:34 +0100
Subject: [PATCH] irqchip/gic-v3-its: Don't override quirk settings with
 default values

When splitting the allocation of the ITS node from its configuration,
some of the default settings were kept in the latter instead of
being moved to the former.

This has the side effect of negating some of the quirk detection that
have happened in between, amongst which the dreaded Synquacer hack
(that also affect Dominic's TI platform).

Move the initialisation of these fields early, so that they can
again be overriden by the Synquacer quirk.

Fixes: 9585a495ac93 ("irqchip/gic-v3-its: Split allocation from initialisation of its_node")
Reported by: Dominic Rath <dominic.rath@ibv-augsburg.net>
Signed-off-by: Marc Zyngier <maz@kernel.org>
Link: https://lore.kernel.org/r/20231024084831.GA3788@JADEVM-DRA
---
 drivers/irqchip/irq-gic-v3-its.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 75a2dd550625..a8c89df1a997 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -5112,8 +5112,6 @@ static int __init its_probe_one(struct its_node *its)
 	}
 	its->cmd_base = (void *)page_address(page);
 	its->cmd_write = its->cmd_base;
-	its->get_msi_base = its_irq_get_msi_base;
-	its->msi_domain_flags = IRQ_DOMAIN_FLAG_ISOLATED_MSI;
 
 	err = its_alloc_tables(its);
 	if (err)
@@ -5362,6 +5360,8 @@ static struct its_node __init *its_node_init(struct resource *res,
 	its->typer = gic_read_typer(its_base + GITS_TYPER);
 	its->base = its_base;
 	its->phys_base = res->start;
+	its->get_msi_base = its_irq_get_msi_base;
+	its->msi_domain_flags = IRQ_DOMAIN_FLAG_ISOLATED_MSI;
 
 	its->numa_node = numa_node;
 	its->fwnode_handle = handle;
-- 
2.39.2


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

Re: [PATCH v3 3/5] irqchip/gic-v3-its: Split allocation from initialisation of its_node

[Dominic Rath]

On Tue, Oct 24, 2023 at 11:18:21AM +0100, Marc Zyngier wrote:
> Yeah, that's clearly a regression, and I've confirmed it on my
> Synquacer (which means the TI folks have accurately copied a dumb
> idea). Can you please give the patch below a go on your system and
> confirm asap whether it works for you?
> 

Thanks a lot, with that patch applied on top of 6.6-rc6 MSI-X interrupts
work again for the AM64x.

Best Regards,

Dominic

> > I have no idea whether TI's use of this quirk was "correct", but it did
> > work, and since 6.6-rc6 MSI-X has been broken for us.
> 
> Just as for bad SW, the worse HW ideas get replicated. Then I write
> bad SW for it.
> 
> Thanks,
> 
> 	M.
> 
> From b5571a69f09733ecfa0c944cc48baced6590d024 Mon Sep 17 00:00:00 2001
> From: Marc Zyngier <maz@kernel.org>
> Date: Tue, 24 Oct 2023 11:07:34 +0100
> Subject: [PATCH] irqchip/gic-v3-its: Don't override quirk settings with
>  default values
> 
> When splitting the allocation of the ITS node from its configuration,
> some of the default settings were kept in the latter instead of
> being moved to the former.
> 
> This has the side effect of negating some of the quirk detection that
> have happened in between, amongst which the dreaded Synquacer hack
> (that also affect Dominic's TI platform).
> 
> Move the initialisation of these fields early, so that they can
> again be overriden by the Synquacer quirk.
> 
> Fixes: 9585a495ac93 ("irqchip/gic-v3-its: Split allocation from initialisation of its_node")
> Reported by: Dominic Rath <dominic.rath@ibv-augsburg.net>
> Signed-off-by: Marc Zyngier <maz@kernel.org>

Tested-by: Dominic Rath <dominic.rath@ibv-augsburg.net>

> Link: https://lore.kernel.org/r/20231024084831.GA3788@JADEVM-DRA
> ---
>  drivers/irqchip/irq-gic-v3-its.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index 75a2dd550625..a8c89df1a997 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -5112,8 +5112,6 @@ static int __init its_probe_one(struct its_node *its)
>  	}
>  	its->cmd_base = (void *)page_address(page);
>  	its->cmd_write = its->cmd_base;
> -	its->get_msi_base = its_irq_get_msi_base;
> -	its->msi_domain_flags = IRQ_DOMAIN_FLAG_ISOLATED_MSI;
>  
>  	err = its_alloc_tables(its);
>  	if (err)
> @@ -5362,6 +5360,8 @@ static struct its_node __init *its_node_init(struct resource *res,
>  	its->typer = gic_read_typer(its_base + GITS_TYPER);
>  	its->base = its_base;
>  	its->phys_base = res->start;
> +	its->get_msi_base = its_irq_get_msi_base;
> +	its->msi_domain_flags = IRQ_DOMAIN_FLAG_ISOLATED_MSI;
>  
>  	its->numa_node = numa_node;
>  	its->fwnode_handle = handle;
> -- 
> 2.39.2
> 
> 
> -- 
> Without deviation from the norm, progress is not possible.

[PATCH v3 4/5] ACPICA: Add new MADT GICC/GICR/ITS flags handling [code first]

[Lorenzo Pieralisi]

Add new flags and related fields to the MADT GICC/GICR/ITS
structures according to the code first ECR:

https://bugzilla.tianocore.org/show_bug.cgi?id=4557

Temporary code waiting for ECR approval, for testing
purpose only - eventually ACPICA changes will trickle
into the kernel from the ACPICA project repos.

Signed-off-by: Lorenzo Pieralisi <lpieralisi@kernel.org>
---
 include/acpi/actbl2.h | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/include/acpi/actbl2.h b/include/acpi/actbl2.h
index 3751ae69432f..dd44915efd6b 100644
--- a/include/acpi/actbl2.h
+++ b/include/acpi/actbl2.h
@@ -1046,6 +1046,7 @@ struct acpi_madt_generic_interrupt {
 /* ACPI_MADT_ENABLED                    (1)      Processor is usable if set */
 #define ACPI_MADT_PERFORMANCE_IRQ_MODE  (1<<1)	/* 01: Performance Interrupt Mode */
 #define ACPI_MADT_VGIC_IRQ_MODE         (1<<2)	/* 02: VGIC Maintenance Interrupt mode */
+#define ACPI_MADT_GICC_NON_COHERENT     (1<<4)  /* 04: GIC redistributor is not coherent */
 
 /* 12: Generic Distributor (ACPI 5.0 + ACPI 6.0 changes) */
 
@@ -1090,21 +1091,27 @@ struct acpi_madt_generic_msi_frame {
 
 struct acpi_madt_generic_redistributor {
 	struct acpi_subtable_header header;
-	u16 reserved;		/* reserved - must be zero */
+	u8 flags;
+	u8 reserved;		/* reserved - must be zero */
 	u64 base_address;
 	u32 length;
 };
 
+#define ACPI_MADT_GICR_NON_COHERENT   (1)
+
 /* 15: Generic Translator (ACPI 6.0) */
 
 struct acpi_madt_generic_translator {
 	struct acpi_subtable_header header;
-	u16 reserved;		/* reserved - must be zero */
+	u8 flags;
+	u8 reserved;		/* reserved - must be zero */
 	u32 translation_id;
 	u64 base_address;
 	u32 reserved2;
 };
 
+#define ACPI_MADT_ITS_NON_COHERENT   (1)
+
 /* 16: Multiprocessor wakeup (ACPI 6.4) */
 
 struct acpi_madt_multiproc_wakeup {
-- 
2.34.1

[PATCH v3 5/5] irqchip/gic-v3: Enable non-coherent redistributors/ITSes ACPI probing

[Lorenzo Pieralisi]

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 on ACPI based systems, parse the MADT
GICC/GICR/ITS subtables non-coherent flags to determine whether the
respective components are non-coherent observers and force the shareability
attributes to be programmed into the redistributors and ITSes registers.

An ACPI global function (acpi_get_madt_revision()) is added to retrieve
the MADT revision, in that it is essential to check the MADT revision
before checking for flags that were added with MADT revision 7 so that
if the kernel is booted with ACPI tables (MADT rev < 7) it skips parsing
the newly added flags (that should be zeroed reserved values for MADT
versions < 7 but they could turn out to be buggy and should be ignored).

Signed-off-by: Lorenzo Pieralisi <lpieralisi@kernel.org>
Cc: Robin Murphy <robin.murphy@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Marc Zyngier <maz@kernel.org>
---
 drivers/acpi/processor_core.c    | 21 +++++++++++++++++++++
 drivers/irqchip/irq-gic-common.h |  8 ++++++++
 drivers/irqchip/irq-gic-v3-its.c |  4 ++++
 drivers/irqchip/irq-gic-v3.c     |  9 +++++++++
 include/linux/acpi.h             |  3 +++
 5 files changed, 45 insertions(+)

diff --git a/drivers/acpi/processor_core.c b/drivers/acpi/processor_core.c
index 7dd6dbaa98c3..d3c7c6b0bb23 100644
--- a/drivers/acpi/processor_core.c
+++ b/drivers/acpi/processor_core.c
@@ -215,6 +215,27 @@ phys_cpuid_t __init acpi_map_madt_entry(u32 acpi_id)
 	return rv;
 }
 
+u8 __init acpi_get_madt_revision(void)
+{
+	static u8 madt_revision __initdata;
+	static bool madt_read __initdata;
+	struct acpi_table_header *madt = NULL;
+
+	if (!madt_read) {
+		madt_read = true;
+
+		acpi_get_table(ACPI_SIG_MADT, 0, &madt);
+		if (!madt)
+			return madt_revision;
+
+		madt_revision = madt->revision;
+
+		acpi_put_table(madt);
+	}
+
+	return madt_revision;
+}
+
 static phys_cpuid_t map_mat_entry(acpi_handle handle, int type, u32 acpi_id)
 {
 	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
diff --git a/drivers/irqchip/irq-gic-common.h b/drivers/irqchip/irq-gic-common.h
index f407cce9ecaa..8dffee95f7e8 100644
--- a/drivers/irqchip/irq-gic-common.h
+++ b/drivers/irqchip/irq-gic-common.h
@@ -6,6 +6,7 @@
 #ifndef _IRQ_GIC_COMMON_H
 #define _IRQ_GIC_COMMON_H
 
+#include <linux/acpi.h>
 #include <linux/of.h>
 #include <linux/irqdomain.h>
 #include <linux/irqchip/arm-gic-common.h>
@@ -29,6 +30,13 @@ 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);
 
+#ifdef CONFIG_ACPI
+static inline bool gic_acpi_non_coherent_flag(u32 flags, u32 mask)
+{
+	return (acpi_get_madt_revision() >= 7) && (flags & mask);
+}
+#endif
+
 #define RDIST_FLAGS_PROPBASE_NEEDS_FLUSHING    (1 << 0)
 #define RDIST_FLAGS_RD_TABLES_PREALLOCATED     (1 << 1)
 #define RDIST_FLAGS_FORCE_NON_SHAREABLE        (1 << 2)
diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 75a2dd550625..72ae9422a26f 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -5574,6 +5574,10 @@ static int __init gic_acpi_parse_madt_its(union acpi_subtable_headers *header,
 		goto node_err;
 	}
 
+	if (gic_acpi_non_coherent_flag(its_entry->flags,
+				       ACPI_MADT_ITS_NON_COHERENT))
+		its->flags |= ITS_FLAGS_FORCE_NON_SHAREABLE;
+
 	err = its_probe_one(its);
 	if (!err)
 		return 0;
diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index f59ac9586b7b..720d76790ada 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -2364,6 +2364,11 @@ gic_acpi_parse_madt_redist(union acpi_subtable_headers *header,
 		pr_err("Couldn't map GICR region @%llx\n", redist->base_address);
 		return -ENOMEM;
 	}
+
+	if (gic_acpi_non_coherent_flag(redist->flags,
+				       ACPI_MADT_GICR_NON_COHERENT))
+		gic_data.rdists.flags |= RDIST_FLAGS_FORCE_NON_SHAREABLE;
+
 	gic_request_region(redist->base_address, redist->length, "GICR");
 
 	gic_acpi_register_redist(redist->base_address, redist_base);
@@ -2389,6 +2394,10 @@ gic_acpi_parse_madt_gicc(union acpi_subtable_headers *header,
 		return -ENOMEM;
 	gic_request_region(gicc->gicr_base_address, size, "GICR");
 
+	if (gic_acpi_non_coherent_flag(gicc->flags,
+				       ACPI_MADT_GICC_NON_COHERENT))
+		gic_data.rdists.flags |= RDIST_FLAGS_FORCE_NON_SHAREABLE;
+
 	gic_acpi_register_redist(gicc->gicr_base_address, redist_base);
 	return 0;
 }
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index a73246c3c35e..56e4e5f39a62 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -298,6 +298,9 @@ static inline bool invalid_phys_cpuid(phys_cpuid_t phys_id)
 	return phys_id == PHYS_CPUID_INVALID;
 }
 
+
+u8 __init acpi_get_madt_revision(void);
+
 /* Validate the processor object's proc_id */
 bool acpi_duplicate_processor_id(int proc_id);
 /* Processor _CTS control */
-- 
2.34.1

Re: [PATCH v3 5/5] irqchip/gic-v3: Enable non-coherent redistributors/ITSes ACPI probing

[Lorenzo Pieralisi]

On Fri, Oct 06, 2023 at 02:59:29PM +0200, Lorenzo Pieralisi 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 on ACPI based systems, parse the MADT
> GICC/GICR/ITS subtables non-coherent flags to determine whether the
> respective components are non-coherent observers and force the shareability
> attributes to be programmed into the redistributors and ITSes registers.
> 
> An ACPI global function (acpi_get_madt_revision()) is added to retrieve
> the MADT revision, in that it is essential to check the MADT revision
> before checking for flags that were added with MADT revision 7 so that
> if the kernel is booted with ACPI tables (MADT rev < 7) it skips parsing
> the newly added flags (that should be zeroed reserved values for MADT
> versions < 7 but they could turn out to be buggy and should be ignored).
> 
> Signed-off-by: Lorenzo Pieralisi <lpieralisi@kernel.org>
> Cc: Robin Murphy <robin.murphy@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Cc: Marc Zyngier <maz@kernel.org>
> ---
>  drivers/acpi/processor_core.c    | 21 +++++++++++++++++++++
>  drivers/irqchip/irq-gic-common.h |  8 ++++++++
>  drivers/irqchip/irq-gic-v3-its.c |  4 ++++
>  drivers/irqchip/irq-gic-v3.c     |  9 +++++++++
>  include/linux/acpi.h             |  3 +++
>  5 files changed, 45 insertions(+)

Hi Marc,

just a quick note to ask if, from an ACPI binding POW
this patch and related approach make sense to you.

If so, we can start the process to get the ACPI changes drafted
in:

https://bugzilla.tianocore.org/show_bug.cgi?id=4557

and deployed in this patch into the ACPI specs, I can log
an "ACK" in the tianocoreBZ entry above (we will be able to
rework the code as much as we want, this is just for the
bindings).

Thanks,
Lorenzo

> 
> diff --git a/drivers/acpi/processor_core.c b/drivers/acpi/processor_core.c
> index 7dd6dbaa98c3..d3c7c6b0bb23 100644
> --- a/drivers/acpi/processor_core.c
> +++ b/drivers/acpi/processor_core.c
> @@ -215,6 +215,27 @@ phys_cpuid_t __init acpi_map_madt_entry(u32 acpi_id)
>  	return rv;
>  }
>  
> +u8 __init acpi_get_madt_revision(void)
> +{
> +	static u8 madt_revision __initdata;
> +	static bool madt_read __initdata;
> +	struct acpi_table_header *madt = NULL;
> +
> +	if (!madt_read) {
> +		madt_read = true;
> +
> +		acpi_get_table(ACPI_SIG_MADT, 0, &madt);
> +		if (!madt)
> +			return madt_revision;
> +
> +		madt_revision = madt->revision;
> +
> +		acpi_put_table(madt);
> +	}
> +
> +	return madt_revision;
> +}
> +
>  static phys_cpuid_t map_mat_entry(acpi_handle handle, int type, u32 acpi_id)
>  {
>  	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> diff --git a/drivers/irqchip/irq-gic-common.h b/drivers/irqchip/irq-gic-common.h
> index f407cce9ecaa..8dffee95f7e8 100644
> --- a/drivers/irqchip/irq-gic-common.h
> +++ b/drivers/irqchip/irq-gic-common.h
> @@ -6,6 +6,7 @@
>  #ifndef _IRQ_GIC_COMMON_H
>  #define _IRQ_GIC_COMMON_H
>  
> +#include <linux/acpi.h>
>  #include <linux/of.h>
>  #include <linux/irqdomain.h>
>  #include <linux/irqchip/arm-gic-common.h>
> @@ -29,6 +30,13 @@ 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);
>  
> +#ifdef CONFIG_ACPI
> +static inline bool gic_acpi_non_coherent_flag(u32 flags, u32 mask)
> +{
> +	return (acpi_get_madt_revision() >= 7) && (flags & mask);
> +}
> +#endif
> +
>  #define RDIST_FLAGS_PROPBASE_NEEDS_FLUSHING    (1 << 0)
>  #define RDIST_FLAGS_RD_TABLES_PREALLOCATED     (1 << 1)
>  #define RDIST_FLAGS_FORCE_NON_SHAREABLE        (1 << 2)
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index 75a2dd550625..72ae9422a26f 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -5574,6 +5574,10 @@ static int __init gic_acpi_parse_madt_its(union acpi_subtable_headers *header,
>  		goto node_err;
>  	}
>  
> +	if (gic_acpi_non_coherent_flag(its_entry->flags,
> +				       ACPI_MADT_ITS_NON_COHERENT))
> +		its->flags |= ITS_FLAGS_FORCE_NON_SHAREABLE;
> +
>  	err = its_probe_one(its);
>  	if (!err)
>  		return 0;
> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> index f59ac9586b7b..720d76790ada 100644
> --- a/drivers/irqchip/irq-gic-v3.c
> +++ b/drivers/irqchip/irq-gic-v3.c
> @@ -2364,6 +2364,11 @@ gic_acpi_parse_madt_redist(union acpi_subtable_headers *header,
>  		pr_err("Couldn't map GICR region @%llx\n", redist->base_address);
>  		return -ENOMEM;
>  	}
> +
> +	if (gic_acpi_non_coherent_flag(redist->flags,
> +				       ACPI_MADT_GICR_NON_COHERENT))
> +		gic_data.rdists.flags |= RDIST_FLAGS_FORCE_NON_SHAREABLE;
> +
>  	gic_request_region(redist->base_address, redist->length, "GICR");
>  
>  	gic_acpi_register_redist(redist->base_address, redist_base);
> @@ -2389,6 +2394,10 @@ gic_acpi_parse_madt_gicc(union acpi_subtable_headers *header,
>  		return -ENOMEM;
>  	gic_request_region(gicc->gicr_base_address, size, "GICR");
>  
> +	if (gic_acpi_non_coherent_flag(gicc->flags,
> +				       ACPI_MADT_GICC_NON_COHERENT))
> +		gic_data.rdists.flags |= RDIST_FLAGS_FORCE_NON_SHAREABLE;
> +
>  	gic_acpi_register_redist(gicc->gicr_base_address, redist_base);
>  	return 0;
>  }
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index a73246c3c35e..56e4e5f39a62 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -298,6 +298,9 @@ static inline bool invalid_phys_cpuid(phys_cpuid_t phys_id)
>  	return phys_id == PHYS_CPUID_INVALID;
>  }
>  
> +
> +u8 __init acpi_get_madt_revision(void);
> +
>  /* Validate the processor object's proc_id */
>  bool acpi_duplicate_processor_id(int proc_id);
>  /* Processor _CTS control */
> -- 
> 2.34.1
> 

Re: [PATCH v3 5/5] irqchip/gic-v3: Enable non-coherent redistributors/ITSes ACPI probing

[Marc Zyngier]

On Tue, 17 Oct 2023 15:19:46 +0100,
Lorenzo Pieralisi <lpieralisi@kernel.org> wrote:
> 
> On Fri, Oct 06, 2023 at 02:59:29PM +0200, Lorenzo Pieralisi 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 on ACPI based systems, parse the MADT
> > GICC/GICR/ITS subtables non-coherent flags to determine whether the
> > respective components are non-coherent observers and force the shareability
> > attributes to be programmed into the redistributors and ITSes registers.
> > 
> > An ACPI global function (acpi_get_madt_revision()) is added to retrieve
> > the MADT revision, in that it is essential to check the MADT revision
> > before checking for flags that were added with MADT revision 7 so that
> > if the kernel is booted with ACPI tables (MADT rev < 7) it skips parsing
> > the newly added flags (that should be zeroed reserved values for MADT
> > versions < 7 but they could turn out to be buggy and should be ignored).
> > 
> > Signed-off-by: Lorenzo Pieralisi <lpieralisi@kernel.org>
> > Cc: Robin Murphy <robin.murphy@arm.com>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> > Cc: Marc Zyngier <maz@kernel.org>
> > ---
> >  drivers/acpi/processor_core.c    | 21 +++++++++++++++++++++
> >  drivers/irqchip/irq-gic-common.h |  8 ++++++++
> >  drivers/irqchip/irq-gic-v3-its.c |  4 ++++
> >  drivers/irqchip/irq-gic-v3.c     |  9 +++++++++
> >  include/linux/acpi.h             |  3 +++
> >  5 files changed, 45 insertions(+)
> 
> Hi Marc,
> 
> just a quick note to ask if, from an ACPI binding POW

I guess you mean POV. POW has an entirely different meaning... :-/

> this patch and related approach make sense to you.
> 
> If so, we can start the process to get the ACPI changes drafted
> in:
> 
> https://bugzilla.tianocore.org/show_bug.cgi?id=4557
> 
> and deployed in this patch into the ACPI specs, I can log
> an "ACK" in the tianocoreBZ entry above (we will be able to
> rework the code as much as we want, this is just for the
> bindings).

I'm OK with the overall shape of it. However, I wonder what the
rationale is for spreading the redistributor property all over the map
(in both GICC and GICR structures), while it could be set once and for
all in the core MADT flags (32 bits, of which only one is in use).

It is bad enough that there are two ways of getting the GICR regions.
Why can't the properties that apply to all of the be common?

Thanks,

	M.

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

Re: [PATCH v3 5/5] irqchip/gic-v3: Enable non-coherent redistributors/ITSes ACPI probing

[Lorenzo Pieralisi]

On Tue, Oct 17, 2023 at 05:44:28PM +0100, Marc Zyngier wrote:
> On Tue, 17 Oct 2023 15:19:46 +0100,
> Lorenzo Pieralisi <lpieralisi@kernel.org> wrote:
> > 
> > On Fri, Oct 06, 2023 at 02:59:29PM +0200, Lorenzo Pieralisi 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 on ACPI based systems, parse the MADT
> > > GICC/GICR/ITS subtables non-coherent flags to determine whether the
> > > respective components are non-coherent observers and force the shareability
> > > attributes to be programmed into the redistributors and ITSes registers.
> > > 
> > > An ACPI global function (acpi_get_madt_revision()) is added to retrieve
> > > the MADT revision, in that it is essential to check the MADT revision
> > > before checking for flags that were added with MADT revision 7 so that
> > > if the kernel is booted with ACPI tables (MADT rev < 7) it skips parsing
> > > the newly added flags (that should be zeroed reserved values for MADT
> > > versions < 7 but they could turn out to be buggy and should be ignored).
> > > 
> > > Signed-off-by: Lorenzo Pieralisi <lpieralisi@kernel.org>
> > > Cc: Robin Murphy <robin.murphy@arm.com>
> > > Cc: Mark Rutland <mark.rutland@arm.com>
> > > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> > > Cc: Marc Zyngier <maz@kernel.org>
> > > ---
> > >  drivers/acpi/processor_core.c    | 21 +++++++++++++++++++++
> > >  drivers/irqchip/irq-gic-common.h |  8 ++++++++
> > >  drivers/irqchip/irq-gic-v3-its.c |  4 ++++
> > >  drivers/irqchip/irq-gic-v3.c     |  9 +++++++++
> > >  include/linux/acpi.h             |  3 +++
> > >  5 files changed, 45 insertions(+)
> > 
> > Hi Marc,
> > 
> > just a quick note to ask if, from an ACPI binding POW
> 
> I guess you mean POV. POW has an entirely different meaning... :-/
> 
> > this patch and related approach make sense to you.
> > 
> > If so, we can start the process to get the ACPI changes drafted
> > in:
> > 
> > https://bugzilla.tianocore.org/show_bug.cgi?id=4557
> > 
> > and deployed in this patch into the ACPI specs, I can log
> > an "ACK" in the tianocoreBZ entry above (we will be able to
> > rework the code as much as we want, this is just for the
> > bindings).
> 
> I'm OK with the overall shape of it. However, I wonder what the
> rationale is for spreading the redistributor property all over the map
> (in both GICC and GICR structures), while it could be set once and for
> all in the core MADT flags (32 bits, of which only one is in use).
> 
> It is bad enough that there are two ways of getting the GICR regions.
> Why can't the properties that apply to all of the be common?

I don't think we are allowed to add arch specific flags to the MADT
since those, supposedly, are cross-architecture (and the only one
defined is quite old, though x86 specific).

The reason behind spreading the property is the nature of GICC/GICR
subtables themselves - we wanted to apply flags only in subtables
relevant to the components in question.

We could try to add a global flag to the MADT but I would not be
surprised if the ECR would be rejected then for the reason I explained
above.

Thanks,
Lorenzo

Re: [PATCH v3 5/5] irqchip/gic-v3: Enable non-coherent redistributors/ITSes ACPI probing

[Marc Zyngier]

On Wed, 18 Oct 2023 09:42:14 +0100,
Lorenzo Pieralisi <lpieralisi@kernel.org> wrote:
> 
> On Tue, Oct 17, 2023 at 05:44:28PM +0100, Marc Zyngier wrote:
> > On Tue, 17 Oct 2023 15:19:46 +0100,
> > Lorenzo Pieralisi <lpieralisi@kernel.org> wrote:
> > > 
> > > On Fri, Oct 06, 2023 at 02:59:29PM +0200, Lorenzo Pieralisi 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 on ACPI based systems, parse the MADT
> > > > GICC/GICR/ITS subtables non-coherent flags to determine whether the
> > > > respective components are non-coherent observers and force the shareability
> > > > attributes to be programmed into the redistributors and ITSes registers.
> > > > 
> > > > An ACPI global function (acpi_get_madt_revision()) is added to retrieve
> > > > the MADT revision, in that it is essential to check the MADT revision
> > > > before checking for flags that were added with MADT revision 7 so that
> > > > if the kernel is booted with ACPI tables (MADT rev < 7) it skips parsing
> > > > the newly added flags (that should be zeroed reserved values for MADT
> > > > versions < 7 but they could turn out to be buggy and should be ignored).
> > > > 
> > > > Signed-off-by: Lorenzo Pieralisi <lpieralisi@kernel.org>
> > > > Cc: Robin Murphy <robin.murphy@arm.com>
> > > > Cc: Mark Rutland <mark.rutland@arm.com>
> > > > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> > > > Cc: Marc Zyngier <maz@kernel.org>
> > > > ---
> > > >  drivers/acpi/processor_core.c    | 21 +++++++++++++++++++++
> > > >  drivers/irqchip/irq-gic-common.h |  8 ++++++++
> > > >  drivers/irqchip/irq-gic-v3-its.c |  4 ++++
> > > >  drivers/irqchip/irq-gic-v3.c     |  9 +++++++++
> > > >  include/linux/acpi.h             |  3 +++
> > > >  5 files changed, 45 insertions(+)
> > > 
> > > Hi Marc,
> > > 
> > > just a quick note to ask if, from an ACPI binding POW
> > 
> > I guess you mean POV. POW has an entirely different meaning... :-/
> > 
> > > this patch and related approach make sense to you.
> > > 
> > > If so, we can start the process to get the ACPI changes drafted
> > > in:
> > > 
> > > https://bugzilla.tianocore.org/show_bug.cgi?id=4557
> > > 
> > > and deployed in this patch into the ACPI specs, I can log
> > > an "ACK" in the tianocoreBZ entry above (we will be able to
> > > rework the code as much as we want, this is just for the
> > > bindings).
> > 
> > I'm OK with the overall shape of it. However, I wonder what the
> > rationale is for spreading the redistributor property all over the map
> > (in both GICC and GICR structures), while it could be set once and for
> > all in the core MADT flags (32 bits, of which only one is in use).
> > 
> > It is bad enough that there are two ways of getting the GICR regions.
> > Why can't the properties that apply to all of the be common?
> 
> I don't think we are allowed to add arch specific flags to the MADT
> since those, supposedly, are cross-architecture (and the only one
> defined is quite old, though x86 specific).

There is nothing that is truly cross-arch in this table. *everything*
in MADT is arch-specific.

> The reason behind spreading the property is the nature of GICC/GICR
> subtables themselves - we wanted to apply flags only in subtables
> relevant to the components in question.
>
> We could try to add a global flag to the MADT but I would not be
> surprised if the ECR would be rejected then for the reason I explained
> above.

I don't think that's much of a reason, but I really don't care enough
about this to argue otherwise.

	M.

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