[V4,08/23] RISC-V: ACPI: Cache and retrieve the RINTC structure

  RINTC structures in the MADT provide mapping between the hartid
and the CPU. This is required many times even at run time like
cpuinfo. So, instead of parsing the ACPI table every time, cache
the RINTC structures and provide a function to get the correct
RINTC structure for a given cpu.

Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 arch/riscv/include/asm/acpi.h |  2 ++
 arch/riscv/kernel/acpi.c      | 60 +++++++++++++++++++++++++++++++++++
 2 files changed, 62 insertions(+)
  

On Tue, Apr 04, 2023 at 11:50:22PM +0530, Sunil V L wrote:
> RINTC structures in the MADT provide mapping between the hartid
> and the CPU. This is required many times even at run time like
> cpuinfo. So, instead of parsing the ACPI table every time, cache
> the RINTC structures and provide a function to get the correct
> RINTC structure for a given cpu.
> 
> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  arch/riscv/include/asm/acpi.h |  2 ++
>  arch/riscv/kernel/acpi.c      | 60 +++++++++++++++++++++++++++++++++++
>  2 files changed, 62 insertions(+)
> 
> diff --git a/arch/riscv/include/asm/acpi.h b/arch/riscv/include/asm/acpi.h
> index 9be52b6ffae1..1606dce8992e 100644
> --- a/arch/riscv/include/asm/acpi.h
> +++ b/arch/riscv/include/asm/acpi.h
> @@ -59,6 +59,8 @@ static inline bool acpi_has_cpu_in_madt(void)
>  
>  static inline void arch_fix_phys_package_id(int num, u32 slot) { }
>  
> +struct acpi_madt_rintc *acpi_cpu_get_madt_rintc(int cpu);
> +u32 get_acpi_id_for_cpu(int cpu);
>  #endif /* CONFIG_ACPI */
>  
>  #endif /*_ASM_ACPI_H*/
> diff --git a/arch/riscv/kernel/acpi.c b/arch/riscv/kernel/acpi.c
> index 81d448c41714..40ab55309c70 100644
> --- a/arch/riscv/kernel/acpi.c
> +++ b/arch/riscv/kernel/acpi.c
> @@ -24,6 +24,66 @@ EXPORT_SYMBOL(acpi_disabled);
>  int acpi_pci_disabled = 1;	/* skip ACPI PCI scan and IRQ initialization */
>  EXPORT_SYMBOL(acpi_pci_disabled);
>  
> +static struct acpi_madt_rintc cpu_madt_rintc[NR_CPUS];
> +
> +static int acpi_parse_madt_rintc(union acpi_subtable_headers *header, const unsigned long end)
> +{
> +	struct acpi_madt_rintc *rintc = (struct acpi_madt_rintc *)header;
> +	int cpuid;
> +
> +	if (!(rintc->flags & ACPI_MADT_ENABLED))
> +		return 0;
> +
> +	cpuid = riscv_hartid_to_cpuid(rintc->hart_id);
> +	/*
> +	 * When CONFIG_SMP is disabled, mapping won't be created for
> +	 * all cpus.
> +	 * CPUs more than NR_CPUS, will be ignored.
> +	 */
> +	if (cpuid >= 0 && cpuid < NR_CPUS)
> +		cpu_madt_rintc[cpuid] = *rintc;
> +
> +	return 0;
> +}
> +
> +static int acpi_init_rintc_array(void)
> +{
> +	if (acpi_table_parse_madt(ACPI_MADT_TYPE_RINTC, acpi_parse_madt_rintc, 0) > 0)

Does this not return an actual ERRNO? Naïvely following the call chain,
it looks like it should do. If it does, why not return that instead of
always returning -ENODEV?

> +		return 0;
> +
> +	return -ENODEV;
> +}
> +
> +/*
> + * Instead of parsing (and freeing) the ACPI table, cache
> + * the RINTC structures since they are frequently used
> + * like in  cpuinfo.
> + */
> +struct acpi_madt_rintc *acpi_cpu_get_madt_rintc(int cpu)
> +{
> +	static bool rintc_init_done;
> +
> +	if (!rintc_init_done) {
> +		if (acpi_init_rintc_array()) {
> +			pr_err("No valid RINTC entries exist\n");
> +			return NULL;
> +		}
> +
> +		rintc_init_done = true;
> +	}
> +
> +	return &cpu_madt_rintc[cpu];
> +}
> +
> +u32 get_acpi_id_for_cpu(int cpu)
> +{
> +	struct acpi_madt_rintc *rintc = acpi_cpu_get_madt_rintc(cpu);
> +
> +	BUG_ON(!rintc);
> +
> +	return rintc->uid;
> +}
> +
>  /*
>   * __acpi_map_table() will be called before paging_init(), so early_ioremap()
>   * or early_memremap() should be called here to for ACPI table mapping.
> -- 
> 2.34.1
> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
  

On Tue, Apr 04, 2023 at 11:50:22PM +0530, Sunil V L wrote:
> RINTC structures in the MADT provide mapping between the hartid
> and the CPU. This is required many times even at run time like
> cpuinfo. So, instead of parsing the ACPI table every time, cache
> the RINTC structures and provide a function to get the correct
> RINTC structure for a given cpu.
> 
> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  arch/riscv/include/asm/acpi.h |  2 ++
>  arch/riscv/kernel/acpi.c      | 60 +++++++++++++++++++++++++++++++++++
>  2 files changed, 62 insertions(+)
> 
> diff --git a/arch/riscv/include/asm/acpi.h b/arch/riscv/include/asm/acpi.h
> index 9be52b6ffae1..1606dce8992e 100644
> --- a/arch/riscv/include/asm/acpi.h
> +++ b/arch/riscv/include/asm/acpi.h
> @@ -59,6 +59,8 @@ static inline bool acpi_has_cpu_in_madt(void)
>  
>  static inline void arch_fix_phys_package_id(int num, u32 slot) { }
>  
> +struct acpi_madt_rintc *acpi_cpu_get_madt_rintc(int cpu);
> +u32 get_acpi_id_for_cpu(int cpu);
>  #endif /* CONFIG_ACPI */
>  
>  #endif /*_ASM_ACPI_H*/
> diff --git a/arch/riscv/kernel/acpi.c b/arch/riscv/kernel/acpi.c
> index 81d448c41714..40ab55309c70 100644
> --- a/arch/riscv/kernel/acpi.c
> +++ b/arch/riscv/kernel/acpi.c
> @@ -24,6 +24,66 @@ EXPORT_SYMBOL(acpi_disabled);
>  int acpi_pci_disabled = 1;	/* skip ACPI PCI scan and IRQ initialization */
>  EXPORT_SYMBOL(acpi_pci_disabled);
>  
> +static struct acpi_madt_rintc cpu_madt_rintc[NR_CPUS];
> +
> +static int acpi_parse_madt_rintc(union acpi_subtable_headers *header, const unsigned long end)
> +{
> +	struct acpi_madt_rintc *rintc = (struct acpi_madt_rintc *)header;
> +	int cpuid;
> +
> +	if (!(rintc->flags & ACPI_MADT_ENABLED))
> +		return 0;
> +
> +	cpuid = riscv_hartid_to_cpuid(rintc->hart_id);
> +	/*
> +	 * When CONFIG_SMP is disabled, mapping won't be created for
> +	 * all cpus.
> +	 * CPUs more than NR_CPUS, will be ignored.
> +	 */
> +	if (cpuid >= 0 && cpuid < NR_CPUS)
> +		cpu_madt_rintc[cpuid] = *rintc;
> +
> +	return 0;
> +}
> +
> +static int acpi_init_rintc_array(void)
> +{
> +	if (acpi_table_parse_madt(ACPI_MADT_TYPE_RINTC, acpi_parse_madt_rintc, 0) > 0)
> +		return 0;
> +
> +	return -ENODEV;

As Conor pointed out, the errors could be propagated from
acpi_table_parse_madt(), which could reduce this function to

 return acpi_table_parse_madt(ACPI_MADT_TYPE_RINTC, acpi_parse_madt_rintc, 0);

where the '< 0' check would be in the caller below. That sounds good to
me, but then I'd take that a step further and just drop this helper
altogether.

> +}
> +
> +/*
> + * Instead of parsing (and freeing) the ACPI table, cache
> + * the RINTC structures since they are frequently used
> + * like in  cpuinfo.
             ^ extra space

> + */
> +struct acpi_madt_rintc *acpi_cpu_get_madt_rintc(int cpu)
> +{
> +	static bool rintc_init_done;
> +
> +	if (!rintc_init_done) {
> +		if (acpi_init_rintc_array()) {
> +			pr_err("No valid RINTC entries exist\n");
> +			return NULL;
> +		}
> +
> +		rintc_init_done = true;
> +	}
> +
> +	return &cpu_madt_rintc[cpu];
> +}
> +
> +u32 get_acpi_id_for_cpu(int cpu)
> +{
> +	struct acpi_madt_rintc *rintc = acpi_cpu_get_madt_rintc(cpu);
> +
> +	BUG_ON(!rintc);
> +
> +	return rintc->uid;
> +}
> +
>  /*
>   * __acpi_map_table() will be called before paging_init(), so early_ioremap()
>   * or early_memremap() should be called here to for ACPI table mapping.
> -- 
> 2.34.1
>

Otherwise,

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

Thanks,
drew
  

On Wed, Apr 05, 2023 at 05:17:48PM +0200, Andrew Jones wrote:
> On Tue, Apr 04, 2023 at 11:50:22PM +0530, Sunil V L wrote:
> > RINTC structures in the MADT provide mapping between the hartid
> > and the CPU. This is required many times even at run time like
> > cpuinfo. So, instead of parsing the ACPI table every time, cache
> > the RINTC structures and provide a function to get the correct
> > RINTC structure for a given cpu.
> > 
> > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >  arch/riscv/include/asm/acpi.h |  2 ++
> >  arch/riscv/kernel/acpi.c      | 60 +++++++++++++++++++++++++++++++++++
> >  2 files changed, 62 insertions(+)
> > 
> > diff --git a/arch/riscv/include/asm/acpi.h b/arch/riscv/include/asm/acpi.h
> > index 9be52b6ffae1..1606dce8992e 100644
> > --- a/arch/riscv/include/asm/acpi.h
> > +++ b/arch/riscv/include/asm/acpi.h
> > @@ -59,6 +59,8 @@ static inline bool acpi_has_cpu_in_madt(void)
> >  
> >  static inline void arch_fix_phys_package_id(int num, u32 slot) { }
> >  
> > +struct acpi_madt_rintc *acpi_cpu_get_madt_rintc(int cpu);
> > +u32 get_acpi_id_for_cpu(int cpu);
> >  #endif /* CONFIG_ACPI */
> >  
> >  #endif /*_ASM_ACPI_H*/
> > diff --git a/arch/riscv/kernel/acpi.c b/arch/riscv/kernel/acpi.c
> > index 81d448c41714..40ab55309c70 100644
> > --- a/arch/riscv/kernel/acpi.c
> > +++ b/arch/riscv/kernel/acpi.c
> > @@ -24,6 +24,66 @@ EXPORT_SYMBOL(acpi_disabled);
> >  int acpi_pci_disabled = 1;	/* skip ACPI PCI scan and IRQ initialization */
> >  EXPORT_SYMBOL(acpi_pci_disabled);
> >  
> > +static struct acpi_madt_rintc cpu_madt_rintc[NR_CPUS];
> > +
> > +static int acpi_parse_madt_rintc(union acpi_subtable_headers *header, const unsigned long end)
> > +{
> > +	struct acpi_madt_rintc *rintc = (struct acpi_madt_rintc *)header;
> > +	int cpuid;
> > +
> > +	if (!(rintc->flags & ACPI_MADT_ENABLED))
> > +		return 0;
> > +
> > +	cpuid = riscv_hartid_to_cpuid(rintc->hart_id);
> > +	/*
> > +	 * When CONFIG_SMP is disabled, mapping won't be created for
> > +	 * all cpus.
> > +	 * CPUs more than NR_CPUS, will be ignored.
> > +	 */
> > +	if (cpuid >= 0 && cpuid < NR_CPUS)
> > +		cpu_madt_rintc[cpuid] = *rintc;
> > +
> > +	return 0;
> > +}
> > +
> > +static int acpi_init_rintc_array(void)
> > +{
> > +	if (acpi_table_parse_madt(ACPI_MADT_TYPE_RINTC, acpi_parse_madt_rintc, 0) > 0)
> > +		return 0;
> > +
> > +	return -ENODEV;
> 
> As Conor pointed out, the errors could be propagated from
> acpi_table_parse_madt(), which could reduce this function to
> 
>  return acpi_table_parse_madt(ACPI_MADT_TYPE_RINTC, acpi_parse_madt_rintc, 0);
> 
> where the '< 0' check would be in the caller below. That sounds good to
> me, but then I'd take that a step further and just drop this helper
> altogether.
> 
Thanks, Conor, Drew. I used similar to how others have used
acpi_table_parse_madt(). But your suggestion makes sense. Will remove
the wrapper function also.

Thanks,
Sunil
  

On Tue, 04 Apr 2023 11:20:22 PDT (-0700), sunilvl@ventanamicro.com wrote:
> RINTC structures in the MADT provide mapping between the hartid
> and the CPU. This is required many times even at run time like
> cpuinfo. So, instead of parsing the ACPI table every time, cache
> the RINTC structures and provide a function to get the correct
> RINTC structure for a given cpu.
>
> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  arch/riscv/include/asm/acpi.h |  2 ++
>  arch/riscv/kernel/acpi.c      | 60 +++++++++++++++++++++++++++++++++++
>  2 files changed, 62 insertions(+)
>
> diff --git a/arch/riscv/include/asm/acpi.h b/arch/riscv/include/asm/acpi.h
> index 9be52b6ffae1..1606dce8992e 100644
> --- a/arch/riscv/include/asm/acpi.h
> +++ b/arch/riscv/include/asm/acpi.h
> @@ -59,6 +59,8 @@ static inline bool acpi_has_cpu_in_madt(void)
>
>  static inline void arch_fix_phys_package_id(int num, u32 slot) { }
>
> +struct acpi_madt_rintc *acpi_cpu_get_madt_rintc(int cpu);
> +u32 get_acpi_id_for_cpu(int cpu);
>  #endif /* CONFIG_ACPI */
>
>  #endif /*_ASM_ACPI_H*/
> diff --git a/arch/riscv/kernel/acpi.c b/arch/riscv/kernel/acpi.c
> index 81d448c41714..40ab55309c70 100644
> --- a/arch/riscv/kernel/acpi.c
> +++ b/arch/riscv/kernel/acpi.c
> @@ -24,6 +24,66 @@ EXPORT_SYMBOL(acpi_disabled);
>  int acpi_pci_disabled = 1;	/* skip ACPI PCI scan and IRQ initialization */
>  EXPORT_SYMBOL(acpi_pci_disabled);
>
> +static struct acpi_madt_rintc cpu_madt_rintc[NR_CPUS];
> +
> +static int acpi_parse_madt_rintc(union acpi_subtable_headers *header, const unsigned long end)
> +{
> +	struct acpi_madt_rintc *rintc = (struct acpi_madt_rintc *)header;
> +	int cpuid;
> +
> +	if (!(rintc->flags & ACPI_MADT_ENABLED))
> +		return 0;
> +
> +	cpuid = riscv_hartid_to_cpuid(rintc->hart_id);

Unless I'm missing something, this races with CPUs coming online.  Maybe 
that's a rare enough case we don't care, but I think we'd also just have 
simpler logic if we fixed it...

> +	/*
> +	 * When CONFIG_SMP is disabled, mapping won't be created for
> +	 * all cpus.
> +	 * CPUs more than NR_CPUS, will be ignored.
> +	 */
> +	if (cpuid >= 0 && cpuid < NR_CPUS)
> +		cpu_madt_rintc[cpuid] = *rintc;
> +
> +	return 0;
> +}
> +
> +static int acpi_init_rintc_array(void)
> +{
> +	if (acpi_table_parse_madt(ACPI_MADT_TYPE_RINTC, acpi_parse_madt_rintc, 0) > 0)
> +		return 0;
> +
> +	return -ENODEV;
> +}
> +
> +/*
> + * Instead of parsing (and freeing) the ACPI table, cache
> + * the RINTC structures since they are frequently used
> + * like in  cpuinfo.
> + */
> +struct acpi_madt_rintc *acpi_cpu_get_madt_rintc(int cpu)
> +{
> +	static bool rintc_init_done;

... basically just get rid of this global variable, and instead have a

    if (!&cpu_madt_rintc[cpu])
        ... parse ...
    
    return &cpu_madt_rintc[cpu];

that'd probably let us get rid of a handful of these helpers too, as now 
it's just a call to the parsing bits.

> +
> +	if (!rintc_init_done) {
> +		if (acpi_init_rintc_array()) {
> +			pr_err("No valid RINTC entries exist\n");
> +			return NULL;
> +		}
> +
> +		rintc_init_done = true;
> +	}
> +
> +	return &cpu_madt_rintc[cpu];
> +}
> +
> +u32 get_acpi_id_for_cpu(int cpu)
> +{
> +	struct acpi_madt_rintc *rintc = acpi_cpu_get_madt_rintc(cpu);
> +
> +	BUG_ON(!rintc);

We should have some better error reporting here.  It looks like all the 
callerss of get_acpi_id_for_cpu() are tolerant of a nonsense ID being 
returned, so maybe we just pr_warn() something users can understand and 
then return -1 or something?

> +
> +	return rintc->uid;
> +}
> +
>  /*
>   * __acpi_map_table() will be called before paging_init(), so early_ioremap()
>   * or early_memremap() should be called here to for ACPI table mapping.
  

Hi Palmer,

On Wed, Apr 26, 2023 at 11:45:00AM -0700, Palmer Dabbelt wrote:
> On Tue, 04 Apr 2023 11:20:22 PDT (-0700), sunilvl@ventanamicro.com wrote:
> > RINTC structures in the MADT provide mapping between the hartid
> > and the CPU. This is required many times even at run time like
> > cpuinfo. So, instead of parsing the ACPI table every time, cache
> > the RINTC structures and provide a function to get the correct
> > RINTC structure for a given cpu.
> > 
> > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >  arch/riscv/include/asm/acpi.h |  2 ++
> >  arch/riscv/kernel/acpi.c      | 60 +++++++++++++++++++++++++++++++++++
> >  2 files changed, 62 insertions(+)
> > 
> > diff --git a/arch/riscv/include/asm/acpi.h b/arch/riscv/include/asm/acpi.h
> > index 9be52b6ffae1..1606dce8992e 100644
> > --- a/arch/riscv/include/asm/acpi.h
> > +++ b/arch/riscv/include/asm/acpi.h
> > @@ -59,6 +59,8 @@ static inline bool acpi_has_cpu_in_madt(void)
> > 
> >  static inline void arch_fix_phys_package_id(int num, u32 slot) { }
> > 
> > +struct acpi_madt_rintc *acpi_cpu_get_madt_rintc(int cpu);
> > +u32 get_acpi_id_for_cpu(int cpu);
> >  #endif /* CONFIG_ACPI */
> > 
> >  #endif /*_ASM_ACPI_H*/
> > diff --git a/arch/riscv/kernel/acpi.c b/arch/riscv/kernel/acpi.c
> > index 81d448c41714..40ab55309c70 100644
> > --- a/arch/riscv/kernel/acpi.c
> > +++ b/arch/riscv/kernel/acpi.c
> > @@ -24,6 +24,66 @@ EXPORT_SYMBOL(acpi_disabled);
> >  int acpi_pci_disabled = 1;	/* skip ACPI PCI scan and IRQ initialization */
> >  EXPORT_SYMBOL(acpi_pci_disabled);
> > 
> > +static struct acpi_madt_rintc cpu_madt_rintc[NR_CPUS];
> > +
> > +static int acpi_parse_madt_rintc(union acpi_subtable_headers *header, const unsigned long end)
> > +{
> > +	struct acpi_madt_rintc *rintc = (struct acpi_madt_rintc *)header;
> > +	int cpuid;
> > +
> > +	if (!(rintc->flags & ACPI_MADT_ENABLED))
> > +		return 0;
> > +
> > +	cpuid = riscv_hartid_to_cpuid(rintc->hart_id);
> 
> Unless I'm missing something, this races with CPUs coming online.  Maybe
> that's a rare enough case we don't care, but I think we'd also just have
> simpler logic if we fixed it...
> 
This depend only on cpuid_to_hartid_map filled up. I wish I could
initialize this RINTC mapping in setup_smp() itself like ARM64. But in
RISC-V, this file smpboot.c gets built only when CONFIG_SMP is enabled.
Hence, we need to initialize this array outside of setup_smp().

I can update the code to initialize this from setup_arch() immediately
after setup_smp() if ACPI is enabled. That should avoid the global
variable check also. Let me know if you prefer this.

> > +	/*
> > +	 * When CONFIG_SMP is disabled, mapping won't be created for
> > +	 * all cpus.
> > +	 * CPUs more than NR_CPUS, will be ignored.
> > +	 */
> > +	if (cpuid >= 0 && cpuid < NR_CPUS)
> > +		cpu_madt_rintc[cpuid] = *rintc;
> > +
> > +	return 0;
> > +}
> > +
> > +static int acpi_init_rintc_array(void)
> > +{
> > +	if (acpi_table_parse_madt(ACPI_MADT_TYPE_RINTC, acpi_parse_madt_rintc, 0) > 0)
> > +		return 0;
> > +
> > +	return -ENODEV;
> > +}
> > +
> > +/*
> > + * Instead of parsing (and freeing) the ACPI table, cache
> > + * the RINTC structures since they are frequently used
> > + * like in  cpuinfo.
> > + */
> > +struct acpi_madt_rintc *acpi_cpu_get_madt_rintc(int cpu)
> > +{
> > +	static bool rintc_init_done;
> 
> ... basically just get rid of this global variable, and instead have a
> 
>    if (!&cpu_madt_rintc[cpu])
>        ... parse ...
>    return &cpu_madt_rintc[cpu];
> 
> that'd probably let us get rid of a handful of these helpers too, as now
> it's just a call to the parsing bits.
> 
I am afraid this (!&cpu_madt_rintc[cpu]) check won't work since we are
not caching the RINTC pointers but actual contents itself. So, the
address is always valid. However, as per Drew's earlier feedback I am
going to reduce one helper. I am planning to send the next version of
this patch once 6.4 rc1 is available since the ACPICA patches are merged
now.

> > +
> > +	if (!rintc_init_done) {
> > +		if (acpi_init_rintc_array()) {
> > +			pr_err("No valid RINTC entries exist\n");
> > +			return NULL;
> > +		}
> > +
> > +		rintc_init_done = true;
> > +	}
> > +
> > +	return &cpu_madt_rintc[cpu];
> > +}
> > +
> > +u32 get_acpi_id_for_cpu(int cpu)
> > +{
> > +	struct acpi_madt_rintc *rintc = acpi_cpu_get_madt_rintc(cpu);
> > +
> > +	BUG_ON(!rintc);
> 
> We should have some better error reporting here.  It looks like all the
> callerss of get_acpi_id_for_cpu() are tolerant of a nonsense ID being
> returned, so maybe we just pr_warn() something users can understand and then
> return -1 or something?
> 

RINTC is mandatory for ACPI systems. Also, all 32bit values are valid
for UID. So, there is no bogus value we can return. 

Actually, I just realized this check is redundant. It will never be NULL
since it is a static array. So, we can just get rid of the BUG.

Thanks!
Sunil
  

On Thu, Apr 27, 2023 at 02:52:50PM +0530, Sunil V L wrote:
> Hi Palmer,
> 
> On Wed, Apr 26, 2023 at 11:45:00AM -0700, Palmer Dabbelt wrote:
> > On Tue, 04 Apr 2023 11:20:22 PDT (-0700), sunilvl@ventanamicro.com wrote:
> > > RINTC structures in the MADT provide mapping between the hartid
> > > and the CPU. This is required many times even at run time like
> > > cpuinfo. So, instead of parsing the ACPI table every time, cache
> > > the RINTC structures and provide a function to get the correct
> > > RINTC structure for a given cpu.
> > > 
> > > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> > > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > ---
> > >  arch/riscv/include/asm/acpi.h |  2 ++
> > >  arch/riscv/kernel/acpi.c      | 60 +++++++++++++++++++++++++++++++++++
> > >  2 files changed, 62 insertions(+)
> > > 
> > > diff --git a/arch/riscv/include/asm/acpi.h b/arch/riscv/include/asm/acpi.h
> > > index 9be52b6ffae1..1606dce8992e 100644
> > > --- a/arch/riscv/include/asm/acpi.h
> > > +++ b/arch/riscv/include/asm/acpi.h
> > > @@ -59,6 +59,8 @@ static inline bool acpi_has_cpu_in_madt(void)
> > > 
> > >  static inline void arch_fix_phys_package_id(int num, u32 slot) { }
> > > 
> > > +struct acpi_madt_rintc *acpi_cpu_get_madt_rintc(int cpu);
> > > +u32 get_acpi_id_for_cpu(int cpu);
> > >  #endif /* CONFIG_ACPI */
> > > 
> > >  #endif /*_ASM_ACPI_H*/
> > > diff --git a/arch/riscv/kernel/acpi.c b/arch/riscv/kernel/acpi.c
> > > index 81d448c41714..40ab55309c70 100644
> > > --- a/arch/riscv/kernel/acpi.c
> > > +++ b/arch/riscv/kernel/acpi.c
> > > @@ -24,6 +24,66 @@ EXPORT_SYMBOL(acpi_disabled);
> > >  int acpi_pci_disabled = 1;	/* skip ACPI PCI scan and IRQ initialization */
> > >  EXPORT_SYMBOL(acpi_pci_disabled);
> > > 
> > > +static struct acpi_madt_rintc cpu_madt_rintc[NR_CPUS];
> > > +
> > > +static int acpi_parse_madt_rintc(union acpi_subtable_headers *header, const unsigned long end)
> > > +{
> > > +	struct acpi_madt_rintc *rintc = (struct acpi_madt_rintc *)header;
> > > +	int cpuid;
> > > +
> > > +	if (!(rintc->flags & ACPI_MADT_ENABLED))
> > > +		return 0;
> > > +
> > > +	cpuid = riscv_hartid_to_cpuid(rintc->hart_id);
> > 
> > Unless I'm missing something, this races with CPUs coming online.  Maybe
> > that's a rare enough case we don't care, but I think we'd also just have
> > simpler logic if we fixed it...
> > 
> This depend only on cpuid_to_hartid_map filled up. I wish I could
> initialize this RINTC mapping in setup_smp() itself like ARM64. But in
> RISC-V, this file smpboot.c gets built only when CONFIG_SMP is enabled.
> Hence, we need to initialize this array outside of setup_smp().
> 
> I can update the code to initialize this from setup_arch() immediately
> after setup_smp() if ACPI is enabled. That should avoid the global
> variable check also. Let me know if you prefer this.
> 
> > > +	/*
> > > +	 * When CONFIG_SMP is disabled, mapping won't be created for
> > > +	 * all cpus.
> > > +	 * CPUs more than NR_CPUS, will be ignored.
> > > +	 */
> > > +	if (cpuid >= 0 && cpuid < NR_CPUS)
> > > +		cpu_madt_rintc[cpuid] = *rintc;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int acpi_init_rintc_array(void)
> > > +{
> > > +	if (acpi_table_parse_madt(ACPI_MADT_TYPE_RINTC, acpi_parse_madt_rintc, 0) > 0)
> > > +		return 0;
> > > +
> > > +	return -ENODEV;
> > > +}
> > > +
> > > +/*
> > > + * Instead of parsing (and freeing) the ACPI table, cache
> > > + * the RINTC structures since they are frequently used
> > > + * like in  cpuinfo.
> > > + */
> > > +struct acpi_madt_rintc *acpi_cpu_get_madt_rintc(int cpu)
> > > +{
> > > +	static bool rintc_init_done;
> > 
> > ... basically just get rid of this global variable, and instead have a
> > 
> >    if (!&cpu_madt_rintc[cpu])
> >        ... parse ...
> >    return &cpu_madt_rintc[cpu];
> > 
> > that'd probably let us get rid of a handful of these helpers too, as now
> > it's just a call to the parsing bits.
> > 
> I am afraid this (!&cpu_madt_rintc[cpu]) check won't work since we are
> not caching the RINTC pointers but actual contents itself. So, the
> address is always valid. However, as per Drew's earlier feedback I am
> going to reduce one helper. I am planning to send the next version of
> this patch once 6.4 rc1 is available since the ACPICA patches are merged
> now.
> 
> > > +
> > > +	if (!rintc_init_done) {
> > > +		if (acpi_init_rintc_array()) {
> > > +			pr_err("No valid RINTC entries exist\n");
> > > +			return NULL;
> > > +		}
> > > +
> > > +		rintc_init_done = true;
> > > +	}
> > > +
> > > +	return &cpu_madt_rintc[cpu];
> > > +}
> > > +
> > > +u32 get_acpi_id_for_cpu(int cpu)
> > > +{
> > > +	struct acpi_madt_rintc *rintc = acpi_cpu_get_madt_rintc(cpu);
> > > +
> > > +	BUG_ON(!rintc);
> > 
> > We should have some better error reporting here.  It looks like all the
> > callerss of get_acpi_id_for_cpu() are tolerant of a nonsense ID being
> > returned, so maybe we just pr_warn() something users can understand and then
> > return -1 or something?
> > 
> 
> RINTC is mandatory for ACPI systems. Also, all 32bit values are valid
> for UID. So, there is no bogus value we can return. 
> 
> Actually, I just realized this check is redundant. It will never be NULL
> since it is a static array. So, we can just get rid of the BUG.

It can be NULL on the first call of acpi_cpu_get_madt_rintc(), which is
a good time to BUG if there's isn't an RINTC.

Thanks,
drew
  

On Thu, Apr 27, 2023 at 12:25:42PM +0200, Andrew Jones wrote:
> On Thu, Apr 27, 2023 at 02:52:50PM +0530, Sunil V L wrote:
> > Hi Palmer,
> > 
> > On Wed, Apr 26, 2023 at 11:45:00AM -0700, Palmer Dabbelt wrote:
> > > On Tue, 04 Apr 2023 11:20:22 PDT (-0700), sunilvl@ventanamicro.com wrote:
> > > > RINTC structures in the MADT provide mapping between the hartid
> > > > and the CPU. This is required many times even at run time like
> > > > cpuinfo. So, instead of parsing the ACPI table every time, cache
> > > > the RINTC structures and provide a function to get the correct
> > > > RINTC structure for a given cpu.
> > > > 
> > > > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> > > > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > ---
> > > >  arch/riscv/include/asm/acpi.h |  2 ++
> > > >  arch/riscv/kernel/acpi.c      | 60 +++++++++++++++++++++++++++++++++++
> > > >  2 files changed, 62 insertions(+)
> > > > 
> > > > diff --git a/arch/riscv/include/asm/acpi.h b/arch/riscv/include/asm/acpi.h
> > > > index 9be52b6ffae1..1606dce8992e 100644
> > > > --- a/arch/riscv/include/asm/acpi.h
> > > > +++ b/arch/riscv/include/asm/acpi.h
> > > > @@ -59,6 +59,8 @@ static inline bool acpi_has_cpu_in_madt(void)
> > > > 
> > > >  static inline void arch_fix_phys_package_id(int num, u32 slot) { }
> > > > 
> > > > +struct acpi_madt_rintc *acpi_cpu_get_madt_rintc(int cpu);
> > > > +u32 get_acpi_id_for_cpu(int cpu);
> > > >  #endif /* CONFIG_ACPI */
> > > > 
> > > >  #endif /*_ASM_ACPI_H*/
> > > > diff --git a/arch/riscv/kernel/acpi.c b/arch/riscv/kernel/acpi.c
> > > > index 81d448c41714..40ab55309c70 100644
> > > > --- a/arch/riscv/kernel/acpi.c
> > > > +++ b/arch/riscv/kernel/acpi.c
> > > > @@ -24,6 +24,66 @@ EXPORT_SYMBOL(acpi_disabled);
> > > >  int acpi_pci_disabled = 1;	/* skip ACPI PCI scan and IRQ initialization */
> > > >  EXPORT_SYMBOL(acpi_pci_disabled);
> > > > 
> > > > +static struct acpi_madt_rintc cpu_madt_rintc[NR_CPUS];
> > > > +
> > > > +static int acpi_parse_madt_rintc(union acpi_subtable_headers *header, const unsigned long end)
> > > > +{
> > > > +	struct acpi_madt_rintc *rintc = (struct acpi_madt_rintc *)header;
> > > > +	int cpuid;
> > > > +
> > > > +	if (!(rintc->flags & ACPI_MADT_ENABLED))
> > > > +		return 0;
> > > > +
> > > > +	cpuid = riscv_hartid_to_cpuid(rintc->hart_id);
> > > 
> > > Unless I'm missing something, this races with CPUs coming online.  Maybe
> > > that's a rare enough case we don't care, but I think we'd also just have
> > > simpler logic if we fixed it...
> > > 
> > This depend only on cpuid_to_hartid_map filled up. I wish I could
> > initialize this RINTC mapping in setup_smp() itself like ARM64. But in
> > RISC-V, this file smpboot.c gets built only when CONFIG_SMP is enabled.
> > Hence, we need to initialize this array outside of setup_smp().
> > 
> > I can update the code to initialize this from setup_arch() immediately
> > after setup_smp() if ACPI is enabled. That should avoid the global
> > variable check also. Let me know if you prefer this.
> > 
> > > > +	/*
> > > > +	 * When CONFIG_SMP is disabled, mapping won't be created for
> > > > +	 * all cpus.
> > > > +	 * CPUs more than NR_CPUS, will be ignored.
> > > > +	 */
> > > > +	if (cpuid >= 0 && cpuid < NR_CPUS)
> > > > +		cpu_madt_rintc[cpuid] = *rintc;
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int acpi_init_rintc_array(void)
> > > > +{
> > > > +	if (acpi_table_parse_madt(ACPI_MADT_TYPE_RINTC, acpi_parse_madt_rintc, 0) > 0)
> > > > +		return 0;
> > > > +
> > > > +	return -ENODEV;
> > > > +}
> > > > +
> > > > +/*
> > > > + * Instead of parsing (and freeing) the ACPI table, cache
> > > > + * the RINTC structures since they are frequently used
> > > > + * like in  cpuinfo.
> > > > + */
> > > > +struct acpi_madt_rintc *acpi_cpu_get_madt_rintc(int cpu)
> > > > +{
> > > > +	static bool rintc_init_done;
> > > 
> > > ... basically just get rid of this global variable, and instead have a
> > > 
> > >    if (!&cpu_madt_rintc[cpu])
> > >        ... parse ...
> > >    return &cpu_madt_rintc[cpu];
> > > 
> > > that'd probably let us get rid of a handful of these helpers too, as now
> > > it's just a call to the parsing bits.
> > > 
> > I am afraid this (!&cpu_madt_rintc[cpu]) check won't work since we are
> > not caching the RINTC pointers but actual contents itself. So, the
> > address is always valid. However, as per Drew's earlier feedback I am
> > going to reduce one helper. I am planning to send the next version of
> > this patch once 6.4 rc1 is available since the ACPICA patches are merged
> > now.
> > 
> > > > +
> > > > +	if (!rintc_init_done) {
> > > > +		if (acpi_init_rintc_array()) {
> > > > +			pr_err("No valid RINTC entries exist\n");
> > > > +			return NULL;
> > > > +		}
> > > > +
> > > > +		rintc_init_done = true;
> > > > +	}
> > > > +
> > > > +	return &cpu_madt_rintc[cpu];
> > > > +}
> > > > +
> > > > +u32 get_acpi_id_for_cpu(int cpu)
> > > > +{
> > > > +	struct acpi_madt_rintc *rintc = acpi_cpu_get_madt_rintc(cpu);
> > > > +
> > > > +	BUG_ON(!rintc);
> > > 
> > > We should have some better error reporting here.  It looks like all the
> > > callerss of get_acpi_id_for_cpu() are tolerant of a nonsense ID being
> > > returned, so maybe we just pr_warn() something users can understand and then
> > > return -1 or something?
> > > 
> > 
> > RINTC is mandatory for ACPI systems. Also, all 32bit values are valid
> > for UID. So, there is no bogus value we can return. 
> > 
> > Actually, I just realized this check is redundant. It will never be NULL
> > since it is a static array. So, we can just get rid of the BUG.
> 
> It can be NULL on the first call of acpi_cpu_get_madt_rintc(), which is
> a good time to BUG if there's isn't an RINTC.
> 
Sorry, I mean if we change the initialization to get called from
setup_arch, then we can get rid of this check along with global variable
check, correct?

Thanks,
Sunil
  

On Thu, Apr 27, 2023 at 04:22:56PM +0530, Sunil V L wrote:
> On Thu, Apr 27, 2023 at 12:25:42PM +0200, Andrew Jones wrote:
> > On Thu, Apr 27, 2023 at 02:52:50PM +0530, Sunil V L wrote:
> > > Hi Palmer,
> > > 
> > > On Wed, Apr 26, 2023 at 11:45:00AM -0700, Palmer Dabbelt wrote:
> > > > On Tue, 04 Apr 2023 11:20:22 PDT (-0700), sunilvl@ventanamicro.com wrote:
> > > > > RINTC structures in the MADT provide mapping between the hartid
> > > > > and the CPU. This is required many times even at run time like
> > > > > cpuinfo. So, instead of parsing the ACPI table every time, cache
> > > > > the RINTC structures and provide a function to get the correct
> > > > > RINTC structure for a given cpu.
> > > > > 
> > > > > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> > > > > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > > ---
> > > > >  arch/riscv/include/asm/acpi.h |  2 ++
> > > > >  arch/riscv/kernel/acpi.c      | 60 +++++++++++++++++++++++++++++++++++
> > > > >  2 files changed, 62 insertions(+)
> > > > > 
> > > > > diff --git a/arch/riscv/include/asm/acpi.h b/arch/riscv/include/asm/acpi.h
> > > > > index 9be52b6ffae1..1606dce8992e 100644
> > > > > --- a/arch/riscv/include/asm/acpi.h
> > > > > +++ b/arch/riscv/include/asm/acpi.h
> > > > > @@ -59,6 +59,8 @@ static inline bool acpi_has_cpu_in_madt(void)
> > > > > 
> > > > >  static inline void arch_fix_phys_package_id(int num, u32 slot) { }
> > > > > 
> > > > > +struct acpi_madt_rintc *acpi_cpu_get_madt_rintc(int cpu);
> > > > > +u32 get_acpi_id_for_cpu(int cpu);
> > > > >  #endif /* CONFIG_ACPI */
> > > > > 
> > > > >  #endif /*_ASM_ACPI_H*/
> > > > > diff --git a/arch/riscv/kernel/acpi.c b/arch/riscv/kernel/acpi.c
> > > > > index 81d448c41714..40ab55309c70 100644
> > > > > --- a/arch/riscv/kernel/acpi.c
> > > > > +++ b/arch/riscv/kernel/acpi.c
> > > > > @@ -24,6 +24,66 @@ EXPORT_SYMBOL(acpi_disabled);
> > > > >  int acpi_pci_disabled = 1;	/* skip ACPI PCI scan and IRQ initialization */
> > > > >  EXPORT_SYMBOL(acpi_pci_disabled);
> > > > > 
> > > > > +static struct acpi_madt_rintc cpu_madt_rintc[NR_CPUS];
> > > > > +
> > > > > +static int acpi_parse_madt_rintc(union acpi_subtable_headers *header, const unsigned long end)
> > > > > +{
> > > > > +	struct acpi_madt_rintc *rintc = (struct acpi_madt_rintc *)header;
> > > > > +	int cpuid;
> > > > > +
> > > > > +	if (!(rintc->flags & ACPI_MADT_ENABLED))
> > > > > +		return 0;
> > > > > +
> > > > > +	cpuid = riscv_hartid_to_cpuid(rintc->hart_id);
> > > > 
> > > > Unless I'm missing something, this races with CPUs coming online.  Maybe
> > > > that's a rare enough case we don't care, but I think we'd also just have
> > > > simpler logic if we fixed it...
> > > > 
> > > This depend only on cpuid_to_hartid_map filled up. I wish I could
> > > initialize this RINTC mapping in setup_smp() itself like ARM64. But in
> > > RISC-V, this file smpboot.c gets built only when CONFIG_SMP is enabled.
> > > Hence, we need to initialize this array outside of setup_smp().
> > > 
> > > I can update the code to initialize this from setup_arch() immediately
> > > after setup_smp() if ACPI is enabled. That should avoid the global
> > > variable check also. Let me know if you prefer this.
> > > 
> > > > > +	/*
> > > > > +	 * When CONFIG_SMP is disabled, mapping won't be created for
> > > > > +	 * all cpus.
> > > > > +	 * CPUs more than NR_CPUS, will be ignored.
> > > > > +	 */
> > > > > +	if (cpuid >= 0 && cpuid < NR_CPUS)
> > > > > +		cpu_madt_rintc[cpuid] = *rintc;
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +static int acpi_init_rintc_array(void)
> > > > > +{
> > > > > +	if (acpi_table_parse_madt(ACPI_MADT_TYPE_RINTC, acpi_parse_madt_rintc, 0) > 0)
> > > > > +		return 0;
> > > > > +
> > > > > +	return -ENODEV;
> > > > > +}
> > > > > +
> > > > > +/*
> > > > > + * Instead of parsing (and freeing) the ACPI table, cache
> > > > > + * the RINTC structures since they are frequently used
> > > > > + * like in  cpuinfo.
> > > > > + */
> > > > > +struct acpi_madt_rintc *acpi_cpu_get_madt_rintc(int cpu)
> > > > > +{
> > > > > +	static bool rintc_init_done;
> > > > 
> > > > ... basically just get rid of this global variable, and instead have a
> > > > 
> > > >    if (!&cpu_madt_rintc[cpu])
> > > >        ... parse ...
> > > >    return &cpu_madt_rintc[cpu];
> > > > 
> > > > that'd probably let us get rid of a handful of these helpers too, as now
> > > > it's just a call to the parsing bits.
> > > > 
> > > I am afraid this (!&cpu_madt_rintc[cpu]) check won't work since we are
> > > not caching the RINTC pointers but actual contents itself. So, the
> > > address is always valid. However, as per Drew's earlier feedback I am
> > > going to reduce one helper. I am planning to send the next version of
> > > this patch once 6.4 rc1 is available since the ACPICA patches are merged
> > > now.
> > > 
> > > > > +
> > > > > +	if (!rintc_init_done) {
> > > > > +		if (acpi_init_rintc_array()) {
> > > > > +			pr_err("No valid RINTC entries exist\n");
> > > > > +			return NULL;
> > > > > +		}
> > > > > +
> > > > > +		rintc_init_done = true;
> > > > > +	}
> > > > > +
> > > > > +	return &cpu_madt_rintc[cpu];
> > > > > +}
> > > > > +
> > > > > +u32 get_acpi_id_for_cpu(int cpu)
> > > > > +{
> > > > > +	struct acpi_madt_rintc *rintc = acpi_cpu_get_madt_rintc(cpu);
> > > > > +
> > > > > +	BUG_ON(!rintc);
> > > > 
> > > > We should have some better error reporting here.  It looks like all the
> > > > callerss of get_acpi_id_for_cpu() are tolerant of a nonsense ID being
> > > > returned, so maybe we just pr_warn() something users can understand and then
> > > > return -1 or something?
> > > > 
> > > 
> > > RINTC is mandatory for ACPI systems. Also, all 32bit values are valid
> > > for UID. So, there is no bogus value we can return. 
> > > 
> > > Actually, I just realized this check is redundant. It will never be NULL
> > > since it is a static array. So, we can just get rid of the BUG.
> > 
> > It can be NULL on the first call of acpi_cpu_get_madt_rintc(), which is
> > a good time to BUG if there's isn't an RINTC.
> > 
> Sorry, I mean if we change the initialization to get called from
> setup_arch, then we can get rid of this check along with global variable
> check, correct?

Sounds good to me, but now I think we're pushing the question of whether
to BUG or not on a missing RINTC to that new init function, because
otherwise we'll still end up in get_acpi_id_for_cpu() eventually with
or without a valid rintc from which we get the uid (and the uid has no
specified bogus value).

Thanks,
drew