|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [RFC PATCH v1 16/21] ARM: NUMA: Extract proximity from SRAT table
On Thu, Mar 2, 2017 at 10:51 PM, Julien Grall <julien.grall@xxxxxxx> wrote:
> (+ Jan as ACPI maintainer)
>
> Hello Vijay,
>
> On 09/02/17 15:57, vijay.kilari@xxxxxxxxx wrote:
>>
>> From: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxx>
>>
>> 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@xxxxxxxxxx>
>> ---
>> 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
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |