| Message ID | 1486655834-9708-17-git-send-email-vijay.kilari@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
(+ Jan as ACPI maintainer) Hello Vijay, On 09/02/17 15:57, vijay.kilari@gmail.com wrote: > From: Vijaya Kumar K <Vijaya.Kumar@cavium.com> > > Register SRAT entry handler for type > ACPI_SRAT_TYPE_GICC_AFFINITY to parse SRAT table > and extract proximity for all CPU IDs. > > Signed-off-by: Vijaya Kumar <Vijaya.Kumar@cavium.com> > --- > xen/arch/arm/acpi_numa.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++ > xen/drivers/acpi/numa.c | 37 +++++++++++++++++++++++++++++++ > xen/drivers/acpi/osl.c | 2 ++ > xen/include/acpi/actbl1.h | 17 ++++++++++++++- > xen/include/xen/acpi.h | 39 +++++++++++++++++++++++++++++++++ > 5 files changed, 149 insertions(+), 1 deletion(-) > > diff --git a/xen/arch/arm/acpi_numa.c b/xen/arch/arm/acpi_numa.c > index 3ee87f2..f659275 100644 > --- a/xen/arch/arm/acpi_numa.c > +++ b/xen/arch/arm/acpi_numa.c > @@ -105,6 +105,61 @@ static int __init acpi_parse_madt_handler(struct acpi_subtable_header *header, > return 0; > } > > +/* Callback for Proximity Domain -> ACPI processor UID mapping */ > +void __init acpi_numa_gicc_affinity_init(const struct acpi_srat_gicc_affinity *pa) > +{ > + int pxm, node; > + u64 mpidr = 0; mpidr does not need to be set to 0. > + static u32 cpus_in_srat; > + > + if ( srat_disabled() ) > + return; > + > + if ( pa->header.length < sizeof(struct acpi_srat_gicc_affinity) ) > + { > + printk(XENLOG_WARNING "SRAT: Invalid SRAT header length: %d\n", > + pa->header.length); > + bad_srat(); > + return; > + } > + > + if ( !(pa->flags & ACPI_SRAT_GICC_ENABLED) ) > + return; > + > + if ( cpus_in_srat >= NR_CPUS ) > + { > + printk(XENLOG_WARNING This should be XENLOG_ERROR. > + "SRAT: cpu_to_node_map[%d] is too small to fit all cpus\n", > + NR_CPUS); > + return; > + } > + > + pxm = pa->proximity_domain; > + node = setup_node(pxm); > + if ( node == NUMA_NO_NODE || node >= MAX_NUMNODES ) Looking at the implementation of setup_node, node will either be equal to NUMA_NO_NODE or valid. It is not possible to have node >= MAX_NUMNODES. > + { > + printk(XENLOG_WARNING "SRAT: Too many proximity domains %d\n", pxm); setup_node is already printing an error if we have too many proximity domains. So no need to duplicate twice. > + bad_srat(); > + return; > + } > + > + mpidr = acpi_get_cpu_mpidr(pa->acpi_processor_uid); > + if ( mpidr == MPIDR_INVALID ) > + { > + printk(XENLOG_WARNING s/XENLOG_WARNING/XENLOG_ERROR/ > + "SRAT: PXM %d with ACPI ID %d has no valid MPIDR in MADT\n", > + pxm, pa->acpi_processor_uid); > + bad_srat(); > + return; > + } > + > + node_set(node, numa_nodes_parsed); > + cpus_in_srat++; > + acpi_numa = 1; > + printk(XENLOG_INFO "SRAT: PXM %d -> MPIDR 0x%lx -> Node %d\n", > + pxm, mpidr, node); > +} > + > void __init acpi_map_uid_to_mpidr(void) > { > int i; > diff --git a/xen/drivers/acpi/numa.c b/xen/drivers/acpi/numa.c > index 50bf9f8..ce22e88 100644 > --- a/xen/drivers/acpi/numa.c > +++ b/xen/drivers/acpi/numa.c > @@ -25,9 +25,11 @@ > #include <xen/init.h> > #include <xen/types.h> > #include <xen/errno.h> > +#include <xen/mm.h> Why do you need to inlucde xen/mm.h and ... > #include <xen/acpi.h> > #include <xen/numa.h> > #include <acpi/acmacros.h> > +#include <asm/mm.h> asm/mm.h? > > #define ACPI_NUMA 0x80000000 > #define _COMPONENT ACPI_NUMA > @@ -105,6 +107,21 @@ void __init acpi_table_print_srat_entry(struct acpi_subtable_header * header) > } > #endif /* ACPI_DEBUG_OUTPUT */ > break; > + case ACPI_SRAT_TYPE_GICC_AFFINITY: > +#ifdef ACPI_DEBUG_OUTPUT > + { > + struct acpi_srat_gicc_affinity *p = > + (struct acpi_srat_gicc_affinity *)header; > + ACPI_DEBUG_PRINT((ACPI_DB_INFO, > + "SRAT Processor (acpi id[0x%04x]) in" > + " proximity domain %d %s\n", > + p->acpi_processor_uid, > + p->proximity_domain, > + (p->flags & ACPI_SRAT_GICC_ENABLED) ? > + "enabled" : "disabled"); > + } > +#endif /* ACPI_DEBUG_OUTPUT */ > + break; > default: > printk(KERN_WARNING PREFIX > "Found unsupported SRAT entry (type = %#x)\n", > @@ -185,6 +202,24 @@ int __init acpi_parse_srat(struct acpi_table_header *table) > return 0; > } > > +static int __init > +acpi_parse_gicc_affinity(struct acpi_subtable_header *header, > + const unsigned long end) > +{ > + const struct acpi_srat_gicc_affinity *processor_affinity > + = (struct acpi_srat_gicc_affinity *)header; > + > + if (!processor_affinity) > + return -EINVAL; > + > + acpi_table_print_srat_entry(header); > + > + /* let architecture-dependent part to do it */ > + acpi_numa_gicc_affinity_init(processor_affinity); > + > + return 0; > +} > + > int __init > acpi_table_parse_srat(int id, acpi_madt_entry_handler handler, > unsigned int max_entries) > @@ -205,6 +240,8 @@ int __init acpi_numa_init(void) > acpi_table_parse_srat(ACPI_SRAT_TYPE_MEMORY_AFFINITY, > acpi_parse_memory_affinity, > NR_NODE_MEMBLKS); > + acpi_table_parse_srat(ACPI_SRAT_TYPE_GICC_AFFINITY, > + acpi_parse_gicc_affinity, NR_CPUS); > } > > /* SLIT: System Locality Information Table */ > diff --git a/xen/drivers/acpi/osl.c b/xen/drivers/acpi/osl.c > index 7199047..7046816 100644 > --- a/xen/drivers/acpi/osl.c > +++ b/xen/drivers/acpi/osl.c > @@ -29,6 +29,7 @@ > #include <xen/pfn.h> > #include <xen/types.h> > #include <xen/errno.h> > +#include <xen/mm.h> > #include <xen/acpi.h> > #include <xen/numa.h> > #include <acpi/acmacros.h> > @@ -39,6 +40,7 @@ > #include <xen/efi.h> > #include <xen/vmap.h> > #include <xen/kconfig.h> > +#include <asm/mm.h> > > #define _COMPONENT ACPI_OS_SERVICES > ACPI_MODULE_NAME("osl") > diff --git a/xen/include/acpi/actbl1.h b/xen/include/acpi/actbl1.h > index e199136..b84bfba 100644 > --- a/xen/include/acpi/actbl1.h > +++ b/xen/include/acpi/actbl1.h > @@ -949,7 +949,8 @@ enum acpi_srat_type { > ACPI_SRAT_TYPE_CPU_AFFINITY = 0, > ACPI_SRAT_TYPE_MEMORY_AFFINITY = 1, > ACPI_SRAT_TYPE_X2APIC_CPU_AFFINITY = 2, > - ACPI_SRAT_TYPE_RESERVED = 3 /* 3 and greater are reserved */ > + ACPI_SRAT_TYPE_GICC_AFFINITY = 3, > + ACPI_SRAT_TYPE_RESERVED = 4 /* 4 and greater are reserved */ > }; > > /* > @@ -1007,6 +1008,20 @@ struct acpi_srat_x2apic_cpu_affinity { > > #define ACPI_SRAT_CPU_ENABLED (1) /* 00: Use affinity structure */ > > +/* 3: GICC Affinity (ACPI 5.1) */ > + > +struct acpi_srat_gicc_affinity { > + struct acpi_subtable_header header; > + u32 proximity_domain; > + u32 acpi_processor_uid; > + u32 flags; > + u32 clock_domain; > +}; > + > +/* Flags for struct acpi_srat_gicc_affinity */ > + > +#define ACPI_SRAT_GICC_ENABLED (1) /* 00: Use affinity structure */ > + > /* Reset to default packing */ > > #pragma pack() > diff --git a/xen/include/xen/acpi.h b/xen/include/xen/acpi.h > index 30ec0ee..67fe1bb 100644 > --- a/xen/include/xen/acpi.h > +++ b/xen/include/xen/acpi.h > @@ -92,10 +92,49 @@ void acpi_table_print_srat_entry (struct acpi_subtable_header *srat); > > /* the following four functions are architecture-dependent */ > void acpi_numa_slit_init (struct acpi_table_slit *slit); > +#if defined(CONFIG_X86) > void acpi_numa_processor_affinity_init(const struct acpi_srat_cpu_affinity *); > void acpi_numa_x2apic_affinity_init(const struct acpi_srat_x2apic_cpu_affinity *); > void acpi_numa_memory_affinity_init(const struct acpi_srat_mem_affinity *); > void acpi_numa_arch_fixup(void); > +static inline void > +acpi_numa_gicc_affinity_init(const struct acpi_srat_gicc_affinity *pa) > +{ > + return; > +} > +#elif defined(CONFIG_ARM) > +static inline void > +acpi_numa_processor_affinity_init(const struct acpi_srat_cpu_affinity *cpu_aff) > +{ > + return; > +} > +static inline void > +acpi_numa_x2apic_affinity_init(const struct acpi_srat_x2apic_cpu_affinity *x2apic) > +{ > + return; > +} > +#if defined(CONFIG_ACPI_NUMA) > +void acpi_numa_gicc_affinity_init(const struct acpi_srat_gicc_affinity *pa); > +void acpi_numa_memory_affinity_init(const struct acpi_srat_mem_affinity *); > +void acpi_numa_arch_fixup(void); > +#else > +static inline void > +acpi_numa_gicc_affinity_init(const struct acpi_srat_gicc_affinity *pa) > +{ > + return; > +} > +static inline void > +acpi_numa_memory_affinity_init(const struct acpi_srat_mem_affinity *ma) > +{ > + return; > +} > +static inline void > +acpi_numa_arch_fixup(void) > +{ > + return; > +} > +#endif /* CONFIG_ACPI_NUMA */ This is quite disgusting. We should avoid any #ifdef CONFIG_{X86,ARM} in common header. Also, x2apic and gicc are respectively x86-specific and arm-specific. So I think we should move the parsing in a separate arch-depend function to avoid those ifdery. By that I mean having a function acpi_table_arch_parse_srat that will parse x2apci on x86 and gicc on ARM. Jan, what do you think? Cheers, > +#endif /* CONFIG_X86 */ > > #ifdef CONFIG_ACPI_HOTPLUG_CPU > /* Arch dependent functions for cpu hotplug support */ >
On Thu, Mar 2, 2017 at 10:51 PM, Julien Grall <julien.grall@arm.com> wrote: > (+ Jan as ACPI maintainer) > > Hello Vijay, > > On 09/02/17 15:57, vijay.kilari@gmail.com wrote: >> >> From: Vijaya Kumar K <Vijaya.Kumar@cavium.com> >> >> Register SRAT entry handler for type >> ACPI_SRAT_TYPE_GICC_AFFINITY to parse SRAT table >> and extract proximity for all CPU IDs. >> >> Signed-off-by: Vijaya Kumar <Vijaya.Kumar@cavium.com> >> --- >> xen/arch/arm/acpi_numa.c | 55 >> +++++++++++++++++++++++++++++++++++++++++++++++ >> xen/drivers/acpi/numa.c | 37 +++++++++++++++++++++++++++++++ >> xen/drivers/acpi/osl.c | 2 ++ >> xen/include/acpi/actbl1.h | 17 ++++++++++++++- >> xen/include/xen/acpi.h | 39 +++++++++++++++++++++++++++++++++ >> 5 files changed, 149 insertions(+), 1 deletion(-) >> >> diff --git a/xen/arch/arm/acpi_numa.c b/xen/arch/arm/acpi_numa.c >> index 3ee87f2..f659275 100644 >> --- a/xen/arch/arm/acpi_numa.c >> +++ b/xen/arch/arm/acpi_numa.c >> @@ -105,6 +105,61 @@ static int __init acpi_parse_madt_handler(struct >> acpi_subtable_header *header, >> return 0; >> } >> >> +/* Callback for Proximity Domain -> ACPI processor UID mapping */ >> +void __init acpi_numa_gicc_affinity_init(const struct >> acpi_srat_gicc_affinity *pa) >> +{ >> + int pxm, node; >> + u64 mpidr = 0; > > > mpidr does not need to be set to 0. > >> + static u32 cpus_in_srat; >> + >> + if ( srat_disabled() ) >> + return; >> + >> + if ( pa->header.length < sizeof(struct acpi_srat_gicc_affinity) ) >> + { >> + printk(XENLOG_WARNING "SRAT: Invalid SRAT header length: %d\n", >> + pa->header.length); >> + bad_srat(); >> + return; >> + } >> + >> + if ( !(pa->flags & ACPI_SRAT_GICC_ENABLED) ) >> + return; >> + >> + if ( cpus_in_srat >= NR_CPUS ) >> + { >> + printk(XENLOG_WARNING > > > This should be XENLOG_ERROR. OK > >> + "SRAT: cpu_to_node_map[%d] is too small to fit all >> cpus\n", >> + NR_CPUS); >> + return; >> + } >> + >> + pxm = pa->proximity_domain; >> + node = setup_node(pxm); >> + if ( node == NUMA_NO_NODE || node >= MAX_NUMNODES ) > > > Looking at the implementation of setup_node, node will either be equal to > NUMA_NO_NODE or valid. It is not possible to have node >= MAX_NUMNODES. ok > >> + { >> + printk(XENLOG_WARNING "SRAT: Too many proximity domains %d\n", >> pxm); > > > setup_node is already printing an error if we have too many proximity > domains. So no need to duplicate twice. ok > >> + bad_srat(); >> + return; >> + } >> + >> + mpidr = acpi_get_cpu_mpidr(pa->acpi_processor_uid); >> + if ( mpidr == MPIDR_INVALID ) >> + { >> + printk(XENLOG_WARNING > > > s/XENLOG_WARNING/XENLOG_ERROR/ > >> + "SRAT: PXM %d with ACPI ID %d has no valid MPIDR in >> MADT\n", >> + pxm, pa->acpi_processor_uid); >> + bad_srat(); >> + return; >> + } >> + >> + node_set(node, numa_nodes_parsed); >> + cpus_in_srat++; >> + acpi_numa = 1; >> + printk(XENLOG_INFO "SRAT: PXM %d -> MPIDR 0x%lx -> Node %d\n", >> + pxm, mpidr, node); >> +} >> + >> void __init acpi_map_uid_to_mpidr(void) >> { >> int i; >> diff --git a/xen/drivers/acpi/numa.c b/xen/drivers/acpi/numa.c >> index 50bf9f8..ce22e88 100644 >> --- a/xen/drivers/acpi/numa.c >> +++ b/xen/drivers/acpi/numa.c >> @@ -25,9 +25,11 @@ >> #include <xen/init.h> >> #include <xen/types.h> >> #include <xen/errno.h> >> +#include <xen/mm.h> > > > Why do you need to inlucde xen/mm.h and ... > >> #include <xen/acpi.h> >> #include <xen/numa.h> >> #include <acpi/acmacros.h> >> +#include <asm/mm.h> > > > asm/mm.h? I remember when CONFIG_ACPI +CONFIG_NUMA is enabled there is compilation error. > > >> >> #define ACPI_NUMA 0x80000000 >> #define _COMPONENT ACPI_NUMA >> @@ -105,6 +107,21 @@ void __init acpi_table_print_srat_entry(struct >> acpi_subtable_header * header) >> } >> #endif /* ACPI_DEBUG_OUTPUT */ >> break; >> + case ACPI_SRAT_TYPE_GICC_AFFINITY: >> +#ifdef ACPI_DEBUG_OUTPUT >> + { >> + struct acpi_srat_gicc_affinity *p = >> + (struct acpi_srat_gicc_affinity *)header; >> + ACPI_DEBUG_PRINT((ACPI_DB_INFO, >> + "SRAT Processor (acpi >> id[0x%04x]) in" >> + " proximity domain %d %s\n", >> + p->acpi_processor_uid, >> + p->proximity_domain, >> + (p->flags & >> ACPI_SRAT_GICC_ENABLED) ? >> + "enabled" : "disabled"); >> + } >> +#endif /* ACPI_DEBUG_OUTPUT */ >> + break; >> default: >> printk(KERN_WARNING PREFIX >> "Found unsupported SRAT entry (type = %#x)\n", >> @@ -185,6 +202,24 @@ int __init acpi_parse_srat(struct acpi_table_header >> *table) >> return 0; >> } >> >> +static int __init >> +acpi_parse_gicc_affinity(struct acpi_subtable_header *header, >> + const unsigned long end) >> +{ >> + const struct acpi_srat_gicc_affinity *processor_affinity >> + = (struct acpi_srat_gicc_affinity *)header; >> + >> + if (!processor_affinity) >> + return -EINVAL; >> + >> + acpi_table_print_srat_entry(header); >> + >> + /* let architecture-dependent part to do it */ >> + acpi_numa_gicc_affinity_init(processor_affinity); >> + >> + return 0; >> +} >> + >> int __init >> acpi_table_parse_srat(int id, acpi_madt_entry_handler handler, >> unsigned int max_entries) >> @@ -205,6 +240,8 @@ int __init acpi_numa_init(void) >> acpi_table_parse_srat(ACPI_SRAT_TYPE_MEMORY_AFFINITY, >> acpi_parse_memory_affinity, >> NR_NODE_MEMBLKS); >> + acpi_table_parse_srat(ACPI_SRAT_TYPE_GICC_AFFINITY, >> + acpi_parse_gicc_affinity, NR_CPUS); >> } >> >> /* SLIT: System Locality Information Table */ >> diff --git a/xen/drivers/acpi/osl.c b/xen/drivers/acpi/osl.c >> index 7199047..7046816 100644 >> --- a/xen/drivers/acpi/osl.c >> +++ b/xen/drivers/acpi/osl.c >> @@ -29,6 +29,7 @@ >> #include <xen/pfn.h> >> #include <xen/types.h> >> #include <xen/errno.h> >> +#include <xen/mm.h> >> #include <xen/acpi.h> >> #include <xen/numa.h> >> #include <acpi/acmacros.h> >> @@ -39,6 +40,7 @@ >> #include <xen/efi.h> >> #include <xen/vmap.h> >> #include <xen/kconfig.h> >> +#include <asm/mm.h> >> >> #define _COMPONENT ACPI_OS_SERVICES >> ACPI_MODULE_NAME("osl") >> diff --git a/xen/include/acpi/actbl1.h b/xen/include/acpi/actbl1.h >> index e199136..b84bfba 100644 >> --- a/xen/include/acpi/actbl1.h >> +++ b/xen/include/acpi/actbl1.h >> @@ -949,7 +949,8 @@ enum acpi_srat_type { >> ACPI_SRAT_TYPE_CPU_AFFINITY = 0, >> ACPI_SRAT_TYPE_MEMORY_AFFINITY = 1, >> ACPI_SRAT_TYPE_X2APIC_CPU_AFFINITY = 2, >> - ACPI_SRAT_TYPE_RESERVED = 3 /* 3 and greater are reserved */ >> + ACPI_SRAT_TYPE_GICC_AFFINITY = 3, >> + ACPI_SRAT_TYPE_RESERVED = 4 /* 4 and greater are reserved */ >> }; >> >> /* >> @@ -1007,6 +1008,20 @@ struct acpi_srat_x2apic_cpu_affinity { >> >> #define ACPI_SRAT_CPU_ENABLED (1) /* 00: Use affinity >> structure */ >> >> +/* 3: GICC Affinity (ACPI 5.1) */ >> + >> +struct acpi_srat_gicc_affinity { >> + struct acpi_subtable_header header; >> + u32 proximity_domain; >> + u32 acpi_processor_uid; >> + u32 flags; >> + u32 clock_domain; >> +}; >> + >> +/* Flags for struct acpi_srat_gicc_affinity */ >> + >> +#define ACPI_SRAT_GICC_ENABLED (1) /* 00: Use affinity structure */ >> + >> /* Reset to default packing */ >> >> #pragma pack() >> diff --git a/xen/include/xen/acpi.h b/xen/include/xen/acpi.h >> index 30ec0ee..67fe1bb 100644 >> --- a/xen/include/xen/acpi.h >> +++ b/xen/include/xen/acpi.h >> @@ -92,10 +92,49 @@ void acpi_table_print_srat_entry (struct >> acpi_subtable_header *srat); >> >> /* the following four functions are architecture-dependent */ >> void acpi_numa_slit_init (struct acpi_table_slit *slit); >> +#if defined(CONFIG_X86) >> void acpi_numa_processor_affinity_init(const struct >> acpi_srat_cpu_affinity *); >> void acpi_numa_x2apic_affinity_init(const struct >> acpi_srat_x2apic_cpu_affinity *); >> void acpi_numa_memory_affinity_init(const struct acpi_srat_mem_affinity >> *); >> void acpi_numa_arch_fixup(void); >> +static inline void >> +acpi_numa_gicc_affinity_init(const struct acpi_srat_gicc_affinity *pa) >> +{ >> + return; >> +} >> +#elif defined(CONFIG_ARM) >> +static inline void >> +acpi_numa_processor_affinity_init(const struct acpi_srat_cpu_affinity >> *cpu_aff) >> +{ >> + return; >> +} >> +static inline void >> +acpi_numa_x2apic_affinity_init(const struct acpi_srat_x2apic_cpu_affinity >> *x2apic) >> +{ >> + return; >> +} >> +#if defined(CONFIG_ACPI_NUMA) >> +void acpi_numa_gicc_affinity_init(const struct acpi_srat_gicc_affinity >> *pa); >> +void acpi_numa_memory_affinity_init(const struct acpi_srat_mem_affinity >> *); >> +void acpi_numa_arch_fixup(void); >> +#else >> +static inline void >> +acpi_numa_gicc_affinity_init(const struct acpi_srat_gicc_affinity *pa) >> +{ >> + return; >> +} >> +static inline void >> +acpi_numa_memory_affinity_init(const struct acpi_srat_mem_affinity *ma) >> +{ >> + return; >> +} >> +static inline void >> +acpi_numa_arch_fixup(void) >> +{ >> + return; >> +} >> +#endif /* CONFIG_ACPI_NUMA */ > > > This is quite disgusting. We should avoid any #ifdef CONFIG_{X86,ARM} in > common header. > > Also, x2apic and gicc are respectively x86-specific and arm-specific. So I > think we should move the parsing in a separate arch-depend function to avoid > those ifdery. > > By that I mean having a function acpi_table_arch_parse_srat that will parse > x2apci on x86 and gicc on ARM. Jan, what do you think? Linux also follows similar approach https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/include/linux/acpi.h?id=refs/tags/v4.10#n265 Regards Vijay
Hello Vijay, On 03/03/17 12:39, Vijay Kilari wrote: > On Thu, Mar 2, 2017 at 10:51 PM, Julien Grall <julien.grall@arm.com> wrote: >>> diff --git a/xen/drivers/acpi/numa.c b/xen/drivers/acpi/numa.c >>> index 50bf9f8..ce22e88 100644 >>> --- a/xen/drivers/acpi/numa.c >>> +++ b/xen/drivers/acpi/numa.c >>> @@ -25,9 +25,11 @@ >>> #include <xen/init.h> >>> #include <xen/types.h> >>> #include <xen/errno.h> >>> +#include <xen/mm.h> >> >> >> Why do you need to inlucde xen/mm.h and ... >> >>> #include <xen/acpi.h> >>> #include <xen/numa.h> >>> #include <acpi/acmacros.h> >>> +#include <asm/mm.h> >> >> >> asm/mm.h? > > I remember when CONFIG_ACPI +CONFIG_NUMA is enabled > there is compilation error. Regarding asm/mm.h, it is already included by xen/mm.h. So no point to add it. Now regarding xen/mm.h, just saying there is a compilation error is not helpful. Can you expand why it is needed? [...] >> This is quite disgusting. We should avoid any #ifdef CONFIG_{X86,ARM} in >> common header. >> >> Also, x2apic and gicc are respectively x86-specific and arm-specific. So I >> think we should move the parsing in a separate arch-depend function to avoid >> those ifdery. >> >> By that I mean having a function acpi_table_arch_parse_srat that will parse >> x2apci on x86 and gicc on ARM. Jan, what do you think? > > Linux also follows similar approach > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/include/linux/acpi.h?id=refs/tags/v4.10#n265 So? What are you trying to prove? The linux version is much readable than yours. Anyway, we should limit CONFIG_{X86,ARM} ifdefery in common code. Currently, I see no point to have those ifdefery when it is possible to add an arch-depend function. Regards,
On Fri, Mar 3, 2017 at 7:14 PM, Julien Grall <julien.grall@arm.com> wrote: > Hello Vijay, > > On 03/03/17 12:39, Vijay Kilari wrote: >> >> On Thu, Mar 2, 2017 at 10:51 PM, Julien Grall <julien.grall@arm.com> >> wrote: >>>> >>>> diff --git a/xen/drivers/acpi/numa.c b/xen/drivers/acpi/numa.c >>>> index 50bf9f8..ce22e88 100644 >>>> --- a/xen/drivers/acpi/numa.c >>>> +++ b/xen/drivers/acpi/numa.c >>>> @@ -25,9 +25,11 @@ >>>> #include <xen/init.h> >>>> #include <xen/types.h> >>>> #include <xen/errno.h> >>>> +#include <xen/mm.h> >>> >>> >>> >>> Why do you need to inlucde xen/mm.h and ... >>> >>>> #include <xen/acpi.h> >>>> #include <xen/numa.h> >>>> #include <acpi/acmacros.h> >>>> +#include <asm/mm.h> >>> >>> >>> >>> asm/mm.h? >> >> >> I remember when CONFIG_ACPI +CONFIG_NUMA is enabled >> there is compilation error. > > > Regarding asm/mm.h, it is already included by xen/mm.h. So no point to add > it. > > Now regarding xen/mm.h, just saying there is a compilation error is not > helpful. Can you expand why it is needed? I remember just adding xen/mm.h has not solved the problem. Anyway I will check this when I work for next revision. > > [...] > >>> This is quite disgusting. We should avoid any #ifdef CONFIG_{X86,ARM} in >>> common header. >>> >>> Also, x2apic and gicc are respectively x86-specific and arm-specific. So >>> I >>> think we should move the parsing in a separate arch-depend function to >>> avoid >>> those ifdery. >>> >>> By that I mean having a function acpi_table_arch_parse_srat that will >>> parse >>> x2apci on x86 and gicc on ARM. Jan, what do you think? >> >> >> Linux also follows similar approach >> >> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/include/linux/acpi.h?id=refs/tags/v4.10#n265 > > > So? What are you trying to prove? > > The linux version is much readable than yours. Anyway, we should limit > CONFIG_{X86,ARM} ifdefery in common code. > > Currently, I see no point to have those ifdefery when it is possible to add > an arch-depend function. This is because in xen we have another level of config CONFIG_ACPI_NUMA. I have plans to reuse cpu and memory part next revision. Regards Vijay
On 03/03/17 13:50, Vijay Kilari wrote: > On Fri, Mar 3, 2017 at 7:14 PM, Julien Grall <julien.grall@arm.com> wrote: >>>> This is quite disgusting. We should avoid any #ifdef CONFIG_{X86,ARM} in >>>> common header. >>>> >>>> Also, x2apic and gicc are respectively x86-specific and arm-specific. So >>>> I >>>> think we should move the parsing in a separate arch-depend function to >>>> avoid >>>> those ifdery. >>>> >>>> By that I mean having a function acpi_table_arch_parse_srat that will >>>> parse >>>> x2apci on x86 and gicc on ARM. Jan, what do you think? >>> >>> >>> Linux also follows similar approach >>> >>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/include/linux/acpi.h?id=refs/tags/v4.10#n265 >> >> >> So? What are you trying to prove? >> >> The linux version is much readable than yours. Anyway, we should limit >> CONFIG_{X86,ARM} ifdefery in common code. >> >> Currently, I see no point to have those ifdefery when it is possible to add >> an arch-depend function. > > This is because in xen we have another level of config CONFIG_ACPI_NUMA. > I have plans to reuse cpu and memory part next revision. This does not explain why you want to do like Linux.
On Fri, Mar 3, 2017 at 7:22 PM, Julien Grall <julien.grall@arm.com> wrote: > > > On 03/03/17 13:50, Vijay Kilari wrote: >> >> On Fri, Mar 3, 2017 at 7:14 PM, Julien Grall <julien.grall@arm.com> wrote: >>>>> >>>>> This is quite disgusting. We should avoid any #ifdef CONFIG_{X86,ARM} >>>>> in >>>>> common header. >>>>> >>>>> Also, x2apic and gicc are respectively x86-specific and arm-specific. >>>>> So >>>>> I >>>>> think we should move the parsing in a separate arch-depend function to >>>>> avoid >>>>> those ifdery. >>>>> >>>>> By that I mean having a function acpi_table_arch_parse_srat that will >>>>> parse >>>>> x2apci on x86 and gicc on ARM. Jan, what do you think? >>>> >>>> >>>> >>>> Linux also follows similar approach >>>> >>>> >>>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/include/linux/acpi.h?id=refs/tags/v4.10#n265 >>> >>> >>> >>> So? What are you trying to prove? >>> >>> The linux version is much readable than yours. Anyway, we should limit >>> CONFIG_{X86,ARM} ifdefery in common code. >>> >>> Currently, I see no point to have those ifdefery when it is possible to >>> add >>> an arch-depend function. >> >> >> This is because in xen we have another level of config CONFIG_ACPI_NUMA. >> I have plans to reuse cpu and memory part next revision. > > > This does not explain why you want to do like Linux. Basically want to reuse xen/drivers/acpi/numa.c which is common for both x86 and ARM. If not, then we have move some arch specific as you mentioned. I have another idea where in, if all the NUMA ACPI code is programmed under CONFIG_NUMA and only initialization is kept under CONFIG_ACPI_NUMA similar to x86 then we don't need to pollute this header much and limit the changes. I will try to implement this and see how simple it can go and let you know. OK? > > -- > Julien Grall
On 03/03/17 14:45, Vijay Kilari wrote: > On Fri, Mar 3, 2017 at 7:22 PM, Julien Grall <julien.grall@arm.com> wrote: >> >> >> On 03/03/17 13:50, Vijay Kilari wrote: >>> >>> On Fri, Mar 3, 2017 at 7:14 PM, Julien Grall <julien.grall@arm.com> wrote: >>>>>> >>>>>> This is quite disgusting. We should avoid any #ifdef CONFIG_{X86,ARM} >>>>>> in >>>>>> common header. >>>>>> >>>>>> Also, x2apic and gicc are respectively x86-specific and arm-specific. >>>>>> So >>>>>> I >>>>>> think we should move the parsing in a separate arch-depend function to >>>>>> avoid >>>>>> those ifdery. >>>>>> >>>>>> By that I mean having a function acpi_table_arch_parse_srat that will >>>>>> parse >>>>>> x2apci on x86 and gicc on ARM. Jan, what do you think? >>>>> >>>>> >>>>> >>>>> Linux also follows similar approach >>>>> >>>>> >>>>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/include/linux/acpi.h?id=refs/tags/v4.10#n265 >>>> >>>> >>>> >>>> So? What are you trying to prove? >>>> >>>> The linux version is much readable than yours. Anyway, we should limit >>>> CONFIG_{X86,ARM} ifdefery in common code. >>>> >>>> Currently, I see no point to have those ifdefery when it is possible to >>>> add >>>> an arch-depend function. >>> >>> >>> This is because in xen we have another level of config CONFIG_ACPI_NUMA. >>> I have plans to reuse cpu and memory part next revision. >> >> >> This does not explain why you want to do like Linux. > > Basically want to reuse xen/drivers/acpi/numa.c which is common for > both x86 and ARM. > If not, then we have move some arch specific as you mentioned. I think you misunderstood what I suggested. I only asked to do something like: int __init acpi_numa_init(void) { if (!acpi_parse_table(....)) { acpi_table_parse_srat(TYPE_CPU_AFFINITY); acpi_table_parse_srat(TYPE_MEMORY_AFFINITY); acpi_table_arch_parse_srat(); } } And then for x86 void acpi_table_arch_parse_start(void) { acpi_table_parse_srat(TYPE_X2APIC_CPU_AFFINITY); } And for ARM void acpi_table_arch_parse_start(void) { acpi_table_parse_srat(TYPE_GICC_AFFINITY); } The code is still as common but a function is called for arch specific setup. This does not require any ifdefery. > > I have another idea where in, if all the NUMA ACPI code is programmed under > CONFIG_NUMA and only initialization is kept under CONFIG_ACPI_NUMA > similar to x86 > then we don't need to pollute this header much and limit the changes. > > I will try to implement this and see how simple it can go and let you know. OK? I don't want to see the common header polluted with #ifdef CONFIG_X86 and #ifdef CONFIG_ARM. This is just not right. Cheers,
On Fri, Mar 3, 2017 at 8:22 PM, Julien Grall <julien.grall@arm.com> wrote: > > > On 03/03/17 14:45, Vijay Kilari wrote: >> >> On Fri, Mar 3, 2017 at 7:22 PM, Julien Grall <julien.grall@arm.com> wrote: >>> >>> >>> >>> On 03/03/17 13:50, Vijay Kilari wrote: >>>> >>>> >>>> On Fri, Mar 3, 2017 at 7:14 PM, Julien Grall <julien.grall@arm.com> >>>> wrote: >>>>>>> >>>>>>> >>>>>>> This is quite disgusting. We should avoid any #ifdef CONFIG_{X86,ARM} >>>>>>> in >>>>>>> common header. >>>>>>> >>>>>>> Also, x2apic and gicc are respectively x86-specific and arm-specific. >>>>>>> So >>>>>>> I >>>>>>> think we should move the parsing in a separate arch-depend function >>>>>>> to >>>>>>> avoid >>>>>>> those ifdery. >>>>>>> >>>>>>> By that I mean having a function acpi_table_arch_parse_srat that will >>>>>>> parse >>>>>>> x2apci on x86 and gicc on ARM. Jan, what do you think? >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> Linux also follows similar approach >>>>>> >>>>>> >>>>>> >>>>>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/include/linux/acpi.h?id=refs/tags/v4.10#n265 >>>>> >>>>> >>>>> >>>>> >>>>> So? What are you trying to prove? >>>>> >>>>> The linux version is much readable than yours. Anyway, we should limit >>>>> CONFIG_{X86,ARM} ifdefery in common code. >>>>> >>>>> Currently, I see no point to have those ifdefery when it is possible to >>>>> add >>>>> an arch-depend function. >>>> >>>> >>>> >>>> This is because in xen we have another level of config CONFIG_ACPI_NUMA. >>>> I have plans to reuse cpu and memory part next revision. >>> >>> >>> >>> This does not explain why you want to do like Linux. >> >> >> Basically want to reuse xen/drivers/acpi/numa.c which is common for >> both x86 and ARM. >> If not, then we have move some arch specific as you mentioned. > > > I think you misunderstood what I suggested. I only asked to do something > like: Got it. > > int __init acpi_numa_init(void) > { > if (!acpi_parse_table(....)) { > acpi_table_parse_srat(TYPE_CPU_AFFINITY); This is not defined for ARM. We have to make this also arch specific. So all arch specific code from xen/drivers/acpi/numa.c should be moved to arch specific to xen/arch/x86/srat.c > acpi_table_parse_srat(TYPE_MEMORY_AFFINITY); > acpi_table_arch_parse_srat(); > } > } > > And then for x86 > > void acpi_table_arch_parse_start(void) > { > acpi_table_parse_srat(TYPE_X2APIC_CPU_AFFINITY); > } > > And for ARM > > void acpi_table_arch_parse_start(void) > { > acpi_table_parse_srat(TYPE_GICC_AFFINITY); > } > > > The code is still as common but a function is called for arch specific > setup. This does not require any ifdefery. > >> >> I have another idea where in, if all the NUMA ACPI code is programmed >> under >> CONFIG_NUMA and only initialization is kept under CONFIG_ACPI_NUMA >> similar to x86 >> then we don't need to pollute this header much and limit the changes. >> >> I will try to implement this and see how simple it can go and let you >> know. OK? > > > I don't want to see the common header polluted with #ifdef CONFIG_X86 and > #ifdef CONFIG_ARM. This is just not right. > > Cheers, > > -- > Julien Grall
>>> On 03.03.17 at 16:16, <vijay.kilari@gmail.com> wrote: > On Fri, Mar 3, 2017 at 8:22 PM, Julien Grall <julien.grall@arm.com> wrote: >> int __init acpi_numa_init(void) >> { >> if (!acpi_parse_table(....)) { >> acpi_table_parse_srat(TYPE_CPU_AFFINITY); > > This is not defined for ARM. We have to make this also arch specific. > So all arch specific code from xen/drivers/acpi/numa.c should be moved > to arch specific to xen/arch/x86/srat.c There surely is a way to specify processor affinity on ARM? Jan
On Fri, Mar 3, 2017 at 8:52 PM, Jan Beulich <JBeulich@suse.com> wrote: >>>> On 03.03.17 at 16:16, <vijay.kilari@gmail.com> wrote: >> On Fri, Mar 3, 2017 at 8:22 PM, Julien Grall <julien.grall@arm.com> wrote: >>> int __init acpi_numa_init(void) >>> { >>> if (!acpi_parse_table(....)) { >>> acpi_table_parse_srat(TYPE_CPU_AFFINITY); >> >> This is not defined for ARM. We have to make this also arch specific. >> So all arch specific code from xen/drivers/acpi/numa.c should be moved >> to arch specific to xen/arch/x86/srat.c > > There surely is a way to specify processor affinity on ARM? In ARM, we use ACPI_SRAT_TYPE_GICC_AFFINITY type entry in SRAT to extract cpu to proximity mapping
diff --git a/xen/arch/arm/acpi_numa.c b/xen/arch/arm/acpi_numa.c index 3ee87f2..f659275 100644 --- a/xen/arch/arm/acpi_numa.c +++ b/xen/arch/arm/acpi_numa.c @@ -105,6 +105,61 @@ static int __init acpi_parse_madt_handler(struct acpi_subtable_header *header, return 0; } +/* Callback for Proximity Domain -> ACPI processor UID mapping */ +void __init acpi_numa_gicc_affinity_init(const struct acpi_srat_gicc_affinity *pa) +{ + int pxm, node; + u64 mpidr = 0; + static u32 cpus_in_srat; + + if ( srat_disabled() ) + return; + + if ( pa->header.length < sizeof(struct acpi_srat_gicc_affinity) ) + { + printk(XENLOG_WARNING "SRAT: Invalid SRAT header length: %d\n", + pa->header.length); + bad_srat(); + return; + } + + if ( !(pa->flags & ACPI_SRAT_GICC_ENABLED) ) + return; + + if ( cpus_in_srat >= NR_CPUS ) + { + printk(XENLOG_WARNING + "SRAT: cpu_to_node_map[%d] is too small to fit all cpus\n", + NR_CPUS); + return; + } + + pxm = pa->proximity_domain; + node = setup_node(pxm); + if ( node == NUMA_NO_NODE || node >= MAX_NUMNODES ) + { + printk(XENLOG_WARNING "SRAT: Too many proximity domains %d\n", pxm); + bad_srat(); + return; + } + + mpidr = acpi_get_cpu_mpidr(pa->acpi_processor_uid); + if ( mpidr == MPIDR_INVALID ) + { + printk(XENLOG_WARNING + "SRAT: PXM %d with ACPI ID %d has no valid MPIDR in MADT\n", + pxm, pa->acpi_processor_uid); + bad_srat(); + return; + } + + node_set(node, numa_nodes_parsed); + cpus_in_srat++; + acpi_numa = 1; + printk(XENLOG_INFO "SRAT: PXM %d -> MPIDR 0x%lx -> Node %d\n", + pxm, mpidr, node); +} + void __init acpi_map_uid_to_mpidr(void) { int i; diff --git a/xen/drivers/acpi/numa.c b/xen/drivers/acpi/numa.c index 50bf9f8..ce22e88 100644 --- a/xen/drivers/acpi/numa.c +++ b/xen/drivers/acpi/numa.c @@ -25,9 +25,11 @@ #include <xen/init.h> #include <xen/types.h> #include <xen/errno.h> +#include <xen/mm.h> #include <xen/acpi.h> #include <xen/numa.h> #include <acpi/acmacros.h> +#include <asm/mm.h> #define ACPI_NUMA 0x80000000 #define _COMPONENT ACPI_NUMA @@ -105,6 +107,21 @@ void __init acpi_table_print_srat_entry(struct acpi_subtable_header * header) } #endif /* ACPI_DEBUG_OUTPUT */ break; + case ACPI_SRAT_TYPE_GICC_AFFINITY: +#ifdef ACPI_DEBUG_OUTPUT + { + struct acpi_srat_gicc_affinity *p = + (struct acpi_srat_gicc_affinity *)header; + ACPI_DEBUG_PRINT((ACPI_DB_INFO, + "SRAT Processor (acpi id[0x%04x]) in" + " proximity domain %d %s\n", + p->acpi_processor_uid, + p->proximity_domain, + (p->flags & ACPI_SRAT_GICC_ENABLED) ? + "enabled" : "disabled"); + } +#endif /* ACPI_DEBUG_OUTPUT */ + break; default: printk(KERN_WARNING PREFIX "Found unsupported SRAT entry (type = %#x)\n", @@ -185,6 +202,24 @@ int __init acpi_parse_srat(struct acpi_table_header *table) return 0; } +static int __init +acpi_parse_gicc_affinity(struct acpi_subtable_header *header, + const unsigned long end) +{ + const struct acpi_srat_gicc_affinity *processor_affinity + = (struct acpi_srat_gicc_affinity *)header; + + if (!processor_affinity) + return -EINVAL; + + acpi_table_print_srat_entry(header); + + /* let architecture-dependent part to do it */ + acpi_numa_gicc_affinity_init(processor_affinity); + + return 0; +} + int __init acpi_table_parse_srat(int id, acpi_madt_entry_handler handler, unsigned int max_entries) @@ -205,6 +240,8 @@ int __init acpi_numa_init(void) acpi_table_parse_srat(ACPI_SRAT_TYPE_MEMORY_AFFINITY, acpi_parse_memory_affinity, NR_NODE_MEMBLKS); + acpi_table_parse_srat(ACPI_SRAT_TYPE_GICC_AFFINITY, + acpi_parse_gicc_affinity, NR_CPUS); } /* SLIT: System Locality Information Table */ diff --git a/xen/drivers/acpi/osl.c b/xen/drivers/acpi/osl.c index 7199047..7046816 100644 --- a/xen/drivers/acpi/osl.c +++ b/xen/drivers/acpi/osl.c @@ -29,6 +29,7 @@ #include <xen/pfn.h> #include <xen/types.h> #include <xen/errno.h> +#include <xen/mm.h> #include <xen/acpi.h> #include <xen/numa.h> #include <acpi/acmacros.h> @@ -39,6 +40,7 @@ #include <xen/efi.h> #include <xen/vmap.h> #include <xen/kconfig.h> +#include <asm/mm.h> #define _COMPONENT ACPI_OS_SERVICES ACPI_MODULE_NAME("osl") diff --git a/xen/include/acpi/actbl1.h b/xen/include/acpi/actbl1.h index e199136..b84bfba 100644 --- a/xen/include/acpi/actbl1.h +++ b/xen/include/acpi/actbl1.h @@ -949,7 +949,8 @@ enum acpi_srat_type { ACPI_SRAT_TYPE_CPU_AFFINITY = 0, ACPI_SRAT_TYPE_MEMORY_AFFINITY = 1, ACPI_SRAT_TYPE_X2APIC_CPU_AFFINITY = 2, - ACPI_SRAT_TYPE_RESERVED = 3 /* 3 and greater are reserved */ + ACPI_SRAT_TYPE_GICC_AFFINITY = 3, + ACPI_SRAT_TYPE_RESERVED = 4 /* 4 and greater are reserved */ }; /* @@ -1007,6 +1008,20 @@ struct acpi_srat_x2apic_cpu_affinity { #define ACPI_SRAT_CPU_ENABLED (1) /* 00: Use affinity structure */ +/* 3: GICC Affinity (ACPI 5.1) */ + +struct acpi_srat_gicc_affinity { + struct acpi_subtable_header header; + u32 proximity_domain; + u32 acpi_processor_uid; + u32 flags; + u32 clock_domain; +}; + +/* Flags for struct acpi_srat_gicc_affinity */ + +#define ACPI_SRAT_GICC_ENABLED (1) /* 00: Use affinity structure */ + /* Reset to default packing */ #pragma pack() diff --git a/xen/include/xen/acpi.h b/xen/include/xen/acpi.h index 30ec0ee..67fe1bb 100644 --- a/xen/include/xen/acpi.h +++ b/xen/include/xen/acpi.h @@ -92,10 +92,49 @@ void acpi_table_print_srat_entry (struct acpi_subtable_header *srat); /* the following four functions are architecture-dependent */ void acpi_numa_slit_init (struct acpi_table_slit *slit); +#if defined(CONFIG_X86) void acpi_numa_processor_affinity_init(const struct acpi_srat_cpu_affinity *); void acpi_numa_x2apic_affinity_init(const struct acpi_srat_x2apic_cpu_affinity *); void acpi_numa_memory_affinity_init(const struct acpi_srat_mem_affinity *); void acpi_numa_arch_fixup(void); +static inline void +acpi_numa_gicc_affinity_init(const struct acpi_srat_gicc_affinity *pa) +{ + return; +} +#elif defined(CONFIG_ARM) +static inline void +acpi_numa_processor_affinity_init(const struct acpi_srat_cpu_affinity *cpu_aff) +{ + return; +} +static inline void +acpi_numa_x2apic_affinity_init(const struct acpi_srat_x2apic_cpu_affinity *x2apic) +{ + return; +} +#if defined(CONFIG_ACPI_NUMA) +void acpi_numa_gicc_affinity_init(const struct acpi_srat_gicc_affinity *pa); +void acpi_numa_memory_affinity_init(const struct acpi_srat_mem_affinity *); +void acpi_numa_arch_fixup(void); +#else +static inline void +acpi_numa_gicc_affinity_init(const struct acpi_srat_gicc_affinity *pa) +{ + return; +} +static inline void +acpi_numa_memory_affinity_init(const struct acpi_srat_mem_affinity *ma) +{ + return; +} +static inline void +acpi_numa_arch_fixup(void) +{ + return; +} +#endif /* CONFIG_ACPI_NUMA */ +#endif /* CONFIG_X86 */ #ifdef CONFIG_ACPI_HOTPLUG_CPU /* Arch dependent functions for cpu hotplug support */