| Message ID | 20230404182037.863533-9-sunilvl@ventanamicro.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add basic ACPI support for RISC-V | expand |
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
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; +} + +/* + * 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.