diff mbox series

[V2,10/21] RISC-V: smpboot: Add ACPI support in smp_setup()

Message ID 20230216182043.1946553-11-sunilvl@ventanamicro.com (mailing list archive)
State Superseded
Headers show
Series Add basic ACPI support for RISC-V | expand

Checks

Context Check Description
conchuod/tree_selection fail Failed to apply to next/pending-fixes or riscv/for-next

Commit Message

Sunil V L Feb. 16, 2023, 6:20 p.m. UTC 
Enable SMP boot on ACPI based platforms by using the RINTC
structures in the MADT table.

Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
---
 arch/riscv/include/asm/acpi.h |  7 ++++
 arch/riscv/kernel/smpboot.c   | 70 ++++++++++++++++++++++++++++++++++-
 2 files changed, 76 insertions(+), 1 deletion(-)

Comments

Andrew Jones Feb. 20, 2023, 5:08 p.m. UTC | #1 
On Thu, Feb 16, 2023 at 11:50:32PM +0530, Sunil V L wrote:
> Enable SMP boot on ACPI based platforms by using the RINTC
> structures in the MADT table.
> 
> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> ---
>  arch/riscv/include/asm/acpi.h |  7 ++++
>  arch/riscv/kernel/smpboot.c   | 70 ++++++++++++++++++++++++++++++++++-
>  2 files changed, 76 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/riscv/include/asm/acpi.h b/arch/riscv/include/asm/acpi.h
> index 7bc49f65c86b..3c3a8ac3b37a 100644
> --- a/arch/riscv/include/asm/acpi.h
> +++ b/arch/riscv/include/asm/acpi.h
> @@ -60,6 +60,13 @@ static inline void arch_fix_phys_package_id(int num, u32 slot) { }
>  
>  int acpi_get_riscv_isa(struct acpi_table_header *table,
>  		       unsigned int cpu, const char **isa);
> +
> +#ifdef CONFIG_ACPI_NUMA
> +int acpi_numa_get_nid(unsigned int cpu);
> +#else
> +static inline int acpi_numa_get_nid(unsigned int cpu) { return NUMA_NO_NODE; }
> +#endif /* CONFIG_ACPI_NUMA */

The #ifdef stuff seems premature since we're not providing an
implementation for acpi_numa_get_nid() or selecting ACPI_NUMA, but OK.

