[v7,11/16] irqchip/gic-v3: Add support for ACPI's disabled but 'online capable' CPUs

Commit Message

Jonathan Cameron April 18, 2024, 1:54 p.m. UTC

From: James Morse <[e-mail]>

To support virtual CPU hotplug, ACPI has added an 'online capable' bit
to the MADT GICC entries. This indicates a disabled CPU entry may not
be possible to online via PSCI until firmware has set enabled bit in
_STA.

This means that a "usable" GIC is one that is marked as either enabled,
or online capable. Therefore, change acpi_gicc_is_usable() to check both
bits. However, we need to change the test in gic_acpi_match_gicc() back
to testing just the enabled bit so the count of enabled distributors is
correct.

What about the redistributor in the GICC entry? ACPI doesn't want to say.
Assume the worst: When a redistributor is described in the GICC entry,
but the entry is marked as disabled at boot, assume the redistributor
is inaccessible.

The GICv3 driver doesn't support late online of redistributors, so this
means the corresponding CPU can't be brought online either. Clear the
possible and present bits.

Systems that want CPU hotplug in a VM can ensure their redistributors
are always-on, and describe them that way with a GICR entry in the MADT.

When mapping redistributors found via GICC entries, handle the case
where the arch code believes the CPU is present and possible, but it
does not have an accessible redistributor. Print a warning and clear
the present and possible bits.

Signed-off-by: James Morse <[e-mail]>
Signed-off-by: Russell King (Oracle) <[e-mail]>
Signed-off-by: Jonathan Cameron <[e-mail]>

---
v7: No Change.
---
 drivers/irqchip/irq-gic-v3.c | 21 +++++++++++++++++++--
 include/linux/acpi.h         |  3 ++-
 2 files changed, 21 insertions(+), 3 deletions(-)

Comments

Jonathan Cameron April 22, 2024, 10:40 a.m. UTC | #1

On Thu, 18 Apr 2024 14:54:07 +0100
Jonathan Cameron <[e-mail]> wrote:

> From: James Morse <[e-mail]>
> 
> To support virtual CPU hotplug, ACPI has added an 'online capable' bit
> to the MADT GICC entries. This indicates a disabled CPU entry may not
> be possible to online via PSCI until firmware has set enabled bit in
> _STA.
> 
> This means that a "usable" GIC is one that is marked as either enabled,
> or online capable. Therefore, change acpi_gicc_is_usable() to check both
> bits. However, we need to change the test in gic_acpi_match_gicc() back
> to testing just the enabled bit so the count of enabled distributors is
> correct.
> 
> What about the redistributor in the GICC entry? ACPI doesn't want to say.
> Assume the worst: When a redistributor is described in the GICC entry,
> but the entry is marked as disabled at boot, assume the redistributor
> is inaccessible.
> 
> The GICv3 driver doesn't support late online of redistributors, so this
> means the corresponding CPU can't be brought online either. Clear the
> possible and present bits.
> 
> Systems that want CPU hotplug in a VM can ensure their redistributors
> are always-on, and describe them that way with a GICR entry in the MADT.
> 
> When mapping redistributors found via GICC entries, handle the case
> where the arch code believes the CPU is present and possible, but it
> does not have an accessible redistributor. Print a warning and clear
> the present and possible bits.
> 
> Signed-off-by: James Morse <[e-mail]>
> Signed-off-by: Russell King (Oracle) <[e-mail]>
> Signed-off-by: Jonathan Cameron <[e-mail]>

+CC Marc,

Whilst this has been unchanged for a long time, I'm not 100% sure
we've specifically drawn your attention to it before now.

Jonathan

