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

[Vijay Kilari <vijay.kilari@xxxxxxxxx>]

On Thu, Mar 2, 2017 at 9:58 PM, Julien Grall <julien.grall@xxxxxxx> wrote:
> 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/

shouldn't be in xen/arch/arm/numa/?
>
>
>>  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.
OK.
>
>
>> +    {
>> +        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?
I will check. But I see that this is done quite late in smp_init().

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