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
This series is v3 of previous series:
v2: https://lore.kernel.org/all/[email protected]
v1: https://lore.kernel.org/all/[email protected]
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
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 <[email protected]>
Signed-off-by: Lorenzo Pieralisi <[email protected]>
Cc: Rob Herring <[email protected]>
---
.../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
From: Marc Zyngier <[email protected]>
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 <[email protected]>
---
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
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 <[email protected]>
Cc: Robin Murphy <[email protected]>
Cc: Mark Rutland <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: Marc Zyngier <[email protected]>
---
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
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 <[email protected]>
Cc: Robin Murphy <[email protected]>
Cc: Mark Rutland <[email protected]>
Cc: Marc Zyngier <[email protected]>
---
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
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 <[email protected]>
---
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
The following commit has been merged into the irq/irqchip-fixes branch of irqchip:
Commit-ID: 3a0fff0fb6a3861fa05416f21858cf0c75cbf944
Gitweb: https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms/3a0fff0fb6a3861fa05416f21858cf0c75cbf944
Author: Lorenzo Pieralisi <[email protected]>
AuthorDate: Fri, 06 Oct 2023 14:59:26 +02:00
Committer: Marc Zyngier <[email protected]>
CommitterDate: Sat, 07 Oct 2023 12:47:12 +01:00
irqchip/gic-v3: Enable non-coherent redistributors/ITSes DT probing
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 <[email protected]>
Cc: Robin Murphy <[email protected]>
Cc: Mark Rutland <[email protected]>
Cc: Marc Zyngier <[email protected]>
Signed-off-by: Marc Zyngier <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
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 3db4592..f407cce 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 5e57b60..75a2dd5 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
{
@@ -4809,6 +4813,11 @@ static const struct gic_quirk its_quirks[] = {
},
#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 eedfa8e..f59ac95 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",
@@ -1924,6 +1932,11 @@ static const struct gic_quirk gic_quirks[] = {
.init = gic_enable_quirk_arm64_2941627,
},
{
+ .desc = "GICv3: non-coherent attribute",
+ .property = "dma-noncoherent",
+ .init = rd_set_non_coherent,
+ },
+ {
}
};
The following commit has been merged into the irq/irqchip-fixes branch of irqchip:
Commit-ID: 9585a495ac936049dba141e8f9d99159ca06d46a
Gitweb: https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms/9585a495ac936049dba141e8f9d99159ca06d46a
Author: Marc Zyngier <[email protected]>
AuthorDate: Fri, 06 Oct 2023 14:59:27 +02:00
Committer: Marc Zyngier <[email protected]>
CommitterDate: Sat, 07 Oct 2023 12:47:12 +01:00
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 <[email protected]>
Signed-off-by: Lorenzo Pieralisi <[email protected]>
Signed-off-by: Marc Zyngier <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
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 e0c2b10..5e57b60 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -4952,7 +4952,7 @@ out_unmap:
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 @@ out_free_cmd:
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,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);
The following commit has been merged into the irq/irqchip-fixes branch of irqchip:
Commit-ID: 5e5c636c69bdba04033161bbb111fbb6f1f6661e
Gitweb: https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms/5e5c636c69bdba04033161bbb111fbb6f1f6661e
Author: Lorenzo Pieralisi <[email protected]>
AuthorDate: Fri, 06 Oct 2023 14:59:25 +02:00
Committer: Marc Zyngier <[email protected]>
CommitterDate: Sat, 07 Oct 2023 12:47:12 +01:00
dt-bindings: interrupt-controller: arm,gic-v3: Add dma-noncoherent property
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 <[email protected]>
Signed-off-by: Lorenzo Pieralisi <[email protected]>
Cc: Rob Herring <[email protected]>
Signed-off-by: Marc Zyngier <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
Documentation/devicetree/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 2bc3847..0f4a062 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":
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 <[email protected]>
> Cc: Robin Murphy <[email protected]>
> Cc: Mark Rutland <[email protected]>
> Cc: "Rafael J. Wysocki" <[email protected]>
> Cc: Marc Zyngier <[email protected]>
> ---
> 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
>
On Tue, 17 Oct 2023 15:19:46 +0100,
Lorenzo Pieralisi <[email protected]> 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 <[email protected]>
> > Cc: Robin Murphy <[email protected]>
> > Cc: Mark Rutland <[email protected]>
> > Cc: "Rafael J. Wysocki" <[email protected]>
> > Cc: Marc Zyngier <[email protected]>
> > ---
> > 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.
On Tue, Oct 17, 2023 at 05:44:28PM +0100, Marc Zyngier wrote:
> On Tue, 17 Oct 2023 15:19:46 +0100,
> Lorenzo Pieralisi <[email protected]> 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 <[email protected]>
> > > Cc: Robin Murphy <[email protected]>
> > > Cc: Mark Rutland <[email protected]>
> > > Cc: "Rafael J. Wysocki" <[email protected]>
> > > Cc: Marc Zyngier <[email protected]>
> > > ---
> > > 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
On Wed, 18 Oct 2023 09:42:14 +0100,
Lorenzo Pieralisi <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > > Cc: Robin Murphy <[email protected]>
> > > > Cc: Mark Rutland <[email protected]>
> > > > Cc: "Rafael J. Wysocki" <[email protected]>
> > > > Cc: Marc Zyngier <[email protected]>
> > > > ---
> > > > 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.
Hi,
On Fri, Oct 06, 2023 at 02:59:27PM +0200, Lorenzo Pieralisi wrote:
> From: Marc Zyngier <[email protected]>
>
> 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
On Tue, 24 Oct 2023 09:48:31 +0100,
Dominic Rath <[email protected]> wrote:
>
> Hi,
>
> On Fri, Oct 06, 2023 at 02:59:27PM +0200, Lorenzo Pieralisi wrote:
> > From: Marc Zyngier <[email protected]>
> >
> > 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 <[email protected]>
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 <[email protected]>
Signed-off-by: Marc Zyngier <[email protected]>
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.
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 <[email protected]>
> 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 <[email protected]>
> Signed-off-by: Marc Zyngier <[email protected]>
Tested-by: Dominic Rath <[email protected]>
> 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.
The following commit has been merged into the irq/urgent branch of tip:
Commit-ID: f199bf5bf84c19a4f488a39d7d694ab10787de35
Gitweb: https://git.kernel.org/tip/f199bf5bf84c19a4f488a39d7d694ab10787de35
Author: Marc Zyngier <[email protected]>
AuthorDate: Tue, 24 Oct 2023 15:34:31 +01:00
Committer: Thomas Gleixner <[email protected]>
CommitterDate: Wed, 25 Oct 2023 21:44:49 +02:00
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 detections 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 <[email protected]>
Signed-off-by: Marc Zyngier <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
Tested-by: Dominic Rath <[email protected]>
Link: https://lore.kernel.org/r/20231024084831.GA3788@JADEVM-DRA
Link: https://lore.kernel.org/r/[email protected]
---
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 75a2dd5..a8c89df 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;
This series is v4 of previous series:
v3: https://lore.kernel.org/all/[email protected]
v2: https://lore.kernel.org/all/[email protected]
v1: https://lore.kernel.org/all/[email protected]
v3 -> v4:
- Dropped patches [1-3], already merged
- Added Linuxized ACPICA changes accepted upstream
- Rebased against v6.7-rc3
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 (3):
ACPICA: MADT: Add GICC online capable bit handling
ACPICA: MADT: Add new MADT GICC/GICR/ITS non-coherent flags handling
irqchip/gic-v3: Enable non-coherent redistributors/ITSes ACPI probing
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/acpi/actbl2.h | 12 ++++++++++--
include/linux/acpi.h | 3 +++
6 files changed, 55 insertions(+), 2 deletions(-)
--
2.34.1
ACPICA commit 16f0befdeddf25756f317907798192bbaa417e5e
Implement code to handle the GICC online capable bit management
added into ACPI v6.5.
Link: https://github.com/acpica/acpica/commit/16f0befd
Signed-off-by: Lorenzo Pieralisi <[email protected]>
Cc: Robert Moore <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
---
include/acpi/actbl2.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/include/acpi/actbl2.h b/include/acpi/actbl2.h
index 3751ae69432f..2b4dd2c3348f 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_ONLINE_CAPABLE (1<<3) /* 03: Processor is online capable */
/* 12: Generic Distributor (ACPI 5.0 + ACPI 6.0 changes) */
--
2.34.1
ACPICA commit c5d2010744b1bf7efba0bd04a8a9c200ef8fb610
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
Update the MADT template to the latest MADT revision.
Link: https://github.com/acpica/acpica/commit/c5d20107
Signed-off-by: Lorenzo Pieralisi <[email protected]>
Cc: Robert Moore <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
---
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 2b4dd2c3348f..9775384d61c6 100644
--- a/include/acpi/actbl2.h
+++ b/include/acpi/actbl2.h
@@ -1047,6 +1047,7 @@ struct acpi_madt_generic_interrupt {
#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_ONLINE_CAPABLE (1<<3) /* 03: Processor is online capable */
+#define ACPI_MADT_GICC_NON_COHERENT (1<<4) /* 04: GIC redistributor is not coherent */
/* 12: Generic Distributor (ACPI 5.0 + ACPI 6.0 changes) */
@@ -1091,21 +1092,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
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 <[email protected]>
Cc: Robin Murphy <[email protected]>
Cc: Mark Rutland <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: Marc Zyngier <[email protected]>
---
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 b203cfe28550..c253d151275e 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 9a7a74239eab..8d088fca65a1 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -5578,6 +5578,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 98b0329b7154..48e02838fdc8 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -2356,6 +2356,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);
@@ -2380,6 +2385,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 54189e0e5f41..a292f2bdb693 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -283,6 +283,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
On Wed, Dec 27, 2023 at 12:00 PM Lorenzo Pieralisi
<[email protected]> wrote:
>
> This series is v4 of previous series:
>
> v3: https://lore.kernel.org/all/[email protected]
> v2: https://lore.kernel.org/all/[email protected]
> v1: https://lore.kernel.org/all/[email protected]
>
> v3 -> v4:
> - Dropped patches [1-3], already merged
> - Added Linuxized ACPICA changes accepted upstream
> - Rebased against v6.7-rc3
>
> 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 (3):
> ACPICA: MADT: Add GICC online capable bit handling
> ACPICA: MADT: Add new MADT GICC/GICR/ITS non-coherent flags handling
> irqchip/gic-v3: Enable non-coherent redistributors/ITSes ACPI probing
>
> 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/acpi/actbl2.h | 12 ++++++++++--
> include/linux/acpi.h | 3 +++
> 6 files changed, 55 insertions(+), 2 deletions(-)
>
> --
I can apply the first 2 patches, but I would need an ACK for the 3rd one.
Alternatively, feel free to add
Acked-by: Rafael J. Wysocki <[email protected]>
to the first 2 patches and route them via ARM64.
Thanks!
On Wed, 27 Dec 2023 11:00:38 +0000,
Lorenzo Pieralisi <[email protected]> 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
IWB/OWB *barriers*? Unless you're talking about something else,
IWB/OWB refers to cacheability, and only that.
> 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 <[email protected]>
> Cc: Robin Murphy <[email protected]>
> Cc: Mark Rutland <[email protected]>
> Cc: "Rafael J. Wysocki" <[email protected]>
> Cc: Marc Zyngier <[email protected]>
> ---
> 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 b203cfe28550..c253d151275e 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;
Huh. Why do we need this hack? What's the issue with accessing the
MADT? Can it disappear from under our feet? While we're walking it?
> +
> + acpi_get_table(ACPI_SIG_MADT, 0, &madt);
> + if (!madt)
> + return madt_revision;
What does this mean? Can we have a revision 0 of MADT?
> +
> + 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);
> +}
Given that this checks *any* flag (or a combination of flags), the
name of the helper is extremely misleading. Also, GICC flags are not
necessarily tied to revision 7 of MADT.
To be honest, I don't think this helper bring much, and I'd rather see
an explicit check (or 3) for the revision in the driver code.
> +#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 9a7a74239eab..8d088fca65a1 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -5578,6 +5578,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 98b0329b7154..48e02838fdc8 100644
> --- a/drivers/irqchip/irq-gic-v3.c
> +++ b/drivers/irqchip/irq-gic-v3.c
> @@ -2356,6 +2356,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);
> @@ -2380,6 +2385,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 54189e0e5f41..a292f2bdb693 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -283,6 +283,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 */
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
On Wed, 03 Jan 2024 13:43:16 +0000,
"Rafael J. Wysocki" <[email protected]> wrote:
>
> On Wed, Dec 27, 2023 at 12:00 PM Lorenzo Pieralisi
> <[email protected]> wrote:
> >
> > This series is v4 of previous series:
> >
> > v3: https://lore.kernel.org/all/[email protected]
> > v2: https://lore.kernel.org/all/[email protected]
> > v1: https://lore.kernel.org/all/[email protected]
> >
> > v3 -> v4:
> > - Dropped patches [1-3], already merged
> > - Added Linuxized ACPICA changes accepted upstream
> > - Rebased against v6.7-rc3
> >
> > 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 (3):
> > ACPICA: MADT: Add GICC online capable bit handling
> > ACPICA: MADT: Add new MADT GICC/GICR/ITS non-coherent flags handling
> > irqchip/gic-v3: Enable non-coherent redistributors/ITSes ACPI probing
> >
> > 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/acpi/actbl2.h | 12 ++++++++++--
> > include/linux/acpi.h | 3 +++
> > 6 files changed, 55 insertions(+), 2 deletions(-)
> >
> > --
>
> I can apply the first 2 patches, but I would need an ACK for the 3rd one.
>
> Alternatively, feel free to add
>
> Acked-by: Rafael J. Wysocki <[email protected]>
>
> to the first 2 patches and route them via ARM64.
Thanks for that. I have some comments on the third patch, which I'd
like to see addressed beforehand. This is probably all 6.9 material
anyway (nobody is affected by this so far).
M.
--
Without deviation from the norm, progress is not possible.
On Thu, Jan 04, 2024 at 11:34:48AM +0000, Marc Zyngier wrote:
> On Wed, 03 Jan 2024 13:43:16 +0000,
> "Rafael J. Wysocki" <[email protected]> wrote:
> >
> > On Wed, Dec 27, 2023 at 12:00 PM Lorenzo Pieralisi
> > <[email protected]> wrote:
> > >
> > > This series is v4 of previous series:
> > >
> > > v3: https://lore.kernel.org/all/[email protected]
> > > v2: https://lore.kernel.org/all/[email protected]
> > > v1: https://lore.kernel.org/all/[email protected]
> > >
> > > v3 -> v4:
> > > - Dropped patches [1-3], already merged
> > > - Added Linuxized ACPICA changes accepted upstream
> > > - Rebased against v6.7-rc3
> > >
> > > 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 (3):
> > > ACPICA: MADT: Add GICC online capable bit handling
> > > ACPICA: MADT: Add new MADT GICC/GICR/ITS non-coherent flags handling
> > > irqchip/gic-v3: Enable non-coherent redistributors/ITSes ACPI probing
> > >
> > > 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/acpi/actbl2.h | 12 ++++++++++--
> > > include/linux/acpi.h | 3 +++
> > > 6 files changed, 55 insertions(+), 2 deletions(-)
> > >
> > > --
> >
> > I can apply the first 2 patches, but I would need an ACK for the 3rd one.
> >
> > Alternatively, feel free to add
> >
> > Acked-by: Rafael J. Wysocki <[email protected]>
> >
> > to the first 2 patches and route them via ARM64.
>
> Thanks for that. I have some comments on the third patch, which I'd
> like to see addressed beforehand. This is probably all 6.9 material
> anyway (nobody is affected by this so far).
From a purely selfish point of view, it would be useful to have the
first patch merged merely to reduce the burden of patches for vcpu
hotplug.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Thu, Jan 4, 2024 at 1:04 PM Russell King (Oracle)
<[email protected]> wrote:
>
> On Thu, Jan 04, 2024 at 11:34:48AM +0000, Marc Zyngier wrote:
> > On Wed, 03 Jan 2024 13:43:16 +0000,
> > "Rafael J. Wysocki" <[email protected]> wrote:
> > >
> > > On Wed, Dec 27, 2023 at 12:00 PM Lorenzo Pieralisi
> > > <[email protected]> wrote:
> > > >
> > > > This series is v4 of previous series:
> > > >
> > > > v3: https://lore.kernel.org/all/[email protected]
> > > > v2: https://lore.kernel.org/all/[email protected]
> > > > v1: https://lore.kernel.org/all/[email protected]
> > > >
> > > > v3 -> v4:
> > > > - Dropped patches [1-3], already merged
> > > > - Added Linuxized ACPICA changes accepted upstream
> > > > - Rebased against v6.7-rc3
> > > >
> > > > 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 (3):
> > > > ACPICA: MADT: Add GICC online capable bit handling
> > > > ACPICA: MADT: Add new MADT GICC/GICR/ITS non-coherent flags handling
> > > > irqchip/gic-v3: Enable non-coherent redistributors/ITSes ACPI probing
> > > >
> > > > 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/acpi/actbl2.h | 12 ++++++++++--
> > > > include/linux/acpi.h | 3 +++
> > > > 6 files changed, 55 insertions(+), 2 deletions(-)
> > > >
> > > > --
> > >
> > > I can apply the first 2 patches, but I would need an ACK for the 3rd one.
> > >
> > > Alternatively, feel free to add
> > >
> > > Acked-by: Rafael J. Wysocki <[email protected]>
> > >
> > > to the first 2 patches and route them via ARM64.
> >
> > Thanks for that. I have some comments on the third patch, which I'd
> > like to see addressed beforehand. This is probably all 6.9 material
> > anyway (nobody is affected by this so far).
>
> From a purely selfish point of view, it would be useful to have the
> first patch merged merely to reduce the burden of patches for vcpu
> hotplug.
OK, since there will be at least one more iteration of patch [3/3]
AFAICS, I'll queue up patches [1-2/3] for 6.8 (next week, though).
On Thu, Jan 04, 2024 at 02:21:44PM +0100, Rafael J. Wysocki wrote:
> On Thu, Jan 4, 2024 at 1:04 PM Russell King (Oracle)
> <[email protected]> wrote:
> >
> > On Thu, Jan 04, 2024 at 11:34:48AM +0000, Marc Zyngier wrote:
> > > On Wed, 03 Jan 2024 13:43:16 +0000,
> > > "Rafael J. Wysocki" <[email protected]> wrote:
> > > >
> > > > On Wed, Dec 27, 2023 at 12:00 PM Lorenzo Pieralisi
> > > > <[email protected]> wrote:
> > > > >
> > > > > This series is v4 of previous series:
> > > > >
> > > > > v3: https://lore.kernel.org/all/[email protected]
> > > > > v2: https://lore.kernel.org/all/[email protected]
> > > > > v1: https://lore.kernel.org/all/[email protected]
> > > > >
> > > > > v3 -> v4:
> > > > > - Dropped patches [1-3], already merged
> > > > > - Added Linuxized ACPICA changes accepted upstream
> > > > > - Rebased against v6.7-rc3
> > > > >
> > > > > 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 (3):
> > > > > ACPICA: MADT: Add GICC online capable bit handling
> > > > > ACPICA: MADT: Add new MADT GICC/GICR/ITS non-coherent flags handling
> > > > > irqchip/gic-v3: Enable non-coherent redistributors/ITSes ACPI probing
> > > > >
> > > > > 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/acpi/actbl2.h | 12 ++++++++++--
> > > > > include/linux/acpi.h | 3 +++
> > > > > 6 files changed, 55 insertions(+), 2 deletions(-)
> > > > >
> > > > > --
> > > >
> > > > I can apply the first 2 patches, but I would need an ACK for the 3rd one.
> > > >
> > > > Alternatively, feel free to add
> > > >
> > > > Acked-by: Rafael J. Wysocki <[email protected]>
> > > >
> > > > to the first 2 patches and route them via ARM64.
> > >
> > > Thanks for that. I have some comments on the third patch, which I'd
> > > like to see addressed beforehand. This is probably all 6.9 material
> > > anyway (nobody is affected by this so far).
> >
> > From a purely selfish point of view, it would be useful to have the
> > first patch merged merely to reduce the burden of patches for vcpu
> > hotplug.
>
> OK, since there will be at least one more iteration of patch [3/3]
> AFAICS, I'll queue up patches [1-2/3] for 6.8 (next week, though).
Thanks!
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Thu, Jan 04, 2024 at 11:12:28AM +0000, Marc Zyngier wrote:
> On Wed, 27 Dec 2023 11:00:38 +0000,
> Lorenzo Pieralisi <[email protected]> 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
>
> IWB/OWB *barriers*? Unless you're talking about something else,
> IWB/OWB refers to cacheability, and only that.
Yes, it should be expressed differently. Unfortunately this sentence made
it into the kernel with the DT counterpart - commit 3a0fff0fb6a3 log,
apologies.
> > 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 <[email protected]>
> > Cc: Robin Murphy <[email protected]>
> > Cc: Mark Rutland <[email protected]>
> > Cc: "Rafael J. Wysocki" <[email protected]>
> > Cc: Marc Zyngier <[email protected]>
> > ---
> > 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 b203cfe28550..c253d151275e 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;
>
> Huh. Why do we need this hack? What's the issue with accessing the
> MADT? Can it disappear from under our feet? While we're walking it?
It is an awkward attempt at stashing the revision instead of
calling acpi_get_table() repeatedly (and from multiple files
for the same reason - ie get an MADT rev number).
Side note: get_madt_table() does the same thing and I followed
it - I am not sure it is very helpful either (or maybe
there is something I don't know behind that reasoning).
I will remove it (or leave it there without the madt_read hack).
> > +
> > + acpi_get_table(ACPI_SIG_MADT, 0, &madt);
> > + if (!madt)
> > + return madt_revision;
>
> What does this mean? Can we have a revision 0 of MADT?
No obviously you are right, it should have just returned an
error code and that's what it wanted to signal.
> > +
> > + 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);
> > +}
>
> Given that this checks *any* flag (or a combination of flags), the
> name of the helper is extremely misleading. Also, GICC flags are not
> necessarily tied to revision 7 of MADT.
>
> To be honest, I don't think this helper bring much, and I'd rather see
> an explicit check (or 3) for the revision in the driver code.
I will do, you have a point.
Thanks,
Lorenzo
> > +#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 9a7a74239eab..8d088fca65a1 100644
> > --- a/drivers/irqchip/irq-gic-v3-its.c
> > +++ b/drivers/irqchip/irq-gic-v3-its.c
> > @@ -5578,6 +5578,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 98b0329b7154..48e02838fdc8 100644
> > --- a/drivers/irqchip/irq-gic-v3.c
> > +++ b/drivers/irqchip/irq-gic-v3.c
> > @@ -2356,6 +2356,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);
> > @@ -2380,6 +2385,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 54189e0e5f41..a292f2bdb693 100644
> > --- a/include/linux/acpi.h
> > +++ b/include/linux/acpi.h
> > @@ -283,6 +283,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 */
>
> Thanks,
>
> M.
>
> --
> Without deviation from the norm, progress is not possible.
On Thu, Jan 04, 2024 at 02:21:44PM +0100, Rafael J. Wysocki wrote:
> On Thu, Jan 4, 2024 at 1:04 PM Russell King (Oracle)
> <[email protected]> wrote:
> >
> > On Thu, Jan 04, 2024 at 11:34:48AM +0000, Marc Zyngier wrote:
> > > On Wed, 03 Jan 2024 13:43:16 +0000,
> > > "Rafael J. Wysocki" <[email protected]> wrote:
> > > >
> > > > On Wed, Dec 27, 2023 at 12:00 PM Lorenzo Pieralisi
> > > > <[email protected]> wrote:
> > > > >
> > > > > This series is v4 of previous series:
> > > > >
> > > > > v3: https://lore.kernel.org/all/[email protected]
> > > > > v2: https://lore.kernel.org/all/[email protected]
> > > > > v1: https://lore.kernel.org/all/[email protected]
> > > > >
> > > > > v3 -> v4:
> > > > > - Dropped patches [1-3], already merged
> > > > > - Added Linuxized ACPICA changes accepted upstream
> > > > > - Rebased against v6.7-rc3
> > > > >
> > > > > 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 (3):
> > > > > ACPICA: MADT: Add GICC online capable bit handling
> > > > > ACPICA: MADT: Add new MADT GICC/GICR/ITS non-coherent flags handling
> > > > > irqchip/gic-v3: Enable non-coherent redistributors/ITSes ACPI probing
> > > > >
> > > > > 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/acpi/actbl2.h | 12 ++++++++++--
> > > > > include/linux/acpi.h | 3 +++
> > > > > 6 files changed, 55 insertions(+), 2 deletions(-)
> > > > >
> > > > > --
> > > >
> > > > I can apply the first 2 patches, but I would need an ACK for the 3rd one.
> > > >
> > > > Alternatively, feel free to add
> > > >
> > > > Acked-by: Rafael J. Wysocki <[email protected]>
> > > >
> > > > to the first 2 patches and route them via ARM64.
> > >
> > > Thanks for that. I have some comments on the third patch, which I'd
> > > like to see addressed beforehand. This is probably all 6.9 material
> > > anyway (nobody is affected by this so far).
> >
> > From a purely selfish point of view, it would be useful to have the
> > first patch merged merely to reduce the burden of patches for vcpu
> > hotplug.
>
> OK, since there will be at least one more iteration of patch [3/3]
> AFAICS, I'll queue up patches [1-2/3] for 6.8 (next week, though).
Thank you Rafael.
Lorenzo
On Mon, 08 Jan 2024 09:43:23 +0000,
Lorenzo Pieralisi <[email protected]> wrote:
>
> On Thu, Jan 04, 2024 at 11:12:28AM +0000, Marc Zyngier wrote:
> > On Wed, 27 Dec 2023 11:00:38 +0000,
> > Lorenzo Pieralisi <[email protected]> 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
> >
> > IWB/OWB *barriers*? Unless you're talking about something else,
> > IWB/OWB refers to cacheability, and only that.
>
> Yes, it should be expressed differently. Unfortunately this sentence made
> it into the kernel with the DT counterpart - commit 3a0fff0fb6a3 log,
> apologies.
Oh well. At least please clean this one up when you repost.
[...]
> > > + if (!madt_read) {
> > > + madt_read = true;
> >
> > Huh. Why do we need this hack? What's the issue with accessing the
> > MADT? Can it disappear from under our feet? While we're walking it?
>
> It is an awkward attempt at stashing the revision instead of
> calling acpi_get_table() repeatedly (and from multiple files
> for the same reason - ie get an MADT rev number).
>
> Side note: get_madt_table() does the same thing and I followed
> it - I am not sure it is very helpful either (or maybe
> there is something I don't know behind that reasoning).
This was introduced as part of 149fe9c293f76, as a cleanup. Not a
great move IMHO.
M.
--
Without deviation from the norm, progress is not possible.
On Wed, Dec 27, 2023 at 12:00 PM Lorenzo Pieralisi
<[email protected]> wrote:
>
> ACPICA commit 16f0befdeddf25756f317907798192bbaa417e5e
>
> Implement code to handle the GICC online capable bit management
> added into ACPI v6.5.
>
> Link: https://github.com/acpica/acpica/commit/16f0befd
> Signed-off-by: Lorenzo Pieralisi <[email protected]>
> Cc: Robert Moore <[email protected]>
> Cc: "Rafael J. Wysocki" <[email protected]>
> ---
> include/acpi/actbl2.h | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/include/acpi/actbl2.h b/include/acpi/actbl2.h
> index 3751ae69432f..2b4dd2c3348f 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_ONLINE_CAPABLE (1<<3) /* 03: Processor is online capable */
>
> /* 12: Generic Distributor (ACPI 5.0 + ACPI 6.0 changes) */
>
> --
Applied as 6.8-rc1 material along with the [2/3].
Thanks!
On Wed, Dec 27, 2023 at 12:00:38PM +0100, Lorenzo Pieralisi wrote:
[...]
> @@ -2380,6 +2385,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;
> +
Quick question before reposting it. We run this function for
every GICC entry, I didn't add a check to make sure all GICC
entries have the same flag value, please let me know if that's
OK.
I don't think there is a point in keeping a live variable across
calls to set the flag once for all either.
Thanks,
Lorenzo
> 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 54189e0e5f41..a292f2bdb693 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -283,6 +283,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
>
On Mon, 22 Jan 2024 16:18:04 +0000,
Lorenzo Pieralisi <[email protected]> wrote:
>
> On Wed, Dec 27, 2023 at 12:00:38PM +0100, Lorenzo Pieralisi wrote:
>
> [...]
>
> > @@ -2380,6 +2385,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;
> > +
>
> Quick question before reposting it. We run this function for
> every GICC entry, I didn't add a check to make sure all GICC
> entries have the same flag value, please let me know if that's
> OK.
>
> I don't think there is a point in keeping a live variable across
> calls to set the flag once for all either.
I don't think that's useful. Once we see the flag being set, we'll
enforce the non-coherency. If it wasn't set before, it's because it
wasn't necessary.
If one day we find a firmware that only randomly exposes the flag,
we'll treat it as a quirk.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.