| Message ID | 20240123110332.112797-2-lpieralisi@kernel.org (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | irqchip/gic-v3: Enable non-coherent GIC designs probing | expand |
On 23/01/2024 11:03 am, 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 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. > > 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 an ACPI MADT table with revision < 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). FWIW, Reviewed-by: Robin Murphy <robin.murphy@arm.com> I guess the most contentious parts disappeared in Marc's refactoring, so what's left seems entirely straightforward and innocuous to me. I'd also agree that there seems no real value in going out of our way to police the firmware for consistency - if at least one entry says to apply the quirk, then applying the quirk is the right and safest thing to do, so all we could really do on top of that is add extra complexity to be able to say "hey, firmware did a silly thing!", but if we're starting from the premise that hardware did an inadvisable thing to begin with, maybe we should have similarly realistic expectations of the corresponding firmware anyway :) Cheers, Robin. > Signed-off-by: Lorenzo Pieralisi <lpieralisi@kernel.org> > Cc: Robin Murphy <robin.murphy@arm.com> > Cc: Mark Rutland <mark.rutland@arm.com> > Cc: "Rafael J. Wysocki" <rafael@kernel.org> > Cc: Marc Zyngier <maz@kernel.org> > --- > drivers/acpi/processor_core.c | 15 +++++++++++++++ > drivers/irqchip/irq-gic-v3-its.c | 4 ++++ > drivers/irqchip/irq-gic-v3.c | 9 +++++++++ > include/linux/acpi.h | 3 +++ > 4 files changed, 31 insertions(+) > > diff --git a/drivers/acpi/processor_core.c b/drivers/acpi/processor_core.c > index b203cfe28550..915713c0e9b7 100644 > --- a/drivers/acpi/processor_core.c > +++ b/drivers/acpi/processor_core.c > @@ -215,6 +215,21 @@ phys_cpuid_t __init acpi_map_madt_entry(u32 acpi_id) > return rv; > } > > +int __init acpi_get_madt_revision(void) > +{ > + struct acpi_table_header *madt = NULL; > + int revision; > + > + if (ACPI_FAILURE(acpi_get_table(ACPI_SIG_MADT, 0, &madt))) > + return -EINVAL; > + > + revision = madt->revision; > + > + acpi_put_table(madt); > + > + return 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-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c > index fec1b58470df..a60c560ce891 100644 > --- a/drivers/irqchip/irq-gic-v3-its.c > +++ b/drivers/irqchip/irq-gic-v3-its.c > @@ -5591,6 +5591,10 @@ static int __init gic_acpi_parse_madt_its(union acpi_subtable_headers *header, > goto node_err; > } > > + if (acpi_get_madt_revision() >= 7 && > + (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..8cb8dff86c12 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 (acpi_get_madt_revision() >= 7 && > + (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 (acpi_get_madt_revision() >= 7 && > + (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 b7165e52b3c6..4eedab0e51c3 100644 > --- a/include/linux/acpi.h > +++ b/include/linux/acpi.h > @@ -284,6 +284,9 @@ static inline bool invalid_phys_cpuid(phys_cpuid_t phys_id) > return phys_id == PHYS_CPUID_INVALID; > } > > + > +int __init acpi_get_madt_revision(void); > + > /* Validate the processor object's proc_id */ > bool acpi_duplicate_processor_id(int proc_id); > /* Processor _CTS control */
On Tue, Jan 23, 2024 at 12:03:32PM +0100, 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 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. > > 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 an ACPI MADT table with revision < 7 it > skips parsing the newly added flags (that should be zeroed reserved > values for MADT versions < 7 but they could turn out to be buggy and > should be ignored). > > Signed-off-by: Lorenzo Pieralisi <lpieralisi@kernel.org> > Cc: Robin Murphy <robin.murphy@arm.com> > Cc: Mark Rutland <mark.rutland@arm.com> > Cc: "Rafael J. Wysocki" <rafael@kernel.org> > Cc: Marc Zyngier <maz@kernel.org> > --- > drivers/acpi/processor_core.c | 15 +++++++++++++++ > drivers/irqchip/irq-gic-v3-its.c | 4 ++++ > drivers/irqchip/irq-gic-v3.c | 9 +++++++++ > include/linux/acpi.h | 3 +++ > 4 files changed, 31 insertions(+) Hi Marc, Rafael, I would kindly ask you please what to do with this patch, it still applies to v6.9-rc5 - I can resend it if needed, ACPICA changes are already merged as-per the cover letter. Thanks, Lorenzo > diff --git a/drivers/acpi/processor_core.c b/drivers/acpi/processor_core.c > index b203cfe28550..915713c0e9b7 100644 > --- a/drivers/acpi/processor_core.c > +++ b/drivers/acpi/processor_core.c > @@ -215,6 +215,21 @@ phys_cpuid_t __init acpi_map_madt_entry(u32 acpi_id) > return rv; > } > > +int __init acpi_get_madt_revision(void) > +{ > + struct acpi_table_header *madt = NULL; > + int revision; > + > + if (ACPI_FAILURE(acpi_get_table(ACPI_SIG_MADT, 0, &madt))) > + return -EINVAL; > + > + revision = madt->revision; > + > + acpi_put_table(madt); > + > + return 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-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c > index fec1b58470df..a60c560ce891 100644 > --- a/drivers/irqchip/irq-gic-v3-its.c > +++ b/drivers/irqchip/irq-gic-v3-its.c > @@ -5591,6 +5591,10 @@ static int __init gic_acpi_parse_madt_its(union acpi_subtable_headers *header, > goto node_err; > } > > + if (acpi_get_madt_revision() >= 7 && > + (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..8cb8dff86c12 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 (acpi_get_madt_revision() >= 7 && > + (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 (acpi_get_madt_revision() >= 7 && > + (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 b7165e52b3c6..4eedab0e51c3 100644 > --- a/include/linux/acpi.h > +++ b/include/linux/acpi.h > @@ -284,6 +284,9 @@ static inline bool invalid_phys_cpuid(phys_cpuid_t phys_id) > return phys_id == PHYS_CPUID_INVALID; > } > > + > +int __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 >
diff --git a/drivers/acpi/processor_core.c b/drivers/acpi/processor_core.c index b203cfe28550..915713c0e9b7 100644 --- a/drivers/acpi/processor_core.c +++ b/drivers/acpi/processor_core.c @@ -215,6 +215,21 @@ phys_cpuid_t __init acpi_map_madt_entry(u32 acpi_id) return rv; } +int __init acpi_get_madt_revision(void) +{ + struct acpi_table_header *madt = NULL; + int revision; + + if (ACPI_FAILURE(acpi_get_table(ACPI_SIG_MADT, 0, &madt))) + return -EINVAL; + + revision = madt->revision; + + acpi_put_table(madt); + + return 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-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c index fec1b58470df..a60c560ce891 100644 --- a/drivers/irqchip/irq-gic-v3-its.c +++ b/drivers/irqchip/irq-gic-v3-its.c @@ -5591,6 +5591,10 @@ static int __init gic_acpi_parse_madt_its(union acpi_subtable_headers *header, goto node_err; } + if (acpi_get_madt_revision() >= 7 && + (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..8cb8dff86c12 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 (acpi_get_madt_revision() >= 7 && + (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 (acpi_get_madt_revision() >= 7 && + (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 b7165e52b3c6..4eedab0e51c3 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -284,6 +284,9 @@ static inline bool invalid_phys_cpuid(phys_cpuid_t phys_id) return phys_id == PHYS_CPUID_INVALID; } + +int __init acpi_get_madt_revision(void); + /* Validate the processor object's proc_id */ bool acpi_duplicate_processor_id(int proc_id); /* Processor _CTS control */
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 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. 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 an ACPI MADT table with revision < 7 it skips parsing the newly added flags (that should be zeroed reserved values for MADT versions < 7 but they could turn out to be buggy and should be ignored). Signed-off-by: Lorenzo Pieralisi <lpieralisi@kernel.org> Cc: Robin Murphy <robin.murphy@arm.com> Cc: Mark Rutland <mark.rutland@arm.com> Cc: "Rafael J. Wysocki" <rafael@kernel.org> Cc: Marc Zyngier <maz@kernel.org> --- drivers/acpi/processor_core.c | 15 +++++++++++++++ drivers/irqchip/irq-gic-v3-its.c | 4 ++++ drivers/irqchip/irq-gic-v3.c | 9 +++++++++ include/linux/acpi.h | 3 +++ 4 files changed, 31 insertions(+)