> +
>  #else
>  static inline int acpi_get_riscv_isa(struct acpi_table_header *table,
>  				     unsigned int cpu, const char **isa)
> diff --git a/arch/riscv/kernel/smpboot.c b/arch/riscv/kernel/smpboot.c
> index 26214ddefaa4..77630f8ed12b 100644
> --- a/arch/riscv/kernel/smpboot.c
> +++ b/arch/riscv/kernel/smpboot.c
> @@ -8,6 +8,7 @@
>   * Copyright (C) 2017 SiFive
>   */
>  
> +#include <linux/acpi.h>
>  #include <linux/arch_topology.h>
>  #include <linux/module.h>
>  #include <linux/init.h>
> @@ -70,6 +71,70 @@ void __init smp_prepare_cpus(unsigned int max_cpus)
>  	}
>  }
>  
> +#ifdef CONFIG_ACPI
> +static unsigned int cpu_count = 1;
> +
> +static int __init acpi_parse_rintc(union acpi_subtable_headers *header, const unsigned long end)
> +{
> +	unsigned long hart;
> +	bool found_boot_cpu = false;

I guess found_boot_cpu should be static?

> +	struct acpi_madt_rintc *processor = (struct acpi_madt_rintc *)header;
> +
> +	/*
> +	 * Each RINTC structure in MADT will have a flag. If ACPI_MADT_ENABLED
> +	 * bit in the flag is not enabled, it means OS should not try to enable
> +	 * the cpu to which RINTC belongs.
> +	 */
> +	if (!(processor->flags & ACPI_MADT_ENABLED))
> +		return 0;
> +
> +	hart = processor->hart_id;
> +	if (hart < 0)
> +		return 0;

A valid hart ID is anything up to INVALID_HARTID, right? Shouldn't we only
be checking for INVALID_HARTID here? And what does it mean to have an
invalid hart ID here? It's not an issue to error/warn about?

> +	if (hart == cpuid_to_hartid_map(0)) {
> +		BUG_ON(found_boot_cpu);

Do we really want to BUG due to bad, but potentially bootable ACPI tables?
I'd BUG for things that can only happen when we break the code, but broken
ACPI tables might be something we want to complain loudly about and then
attempt to limp along.

> +		found_boot_cpu = true;
> +		early_map_cpu_to_node(0, acpi_numa_get_nid(cpu_count));
> +		return 0;
> +	}
> +
> +	if (cpu_count >= NR_CPUS) {
> +		pr_warn("Invalid cpuid [%d] for hartid [%lu]\n",
> +			cpu_count, hart);

cpuid isn't invalid, NR_CPUS is too small for the number of ACPI tables.

> +		return 0;
> +	}
> +
> +	cpuid_to_hartid_map(cpu_count) = hart;
> +	early_map_cpu_to_node(cpu_count, acpi_numa_get_nid(cpu_count));
> +	cpu_count++;
> +
> +	return 0;
> +}
> +
> +static void __init acpi_parse_and_init_cpus(void)
> +{
> +	int cpuid;
> +
> +	cpu_set_ops(0);
> +
> +	/*
> +	 * do a walk of MADT to determine how many CPUs
> +	 * we have including disabled CPUs, and get information
> +	 * we need for SMP init.
> +	 */

I know this comment comes verbatim from arm64, but not only does it
have grammar issues, I'm not sure it's accurate. Where is the count
of disabled CPUs for arm64 or riscv?

> +	acpi_table_parse_madt(ACPI_MADT_TYPE_RINTC, acpi_parse_rintc, 0);
> +
> +	for (cpuid = 1; cpuid < nr_cpu_ids; cpuid++) {
> +		if (cpuid_to_hartid_map(cpuid) != INVALID_HARTID) {
> +			cpu_set_ops(cpuid);
> +			set_cpu_possible(cpuid, true);
> +		}
> +	}
> +}
> +#else
> +#define acpi_parse_and_init_cpus(...)	do { } while (0)
> +#endif
> +
>  static void __init of_parse_and_init_cpus(void)
>  {
>  	struct device_node *dn;
> @@ -118,7 +183,10 @@ static void __init of_parse_and_init_cpus(void)
>  
>  void __init setup_smp(void)
>  {
> -	of_parse_and_init_cpus();
> +	if (acpi_disabled)
> +		of_parse_and_init_cpus();
> +	else
> +		acpi_parse_and_init_cpus();
>  }
>  
>  static int start_secondary_cpu(int cpu, struct task_struct *tidle)
> -- 
> 2.34.1
>

Do we not want to add an entry to acpi_table_print_madt_entry() for RINTC?

Thanks,
drew
Sunil V L Feb. 24, 2023, 4:50 p.m. UTC | #2 
On Mon, Feb 20, 2023 at 06:08:43PM +0100, Andrew Jones wrote:
> On Thu, Feb 16, 2023 at 11:50:32PM +0530, Sunil V L wrote:
> > Enable SMP boot on ACPI based platforms by using the RINTC
> > structures in the MADT table.
> > 
> > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> > ---
> >  arch/riscv/include/asm/acpi.h |  7 ++++
> >  arch/riscv/kernel/smpboot.c   | 70 ++++++++++++++++++++++++++++++++++-
> >  2 files changed, 76 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/riscv/include/asm/acpi.h b/arch/riscv/include/asm/acpi.h
> > index 7bc49f65c86b..3c3a8ac3b37a 100644
> > --- a/arch/riscv/include/asm/acpi.h
> > +++ b/arch/riscv/include/asm/acpi.h
> > @@ -60,6 +60,13 @@ static inline void arch_fix_phys_package_id(int num, u32 slot) { }
> >  
> >  int acpi_get_riscv_isa(struct acpi_table_header *table,
> >  		       unsigned int cpu, const char **isa);
> > +
> > +#ifdef CONFIG_ACPI_NUMA
> > +int acpi_numa_get_nid(unsigned int cpu);
> > +#else
> > +static inline int acpi_numa_get_nid(unsigned int cpu) { return NUMA_NO_NODE; }
> > +#endif /* CONFIG_ACPI_NUMA */
> 
> The #ifdef stuff seems premature since we're not providing an
> implementation for acpi_numa_get_nid() or selecting ACPI_NUMA, but OK.
> 
Yes, will remove it. We can add as part NUMA enablement.

> > +
> >  #else
> >  static inline int acpi_get_riscv_isa(struct acpi_table_header *table,
> >  				     unsigned int cpu, const char **isa)
> > diff --git a/arch/riscv/kernel/smpboot.c b/arch/riscv/kernel/smpboot.c
> > index 26214ddefaa4..77630f8ed12b 100644
> > --- a/arch/riscv/kernel/smpboot.c
> > +++ b/arch/riscv/kernel/smpboot.c
> > @@ -8,6 +8,7 @@
> >   * Copyright (C) 2017 SiFive
> >   */
> >  
> > +#include <linux/acpi.h>
> >  #include <linux/arch_topology.h>
> >  #include <linux/module.h>
> >  #include <linux/init.h>
> > @@ -70,6 +71,70 @@ void __init smp_prepare_cpus(unsigned int max_cpus)
> >  	}
> >  }
> >  
> > +#ifdef CONFIG_ACPI
> > +static unsigned int cpu_count = 1;
> > +
> > +static int __init acpi_parse_rintc(union acpi_subtable_headers *header, const unsigned long end)
> > +{
> > +	unsigned long hart;
> > +	bool found_boot_cpu = false;
> 
> I guess found_boot_cpu should be static?
> 
Good catch!. Thanks!

> > +	struct acpi_madt_rintc *processor = (struct acpi_madt_rintc *)header;
> > +
> > +	/*
> > +	 * Each RINTC structure in MADT will have a flag. If ACPI_MADT_ENABLED
> > +	 * bit in the flag is not enabled, it means OS should not try to enable
> > +	 * the cpu to which RINTC belongs.
> > +	 */
> > +	if (!(processor->flags & ACPI_MADT_ENABLED))
> > +		return 0;
> > +
> > +	hart = processor->hart_id;
> > +	if (hart < 0)
> > +		return 0;
> 
> A valid hart ID is anything up to INVALID_HARTID, right? Shouldn't we only
> be checking for INVALID_HARTID here? And what does it mean to have an
> invalid hart ID here? It's not an issue to error/warn about?
> 
Yes, will check for INVALID_HARTID (though I am not really sure how it
can be invalid). Will add a warning.

> > +	if (hart == cpuid_to_hartid_map(0)) {
> > +		BUG_ON(found_boot_cpu);
> 
> Do we really want to BUG due to bad, but potentially bootable ACPI tables?
> I'd BUG for things that can only happen when we break the code, but broken
> ACPI tables might be something we want to complain loudly about and then
> attempt to limp along.
> 
Okay. I used same logic as in DT. It may be better to use BUG instead of
debugging weird symptoms later, right?

> > +		found_boot_cpu = true;
> > +		early_map_cpu_to_node(0, acpi_numa_get_nid(cpu_count));
> > +		return 0;
> > +	}
> > +
> > +	if (cpu_count >= NR_CPUS) {
> > +		pr_warn("Invalid cpuid [%d] for hartid [%lu]\n",
> > +			cpu_count, hart);
> 
> cpuid isn't invalid, NR_CPUS is too small for the number of ACPI tables.
> 
Okay.

> > +		return 0;
> > +	}
> > +
> > +	cpuid_to_hartid_map(cpu_count) = hart;
> > +	early_map_cpu_to_node(cpu_count, acpi_numa_get_nid(cpu_count));
> > +	cpu_count++;
> > +
> > +	return 0;
> > +}
> > +
> > +static void __init acpi_parse_and_init_cpus(void)
> > +{
> > +	int cpuid;
> > +
> > +	cpu_set_ops(0);
> > +
> > +	/*
> > +	 * do a walk of MADT to determine how many CPUs
> > +	 * we have including disabled CPUs, and get information
> > +	 * we need for SMP init.
> > +	 */
> 
> I know this comment comes verbatim from arm64, but not only does it
> have grammar issues, I'm not sure it's accurate. Where is the count
> of disabled CPUs for arm64 or riscv?
>
MADT will have multiple RINTC structures. Each RINTC structure will have
a flag to indicate whether enabled or disabled. So, we need to walk the
MADT to get all CPUs present. But I think this comment is not required
since comments are added in the parser function.
 
> > +	acpi_table_parse_madt(ACPI_MADT_TYPE_RINTC, acpi_parse_rintc, 0);
> > +
> > +	for (cpuid = 1; cpuid < nr_cpu_ids; cpuid++) {
> > +		if (cpuid_to_hartid_map(cpuid) != INVALID_HARTID) {
> > +			cpu_set_ops(cpuid);
> > +			set_cpu_possible(cpuid, true);
> > +		}
> > +	}
> > +}
> > +#else
> > +#define acpi_parse_and_init_cpus(...)	do { } while (0)
> > +#endif
> > +
> >  static void __init of_parse_and_init_cpus(void)
> >  {
> >  	struct device_node *dn;
> > @@ -118,7 +183,10 @@ static void __init of_parse_and_init_cpus(void)
> >  
> >  void __init setup_smp(void)
> >  {
> > -	of_parse_and_init_cpus();
> > +	if (acpi_disabled)
> > +		of_parse_and_init_cpus();
> > +	else
> > +		acpi_parse_and_init_cpus();
> >  }
> >  
> >  static int start_secondary_cpu(int cpu, struct task_struct *tidle)
> > -- 
> > 2.34.1
> >
> 
> Do we not want to add an entry to acpi_table_print_madt_entry() for RINTC?
> 
Yes. Will add a patch for this to help debugging.

Thanks,
Sunil
Andrew Jones Feb. 24, 2023, 5:06 p.m. UTC | #3 
On Fri, Feb 24, 2023 at 10:20:17PM +0530, Sunil V L wrote:
> On Mon, Feb 20, 2023 at 06:08:43PM +0100, Andrew Jones wrote:
> > On Thu, Feb 16, 2023 at 11:50:32PM +0530, Sunil V L wrote:
> > > Enable SMP boot on ACPI based platforms by using the RINTC
> > > structures in the MADT table.
> > > 
> > > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> > > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> > > ---
> > >  arch/riscv/include/asm/acpi.h |  7 ++++
> > >  arch/riscv/kernel/smpboot.c   | 70 ++++++++++++++++++++++++++++++++++-
> > >  2 files changed, 76 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/arch/riscv/include/asm/acpi.h b/arch/riscv/include/asm/acpi.h
> > > index 7bc49f65c86b..3c3a8ac3b37a 100644
> > > --- a/arch/riscv/include/asm/acpi.h
> > > +++ b/arch/riscv/include/asm/acpi.h
> > > @@ -60,6 +60,13 @@ static inline void arch_fix_phys_package_id(int num, u32 slot) { }
> > >  
> > >  int acpi_get_riscv_isa(struct acpi_table_header *table,
> > >  		       unsigned int cpu, const char **isa);
> > > +
> > > +#ifdef CONFIG_ACPI_NUMA
> > > +int acpi_numa_get_nid(unsigned int cpu);
> > > +#else
> > > +static inline int acpi_numa_get_nid(unsigned int cpu) { return NUMA_NO_NODE; }
> > > +#endif /* CONFIG_ACPI_NUMA */
> > 
> > The #ifdef stuff seems premature since we're not providing an
> > implementation for acpi_numa_get_nid() or selecting ACPI_NUMA, but OK.
> > 
> Yes, will remove it. We can add as part NUMA enablement.
> 
> > > +
> > >  #else
> > >  static inline int acpi_get_riscv_isa(struct acpi_table_header *table,
> > >  				     unsigned int cpu, const char **isa)
> > > diff --git a/arch/riscv/kernel/smpboot.c b/arch/riscv/kernel/smpboot.c
> > > index 26214ddefaa4..77630f8ed12b 100644
> > > --- a/arch/riscv/kernel/smpboot.c
> > > +++ b/arch/riscv/kernel/smpboot.c
> > > @@ -8,6 +8,7 @@
> > >   * Copyright (C) 2017 SiFive
> > >   */
> > >  
> > > +#include <linux/acpi.h>
> > >  #include <linux/arch_topology.h>
> > >  #include <linux/module.h>
> > >  #include <linux/init.h>
> > > @@ -70,6 +71,70 @@ void __init smp_prepare_cpus(unsigned int max_cpus)
> > >  	}
> > >  }
> > >  
> > > +#ifdef CONFIG_ACPI
> > > +static unsigned int cpu_count = 1;
> > > +
> > > +static int __init acpi_parse_rintc(union acpi_subtable_headers *header, const unsigned long end)
> > > +{
> > > +	unsigned long hart;
> > > +	bool found_boot_cpu = false;
> > 
> > I guess found_boot_cpu should be static?
> > 
> Good catch!. Thanks!
> 
> > > +	struct acpi_madt_rintc *processor = (struct acpi_madt_rintc *)header;
> > > +
> > > +	/*
> > > +	 * Each RINTC structure in MADT will have a flag. If ACPI_MADT_ENABLED
> > > +	 * bit in the flag is not enabled, it means OS should not try to enable
> > > +	 * the cpu to which RINTC belongs.
> > > +	 */
> > > +	if (!(processor->flags & ACPI_MADT_ENABLED))
> > > +		return 0;
> > > +
> > > +	hart = processor->hart_id;
> > > +	if (hart < 0)
> > > +		return 0;
> > 
> > A valid hart ID is anything up to INVALID_HARTID, right? Shouldn't we only
> > be checking for INVALID_HARTID here? And what does it mean to have an
> > invalid hart ID here? It's not an issue to error/warn about?
> > 
> Yes, will check for INVALID_HARTID (though I am not really sure how it
> can be invalid). Will add a warning.
> 
> > > +	if (hart == cpuid_to_hartid_map(0)) {
> > > +		BUG_ON(found_boot_cpu);
> > 
> > Do we really want to BUG due to bad, but potentially bootable ACPI tables?
> > I'd BUG for things that can only happen when we break the code, but broken
> > ACPI tables might be something we want to complain loudly about and then
> > attempt to limp along.
> > 
> Okay. I used same logic as in DT. It may be better to use BUG instead of
> debugging weird symptoms later, right?

Maybe? I guess it depends on how obvious the symptoms are, how much they
mess things up, and how easy it is to correct the ACPI tables. I'll leave
this one up to you :-)

Thanks,
drew
diff mbox series

Patch

diff --git a/arch/riscv/include/asm/acpi.h b/arch/riscv/include/asm/acpi.h
index 7bc49f65c86b..3c3a8ac3b37a 100644
--- a/arch/riscv/include/asm/acpi.h
+++ b/arch/riscv/include/asm/acpi.h
@@ -60,6 +60,13 @@  static inline void arch_fix_phys_package_id(int num, u32 slot) { }
 
 int acpi_get_riscv_isa(struct acpi_table_header *table,
 		       unsigned int cpu, const char **isa);
+
+#ifdef CONFIG_ACPI_NUMA
+int acpi_numa_get_nid(unsigned int cpu);
+#else
+static inline int acpi_numa_get_nid(unsigned int cpu) { return NUMA_NO_NODE; }
+#endif /* CONFIG_ACPI_NUMA */
+
 #else
 static inline int acpi_get_riscv_isa(struct acpi_table_header *table,
 				     unsigned int cpu, const char **isa)
diff --git a/arch/riscv/kernel/smpboot.c b/arch/riscv/kernel/smpboot.c
index 26214ddefaa4..77630f8ed12b 100644
--- a/arch/riscv/kernel/smpboot.c
+++ b/arch/riscv/kernel/smpboot.c
@@ -8,6 +8,7 @@ 
  * Copyright (C) 2017 SiFive
  */
 
+#include <linux/acpi.h>
 #include <linux/arch_topology.h>
 #include <linux/module.h>
 #include <linux/init.h>
@@ -70,6 +71,70 @@  void __init smp_prepare_cpus(unsigned int max_cpus)
 	}
 }
 
