RE: [XEN RFC PATCH 23/40] xen/arm: introduce a helper to parse device tree memory node

[Wei Chen <Wei.Chen@xxxxxxx>]

Hi Julien,

> -----Original Message-----
> From: Julien Grall <julien@xxxxxxx>
> Sent: 2021年8月25日 21:49
> To: Wei Chen <Wei.Chen@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx;
> sstabellini@xxxxxxxxxx; jbeulich@xxxxxxxx
> Cc: Bertrand Marquis <Bertrand.Marquis@xxxxxxx>
> Subject: Re: [XEN RFC PATCH 23/40] xen/arm: introduce a helper to parse
> device tree memory node
> 
> Hi Wei,
> 
> On 11/08/2021 11:24, Wei Chen wrote:
> > Memory blocks' NUMA ID information is stored in device tree's
> > memory nodes as "numa-node-id". We need a new helper to parse
> > and verify this ID from memory nodes.
> >
> > In order to support memory affinity in later use, the valid
> > memory ranges and NUMA ID will be saved to tables.
> >
> > Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> > ---
> >   xen/arch/arm/numa_device_tree.c | 130 ++++++++++++++++++++++++++++++++
> >   1 file changed, 130 insertions(+)
> >
> > diff --git a/xen/arch/arm/numa_device_tree.c
> b/xen/arch/arm/numa_device_tree.c
> > index 37cc56acf3..bbe081dcd1 100644
> > --- a/xen/arch/arm/numa_device_tree.c
> > +++ b/xen/arch/arm/numa_device_tree.c
> > @@ -20,11 +20,13 @@
> >   #include <xen/init.h>
> >   #include <xen/nodemask.h>
> >   #include <xen/numa.h>
> > +#include <xen/libfdt/libfdt.h>
> >   #include <xen/device_tree.h>
> >   #include <asm/setup.h>
> >
> >   s8 device_tree_numa = 0;
> >   static nodemask_t processor_nodes_parsed __initdata;
> > +static nodemask_t memory_nodes_parsed __initdata;
> >
> >   static int srat_disabled(void)
> >   {
> > @@ -55,6 +57,79 @@ static int __init
> dtb_numa_processor_affinity_init(nodeid_t node)
> >       return 0;
> >   }
> >
> > +/* Callback for parsing of the memory regions affinity */
> > +static int __init dtb_numa_memory_affinity_init(nodeid_t node,
> > +                                paddr_t start, paddr_t size)
> > +{
> 
> The implementation of this function is quite similar ot the ACPI
> version. Can this be abstracted?

In my draft, I had tried to merge ACPI and DTB versions in one
function. I introduced a number of "if else" to distinguish ACPI
from DTB, especially ACPI hotplug. The function seems very messy.
Not enough benefits to make up for the mess, so I gave up.

But, yes, maybe we can abstract some common code to functions, that
can be reused in two functions. I would try it in next version.


> 
> > +    struct node *nd;
> > +    paddr_t end;
> > +    int i;
> > +
> > +    if ( srat_disabled() )
> > +        return -EINVAL;
> > +
> > +    end = start + size;
> > +    if ( num_node_memblks >= NR_NODE_MEMBLKS )
> > +    {
> > +        dprintk(XENLOG_WARNING,
> > +                "Too many numa entry, try bigger NR_NODE_MEMBLKS \n");
> > +        bad_srat();
> > +        return -EINVAL;
> > +    }
> > +
> > +    /* It is fine to add this area to the nodes data it will be used
> later */
> > +    i = conflicting_memblks(start, end);
> > +    /* No conflicting memory block, we can save it for later usage */;
> > +    if ( i < 0 )
> > +        goto save_memblk;
> > +
> > +    if ( memblk_nodeid[i] == node ) {
> 
> Xen coding style is using:
> 
> if ( ... )
> {
> 
> Note that I may not comment on all the occurents, so please check the
> other places.
> 

Ok.


> > +        /*
> > +         * Overlaps with other memblk in the same node, warning here.
> > +         * This memblk will be merged with conflicted memblk later.
> > +         */
> > +        printk(XENLOG_WARNING
> > +               "DT: NUMA NODE %u (%"PRIx64
> > +               "-%"PRIx64") overlaps with itself (%"PRIx64"-
> %"PRIx64")\n",
> > +               node, start, end,
> > +               node_memblk_range[i].start, node_memblk_range[i].end);
> > +    } else {
> > +        /*
> > +         * Conflict with memblk in other node, this is an error.
> > +         * The NUMA information is invalid, NUMA will be turn off.
> > +         */
> > +        printk(XENLOG_ERR
> > +               "DT: NUMA NODE %u (%"PRIx64"-%"
> > +               PRIx64") overlaps with NODE %u (%"PRIx64"-%"PRIx64")\n",
> > +               node, start, end, memblk_nodeid[i],
> > +               node_memblk_range[i].start, node_memblk_range[i].end);
> > +        bad_srat();
> > +        return -EINVAL;
> > +    }
> > +
> > +save_memblk:
> > +    nd = &nodes[node];
> > +    if ( !node_test_and_set(node, memory_nodes_parsed) ) {
> > +        nd->start = start;
> > +        nd->end = end;
> > +    } else {
> > +        if ( start < nd->start )
> > +            nd->start = start;
> > +        if ( nd->end < end )
> > +            nd->end = end;
> > +    }
> > +
> > +    printk(XENLOG_INFO "DT: NUMA node %u %"PRIx64"-%"PRIx64"\n",
> > +           node, start, end);
> > +
> > +    node_memblk_range[num_node_memblks].start = start;
> > +    node_memblk_range[num_node_memblks].end = end;
> > +    memblk_nodeid[num_node_memblks] = node;
> > +    num_node_memblks++;
> > +
> > +    return 0;
> > +}
> > +
> >   /* Parse CPU NUMA node info */
> >   int __init device_tree_parse_numa_cpu_node(const void *fdt, int node)
> >   {
> > @@ -70,3 +145,58 @@ int __init device_tree_parse_numa_cpu_node(const
> void *fdt, int node)
> >
> >       return dtb_numa_processor_affinity_init(nid);
> >   }
> > +
> > +/* Parse memory node NUMA info */
> > +int __init
> > +device_tree_parse_numa_memory_node(const void *fdt, int node,
> > +    const char *name, uint32_t addr_cells, uint32_t size_cells)
> 
> This is pretty much a copy of process_memory_node(). Can we consider to
> collect the NUMA ID from there? If not, can we at least abstract the code?
> 

