mbox series

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

Message ID 20231006125929.48591-1-lpieralisi@kernel.org
Headers show
Series irqchip/gic-v3: Enable non-coherent GIC designs probing | expand

Message

Lorenzo Pieralisi Oct. 6, 2023, 12:59 p.m. UTC
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(-)

Comments

Lorenzo Pieralisi Oct. 17, 2023, 2:19 p.m. UTC | #1
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
>
Marc Zyngier Oct. 17, 2023, 4:44 p.m. UTC | #2
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.
Lorenzo Pieralisi Oct. 18, 2023, 8:42 a.m. UTC | #3
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
Marc Zyngier Oct. 19, 2023, 11:12 a.m. UTC | #4
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.
Dominic Rath Oct. 24, 2023, 8:48 a.m. UTC | #5
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
Dominic Rath Oct. 24, 2023, 1:13 p.m. UTC | #6
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.