+#ifdef CONFIG_ACPI
+static unsigned int cpu_count = 1;
+
+static int __init acpi_parse_rintc(union acpi_subtable_headers *header, const unsigned long end)
+{
+	unsigned long hart;
+	bool found_boot_cpu = false;
+	struct acpi_madt_rintc *processor = (struct acpi_madt_rintc *)header;
+
+	/*
+	 * Each RINTC structure in MADT will have a flag. If ACPI_MADT_ENABLED
+	 * bit in the flag is not enabled, it means OS should not try to enable
+	 * the cpu to which RINTC belongs.
+	 */
+	if (!(processor->flags & ACPI_MADT_ENABLED))
+		return 0;
+
+	hart = processor->hart_id;
+	if (hart < 0)
+		return 0;
+	if (hart == cpuid_to_hartid_map(0)) {
+		BUG_ON(found_boot_cpu);
+		found_boot_cpu = true;
+		early_map_cpu_to_node(0, acpi_numa_get_nid(cpu_count));
+		return 0;
+	}
+
+	if (cpu_count >= NR_CPUS) {
+		pr_warn("Invalid cpuid [%d] for hartid [%lu]\n",
+			cpu_count, hart);
+		return 0;
+	}
+
+	cpuid_to_hartid_map(cpu_count) = hart;
+	early_map_cpu_to_node(cpu_count, acpi_numa_get_nid(cpu_count));
+	cpu_count++;
+
+	return 0;
+}
+
+static void __init acpi_parse_and_init_cpus(void)
+{
+	int cpuid;
+
+	cpu_set_ops(0);
+
+	/*
+	 * do a walk of MADT to determine how many CPUs
+	 * we have including disabled CPUs, and get information
+	 * we need for SMP init.
+	 */
+	acpi_table_parse_madt(ACPI_MADT_TYPE_RINTC, acpi_parse_rintc, 0);
+
+	for (cpuid = 1; cpuid < nr_cpu_ids; cpuid++) {
+		if (cpuid_to_hartid_map(cpuid) != INVALID_HARTID) {
+			cpu_set_ops(cpuid);
+			set_cpu_possible(cpuid, true);
+		}
+	}
+}
+#else
+#define acpi_parse_and_init_cpus(...)	do { } while (0)
+#endif
+
 static void __init of_parse_and_init_cpus(void)
 {
 	struct device_node *dn;
@@ -118,7 +183,10 @@  static void __init of_parse_and_init_cpus(void)
 
 void __init setup_smp(void)
 {
-	of_parse_and_init_cpus();
+	if (acpi_disabled)
+		of_parse_and_init_cpus();
+	else
+		acpi_parse_and_init_cpus();
 }
 
 static int start_secondary_cpu(int cpu, struct task_struct *tidle)