I had tried to parse NUMA id in process_memory_node or in early_scan_node.
But I found, this change would make NUMA init code in different places.
And I prefer a unify NUMA init entry. Because it may be good for
maintenance. When we want to disable NUMA, we just need to disable it
in one place. But you're right, we still can do some code abstraction.
I will do it in next version.

> > +{
> > +    uint32_t nid;
> > +    int ret = 0, len;
> > +    paddr_t addr, size;
> > +    const struct fdt_property *prop;
> > +    uint32_t idx, ranges;
> > +    const __be32 *addresses;
> > +
> > +    nid = device_tree_get_u32(fdt, node, "numa-node-id", MAX_NUMNODES);
> > +    if ( nid >= MAX_NUMNODES )
> > +    {
> > +        printk(XENLOG_WARNING "Node id %u exceeds maximum value\n",
> nid);
> > +        return -EINVAL;
> > +    }
> > +
> > +    prop = fdt_get_property(fdt, node, "reg", &len);
> > +    if ( !prop )
> > +    {
> > +        printk(XENLOG_WARNING
> > +               "fdt: node `%s': missing `reg' property\n", name);
> > +        return -EINVAL;
> > +    }
> > +
> > +    addresses = (const __be32 *)prop->data;
> > +    ranges = len / (sizeof(__be32)* (addr_cells + size_cells));
> > +    for ( idx = 0; idx < ranges; idx++ )
> > +    {
> > +        device_tree_get_reg(&addresses, addr_cells, size_cells, &addr,
> &size);
> > +        /* Skip zero size ranges */
> > +        if ( !size )
> > +            continue;
> > +
> > +        ret = dtb_numa_memory_affinity_init(nid, addr, size);
> > +        if ( ret ) {
> > +            printk(XENLOG_WARNING
> > +                   "NUMA: process range#%d addr = %lx size=%lx
> failed!\n",
> 
> s/%d/%u/ as idx is an unsigned int
> s/%lx/%"PRI_paddr"/ as addr and size are paddr_t.
> 

OK

> > +                   idx, addr, size);
> > +            return -EINVAL;
> > +        }
> > +    }
> > +
> > +    if ( idx == 0 )
> > +    {
> > +        printk(XENLOG_ERR
> > +               "bad property in memory node, idx=%d ret=%d\n", idx,
> ret);
> > +        return -EINVAL;
> > +    }
> > +
> > +    return 0;
> > +}
> >
> 
> Cheers,
> 
> --
> Julien Grall