> 
> ---
> v7: No Change.
> ---
>  drivers/irqchip/irq-gic-v3.c | 21 +++++++++++++++++++--
>  include/linux/acpi.h         |  3 ++-
>  2 files changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> index 10af15f93d4d..66132251c1bb 100644
> --- a/drivers/irqchip/irq-gic-v3.c
> +++ b/drivers/irqchip/irq-gic-v3.c
> @@ -2363,11 +2363,25 @@ gic_acpi_parse_madt_gicc(union acpi_subtable_headers *header,
>  				(struct acpi_madt_generic_interrupt *)header;
>  	u32 reg = readl_relaxed(acpi_data.dist_base + GICD_PIDR2) & GIC_PIDR2_ARCH_MASK;
>  	u32 size = reg == GIC_PIDR2_ARCH_GICv4 ? SZ_64K * 4 : SZ_64K * 2;
> +	int cpu = get_cpu_for_acpi_id(gicc->uid);
>  	void __iomem *redist_base;
>  
>  	if (!acpi_gicc_is_usable(gicc))
>  		return 0;
>  
> +	/*
> +	 * Capable but disabled CPUs can be brought online later. What about
> +	 * the redistributor? ACPI doesn't want to say!
> +	 * Virtual hotplug systems can use the MADT's "always-on" GICR entries.
> +	 * Otherwise, prevent such CPUs from being brought online.
> +	 */
> +	if (!(gicc->flags & ACPI_MADT_ENABLED)) {
> +		pr_warn_once("CPU %u's redistributor is inaccessible: this CPU can't be brought online\n", cpu);
> +		set_cpu_present(cpu, false);
> +		set_cpu_possible(cpu, false);
> +		return 0;
> +	}
> +
>  	redist_base = ioremap(gicc->gicr_base_address, size);
>  	if (!redist_base)
>  		return -ENOMEM;
> @@ -2413,9 +2427,12 @@ static int __init gic_acpi_match_gicc(union acpi_subtable_headers *header,
>  
>  	/*
>  	 * If GICC is enabled and has valid gicr base address, then it means
> -	 * GICR base is presented via GICC
> +	 * GICR base is presented via GICC. The redistributor is only known to
> +	 * be accessible if the GICC is marked as enabled. If this bit is not
> +	 * set, we'd need to add the redistributor at runtime, which isn't
> +	 * supported.
>  	 */
> -	if (acpi_gicc_is_usable(gicc) && gicc->gicr_base_address)
> +	if (gicc->flags & ACPI_MADT_ENABLED && gicc->gicr_base_address)
>  		acpi_data.enabled_rdists++;
>  
>  	return 0;
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 9844a3f9c4e5..fcfb7bb6789e 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -239,7 +239,8 @@ void acpi_table_print_madt_entry (struct acpi_subtable_header *madt);
>  
>  static inline bool acpi_gicc_is_usable(struct acpi_madt_generic_interrupt *gicc)
>  {
> -	return gicc->flags & ACPI_MADT_ENABLED;
> +	return gicc->flags & (ACPI_MADT_ENABLED |
> +			      ACPI_MADT_GICC_ONLINE_CAPABLE);
>  }
>  
>  /* the following numa functions are architecture-dependent */

Marc Zyngier April 23, 2024, 12:01 p.m. UTC | #2

On Mon, 22 Apr 2024 11:40:20 +0100,
Jonathan Cameron <[e-mail]> wrote:
> 
> On Thu, 18 Apr 2024 14:54:07 +0100
> Jonathan Cameron <[e-mail]> wrote:
> 
> > From: James Morse <[e-mail]>
> > 
> > To support virtual CPU hotplug, ACPI has added an 'online capable' bit
> > to the MADT GICC entries. This indicates a disabled CPU entry may not
> > be possible to online via PSCI until firmware has set enabled bit in
> > _STA.
> > 
> > This means that a "usable" GIC is one that is marked as either enabled,
> > or online capable. Therefore, change acpi_gicc_is_usable() to check both
> > bits. However, we need to change the test in gic_acpi_match_gicc() back
> > to testing just the enabled bit so the count of enabled distributors is
> > correct.
> > 
> > What about the redistributor in the GICC entry? ACPI doesn't want to say.
> > Assume the worst: When a redistributor is described in the GICC entry,
> > but the entry is marked as disabled at boot, assume the redistributor
> > is inaccessible.
> > 
> > The GICv3 driver doesn't support late online of redistributors, so this
> > means the corresponding CPU can't be brought online either. Clear the
> > possible and present bits.
> > 
> > Systems that want CPU hotplug in a VM can ensure their redistributors
> > are always-on, and describe them that way with a GICR entry in the MADT.
> > 
> > When mapping redistributors found via GICC entries, handle the case
> > where the arch code believes the CPU is present and possible, but it
> > does not have an accessible redistributor. Print a warning and clear
> > the present and possible bits.
> > 
> > Signed-off-by: James Morse <[e-mail]>
> > Signed-off-by: Russell King (Oracle) <[e-mail]>
> > Signed-off-by: Jonathan Cameron <[e-mail]>
> 
> +CC Marc,
> 
> Whilst this has been unchanged for a long time, I'm not 100% sure
> we've specifically drawn your attention to it before now.
> 
> Jonathan
> 
> > 
> > ---
> > v7: No Change.
> > ---
> >  drivers/irqchip/irq-gic-v3.c | 21 +++++++++++++++++++--
> >  include/linux/acpi.h         |  3 ++-
> >  2 files changed, 21 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> > index 10af15f93d4d..66132251c1bb 100644
> > --- a/drivers/irqchip/irq-gic-v3.c
> > +++ b/drivers/irqchip/irq-gic-v3.c
> > @@ -2363,11 +2363,25 @@ gic_acpi_parse_madt_gicc(union acpi_subtable_headers *header,
> >  				(struct acpi_madt_generic_interrupt *)header;
> >  	u32 reg = readl_relaxed(acpi_data.dist_base + GICD_PIDR2) & GIC_PIDR2_ARCH_MASK;
> >  	u32 size = reg == GIC_PIDR2_ARCH_GICv4 ? SZ_64K * 4 : SZ_64K * 2;
> > +	int cpu = get_cpu_for_acpi_id(gicc->uid);
> >  	void __iomem *redist_base;
> >  
> >  	if (!acpi_gicc_is_usable(gicc))
> >  		return 0;
> >  
> > +	/*
> > +	 * Capable but disabled CPUs can be brought online later. What about
> > +	 * the redistributor? ACPI doesn't want to say!
> > +	 * Virtual hotplug systems can use the MADT's "always-on" GICR entries.
> > +	 * Otherwise, prevent such CPUs from being brought online.
> > +	 */
> > +	if (!(gicc->flags & ACPI_MADT_ENABLED)) {
> > +		pr_warn_once("CPU %u's redistributor is inaccessible: this CPU can't be brought online\n", cpu);
> > +		set_cpu_present(cpu, false);
> > +		set_cpu_possible(cpu, false);
> > +		return 0;
> > +	}

It seems dangerous to clear those this late in the game, given how
disconnected from the architecture code this is. Are we sure that
nothing has sampled these cpumasks beforehand?

Thanks,

	M.

Jonathan Cameron April 24, 2024, 12:54 p.m. UTC | #3

On Tue, 23 Apr 2024 13:01:21 +0100
Marc Zyngier <[e-mail]> wrote:

> On Mon, 22 Apr 2024 11:40:20 +0100,
> Jonathan Cameron <[e-mail]> wrote:
> > 
> > On Thu, 18 Apr 2024 14:54:07 +0100
> > Jonathan Cameron <[e-mail]> wrote:
> >   
> > > From: James Morse <[e-mail]>
> > > 
> > > To support virtual CPU hotplug, ACPI has added an 'online capable' bit
> > > to the MADT GICC entries. This indicates a disabled CPU entry may not
> > > be possible to online via PSCI until firmware has set enabled bit in
> > > _STA.
> > > 
> > > This means that a "usable" GIC is one that is marked as either enabled,
> > > or online capable. Therefore, change acpi_gicc_is_usable() to check both
> > > bits. However, we need to change the test in gic_acpi_match_gicc() back
> > > to testing just the enabled bit so the count of enabled distributors is
> > > correct.
> > > 
> > > What about the redistributor in the GICC entry? ACPI doesn't want to say.
> > > Assume the worst: When a redistributor is described in the GICC entry,
> > > but the entry is marked as disabled at boot, assume the redistributor
> > > is inaccessible.
> > > 
> > > The GICv3 driver doesn't support late online of redistributors, so this
> > > means the corresponding CPU can't be brought online either. Clear the
> > > possible and present bits.
> > > 
> > > Systems that want CPU hotplug in a VM can ensure their redistributors
> > > are always-on, and describe them that way with a GICR entry in the MADT.
> > > 
> > > When mapping redistributors found via GICC entries, handle the case
> > > where the arch code believes the CPU is present and possible, but it
> > > does not have an accessible redistributor. Print a warning and clear
> > > the present and possible bits.
> > > 
> > > Signed-off-by: James Morse <[e-mail]>
> > > Signed-off-by: Russell King (Oracle) <[e-mail]>
> > > Signed-off-by: Jonathan Cameron <[e-mail]>  
> > 
> > +CC Marc,
> > 
> > Whilst this has been unchanged for a long time, I'm not 100% sure
> > we've specifically drawn your attention to it before now.
> > 
> > Jonathan
> >   
> > > 
> > > ---
> > > v7: No Change.
> > > ---
> > >  drivers/irqchip/irq-gic-v3.c | 21 +++++++++++++++++++--
> > >  include/linux/acpi.h         |  3 ++-
> > >  2 files changed, 21 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> > > index 10af15f93d4d..66132251c1bb 100644
> > > --- a/drivers/irqchip/irq-gic-v3.c
> > > +++ b/drivers/irqchip/irq-gic-v3.c
> > > @@ -2363,11 +2363,25 @@ gic_acpi_parse_madt_gicc(union acpi_subtable_headers *header,
> > >  				(struct acpi_madt_generic_interrupt *)header;
> > >  	u32 reg = readl_relaxed(acpi_data.dist_base + GICD_PIDR2) & GIC_PIDR2_ARCH_MASK;
> > >  	u32 size = reg == GIC_PIDR2_ARCH_GICv4 ? SZ_64K * 4 : SZ_64K * 2;
> > > +	int cpu = get_cpu_for_acpi_id(gicc->uid);
> > >  	void __iomem *redist_base;
> > >  
> > >  	if (!acpi_gicc_is_usable(gicc))
> > >  		return 0;
> > >  
> > > +	/*
> > > +	 * Capable but disabled CPUs can be brought online later. What about
> > > +	 * the redistributor? ACPI doesn't want to say!
> > > +	 * Virtual hotplug systems can use the MADT's "always-on" GICR entries.
> > > +	 * Otherwise, prevent such CPUs from being brought online.
> > > +	 */
> > > +	if (!(gicc->flags & ACPI_MADT_ENABLED)) {
> > > +		pr_warn_once("CPU %u's redistributor is inaccessible: this CPU can't be brought online\n", cpu);
> > > +		set_cpu_present(cpu, false);
> > > +		set_cpu_possible(cpu, false);
> > > +		return 0;
> > > +	}  
> 
> It seems dangerous to clear those this late in the game, given how
> disconnected from the architecture code this is. Are we sure that
> nothing has sampled these cpumasks beforehand?

Hi Marc,

Any firmware that does this is being considered as buggy already
but given it is firmware and the spec doesn't say much about this,
there is always the possibility.

Not much happens between the point where these are setup and
the point where the the gic inits and this code runs, but even if careful
review showed it was fine today, it will be fragile to future changes.

I'm not sure there is a huge disadvantage for such broken firmware in
clearing these masks from the point of view of what is used throughout
the rest of the kernel. Here I think we are just looking to prevent the CPU
being onlined later.

We could add a set_cpu_broken() with appropriate mask.
Given this is very arm64 specific I'm not sure Rafael will be keen on
us checking such a mask in the generic ACPI code, but we could check it in
arch_register_cpu() and just not register the cpu if it matches.
That will cover the vCPU hotplug case.

Does that sounds sensible, or would you prefer something else?

Jonathan







> 
> Thanks,
> 
> 	M.
>

Marc Zyngier April 24, 2024, 3:33 p.m. UTC | #4

On Wed, 24 Apr 2024 13:54:38 +0100,
Jonathan Cameron <[e-mail]> wrote:
> 
> On Tue, 23 Apr 2024 13:01:21 +0100
> Marc Zyngier <[e-mail]> wrote:
> 
> > On Mon, 22 Apr 2024 11:40:20 +0100,
> > Jonathan Cameron <[e-mail]> wrote:
> > > 
> > > On Thu, 18 Apr 2024 14:54:07 +0100
> > > Jonathan Cameron <[e-mail]> wrote:

[...]

> > >   
> > > > +	/*
> > > > +	 * Capable but disabled CPUs can be brought online later. What about
> > > > +	 * the redistributor? ACPI doesn't want to say!
> > > > +	 * Virtual hotplug systems can use the MADT's "always-on" GICR entries.
> > > > +	 * Otherwise, prevent such CPUs from being brought online.
> > > > +	 */
> > > > +	if (!(gicc->flags & ACPI_MADT_ENABLED)) {
> > > > +		pr_warn_once("CPU %u's redistributor is inaccessible: this CPU can't be brought online\n", cpu);
> > > > +		set_cpu_present(cpu, false);
> > > > +		set_cpu_possible(cpu, false);
> > > > +		return 0;
> > > > +	}  
> > 
> > It seems dangerous to clear those this late in the game, given how
> > disconnected from the architecture code this is. Are we sure that
> > nothing has sampled these cpumasks beforehand?
> 
> Hi Marc,
> 
> Any firmware that does this is being considered as buggy already
> but given it is firmware and the spec doesn't say much about this,
> there is always the possibility.

There is no shortage of broken firmware out there, and I expect this
trend to progress.

> Not much happens between the point where these are setup and
> the point where the the gic inits and this code runs, but even if careful
> review showed it was fine today, it will be fragile to future changes.
> 
> I'm not sure there is a huge disadvantage for such broken firmware in
> clearing these masks from the point of view of what is used throughout
> the rest of the kernel. Here I think we are just looking to prevent the CPU
> being onlined later.

I totally agree on the goal, I simply question the way you get to it.

> 
> We could add a set_cpu_broken() with appropriate mask.
> Given this is very arm64 specific I'm not sure Rafael will be keen on
> us checking such a mask in the generic ACPI code, but we could check it in
> arch_register_cpu() and just not register the cpu if it matches.
> That will cover the vCPU hotplug case.
>
> Does that sounds sensible, or would you prefer something else?


Such a 'broken_rdists' mask is exactly what I have in mind, just
keeping it private to the GIC driver, and not expose it anywhere else.
You can then fail the hotplug event early, and avoid changing the
global masks from within the GIC driver. At least, we don't mess with
the internals of the kernel, and the CPU is properly marked as dead
(that mechanism should already work).

I'd expect the handling side to look like this (will not compile, but
you'll get the idea):

diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index 6fb276504bcc..e8f02bfd0e21 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -1009,6 +1009,9 @@ static int __gic_populate_rdist(struct redist_region *region, void __iomem *ptr)
 	u64 typer;
 	u32 aff;
 
+	if (cpumask_test_cpu(smp_processor_id(), &broken_rdists))
+		return 1;
+
 	/*
 	 * Convert affinity to a 32bit value that can be matched to
 	 * GICR_TYPER bits [63:32].
@@ -1260,14 +1263,15 @@ static int gic_dist_supports_lpis(void)
 		!gicv3_nolpi);
 }
 
-static void gic_cpu_init(void)
+static int gic_cpu_init(void)
 {
 	void __iomem *rbase;
-	int i;
+	int ret, i;
 
 	/* Register ourselves with the rest of the world */
-	if (gic_populate_rdist())
-		return;
+	ret = gic_populate_rdist();
+	if (ret)
+		return ret;
 
 	gic_enable_redist(true);
 
@@ -1286,6 +1290,8 @@ static void gic_cpu_init(void)
 
 	/* initialise system registers */
 	gic_cpu_sys_reg_init();
+
+	return 0;
 }
 
 #ifdef CONFIG_SMP
@@ -1295,7 +1301,11 @@ static void gic_cpu_init(void)
 
 static int gic_starting_cpu(unsigned int cpu)
 {
-	gic_cpu_init();
+	int ret;
+
+	ret = gic_cpu_init();
+	if (ret)
+		return ret;
 
 	if (gic_dist_supports_lpis())
 		its_cpu_init();

But the question is: do you rely on these masks having been
"corrected" anywhere else?

Thanks,

	M.

Salil Mehta April 24, 2024, 4:35 p.m. UTC | #5

>  From: Marc Zyngier <[e-mail]>
>  Sent: Wednesday, April 24, 2024 4:33 PM
>  To: Jonathan Cameron <[e-mail]>
>  Cc: Thomas Gleixner <[e-mail]>; Peter Zijlstra
>  <[e-mail]>; [e-mail];
>  [e-mail]; [e-mail]; linux-
>  [e-mail]; [e-mail]; linux-arm-
>  [e-mail]; [e-mail]; [e-mail];
>  Russell King <[e-mail]>; Rafael J . Wysocki
>  <[e-mail]>; Miguel Luis <[e-mail]>; James Morse
>  <[e-mail]>; Salil Mehta <[e-mail]>; Jean-
>  Philippe Brucker <[e-mail]>; Catalin Marinas
>  <[e-mail]>; Will Deacon <[e-mail]>; Linuxarm
>  <[e-mail]>; Ingo Molnar <[e-mail]>; Borislav
>  Petkov <[e-mail]>; Dave Hansen <[e-mail]>;
>  [e-mail]; [e-mail]
>  Subject: Re: [PATCH v7 11/16] irqchip/gic-v3: Add support for ACPI's
>  disabled but 'online capable' CPUs
>  
>  On Wed, 24 Apr 2024 13:54:38 +0100,
>  Jonathan Cameron <[e-mail]> wrote:
>  >
>  > On Tue, 23 Apr 2024 13:01:21 +0100
>  > Marc Zyngier <[e-mail]> wrote:
>  >
>  > > On Mon, 22 Apr 2024 11:40:20 +0100,
>  > > Jonathan Cameron <[e-mail]> wrote:
>  > > >
>  > > > On Thu, 18 Apr 2024 14:54:07 +0100 Jonathan Cameron
>  > > > <[e-mail]> wrote:
>  
>  [...]
>  
>  > > >
>  > > > > +	/*
>  > > > > +	 * Capable but disabled CPUs can be brought online later.  What about
>  > > > > +	 * the redistributor? ACPI doesn't want to say!
>  > > > > +	 * Virtual hotplug systems can use the MADT's "always-on"  GICR entries.
>  > > > > +	 * Otherwise, prevent such CPUs from being brought online.
>  > > > > +	 */
>  > > > > +	if (!(gicc->flags & ACPI_MADT_ENABLED)) {
>  > > > > +		pr_warn_once("CPU %u's redistributor is  inaccessible: this CPU can't be brought online\n", cpu);
>  > > > > +		set_cpu_present(cpu, false);
>  > > > > +		set_cpu_possible(cpu, false);

(a digression) shouldn't we be clearing the enabled mask as well?

                                          set_cpu_enabled(cpu, false);


Best regards
Salil

Jonathan Cameron April 24, 2024, 5:08 p.m. UTC | #6

On Wed, 24 Apr 2024 17:35:54 +0100
Salil Mehta <[e-mail]> wrote:

> >  From: Marc Zyngier <[e-mail]>
> >  Sent: Wednesday, April 24, 2024 4:33 PM
> >  To: Jonathan Cameron <[e-mail]>
> >  Cc: Thomas Gleixner <[e-mail]>; Peter Zijlstra
> >  <[e-mail]>; [e-mail];
> >  [e-mail]; [e-mail]; linux-
> >  [e-mail]; [e-mail]; linux-arm-
> >  [e-mail]; [e-mail]; [e-mail];
> >  Russell King <[e-mail]>; Rafael J . Wysocki
> >  <[e-mail]>; Miguel Luis <[e-mail]>; James Morse
> >  <[e-mail]>; Salil Mehta <[e-mail]>; Jean-
> >  Philippe Brucker <[e-mail]>; Catalin Marinas
> >  <[e-mail]>; Will Deacon <[e-mail]>; Linuxarm
> >  <[e-mail]>; Ingo Molnar <[e-mail]>; Borislav
> >  Petkov <[e-mail]>; Dave Hansen <[e-mail]>;
> >  [e-mail]; [e-mail]
> >  Subject: Re: [PATCH v7 11/16] irqchip/gic-v3: Add support for ACPI's
> >  disabled but 'online capable' CPUs
> >  
> >  On Wed, 24 Apr 2024 13:54:38 +0100,
> >  Jonathan Cameron <[e-mail]> wrote:  
> >  >
> >  > On Tue, 23 Apr 2024 13:01:21 +0100
> >  > Marc Zyngier <[e-mail]> wrote:
> >  >  
> >  > > On Mon, 22 Apr 2024 11:40:20 +0100,
> >  > > Jonathan Cameron <[e-mail]> wrote:  
> >  > > >
> >  > > > On Thu, 18 Apr 2024 14:54:07 +0100 Jonathan Cameron
> >  > > > <[e-mail]> wrote:  
> >  
> >  [...]
> >    
> >  > > >  
> >  > > > > +	/*
> >  > > > > +	 * Capable but disabled CPUs can be brought online later.  What about
> >  > > > > +	 * the redistributor? ACPI doesn't want to say!
> >  > > > > +	 * Virtual hotplug systems can use the MADT's "always-on"  GICR entries.
> >  > > > > +	 * Otherwise, prevent such CPUs from being brought online.
> >  > > > > +	 */
> >  > > > > +	if (!(gicc->flags & ACPI_MADT_ENABLED)) {
> >  > > > > +		pr_warn_once("CPU %u's redistributor is  inaccessible: this CPU can't be brought online\n", cpu);
> >  > > > > +		set_cpu_present(cpu, false);
> >  > > > > +		set_cpu_possible(cpu, false);  
> 
> (a digression) shouldn't we be clearing the enabled mask as well?
> 
>                                           set_cpu_enabled(cpu, false);

FWIW I think not necessary. enabled is only set in register_cpu() and aim here is to
never call that for CPUs in this state.

Anyhow, I got distracted by the firmware bug I found whilst trying to test this but
now have a test setup that hits this path (once deliberately broken), so will
see what we can do about that doesn't have affect those masks.

Jonathan


> 
> 
> Best regards
> Salil

Jonathan Cameron April 25, 2024, 9:28 a.m. UTC | #7

On Wed, 24 Apr 2024 13:54:38 +0100
Jonathan Cameron <[e-mail]> wrote:

> On Tue, 23 Apr 2024 13:01:21 +0100
> Marc Zyngier <[e-mail]> wrote:
> 
> > On Mon, 22 Apr 2024 11:40:20 +0100,
> > Jonathan Cameron <[e-mail]> wrote:  
> > > 
> > > On Thu, 18 Apr 2024 14:54:07 +0100
> > > Jonathan Cameron <[e-mail]> wrote:
> > >     
> > > > From: James Morse <[e-mail]>
> > > > 
> > > > To support virtual CPU hotplug, ACPI has added an 'online capable' bit
> > > > to the MADT GICC entries. This indicates a disabled CPU entry may not
> > > > be possible to online via PSCI until firmware has set enabled bit in
> > > > _STA.
> > > > 
> > > > This means that a "usable" GIC is one that is marked as either enabled,
> > > > or online capable. Therefore, change acpi_gicc_is_usable() to check both
> > > > bits. However, we need to change the test in gic_acpi_match_gicc() back
> > > > to testing just the enabled bit so the count of enabled distributors is
> > > > correct.
> > > > 
> > > > What about the redistributor in the GICC entry? ACPI doesn't want to say.
> > > > Assume the worst: When a redistributor is described in the GICC entry,
> > > > but the entry is marked as disabled at boot, assume the redistributor
> > > > is inaccessible.
> > > > 
> > > > The GICv3 driver doesn't support late online of redistributors, so this
> > > > means the corresponding CPU can't be brought online either. Clear the
> > > > possible and present bits.
> > > > 
> > > > Systems that want CPU hotplug in a VM can ensure their redistributors
> > > > are always-on, and describe them that way with a GICR entry in the MADT.
> > > > 
> > > > When mapping redistributors found via GICC entries, handle the case
> > > > where the arch code believes the CPU is present and possible, but it
> > > > does not have an accessible redistributor. Print a warning and clear
> > > > the present and possible bits.
> > > > 
> > > > Signed-off-by: James Morse <[e-mail]>
> > > > Signed-off-by: Russell King (Oracle) <[e-mail]>
> > > > Signed-off-by: Jonathan Cameron <[e-mail]>    
> > > 
> > > +CC Marc,
> > > 
> > > Whilst this has been unchanged for a long time, I'm not 100% sure
> > > we've specifically drawn your attention to it before now.
> > > 
> > > Jonathan
> > >     
> > > > 
> > > > ---
> > > > v7: No Change.
> > > > ---
> > > >  drivers/irqchip/irq-gic-v3.c | 21 +++++++++++++++++++--
> > > >  include/linux/acpi.h         |  3 ++-
> > > >  2 files changed, 21 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> > > > index 10af15f93d4d..66132251c1bb 100644
> > > > --- a/drivers/irqchip/irq-gic-v3.c
> > > > +++ b/drivers/irqchip/irq-gic-v3.c
> > > > @@ -2363,11 +2363,25 @@ gic_acpi_parse_madt_gicc(union acpi_subtable_headers *header,
> > > >  				(struct acpi_madt_generic_interrupt *)header;
> > > >  	u32 reg = readl_relaxed(acpi_data.dist_base + GICD_PIDR2) & GIC_PIDR2_ARCH_MASK;
> > > >  	u32 size = reg == GIC_PIDR2_ARCH_GICv4 ? SZ_64K * 4 : SZ_64K * 2;
> > > > +	int cpu = get_cpu_for_acpi_id(gicc->uid);
> > > >  	void __iomem *redist_base;
> > > >  
> > > >  	if (!acpi_gicc_is_usable(gicc))
> > > >  		return 0;
> > > >  
> > > > +	/*
> > > > +	 * Capable but disabled CPUs can be brought online later. What about
> > > > +	 * the redistributor? ACPI doesn't want to say!
> > > > +	 * Virtual hotplug systems can use the MADT's "always-on" GICR entries.
> > > > +	 * Otherwise, prevent such CPUs from being brought online.
> > > > +	 */
> > > > +	if (!(gicc->flags & ACPI_MADT_ENABLED)) {
> > > > +		pr_warn_once("CPU %u's redistributor is inaccessible: this CPU can't be brought online\n", cpu);
> > > > +		set_cpu_present(cpu, false);
> > > > +		set_cpu_possible(cpu, false);
> > > > +		return 0;
> > > > +	}    
> > 
> > It seems dangerous to clear those this late in the game, given how
> > disconnected from the architecture code this is. Are we sure that
> > nothing has sampled these cpumasks beforehand?  
> 
> Hi Marc,
> 
> Any firmware that does this is being considered as buggy already
> but given it is firmware and the spec doesn't say much about this,
> there is always the possibility.
> 
> Not much happens between the point where these are setup and
> the point where the the gic inits and this code runs, but even if careful
> review showed it was fine today, it will be fragile to future changes.
> 
> I'm not sure there is a huge disadvantage for such broken firmware in
> clearing these masks from the point of view of what is used throughout
> the rest of the kernel. Here I think we are just looking to prevent the CPU
> being onlined later.
> 
> We could add a set_cpu_broken() with appropriate mask.
> Given this is very arm64 specific I'm not sure Rafael will be keen on
> us checking such a mask in the generic ACPI code, but we could check it in
> arch_register_cpu() and just not register the cpu if it matches.
> That will cover the vCPU hotplug case.
> 
> Does that sounds sensible, or would you prefer something else?

Hi Marc

Some experiments later (faking this on a physical board - I never liked
CPU 120 anyway!) and using a different mask brings it's own minor pain.

When all the rest of the CPUs are brought up cpuhp_bringup_mask() is called
on cpu_present_mask so we need to do a dance in there to use a temporary
mask with broken cpus removed.  I think it makes sense to cut that out
at the top of the cpuhp_bringup_mask() pile of actions rather than trying
to paper over each actual thing that is dying... (looks like an infinite loop
somewhere but I haven't tracked down where yet).

I'll spin a patch so you can see what it looks like, but my concern is
we are just moving the risk from early users of these masks to later cases
where code assumes cpu_present_mask definitely means they are present.
That is probably a small set of cases but not nice either.

Looks like one of those cases where we need to pick the lesser of two evils
which is probably still the cpu_broken_mask approach.

On plus side if we decide to go back to the original approach having seen
that I already have the code :)

Jonathan



> 
> Jonathan
> 
> 
> 
> 
> 
> 
> 
> > 
> > Thanks,
> > 
> > 	M.
> >   
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> [e-mail]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Jonathan Cameron April 25, 2024, 9:56 a.m. UTC | #8

On Thu, 25 Apr 2024 10:28:06 +0100
Jonathan Cameron <[e-mail]> wrote:

> On Wed, 24 Apr 2024 13:54:38 +0100
> Jonathan Cameron <[e-mail]> wrote:
> 
> > On Tue, 23 Apr 2024 13:01:21 +0100
> > Marc Zyngier <[e-mail]> wrote:
> >   
> > > On Mon, 22 Apr 2024 11:40:20 +0100,
> > > Jonathan Cameron <[e-mail]> wrote:    
> > > > 
> > > > On Thu, 18 Apr 2024 14:54:07 +0100
> > > > Jonathan Cameron <[e-mail]> wrote:
> > > >       
> > > > > From: James Morse <[e-mail]>
> > > > > 
> > > > > To support virtual CPU hotplug, ACPI has added an 'online capable' bit
> > > > > to the MADT GICC entries. This indicates a disabled CPU entry may not
> > > > > be possible to online via PSCI until firmware has set enabled bit in
> > > > > _STA.
> > > > > 
> > > > > This means that a "usable" GIC is one that is marked as either enabled,
> > > > > or online capable. Therefore, change acpi_gicc_is_usable() to check both
> > > > > bits. However, we need to change the test in gic_acpi_match_gicc() back
> > > > > to testing just the enabled bit so the count of enabled distributors is
> > > > > correct.
> > > > > 
> > > > > What about the redistributor in the GICC entry? ACPI doesn't want to say.
> > > > > Assume the worst: When a redistributor is described in the GICC entry,
> > > > > but the entry is marked as disabled at boot, assume the redistributor
> > > > > is inaccessible.
> > > > > 
> > > > > The GICv3 driver doesn't support late online of redistributors, so this
> > > > > means the corresponding CPU can't be brought online either. Clear the
> > > > > possible and present bits.
> > > > > 
> > > > > Systems that want CPU hotplug in a VM can ensure their redistributors
> > > > > are always-on, and describe them that way with a GICR entry in the MADT.
> > > > > 
> > > > > When mapping redistributors found via GICC entries, handle the case
> > > > > where the arch code believes the CPU is present and possible, but it
> > > > > does not have an accessible redistributor. Print a warning and clear
> > > > > the present and possible bits.
> > > > > 
> > > > > Signed-off-by: James Morse <[e-mail]>
> > > > > Signed-off-by: Russell King (Oracle) <[e-mail]>
> > > > > Signed-off-by: Jonathan Cameron <[e-mail]>      
> > > > 
> > > > +CC Marc,
> > > > 
> > > > Whilst this has been unchanged for a long time, I'm not 100% sure
> > > > we've specifically drawn your attention to it before now.
> > > > 
> > > > Jonathan
> > > >       
> > > > > 
> > > > > ---
> > > > > v7: No Change.
> > > > > ---
> > > > >  drivers/irqchip/irq-gic-v3.c | 21 +++++++++++++++++++--
> > > > >  include/linux/acpi.h         |  3 ++-
> > > > >  2 files changed, 21 insertions(+), 3 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> > > > > index 10af15f93d4d..66132251c1bb 100644
> > > > > --- a/drivers/irqchip/irq-gic-v3.c
> > > > > +++ b/drivers/irqchip/irq-gic-v3.c
> > > > > @@ -2363,11 +2363,25 @@ gic_acpi_parse_madt_gicc(union acpi_subtable_headers *header,
> > > > >  				(struct acpi_madt_generic_interrupt *)header;
> > > > >  	u32 reg = readl_relaxed(acpi_data.dist_base + GICD_PIDR2) & GIC_PIDR2_ARCH_MASK;
> > > > >  	u32 size = reg == GIC_PIDR2_ARCH_GICv4 ? SZ_64K * 4 : SZ_64K * 2;
> > > > > +	int cpu = get_cpu_for_acpi_id(gicc->uid);
> > > > >  	void __iomem *redist_base;
> > > > >  
> > > > >  	if (!acpi_gicc_is_usable(gicc))
> > > > >  		return 0;
> > > > >  
> > > > > +	/*
> > > > > +	 * Capable but disabled CPUs can be brought online later. What about
> > > > > +	 * the redistributor? ACPI doesn't want to say!
> > > > > +	 * Virtual hotplug systems can use the MADT's "always-on" GICR entries.
> > > > > +	 * Otherwise, prevent such CPUs from being brought online.
> > > > > +	 */
> > > > > +	if (!(gicc->flags & ACPI_MADT_ENABLED)) {
> > > > > +		pr_warn_once("CPU %u's redistributor is inaccessible: this CPU can't be brought online\n", cpu);
> > > > > +		set_cpu_present(cpu, false);
> > > > > +		set_cpu_possible(cpu, false);
> > > > > +		return 0;
> > > > > +	}      
> > > 
> > > It seems dangerous to clear those this late in the game, given how
> > > disconnected from the architecture code this is. Are we sure that
> > > nothing has sampled these cpumasks beforehand?    
> > 
> > Hi Marc,
> > 
> > Any firmware that does this is being considered as buggy already
> > but given it is firmware and the spec doesn't say much about this,
> > there is always the possibility.
> > 
> > Not much happens between the point where these are setup and
> > the point where the the gic inits and this code runs, but even if careful
> > review showed it was fine today, it will be fragile to future changes.
> > 
> > I'm not sure there is a huge disadvantage for such broken firmware in
> > clearing these masks from the point of view of what is used throughout
> > the rest of the kernel. Here I think we are just looking to prevent the CPU
> > being onlined later.
> > 
> > We could add a set_cpu_broken() with appropriate mask.
> > Given this is very arm64 specific I'm not sure Rafael will be keen on
> > us checking such a mask in the generic ACPI code, but we could check it in
> > arch_register_cpu() and just not register the cpu if it matches.
> > That will cover the vCPU hotplug case.
> > 
> > Does that sounds sensible, or would you prefer something else?  
> 
> Hi Marc
> 
> Some experiments later (faking this on a physical board - I never liked
> CPU 120 anyway!) and using a different mask brings it's own minor pain.
> 
> When all the rest of the CPUs are brought up cpuhp_bringup_mask() is called
> on cpu_present_mask so we need to do a dance in there to use a temporary
> mask with broken cpus removed.  I think it makes sense to cut that out
> at the top of the cpuhp_bringup_mask() pile of actions rather than trying
> to paper over each actual thing that is dying... (looks like an infinite loop
> somewhere but I haven't tracked down where yet).
> 
> I'll spin a patch so you can see what it looks like, but my concern is
> we are just moving the risk from early users of these masks to later cases
> where code assumes cpu_present_mask definitely means they are present.
> That is probably a small set of cases but not nice either.
> 
> Looks like one of those cases where we need to pick the lesser of two evils
> which is probably still the cpu_broken_mask approach.
> 
> On plus side if we decide to go back to the original approach having seen
> that I already have the code :)
> 
> Jonathan
> 

Patch on top of this series.  If no one shouts before I have it ready I'll
roll a v8 with the mask introduction as a new patch and the other changes pushed into
appropriate patches.

From 361b76f36bfb4ff74fdceca7ebf14cfa43cae4a9 Mon Sep 17 00:00:00 2001
From: Jonathan Cameron <[e-mail]>
Date: Wed, 24 Apr 2024 17:42:49 +0100
Subject: [PATCH] cpu: Add broken cpu mask to mark CPUs where inconsistent
 firmware means we can't start them.

On ARM64, it is not currently possible to use CPUs where the GICC entry
in ACPI specifies that it is online capable but not enabled. Only
always enabled entries are supported.

Previously if this condition was met, the present and possible cpu masks
were cleared for the relevant cpus.  However, those masks may already
have been used by other code so this is not known to be safe.

An alternative is to use an additional mask (broken) and check that
in the subset of places where these CPUs might be onlined or the
infrastructure to indicate this is possible created.
Specifically in bringup_nonboot_cpus() and in arch_register_cpu().

Signed-off-by: Jonathan Cameron <[e-mail]>
---
 arch/arm64/kernel/smp.c      |  3 +++
 drivers/irqchip/irq-gic-v3.c |  3 +--
 include/linux/cpumask.h      | 19 +++++++++++++++++++
 kernel/cpu.c                 |  8 +++++++-
 4 files changed, 30 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index ccb6ad347df9..39cd6a7c40d8 100644
--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c
@@ -513,6 +513,9 @@ int arch_register_cpu(int cpu)
 	    IS_ENABLED(CONFIG_ACPI_HOTPLUG_CPU))
 		return -EPROBE_DEFER;
 
+	if (cpu_broken(cpu)) /* Inconsistent firmware - can't online */
+		return -ENODEV;
+
 #ifdef CONFIG_ACPI_HOTPLUG_CPU
 	/* For now block anything that looks like physical CPU Hotplug */
 	if (invalid_logical_cpuid(cpu) || !cpu_present(cpu)) {
diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index 66132251c1bb..a0063eb6484d 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -2377,8 +2377,7 @@ gic_acpi_parse_madt_gicc(union acpi_subtable_headers *header,
 	 */
 	if (!(gicc->flags & ACPI_MADT_ENABLED)) {
 		pr_warn_once("CPU %u's redistributor is inaccessible: this CPU can't be brought online\n", cpu);
-		set_cpu_present(cpu, false);
-		set_cpu_possible(cpu, false);
+		set_cpu_broken(cpu);
 		return 0;
 	}
 
diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
index 4b202b94c97a..70a93ad8e590 100644
--- a/include/linux/cpumask.h
+++ b/include/linux/cpumask.h
@@ -96,6 +96,7 @@ static inline void set_nr_cpu_ids(unsigned int nr)
  *     cpu_enabled_mask  - has bit 'cpu' set iff cpu can be brought online
  *     cpu_online_mask  - has bit 'cpu' set iff cpu available to scheduler
  *     cpu_active_mask  - has bit 'cpu' set iff cpu available to migration
+ *     cpu_broken_mask  - has bit 'cpu' set iff the cpu should never be onlined
  *
  *  If !CONFIG_HOTPLUG_CPU, present == possible, and active == online.
  *
@@ -130,12 +131,14 @@ extern struct cpumask __cpu_enabled_mask;
 extern struct cpumask __cpu_present_mask;
 extern struct cpumask __cpu_active_mask;
 extern struct cpumask __cpu_dying_mask;
+extern struct cpumask __cpu_broken_mask;
 #define cpu_possible_mask ((const struct cpumask *)&__cpu_possible_mask)
 #define cpu_online_mask   ((const struct cpumask *)&__cpu_online_mask)
 #define cpu_enabled_mask   ((const struct cpumask *)&__cpu_enabled_mask)
 #define cpu_present_mask  ((const struct cpumask *)&__cpu_present_mask)
 #define cpu_active_mask   ((const struct cpumask *)&__cpu_active_mask)
 #define cpu_dying_mask    ((const struct cpumask *)&__cpu_dying_mask)
+#define cpu_broken_mask   ((const struct cpumask *)&__cpu_broken_mask)
 
 extern atomic_t __num_online_cpus;
 
@@ -1073,6 +1076,12 @@ set_cpu_dying(unsigned int cpu, bool dying)
 		cpumask_clear_cpu(cpu, &__cpu_dying_mask);
 }
 
+static inline void
+set_cpu_broken(unsigned int cpu)
+{
+	cpumask_set_cpu(cpu, &__cpu_broken_mask);
+}
+
 /**
  * to_cpumask - convert a NR_CPUS bitmap to a struct cpumask *
  * @bitmap: the bitmap
@@ -1159,6 +1168,11 @@ static inline bool cpu_dying(unsigned int cpu)
 	return cpumask_test_cpu(cpu, cpu_dying_mask);
 }
 
+static inline bool cpu_broken(unsigned int cpu)
+{
+	return cpumask_test_cpu(cpu, cpu_broken_mask);
+}
+
 #else
 
 #define num_online_cpus()	1U
@@ -1197,6 +1211,11 @@ static inline bool cpu_dying(unsigned int cpu)
 	return false;
 }
 
+static inline bool cpu_broken(unsigned int cpu)
+{
+	return false;
+}
+
 #endif /* NR_CPUS > 1 */
 
 #define cpu_is_offline(cpu)	unlikely(!cpu_online(cpu))
diff --git a/kernel/cpu.c b/kernel/cpu.c
index 537099bf5d02..f8b73a11869e 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -1907,12 +1907,15 @@ static inline bool cpuhp_bringup_cpus_parallel(unsigned int ncpus) { return fals
 
 void __init bringup_nonboot_cpus(unsigned int max_cpus)
 {
+	static const struct cpumask tmp_mask __initdata;
+
 	/* Try parallel bringup optimization if enabled */
 	if (cpuhp_bringup_cpus_parallel(max_cpus))
 		return;
 
+	cpumask_andnot(&tmp_mask, cpu_present_mask, cpu_broken_mask);
 	/* Full per CPU serialized bringup */
-	cpuhp_bringup_mask(cpu_present_mask, max_cpus, CPUHP_ONLINE);
+	cpuhp_bringup_mask(&tmp_mask, max_cpus, CPUHP_ONLINE);
 }
 
 #ifdef CONFIG_PM_SLEEP_SMP
@@ -3129,6 +3132,9 @@ EXPORT_SYMBOL(__cpu_active_mask);
 struct cpumask __cpu_dying_mask __read_mostly;
 EXPORT_SYMBOL(__cpu_dying_mask);
 
+struct cpumask __cpu_broken_mask __ro_after_init;
+EXPORT_SYMBOL(__cpu_broken_mask);
+
 atomic_t __num_online_cpus __read_mostly;
 EXPORT_SYMBOL(__num_online_cpus);

Jonathan Cameron April 25, 2024, 10:13 a.m. UTC | #9

On Thu, 25 Apr 2024 10:56:37 +0100
Jonathan Cameron <[e-mail]> wrote:

> On Thu, 25 Apr 2024 10:28:06 +0100
> Jonathan Cameron <[e-mail]> wrote:
> 
> > On Wed, 24 Apr 2024 13:54:38 +0100
> > Jonathan Cameron <[e-mail]> wrote:
> >   
> > > On Tue, 23 Apr 2024 13:01:21 +0100
> > > Marc Zyngier <[e-mail]> wrote:
> > >     
> > > > On Mon, 22 Apr 2024 11:40:20 +0100,
> > > > Jonathan Cameron <[e-mail]> wrote:      
> > > > > 
> > > > > On Thu, 18 Apr 2024 14:54:07 +0100
> > > > > Jonathan Cameron <[e-mail]> wrote:
> > > > >         
> > > > > > From: James Morse <[e-mail]>
> > > > > > 
> > > > > > To support virtual CPU hotplug, ACPI has added an 'online capable' bit
> > > > > > to the MADT GICC entries. This indicates a disabled CPU entry may not
> > > > > > be possible to online via PSCI until firmware has set enabled bit in
> > > > > > _STA.
> > > > > > 
> > > > > > This means that a "usable" GIC is one that is marked as either enabled,
> > > > > > or online capable. Therefore, change acpi_gicc_is_usable() to check both
> > > > > > bits. However, we need to change the test in gic_acpi_match_gicc() back
> > > > > > to testing just the enabled bit so the count of enabled distributors is
> > > > > > correct.
> > > > > > 
> > > > > > What about the redistributor in the GICC entry? ACPI doesn't want to say.
> > > > > > Assume the worst: When a redistributor is described in the GICC entry,
> > > > > > but the entry is marked as disabled at boot, assume the redistributor
> > > > > > is inaccessible.
> > > > > > 
> > > > > > The GICv3 driver doesn't support late online of redistributors, so this
> > > > > > means the corresponding CPU can't be brought online either. Clear the
> > > > > > possible and present bits.
> > > > > > 
> > > > > > Systems that want CPU hotplug in a VM can ensure their redistributors
> > > > > > are always-on, and describe them that way with a GICR entry in the MADT.
> > > > > > 
> > > > > > When mapping redistributors found via GICC entries, handle the case
> > > > > > where the arch code believes the CPU is present and possible, but it
> > > > > > does not have an accessible redistributor. Print a warning and clear
> > > > > > the present and possible bits.
> > > > > > 
> > > > > > Signed-off-by: James Morse <[e-mail]>
> > > > > > Signed-off-by: Russell King (Oracle) <[e-mail]>
> > > > > > Signed-off-by: Jonathan Cameron <[e-mail]>        
> > > > > 
> > > > > +CC Marc,
> > > > > 
> > > > > Whilst this has been unchanged for a long time, I'm not 100% sure
> > > > > we've specifically drawn your attention to it before now.
> > > > > 
> > > > > Jonathan
> > > > >         
> > > > > > 
> > > > > > ---
> > > > > > v7: No Change.
> > > > > > ---
> > > > > >  drivers/irqchip/irq-gic-v3.c | 21 +++++++++++++++++++--
> > > > > >  include/linux/acpi.h         |  3 ++-
> > > > > >  2 files changed, 21 insertions(+), 3 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> > > > > > index 10af15f93d4d..66132251c1bb 100644
> > > > > > --- a/drivers/irqchip/irq-gic-v3.c
> > > > > > +++ b/drivers/irqchip/irq-gic-v3.c
> > > > > > @@ -2363,11 +2363,25 @@ gic_acpi_parse_madt_gicc(union acpi_subtable_headers *header,
> > > > > >  				(struct acpi_madt_generic_interrupt *)header;
> > > > > >  	u32 reg = readl_relaxed(acpi_data.dist_base + GICD_PIDR2) & GIC_PIDR2_ARCH_MASK;
> > > > > >  	u32 size = reg == GIC_PIDR2_ARCH_GICv4 ? SZ_64K * 4 : SZ_64K * 2;
> > > > > > +	int cpu = get_cpu_for_acpi_id(gicc->uid);
> > > > > >  	void __iomem *redist_base;
> > > > > >  
> > > > > >  	if (!acpi_gicc_is_usable(gicc))
> > > > > >  		return 0;
> > > > > >  
> > > > > > +	/*
> > > > > > +	 * Capable but disabled CPUs can be brought online later. What about
> > > > > > +	 * the redistributor? ACPI doesn't want to say!
> > > > > > +	 * Virtual hotplug systems can use the MADT's "always-on" GICR entries.
> > > > > > +	 * Otherwise, prevent such CPUs from being brought online.
> > > > > > +	 */
> > > > > > +	if (!(gicc->flags & ACPI_MADT_ENABLED)) {
> > > > > > +		pr_warn_once("CPU %u's redistributor is inaccessible: this CPU can't be brought online\n", cpu);
> > > > > > +		set_cpu_present(cpu, false);
> > > > > > +		set_cpu_possible(cpu, false);
> > > > > > +		return 0;
> > > > > > +	}        
> > > > 
> > > > It seems dangerous to clear those this late in the game, given how
> > > > disconnected from the architecture code this is. Are we sure that
> > > > nothing has sampled these cpumasks beforehand?      
> > > 
> > > Hi Marc,
> > > 
> > > Any firmware that does this is being considered as buggy already
> > > but given it is firmware and the spec doesn't say much about this,
> > > there is always the possibility.
> > > 
> > > Not much happens between the point where these are setup and
> > > the point where the the gic inits and this code runs, but even if careful
> > > review showed it was fine today, it will be fragile to future changes.
> > > 
> > > I'm not sure there is a huge disadvantage for such broken firmware in
> > > clearing these masks from the point of view of what is used throughout
> > > the rest of the kernel. Here I think we are just looking to prevent the CPU
> > > being onlined later.
> > > 
> > > We could add a set_cpu_broken() with appropriate mask.
> > > Given this is very arm64 specific I'm not sure Rafael will be keen on
> > > us checking such a mask in the generic ACPI code, but we could check it in
> > > arch_register_cpu() and just not register the cpu if it matches.
> > > That will cover the vCPU hotplug case.
> > > 
> > > Does that sounds sensible, or would you prefer something else?    
> > 
> > Hi Marc
> > 
> > Some experiments later (faking this on a physical board - I never liked
> > CPU 120 anyway!) and using a different mask brings it's own minor pain.
> > 
> > When all the rest of the CPUs are brought up cpuhp_bringup_mask() is called
> > on cpu_present_mask so we need to do a dance in there to use a temporary
> > mask with broken cpus removed.  I think it makes sense to cut that out
> > at the top of the cpuhp_bringup_mask() pile of actions rather than trying
> > to paper over each actual thing that is dying... (looks like an infinite loop
> > somewhere but I haven't tracked down where yet).
> > 
> > I'll spin a patch so you can see what it looks like, but my concern is
> > we are just moving the risk from early users of these masks to later cases
> > where code assumes cpu_present_mask definitely means they are present.
> > That is probably a small set of cases but not nice either.
> > 
> > Looks like one of those cases where we need to pick the lesser of two evils
> > which is probably still the cpu_broken_mask approach.
> > 
> > On plus side if we decide to go back to the original approach having seen
> > that I already have the code :)
> > 
> > Jonathan
> >   
> 
> Patch on top of this series.  If no one shouts before I have it ready I'll
> roll a v8 with the mask introduction as a new patch and the other changes pushed into
> appropriate patches.
> 
> From 361b76f36bfb4ff74fdceca7ebf14cfa43cae4a9 Mon Sep 17 00:00:00 2001
> From: Jonathan Cameron <[e-mail]>
> Date: Wed, 24 Apr 2024 17:42:49 +0100
> Subject: [PATCH] cpu: Add broken cpu mask to mark CPUs where inconsistent
>  firmware means we can't start them.
> 
> On ARM64, it is not currently possible to use CPUs where the GICC entry
> in ACPI specifies that it is online capable but not enabled. Only
> always enabled entries are supported.
> 
> Previously if this condition was met, the present and possible cpu masks
> were cleared for the relevant cpus.  However, those masks may already
> have been used by other code so this is not known to be safe.
> 
> An alternative is to use an additional mask (broken) and check that
> in the subset of places where these CPUs might be onlined or the
> infrastructure to indicate this is possible created.
> Specifically in bringup_nonboot_cpus() and in arch_register_cpu().
> 
> Signed-off-by: Jonathan Cameron <[e-mail]>
Obviously I'd missed Marc's reply on keeping this local to gicv3.
Will give that a go.

Sorry for the noise!

Jonathan

> ---
>  arch/arm64/kernel/smp.c      |  3 +++
>  drivers/irqchip/irq-gic-v3.c |  3 +--
>  include/linux/cpumask.h      | 19 +++++++++++++++++++
>  kernel/cpu.c                 |  8 +++++++-
>  4 files changed, 30 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
> index ccb6ad347df9..39cd6a7c40d8 100644
> --- a/arch/arm64/kernel/smp.c
> +++ b/arch/arm64/kernel/smp.c
> @@ -513,6 +513,9 @@ int arch_register_cpu(int cpu)
>  	    IS_ENABLED(CONFIG_ACPI_HOTPLUG_CPU))
>  		return -EPROBE_DEFER;
>  
> +	if (cpu_broken(cpu)) /* Inconsistent firmware - can't online */
> +		return -ENODEV;
> +
>  #ifdef CONFIG_ACPI_HOTPLUG_CPU
>  	/* For now block anything that looks like physical CPU Hotplug */
>  	if (invalid_logical_cpuid(cpu) || !cpu_present(cpu)) {
> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> index 66132251c1bb..a0063eb6484d 100644
> --- a/drivers/irqchip/irq-gic-v3.c
> +++ b/drivers/irqchip/irq-gic-v3.c
> @@ -2377,8 +2377,7 @@ gic_acpi_parse_madt_gicc(union acpi_subtable_headers *header,
>  	 */
>  	if (!(gicc->flags & ACPI_MADT_ENABLED)) {
>  		pr_warn_once("CPU %u's redistributor is inaccessible: this CPU can't be brought online\n", cpu);
> -		set_cpu_present(cpu, false);
> -		set_cpu_possible(cpu, false);
> +		set_cpu_broken(cpu);
>  		return 0;
>  	}
>  
> diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
> index 4b202b94c97a..70a93ad8e590 100644
> --- a/include/linux/cpumask.h
> +++ b/include/linux/cpumask.h
> @@ -96,6 +96,7 @@ static inline void set_nr_cpu_ids(unsigned int nr)
>   *     cpu_enabled_mask  - has bit 'cpu' set iff cpu can be brought online
>   *     cpu_online_mask  - has bit 'cpu' set iff cpu available to scheduler
>   *     cpu_active_mask  - has bit 'cpu' set iff cpu available to migration
> + *     cpu_broken_mask  - has bit 'cpu' set iff the cpu should never be onlined
>   *
>   *  If !CONFIG_HOTPLUG_CPU, present == possible, and active == online.
>   *
> @@ -130,12 +131,14 @@ extern struct cpumask __cpu_enabled_mask;
>  extern struct cpumask __cpu_present_mask;
>  extern struct cpumask __cpu_active_mask;
>  extern struct cpumask __cpu_dying_mask;
> +extern struct cpumask __cpu_broken_mask;
>  #define cpu_possible_mask ((const struct cpumask *)&__cpu_possible_mask)
>  #define cpu_online_mask   ((const struct cpumask *)&__cpu_online_mask)
>  #define cpu_enabled_mask   ((const struct cpumask *)&__cpu_enabled_mask)
>  #define cpu_present_mask  ((const struct cpumask *)&__cpu_present_mask)
>  #define cpu_active_mask   ((const struct cpumask *)&__cpu_active_mask)
>  #define cpu_dying_mask    ((const struct cpumask *)&__cpu_dying_mask)
> +#define cpu_broken_mask   ((const struct cpumask *)&__cpu_broken_mask)
>  
>  extern atomic_t __num_online_cpus;
>  
> @@ -1073,6 +1076,12 @@ set_cpu_dying(unsigned int cpu, bool dying)
>  		cpumask_clear_cpu(cpu, &__cpu_dying_mask);
>  }
>  
> +static inline void
> +set_cpu_broken(unsigned int cpu)
> +{
> +	cpumask_set_cpu(cpu, &__cpu_broken_mask);
> +}
> +
>  /**
>   * to_cpumask - convert a NR_CPUS bitmap to a struct cpumask *
>   * @bitmap: the bitmap
> @@ -1159,6 +1168,11 @@ static inline bool cpu_dying(unsigned int cpu)
>  	return cpumask_test_cpu(cpu, cpu_dying_mask);
>  }
>  
> +static inline bool cpu_broken(unsigned int cpu)
> +{
> +	return cpumask_test_cpu(cpu, cpu_broken_mask);
> +}
> +
>  #else
>  
>  #define num_online_cpus()	1U
> @@ -1197,6 +1211,11 @@ static inline bool cpu_dying(unsigned int cpu)
>  	return false;
>  }
>  
> +static inline bool cpu_broken(unsigned int cpu)
> +{
> +	return false;
> +}
> +
>  #endif /* NR_CPUS > 1 */
>  
>  #define cpu_is_offline(cpu)	unlikely(!cpu_online(cpu))
> diff --git a/kernel/cpu.c b/kernel/cpu.c
> index 537099bf5d02..f8b73a11869e 100644
> --- a/kernel/cpu.c
> +++ b/kernel/cpu.c
> @@ -1907,12 +1907,15 @@ static inline bool cpuhp_bringup_cpus_parallel(unsigned int ncpus) { return fals
>  
>  void __init bringup_nonboot_cpus(unsigned int max_cpus)
>  {
> +	static const struct cpumask tmp_mask __initdata;
> +
>  	/* Try parallel bringup optimization if enabled */
>  	if (cpuhp_bringup_cpus_parallel(max_cpus))
>  		return;
>  
> +	cpumask_andnot(&tmp_mask, cpu_present_mask, cpu_broken_mask);
>  	/* Full per CPU serialized bringup */
> -	cpuhp_bringup_mask(cpu_present_mask, max_cpus, CPUHP_ONLINE);
> +	cpuhp_bringup_mask(&tmp_mask, max_cpus, CPUHP_ONLINE);
>  }
>  
>  #ifdef CONFIG_PM_SLEEP_SMP
> @@ -3129,6 +3132,9 @@ EXPORT_SYMBOL(__cpu_active_mask);
>  struct cpumask __cpu_dying_mask __read_mostly;
>  EXPORT_SYMBOL(__cpu_dying_mask);
>  
> +struct cpumask __cpu_broken_mask __ro_after_init;
> +EXPORT_SYMBOL(__cpu_broken_mask);
> +
>  atomic_t __num_online_cpus __read_mostly;
>  EXPORT_SYMBOL(__num_online_cpus);
>

Jonathan Cameron April 25, 2024, 10:23 a.m. UTC | #10

On Wed, 24 Apr 2024 18:08:30 +0100
Jonathan Cameron <[e-mail]> wrote:

> On Wed, 24 Apr 2024 17:35:54 +0100
> Salil Mehta <[e-mail]> wrote:
> 
> > >  From: Marc Zyngier <[e-mail]>
> > >  Sent: Wednesday, April 24, 2024 4:33 PM
> > >  To: Jonathan Cameron <[e-mail]>
> > >  Cc: Thomas Gleixner <[e-mail]>; Peter Zijlstra
> > >  <[e-mail]>; [e-mail];
> > >  [e-mail]; [e-mail]; linux-
> > >  [e-mail]; [e-mail]; linux-arm-
> > >  [e-mail]; [e-mail]; [e-mail];
> > >  Russell King <[e-mail]>; Rafael J . Wysocki
> > >  <[e-mail]>; Miguel Luis <[e-mail]>; James Morse
> > >  <[e-mail]>; Salil Mehta <[e-mail]>; Jean-
> > >  Philippe Brucker <[e-mail]>; Catalin Marinas
> > >  <[e-mail]>; Will Deacon <[e-mail]>; Linuxarm
> > >  <[e-mail]>; Ingo Molnar <[e-mail]>; Borislav
> > >  Petkov <[e-mail]>; Dave Hansen <[e-mail]>;
> > >  [e-mail]; [e-mail]
> > >  Subject: Re: [PATCH v7 11/16] irqchip/gic-v3: Add support for ACPI's
> > >  disabled but 'online capable' CPUs
> > >  
> > >  On Wed, 24 Apr 2024 13:54:38 +0100,
> > >  Jonathan Cameron <[e-mail]> wrote:    
> > >  >
> > >  > On Tue, 23 Apr 2024 13:01:21 +0100
> > >  > Marc Zyngier <[e-mail]> wrote:
> > >  >    
> > >  > > On Mon, 22 Apr 2024 11:40:20 +0100,
> > >  > > Jonathan Cameron <[e-mail]> wrote:    
> > >  > > >
> > >  > > > On Thu, 18 Apr 2024 14:54:07 +0100 Jonathan Cameron
> > >  > > > <[e-mail]> wrote:    
> > >  
> > >  [...]
> > >      
> > >  > > >    
> > >  > > > > +	/*
> > >  > > > > +	 * Capable but disabled CPUs can be brought online later.  What about
> > >  > > > > +	 * the redistributor? ACPI doesn't want to say!
> > >  > > > > +	 * Virtual hotplug systems can use the MADT's "always-on"  GICR entries.
> > >  > > > > +	 * Otherwise, prevent such CPUs from being brought online.
> > >  > > > > +	 */
> > >  > > > > +	if (!(gicc->flags & ACPI_MADT_ENABLED)) {
> > >  > > > > +		pr_warn_once("CPU %u's redistributor is  inaccessible: this CPU can't be brought online\n", cpu);
> > >  > > > > +		set_cpu_present(cpu, false);
> > >  > > > > +		set_cpu_possible(cpu, false);    
> > 
> > (a digression) shouldn't we be clearing the enabled mask as well?
> > 
> >                                           set_cpu_enabled(cpu, false);  
> 
> FWIW I think not necessary. enabled is only set in register_cpu() and aim here is to
> never call that for CPUs in this state.
> 
> Anyhow, I got distracted by the firmware bug I found whilst trying to test this but
> now have a test setup that hits this path (once deliberately broken), so will
> see what we can do about that doesn't have affect those masks.

This may be relevant with the context of Marc's email.  Don't crop so much!
However I think we probably don't care. This is bios bug, if we miss report it such
that userspace thinks it can online something that work work, it probably doesn't
matter.

Jonathan

> 
> Jonathan
> 
> 
> > 
> > 
> > Best regards
> > Salil  
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> [e-mail]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Jonathan Cameron April 25, 2024, 12:31 p.m. UTC | #11

On Wed, 24 Apr 2024 16:33:22 +0100
Marc Zyngier <[e-mail]> wrote:

> On Wed, 24 Apr 2024 13:54:38 +0100,
> Jonathan Cameron <[e-mail]> wrote:
> > 
> > On Tue, 23 Apr 2024 13:01:21 +0100
> > Marc Zyngier <[e-mail]> wrote:
> >   
> > > On Mon, 22 Apr 2024 11:40:20 +0100,
> > > Jonathan Cameron <[e-mail]> wrote:  
> > > > 
> > > > On Thu, 18 Apr 2024 14:54:07 +0100
> > > > Jonathan Cameron <[e-mail]> wrote:  
> 
> [...]
> 
> > > >     
> > > > > +	/*
> > > > > +	 * Capable but disabled CPUs can be brought online later. What about
> > > > > +	 * the redistributor? ACPI doesn't want to say!
> > > > > +	 * Virtual hotplug systems can use the MADT's "always-on" GICR entries.
> > > > > +	 * Otherwise, prevent such CPUs from being brought online.
> > > > > +	 */
> > > > > +	if (!(gicc->flags & ACPI_MADT_ENABLED)) {
> > > > > +		pr_warn_once("CPU %u's redistributor is inaccessible: this CPU can't be brought online\n", cpu);
> > > > > +		set_cpu_present(cpu, false);
> > > > > +		set_cpu_possible(cpu, false);
> > > > > +		return 0;
> > > > > +	}    
> > > 
> > > It seems dangerous to clear those this late in the game, given how
> > > disconnected from the architecture code this is. Are we sure that
> > > nothing has sampled these cpumasks beforehand?  
> > 
> > Hi Marc,
> > 
> > Any firmware that does this is being considered as buggy already
> > but given it is firmware and the spec doesn't say much about this,
> > there is always the possibility.  
> 
> There is no shortage of broken firmware out there, and I expect this
> trend to progress.
> 
> > Not much happens between the point where these are setup and
> > the point where the the gic inits and this code runs, but even if careful
> > review showed it was fine today, it will be fragile to future changes.
> > 
> > I'm not sure there is a huge disadvantage for such broken firmware in
> > clearing these masks from the point of view of what is used throughout
> > the rest of the kernel. Here I think we are just looking to prevent the CPU
> > being onlined later.  
> 
> I totally agree on the goal, I simply question the way you get to it.
> 
> > 
> > We could add a set_cpu_broken() with appropriate mask.
> > Given this is very arm64 specific I'm not sure Rafael will be keen on
> > us checking such a mask in the generic ACPI code, but we could check it in
> > arch_register_cpu() and just not register the cpu if it matches.
> > That will cover the vCPU hotplug case.
> >
> > Does that sounds sensible, or would you prefer something else?  
> 
> 
> Such a 'broken_rdists' mask is exactly what I have in mind, just
> keeping it private to the GIC driver, and not expose it anywhere else.
> You can then fail the hotplug event early, and avoid changing the
> global masks from within the GIC driver. At least, we don't mess with
> the internals of the kernel, and the CPU is properly marked as dead
> (that mechanism should already work).
> 
> I'd expect the handling side to look like this (will not compile, but
> you'll get the idea):
Hi Marc,

In general this looks good - but...

I haven't gotten to the bottom of why yet (and it might be a side
effect of how I hacked the test by lying in minimal fashion and
just frigging the MADT read functions) but the hotplug flow is only getting
as far as calling __cpu_up() before it seems to enter an infinite loop.
That is it never gets far enough to fail this test.

Getting stuck in a psci cpu_on call.  I'm guessing something that
we didn't get to in the earlier gicv3 calls before bailing out is blocking that?
Looks like it gets to
SMCCC smc
and is never seen again.

Any ideas on where to look?  The one advantage so far of the higher level
approach is we never tried the hotplug callbacks at all so avoided hitting
that call.  One (little bit horrible) solution that might avoid this would 
be to add another cpuhp state very early on and fail at that stage.
I'm not keen on doing that without a better explanation than I have so far!

Thanks,

J

 
> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> index 6fb276504bcc..e8f02bfd0e21 100644
> --- a/drivers/irqchip/irq-gic-v3.c
> +++ b/drivers/irqchip/irq-gic-v3.c
> @@ -1009,6 +1009,9 @@ static int __gic_populate_rdist(struct redist_region *region, void __iomem *ptr)
>  	u64 typer;
>  	u32 aff;
>  
> +	if (cpumask_test_cpu(smp_processor_id(), &broken_rdists))
> +		return 1;
> +
>  	/*
>  	 * Convert affinity to a 32bit value that can be matched to
>  	 * GICR_TYPER bits [63:32].
> @@ -1260,14 +1263,15 @@ static int gic_dist_supports_lpis(void)
>  		!gicv3_nolpi);
>  }
>  
> -static void gic_cpu_init(void)
> +static int gic_cpu_init(void)
>  {
>  	void __iomem *rbase;
> -	int i;
> +	int ret, i;
>  
>  	/* Register ourselves with the rest of the world */
> -	if (gic_populate_rdist())
> -		return;
> +	ret = gic_populate_rdist();
> +	if (ret)
> +		return ret;
>  
>  	gic_enable_redist(true);
>  
> @@ -1286,6 +1290,8 @@ static void gic_cpu_init(void)
>  
>  	/* initialise system registers */
>  	gic_cpu_sys_reg_init();
> +
> +	return 0;
>  }
>  
>  #ifdef CONFIG_SMP
> @@ -1295,7 +1301,11 @@ static void gic_cpu_init(void)
>  
>  static int gic_starting_cpu(unsigned int cpu)
>  {
> -	gic_cpu_init();
> +	int ret;
> +
> +	ret = gic_cpu_init();
> +	if (ret)
> +		return ret;
>  
>  	if (gic_dist_supports_lpis())
>  		its_cpu_init();
> 
> But the question is: do you rely on these masks having been
> "corrected" anywhere else?
> 
> Thanks,
> 
> 	M.
>

Jonathan Cameron April 25, 2024, 3 p.m. UTC | #12

On Thu, 25 Apr 2024 13:31:50 +0100
Jonathan Cameron <[e-mail]> wrote:

> On Wed, 24 Apr 2024 16:33:22 +0100
> Marc Zyngier <[e-mail]> wrote:
> 
> > On Wed, 24 Apr 2024 13:54:38 +0100,
> > Jonathan Cameron <[e-mail]> wrote:  
> > > 
> > > On Tue, 23 Apr 2024 13:01:21 +0100
> > > Marc Zyngier <[e-mail]> wrote:
> > >     
> > > > On Mon, 22 Apr 2024 11:40:20 +0100,
> > > > Jonathan Cameron <[e-mail]> wrote:    
> > > > > 
> > > > > On Thu, 18 Apr 2024 14:54:07 +0100
> > > > > Jonathan Cameron <[e-mail]> wrote:    
> > 
> > [...]
> >   
> > > > >       
> > > > > > +	/*
> > > > > > +	 * Capable but disabled CPUs can be brought online later. What about
> > > > > > +	 * the redistributor? ACPI doesn't want to say!
> > > > > > +	 * Virtual hotplug systems can use the MADT's "always-on" GICR entries.
> > > > > > +	 * Otherwise, prevent such CPUs from being brought online.
> > > > > > +	 */
> > > > > > +	if (!(gicc->flags & ACPI_MADT_ENABLED)) {
> > > > > > +		pr_warn_once("CPU %u's redistributor is inaccessible: this CPU can't be brought online\n", cpu);
> > > > > > +		set_cpu_present(cpu, false);
> > > > > > +		set_cpu_possible(cpu, false);
> > > > > > +		return 0;
> > > > > > +	}      
> > > > 
> > > > It seems dangerous to clear those this late in the game, given how
> > > > disconnected from the architecture code this is. Are we sure that
> > > > nothing has sampled these cpumasks beforehand?    
> > > 
> > > Hi Marc,
> > > 
> > > Any firmware that does this is being considered as buggy already
> > > but given it is firmware and the spec doesn't say much about this,
> > > there is always the possibility.    
> > 
> > There is no shortage of broken firmware out there, and I expect this
> > trend to progress.
> >   
> > > Not much happens between the point where these are setup and
> > > the point where the the gic inits and this code runs, but even if careful
> > > review showed it was fine today, it will be fragile to future changes.
> > > 
> > > I'm not sure there is a huge disadvantage for such broken firmware in
> > > clearing these masks from the point of view of what is used throughout
> > > the rest of the kernel. Here I think we are just looking to prevent the CPU
> > > being onlined later.    
> > 
> > I totally agree on the goal, I simply question the way you get to it.
> >   
> > > 
> > > We could add a set_cpu_broken() with appropriate mask.
> > > Given this is very arm64 specific I'm not sure Rafael will be keen on
> > > us checking such a mask in the generic ACPI code, but we could check it in
> > > arch_register_cpu() and just not register the cpu if it matches.
> > > That will cover the vCPU hotplug case.
> > >
> > > Does that sounds sensible, or would you prefer something else?    
> > 
> > 
> > Such a 'broken_rdists' mask is exactly what I have in mind, just
> > keeping it private to the GIC driver, and not expose it anywhere else.
> > You can then fail the hotplug event early, and avoid changing the
> > global masks from within the GIC driver. At least, we don't mess with
> > the internals of the kernel, and the CPU is properly marked as dead
> > (that mechanism should already work).
> > 
> > I'd expect the handling side to look like this (will not compile, but
> > you'll get the idea):  
> Hi Marc,
> 
> In general this looks good - but...
> 
> I haven't gotten to the bottom of why yet (and it might be a side
> effect of how I hacked the test by lying in minimal fashion and
> just frigging the MADT read functions) but the hotplug flow is only getting
> as far as calling __cpu_up() before it seems to enter an infinite loop.
> That is it never gets far enough to fail this test.
> 
> Getting stuck in a psci cpu_on call.  I'm guessing something that
> we didn't get to in the earlier gicv3 calls before bailing out is blocking that?
> Looks like it gets to
> SMCCC smc
> and is never seen again.
> 
> Any ideas on where to look?  The one advantage so far of the higher level
> approach is we never tried the hotplug callbacks at all so avoided hitting
> that call.  One (little bit horrible) solution that might avoid this would 
> be to add another cpuhp state very early on and fail at that stage.
> I'm not keen on doing that without a better explanation than I have so far!

Whilst it still doesn't work I suspect I'm loosing ability to print to the console
between that point and somewhat later and real problem is elsewhere.

Jonathan

> 
> Thanks,
> 
> J
> 
>  
> > diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> > index 6fb276504bcc..e8f02bfd0e21 100644
> > --- a/drivers/irqchip/irq-gic-v3.c
> > +++ b/drivers/irqchip/irq-gic-v3.c
> > @@ -1009,6 +1009,9 @@ static int __gic_populate_rdist(struct redist_region *region, void __iomem *ptr)
> >  	u64 typer;
> >  	u32 aff;
> >  
> > +	if (cpumask_test_cpu(smp_processor_id(), &broken_rdists))
> > +		return 1;
> > +
> >  	/*
> >  	 * Convert affinity to a 32bit value that can be matched to
> >  	 * GICR_TYPER bits [63:32].
> > @@ -1260,14 +1263,15 @@ static int gic_dist_supports_lpis(void)
> >  		!gicv3_nolpi);
> >  }
> >  
> > -static void gic_cpu_init(void)
> > +static int gic_cpu_init(void)
> >  {
> >  	void __iomem *rbase;
> > -	int i;
> > +	int ret, i;
> >  
> >  	/* Register ourselves with the rest of the world */
> > -	if (gic_populate_rdist())
> > -		return;
> > +	ret = gic_populate_rdist();
> > +	if (ret)
> > +		return ret;
> >  
> >  	gic_enable_redist(true);
> >  
> > @@ -1286,6 +1290,8 @@ static void gic_cpu_init(void)
> >  
> >  	/* initialise system registers */
> >  	gic_cpu_sys_reg_init();
> > +
> > +	return 0;
> >  }
> >  
> >  #ifdef CONFIG_SMP
> > @@ -1295,7 +1301,11 @@ static void gic_cpu_init(void)
> >  
> >  static int gic_starting_cpu(unsigned int cpu)
> >  {
> > -	gic_cpu_init();
> > +	int ret;
> > +
> > +	ret = gic_cpu_init();
> > +	if (ret)
> > +		return ret;
> >  
> >  	if (gic_dist_supports_lpis())
> >  		its_cpu_init();
> > 
> > But the question is: do you rely on these masks having been
> > "corrected" anywhere else?
> > 
> > Thanks,
> > 
> > 	M.
> >   
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> [e-mail]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Jonathan Cameron April 25, 2024, 4:55 p.m. UTC | #13

On Thu, 25 Apr 2024 16:00:17 +0100
Jonathan Cameron <[e-mail]> wrote:

> On Thu, 25 Apr 2024 13:31:50 +0100
> Jonathan Cameron <[e-mail]> wrote:
> 
> > On Wed, 24 Apr 2024 16:33:22 +0100
> > Marc Zyngier <[e-mail]> wrote:
> >   
> > > On Wed, 24 Apr 2024 13:54:38 +0100,
> > > Jonathan Cameron <[e-mail]> wrote:    
> > > > 
> > > > On Tue, 23 Apr 2024 13:01:21 +0100
> > > > Marc Zyngier <[e-mail]> wrote:
> > > >       
> > > > > On Mon, 22 Apr 2024 11:40:20 +0100,
> > > > > Jonathan Cameron <[e-mail]> wrote:      
> > > > > > 
> > > > > > On Thu, 18 Apr 2024 14:54:07 +0100
> > > > > > Jonathan Cameron <[e-mail]> wrote:      
> > > 
> > > [...]
> > >     
> > > > > >         
> > > > > > > +	/*
> > > > > > > +	 * Capable but disabled CPUs can be brought online later. What about
> > > > > > > +	 * the redistributor? ACPI doesn't want to say!
> > > > > > > +	 * Virtual hotplug systems can use the MADT's "always-on" GICR entries.
> > > > > > > +	 * Otherwise, prevent such CPUs from being brought online.
> > > > > > > +	 */
> > > > > > > +	if (!(gicc->flags & ACPI_MADT_ENABLED)) {
> > > > > > > +		pr_warn_once("CPU %u's redistributor is inaccessible: this CPU can't be brought online\n", cpu);
> > > > > > > +		set_cpu_present(cpu, false);
> > > > > > > +		set_cpu_possible(cpu, false);
> > > > > > > +		return 0;
> > > > > > > +	}        
> > > > > 
> > > > > It seems dangerous to clear those this late in the game, given how
> > > > > disconnected from the architecture code this is. Are we sure that
> > > > > nothing has sampled these cpumasks beforehand?      
> > > > 
> > > > Hi Marc,
> > > > 
> > > > Any firmware that does this is being considered as buggy already
> > > > but given it is firmware and the spec doesn't say much about this,
> > > > there is always the possibility.      
> > > 
> > > There is no shortage of broken firmware out there, and I expect this
> > > trend to progress.
> > >     
> > > > Not much happens between the point where these are setup and
> > > > the point where the the gic inits and this code runs, but even if careful
> > > > review showed it was fine today, it will be fragile to future changes.
> > > > 
> > > > I'm not sure there is a huge disadvantage for such broken firmware in
> > > > clearing these masks from the point of view of what is used throughout
> > > > the rest of the kernel. Here I think we are just looking to prevent the CPU
> > > > being onlined later.      
> > > 
> > > I totally agree on the goal, I simply question the way you get to it.
> > >     
> > > > 
> > > > We could add a set_cpu_broken() with appropriate mask.
> > > > Given this is very arm64 specific I'm not sure Rafael will be keen on
> > > > us checking such a mask in the generic ACPI code, but we could check it in
> > > > arch_register_cpu() and just not register the cpu if it matches.
> > > > That will cover the vCPU hotplug case.
> > > >
> > > > Does that sounds sensible, or would you prefer something else?      
> > > 
> > > 
> > > Such a 'broken_rdists' mask is exactly what I have in mind, just
> > > keeping it private to the GIC driver, and not expose it anywhere else.
> > > You can then fail the hotplug event early, and avoid changing the
> > > global masks from within the GIC driver. At least, we don't mess with
> > > the internals of the kernel, and the CPU is properly marked as dead
> > > (that mechanism should already work).
> > > 
> > > I'd expect the handling side to look like this (will not compile, but
> > > you'll get the idea):    
> > Hi Marc,
> > 
> > In general this looks good - but...
> > 
> > I haven't gotten to the bottom of why yet (and it might be a side
> > effect of how I hacked the test by lying in minimal fashion and
> > just frigging the MADT read functions) but the hotplug flow is only getting
> > as far as calling __cpu_up() before it seems to enter an infinite loop.
> > That is it never gets far enough to fail this test.
> > 
> > Getting stuck in a psci cpu_on call.  I'm guessing something that
> > we didn't get to in the earlier gicv3 calls before bailing out is blocking that?
> > Looks like it gets to
> > SMCCC smc
> > and is never seen again.
> > 
> > Any ideas on where to look?  The one advantage so far of the higher level
> > approach is we never tried the hotplug callbacks at all so avoided hitting
> > that call.  One (little bit horrible) solution that might avoid this would 
> > be to add another cpuhp state very early on and fail at that stage.
> > I'm not keen on doing that without a better explanation than I have so far!  
> 
> Whilst it still doesn't work I suspect I'm loosing ability to print to the console
> between that point and somewhat later and real problem is elsewhere.

Hi again,

Found it I think.  cpuhp calls between cpu:bringup and ap:online 
arm made from notify_cpu_starting() are clearly marked as nofail with a comment.
STARTING must not fail!

https://elixir.bootlin.com/linux/latest/source/kernel/cpu.c#L1642

Whilst I have no immediate idea why that comment is there it is pretty strong
argument against trying to have the CPUHP_AP_IRQ_GIC_STARTING callback fail
and expecting it to carry on working :( 
There would have been a nice print message, but given I don't appear to have
a working console after that stage I never see it.

So the best I have yet come up with for this is the option of a new callback registered
in gic_smp_init()

cpuhp_setup_state_nocalls(CPUHP_BP_PREPARE_DYN,
			  "irqchip/arm/gicv3:checkrdist",
			  gic_broken_rdist, NULL);

with callback being simply 

static int gic_broken_rdist(unsigned int cpu)
{
	if (cpumask_test_cpu(cpu, &broken_rdists))
		return -EINVAL;

	return 0;
}

That gets called cpuhp_up_callbacks() and is allows to fail and roll back the steps.

Not particularly satisfying but keeps the logic confined to the gicv3 driver.

What do you think?

Jonathan

> 
> Jonathan
> 
> > 
> > Thanks,
> > 
> > J
> > 
> >    
> > > diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> > > index 6fb276504bcc..e8f02bfd0e21 100644
> > > --- a/drivers/irqchip/irq-gic-v3.c
> > > +++ b/drivers/irqchip/irq-gic-v3.c
> > > @@ -1009,6 +1009,9 @@ static int __gic_populate_rdist(struct redist_region *region, void __iomem *ptr)
> > >  	u64 typer;
> > >  	u32 aff;
> > >  
> > > +	if (cpumask_test_cpu(smp_processor_id(), &broken_rdists))
> > > +		return 1;
> > > +
> > >  	/*
> > >  	 * Convert affinity to a 32bit value that can be matched to
> > >  	 * GICR_TYPER bits [63:32].
> > > @@ -1260,14 +1263,15 @@ static int gic_dist_supports_lpis(void)
> > >  		!gicv3_nolpi);
> > >  }
> > >  
> > > -static void gic_cpu_init(void)
> > > +static int gic_cpu_init(void)
> > >  {
> > >  	void __iomem *rbase;
> > > -	int i;
> > > +	int ret, i;
> > >  
> > >  	/* Register ourselves with the rest of the world */
> > > -	if (gic_populate_rdist())
> > > -		return;
> > > +	ret = gic_populate_rdist();
> > > +	if (ret)
> > > +		return ret;
> > >  
> > >  	gic_enable_redist(true);
> > >  
> > > @@ -1286,6 +1290,8 @@ static void gic_cpu_init(void)
> > >  
> > >  	/* initialise system registers */
> > >  	gic_cpu_sys_reg_init();
> > > +
> > > +	return 0;
> > >  }
> > >  
> > >  #ifdef CONFIG_SMP
> > > @@ -1295,7 +1301,11 @@ static void gic_cpu_init(void)
> > >  
> > >  static int gic_starting_cpu(unsigned int cpu)
> > >  {
> > > -	gic_cpu_init();
> > > +	int ret;
> > > +
> > > +	ret = gic_cpu_init();
> > > +	if (ret)
> > > +		return ret;
> > >  
> > >  	if (gic_dist_supports_lpis())
> > >  		its_cpu_init();
> > > 
> > > But the question is: do you rely on these masks having been
> > > "corrected" anywhere else?
> > > 
> > > Thanks,
> > > 
> > > 	M.
> > >     
> > 
> > 
> > _______________________________________________
> > linux-arm-kernel mailing list
> > [e-mail]
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel  
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> [e-mail]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Marc Zyngier April 26, 2024, 12:41 p.m. UTC | #14

On Thu, 25 Apr 2024 17:55:27 +0100,
Jonathan Cameron <[e-mail]> wrote:
> 
> On Thu, 25 Apr 2024 16:00:17 +0100
> Jonathan Cameron <[e-mail]> wrote:
> 
> > On Thu, 25 Apr 2024 13:31:50 +0100
> > Jonathan Cameron <[e-mail]> wrote:
> > 
> > > On Wed, 24 Apr 2024 16:33:22 +0100
> > > Marc Zyngier <[e-mail]> wrote:

[...]

> > >   
> > > > I'd expect the handling side to look like this (will not compile, but
> > > > you'll get the idea):    
> > > Hi Marc,
> > > 
> > > In general this looks good - but...
> > > 
> > > I haven't gotten to the bottom of why yet (and it might be a side
> > > effect of how I hacked the test by lying in minimal fashion and
> > > just frigging the MADT read functions) but the hotplug flow is only getting
> > > as far as calling __cpu_up() before it seems to enter an infinite loop.
> > > That is it never gets far enough to fail this test.
> > > 
> > > Getting stuck in a psci cpu_on call.  I'm guessing something that
> > > we didn't get to in the earlier gicv3 calls before bailing out is blocking that?
> > > Looks like it gets to
> > > SMCCC smc
> > > and is never seen again.
> > > 
> > > Any ideas on where to look?  The one advantage so far of the higher level
> > > approach is we never tried the hotplug callbacks at all so avoided hitting
> > > that call.  One (little bit horrible) solution that might avoid this would 
> > > be to add another cpuhp state very early on and fail at that stage.
> > > I'm not keen on doing that without a better explanation than I have so far!  
> > 
> > Whilst it still doesn't work I suspect I'm loosing ability to print to the console
> > between that point and somewhat later and real problem is
> > elsewhere.

Sorry, travelling at the moment, so only spotted this now.

> 
> Hi again,
> 
> Found it I think.  cpuhp calls between cpu:bringup and ap:online 
> arm made from notify_cpu_starting() are clearly marked as nofail with a comment.
> STARTING must not fail!
> 
> https://elixir.bootlin.com/linux/latest/source/kernel/cpu.c#L1642

Ah, now that rings a bell! ;-)

> 
> Whilst I have no immediate idea why that comment is there it is pretty strong
> argument against trying to have the CPUHP_AP_IRQ_GIC_STARTING callback fail
> and expecting it to carry on working :( 
> There would have been a nice print message, but given I don't appear to have
> a working console after that stage I never see it.
> 
> So the best I have yet come up with for this is the option of a new callback registered
> in gic_smp_init()
> 
> cpuhp_setup_state_nocalls(CPUHP_BP_PREPARE_DYN,
> 			  "irqchip/arm/gicv3:checkrdist",
> 			  gic_broken_rdist, NULL);
> 
> with callback being simply 
> 
> static int gic_broken_rdist(unsigned int cpu)
> {
> 	if (cpumask_test_cpu(cpu, &broken_rdists))
> 		return -EINVAL;
> 
> 	return 0;
> }
> 
> That gets called cpuhp_up_callbacks() and is allows to fail and roll back the steps.
> 
> Not particularly satisfying but keeps the logic confined to the gicv3 driver.
> 
> What do you think?

Good enough for me. Cc me on the resulting patch when you repost it so
that I can eyeball it, but this is IMO the right direction.

Thanks,

	M.

Patch

diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index 10af15f93d4d..66132251c1bb 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -2363,11 +2363,25 @@  gic_acpi_parse_madt_gicc(union acpi_subtable_headers *header,
 				(struct acpi_madt_generic_interrupt *)header;
 	u32 reg = readl_relaxed(acpi_data.dist_base + GICD_PIDR2) & GIC_PIDR2_ARCH_MASK;
 	u32 size = reg == GIC_PIDR2_ARCH_GICv4 ? SZ_64K * 4 : SZ_64K * 2;
+	int cpu = get_cpu_for_acpi_id(gicc->uid);
 	void __iomem *redist_base;
 
 	if (!acpi_gicc_is_usable(gicc))
 		return 0;
 
+	/*
+	 * Capable but disabled CPUs can be brought online later. What about
+	 * the redistributor? ACPI doesn't want to say!
+	 * Virtual hotplug systems can use the MADT's "always-on" GICR entries.
+	 * Otherwise, prevent such CPUs from being brought online.
+	 */
+	if (!(gicc->flags & ACPI_MADT_ENABLED)) {
+		pr_warn_once("CPU %u's redistributor is inaccessible: this CPU can't be brought online\n", cpu);
+		set_cpu_present(cpu, false);
+		set_cpu_possible(cpu, false);
+		return 0;
+	}
+
 	redist_base = ioremap(gicc->gicr_base_address, size);
 	if (!redist_base)
 		return -ENOMEM;
@@ -2413,9 +2427,12 @@  static int __init gic_acpi_match_gicc(union acpi_subtable_headers *header,
 
 	/*
 	 * If GICC is enabled and has valid gicr base address, then it means
-	 * GICR base is presented via GICC
+	 * GICR base is presented via GICC. The redistributor is only known to
+	 * be accessible if the GICC is marked as enabled. If this bit is not
+	 * set, we'd need to add the redistributor at runtime, which isn't
+	 * supported.
 	 */
-	if (acpi_gicc_is_usable(gicc) && gicc->gicr_base_address)
+	if (gicc->flags & ACPI_MADT_ENABLED && gicc->gicr_base_address)
 		acpi_data.enabled_rdists++;
 
 	return 0;
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 9844a3f9c4e5..fcfb7bb6789e 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -239,7 +239,8 @@  void acpi_table_print_madt_entry (struct acpi_subtable_header *madt);
 
 static inline bool acpi_gicc_is_usable(struct acpi_madt_generic_interrupt *gicc)
 {
-	return gicc->flags & ACPI_MADT_ENABLED;
+	return gicc->flags & (ACPI_MADT_ENABLED |
+			      ACPI_MADT_GICC_ONLINE_CAPABLE);
 }
 
 /* the following numa functions are architecture-dependent */