Re: [Xen-devel] [RFC PATCH v1 15/21] ARM: NUMA: Extract MPIDR from MADT table

[Julien Grall <julien.grall@xxxxxxx>]

Hello Vijay,

On 09/02/17 15:57, vijay.kilari@xxxxxxxxx wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxx>
>
> Parse MADT table and extract MPIDR for all
> CPU IDs in MADT ACPI_MADT_TYPE_GENERIC_INTERRUPT entries
> and store in cpu_uid_to_hwid[].
>
> This mapping is used by SRAT table parsing to
> extract MPIDR of the CPU ID.
>
> Signed-off-by: Vijaya Kumar <Vijaya.Kumar@xxxxxxxxxx>
> ---
>  xen/arch/arm/Makefile      |   1 +
>  xen/arch/arm/acpi_numa.c   | 122 +++++++++++++++++++++++++++++++++++++++++++++
>  xen/arch/arm/numa.c        |   1 +
This new file should go in xen/arch/arm/acpi/

>  xen/include/asm-arm/acpi.h |   2 +
>  4 files changed, 126 insertions(+)
>
> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> index 7694485..8c5e67b 100644
> --- a/xen/arch/arm/Makefile
> +++ b/xen/arch/arm/Makefile
> @@ -51,6 +51,7 @@ obj-y += vpsci.o
>  obj-y += vuart.o
>  obj-$(CONFIG_NUMA) += numa.o
>  obj-$(CONFIG_NUMA) += dt_numa.o
> +obj-$(CONFIG_ACPI_NUMA) += acpi_numa.o
>
>  #obj-bin-y += ....o
>
> diff --git a/xen/arch/arm/acpi_numa.c b/xen/arch/arm/acpi_numa.c
> new file mode 100644
> index 0000000..3ee87f2
> --- /dev/null
> +++ b/xen/arch/arm/acpi_numa.c
> @@ -0,0 +1,122 @@
> +/*
> + * ACPI based NUMA setup
> + *
> + * Copyright (C) 2016 - Cavium Inc.
> + * Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxx>
> + *
> + * Reads the ACPI MADT and SRAT table to setup NUMA information.
> + *
> + * Contains Excerpts from x86 implementation
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
Xen is GPLv2, please update the license accordingly.

> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <xen/init.h>
> +#include <xen/mm.h>
> +#include <xen/inttypes.h>
> +#include <xen/nodemask.h>
> +#include <xen/acpi.h>
> +#include <xen/numa.h>
> +#include <xen/pfn.h>
> +#include <xen/srat.h>
> +#include <asm/page.h>
> +#include <asm/acpi.h>
> +
> +extern nodemask_t numa_nodes_parsed;
> +struct uid_to_mpidr {
> +    u32 uid;
> +    u64 mpidr;
> +};
> +
> +/* Holds mapping of CPU id to MPIDR read from MADT */
> +static struct uid_to_mpidr cpu_uid_to_hwid[NR_CPUS] __read_mostly;
> +
> +static __init void bad_srat(void)
> +{
> +    int i;
> +
> +    printk(KERN_ERR "SRAT: SRAT not used.\n");
> +    acpi_numa = -1;
> +    for (i = 0; i < ARRAY_SIZE(pxm2node); i++)
> +        pxm2node[i].node = NUMA_NO_NODE;
> +}
> +
> +static u64 acpi_get_cpu_mpidr(int uid)
> +{
> +    int i;
> +
> +    if ( uid < ARRAY_SIZE(cpu_uid_to_hwid) && cpu_uid_to_hwid[uid].uid == uid 
> &&
> +         cpu_uid_to_hwid[uid].mpidr != MPIDR_INVALID )
> +        return cpu_uid_to_hwid[uid].mpidr;
Please don't make a special case. This makes more complicate to read the 
code.
We should just loop to find the entry matching the UID.

> +
> +    for ( i = 0; i < NR_CPUS; i++ )
You can limit the loop by keeping an the number of element in the array.

> +    {
> +        if ( cpu_uid_to_hwid[i].uid == uid )
> +            return cpu_uid_to_hwid[i].mpidr;
> +    }
> +
> +    return MPIDR_INVALID;
> +}
> +
> +static void __init
> +acpi_map_cpu_to_mpidr(struct acpi_madt_generic_interrupt *processor)
> +{
> +    static int cpus = 0;
> +
> +    u64 mpidr = processor->arm_mpidr & MPIDR_HWID_MASK;
> +
> +    if ( mpidr == MPIDR_INVALID )
> +    {
> +        printk("Skip MADT cpu entry with invalid MPIDR\n");
> +        bad_srat();
> +        return;
> +    }
> +
> +    cpu_uid_to_hwid[cpus].mpidr = mpidr;
> +    cpu_uid_to_hwid[cpus].uid = processor->uid;
> +
> +    cpus++;
> +}
> +
> +static int __init acpi_parse_madt_handler(struct acpi_subtable_header *header,
> +                                          const unsigned long end)
> +{
> +    struct acpi_madt_generic_interrupt *p =
> +               container_of(header, struct acpi_madt_generic_interrupt, 
> header);
> +
> +    if ( BAD_MADT_ENTRY(p, end) )
> +    {
> +        /* Though MADT is invalid, we disable NUMA by calling bad_srat() */
> +        bad_srat();
> +        return -EINVAL;
> +    }
> +
> +    acpi_table_print_madt_entry(header);
> +    acpi_map_cpu_to_mpidr(p);
> +
> +    return 0;
> +}
Why do you need to parse the MADT again? Can't this be done in the 
parsing made in acpi/boot.c?
> +
> +void __init acpi_map_uid_to_mpidr(void)
> +{
> +    int i;
unsigned int.

> +
> +    for ( i  = 0; i < NR_CPUS; i++ )
> +    {
> +        cpu_uid_to_hwid[i].mpidr = MPIDR_INVALID;
> +        cpu_uid_to_hwid[i].uid = -1;
> +    }
> +
> +    acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_INTERRUPT,
> +                    acpi_parse_madt_handler, 0);
> +}
> +
> +void __init acpi_numa_arch_fixup(void) {}
Missing emacs magic.

> diff --git a/xen/arch/arm/numa.c b/xen/arch/arm/numa.c
> index 31dc552..5c49347 100644
> --- a/xen/arch/arm/numa.c
> +++ b/xen/arch/arm/numa.c
> @@ -20,6 +20,7 @@
>  #include <xen/mm.h>
>  #include <xen/nodemask.h>
>  #include <xen/pfn.h>
> +#include <xen/acpi.h>
Why this include? This patch should compile without it.

>  #include <asm/mm.h>
>  #include <xen/numa.h>
>  #include <asm/acpi.h>
> diff --git a/xen/include/asm-arm/acpi.h b/xen/include/asm-arm/acpi.h
> index 9f954d3..b1f36f4 100644
> --- a/xen/include/asm-arm/acpi.h
> +++ b/xen/include/asm-arm/acpi.h
> @@ -68,6 +68,8 @@ static inline void enable_acpi(void)
>  {
>      acpi_disabled = 0;
>  }
> +
> +void acpi_map_uid_to_mpidr(void);
>  #else
>  #define acpi_disabled (1)
>  #define disable_acpi()
Regards,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel