Re: [PATCH 15/19] xen/dt: Move bootinfo-independent helpers out of bootinfo-fdt.c

[Alejandro Vallejo <agarciav@xxxxxxx>]

On Sat May 31, 2025 at 2:39 AM CEST, dmkhn wrote:
> On Fri, May 30, 2025 at 02:02:23PM +0200, Alejandro Vallejo wrote:
>> ... back into bootfdt.c
>> 
>> These will be required by x86 later on to detect modules in the early scan of
>> the FDT. They are independent of bootinfo, so it's cleaner for them to exist 
>> in
>> a separate file.
>> 
>> Not a functional change.
>> 
>> Signed-off-by: Alejandro Vallejo <agarciav@xxxxxxx>
>> ---
>>  xen/common/device-tree/Makefile       |   1 +
>>  xen/common/device-tree/bootfdt.c      |  97 ++++++++++++++++++++++++
>>  xen/common/device-tree/bootinfo-fdt.c | 104 --------------------------
>>  3 files changed, 98 insertions(+), 104 deletions(-)
>>  create mode 100644 xen/common/device-tree/bootfdt.c
>> 
>> diff --git a/xen/common/device-tree/Makefile 
>> b/xen/common/device-tree/Makefile
>> index bb6d5ddec5..922c5bba9b 100644
>> --- a/xen/common/device-tree/Makefile
>> +++ b/xen/common/device-tree/Makefile
>> @@ -1,3 +1,4 @@
>> +obj-y += bootfdt.init.o
>>  obj-y += bootinfo-fdt.init.o
>>  obj-y += bootinfo.init.o
>>  obj-y += device-tree.o
>> diff --git a/xen/common/device-tree/bootfdt.c 
>> b/xen/common/device-tree/bootfdt.c
>> new file mode 100644
>> index 0000000000..5decf17faf
>> --- /dev/null
>> +++ b/xen/common/device-tree/bootfdt.c
>> @@ -0,0 +1,97 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +#include <xen/bootfdt.h>
>> +#include <xen/bug.h>
>> +#include <xen/lib.h>
>> +#include <xen/libfdt/libfdt.h>
>> +
>> +uint32_t __init device_tree_get_u32(const void *fdt, int node,
>> +                                    const char *prop_name, uint32_t dflt)
>> +{
>> +    const struct fdt_property *prop;
>> +
>> +    prop = fdt_get_property(fdt, node, prop_name, NULL);
>> +    if ( !prop || prop->len < sizeof(u32) )
>> +        return dflt;
>> +
>> +    return fdt32_to_cpu(*(uint32_t*)prop->data);
>> +}
>> +
>> +int __init device_tree_for_each_node(const void *fdt, int node,
>> +                                     device_tree_node_func func,
>> +                                     void *data)
>> +{
>> +    /*
>> +     * We only care about relative depth increments, assume depth of
>> +     * node is 0 for simplicity.
>> +     */
>> +    int depth = 0;
>> +    const int first_node = node;
>> +    u32 address_cells[DEVICE_TREE_MAX_DEPTH];
>> +    u32 size_cells[DEVICE_TREE_MAX_DEPTH];
>> +    int ret;
>> +
>> +    do {
>> +        const char *name = fdt_get_name(fdt, node, NULL);
>> +        u32 as, ss;
>> +
>> +        if ( depth >= DEVICE_TREE_MAX_DEPTH )
>> +        {
>> +            printk("Warning: device tree node `%s' is nested too deep\n",
>> +                   name);
>
> Use XENLOG_WARNING?
>
>> +            continue;
>> +        }
>> +
>> +        as = depth > 0 ? address_cells[depth-1] : 
>> DT_ROOT_NODE_ADDR_CELLS_DEFAULT;
>> +        ss = depth > 0 ? size_cells[depth-1] : 
>> DT_ROOT_NODE_SIZE_CELLS_DEFAULT;
>> +
>> +        address_cells[depth] = device_tree_get_u32(fdt, node,
>> +                                                   "#address-cells", as);
>> +        size_cells[depth] = device_tree_get_u32(fdt, node,
>> +                                                "#size-cells", ss);
>> +
>> +        /* skip the first node */
>> +        if ( node != first_node )
>> +        {
>> +            ret = func(fdt, node, name, depth, as, ss, data);
>> +            if ( ret != 0 )
>> +                return ret;
>> +        }
>> +
>> +        node = fdt_next_node(fdt, node, &depth);
>> +    } while ( node >= 0 && depth > 0 );
>> +
>> +    return 0;
>> +}
>> +
>> +void __init device_tree_get_reg(const __be32 **cell, uint32_t address_cells,
>> +                                uint32_t size_cells, paddr_t *start,
>> +                                paddr_t *size)
>> +{
>> +    uint64_t dt_start, dt_size;
>> +
>> +    /*
>> +     * dt_next_cell will return uint64_t whereas paddr_t may not be 64-bit.
>> +     * Thus, there is an implicit cast from uint64_t to paddr_t.
>> +     */
>> +    dt_start = dt_next_cell(address_cells, cell);
>> +    dt_size = dt_next_cell(size_cells, cell);
>> +
>> +    if ( dt_start != (paddr_t)dt_start )
>> +    {
>> +        printk("Physical address greater than max width supported\n");
>
> I would add current and expected dt_start values to the printout.
>
>> +        WARN();
>> +    }
>> +
>> +    if ( dt_size != (paddr_t)dt_size )
>> +    {
>> +        printk("Physical size greater than max width supported\n");
>> +        WARN();
>> +    }
>> +
>> +    /*
>> +     * Xen will truncate the address/size if it is greater than the maximum
>> +     * supported width and it will give an appropriate warning.
>> +     */
>> +    *start = dt_start;
>> +    *size = dt_size;
>> +}
>> diff --git a/xen/common/device-tree/bootinfo-fdt.c 
>> b/xen/common/device-tree/bootinfo-fdt.c
>> index bb5f45771e..736f877616 100644
>> --- a/xen/common/device-tree/bootinfo-fdt.c
>> +++ b/xen/common/device-tree/bootinfo-fdt.c
>> @@ -112,39 +112,6 @@ static bool __init device_tree_is_memory_node(const 
>> void *fdt, int node,
>>      return true;
>>  }
>> 
>> -void __init device_tree_get_reg(const __be32 **cell, uint32_t address_cells,
>> -                                uint32_t size_cells, paddr_t *start,
>> -                                paddr_t *size)
>> -{
>> -    uint64_t dt_start, dt_size;
>> -
>> -    /*
>> -     * dt_next_cell will return uint64_t whereas paddr_t may not be 64-bit.
>> -     * Thus, there is an implicit cast from uint64_t to paddr_t.
>> -     */
>> -    dt_start = dt_next_cell(address_cells, cell);
>> -    dt_size = dt_next_cell(size_cells, cell);
>> -
>> -    if ( dt_start != (paddr_t)dt_start )
>> -    {
>> -        printk("Physical address greater than max width supported\n");
>> -        WARN();
>> -    }
>> -
>> -    if ( dt_size != (paddr_t)dt_size )
>> -    {
>> -        printk("Physical size greater than max width supported\n");
>> -        WARN();
>> -    }
>> -
>> -    /*
>> -     * Xen will truncate the address/size if it is greater than the maximum
>> -     * supported width and it will give an appropriate warning.
>> -     */
>> -    *start = dt_start;
>> -    *size = dt_size;
>> -}
>> -
>>  static int __init device_tree_get_meminfo(const void *fdt, int node,
>>                                            const char *prop_name,
>>                                            u32 address_cells, u32 size_cells,
>> @@ -205,77 +172,6 @@ static int __init device_tree_get_meminfo(const void 
>> *fdt, int node,
>>      return 0;
>>  }
>> 
>> -u32 __init device_tree_get_u32(const void *fdt, int node,
>> -                               const char *prop_name, u32 dflt)
>> -{
>> -    const struct fdt_property *prop;
>> -
>> -    prop = fdt_get_property(fdt, node, prop_name, NULL);
>> -    if ( !prop || prop->len < sizeof(u32) )
>> -        return dflt;
>> -
>> -    return fdt32_to_cpu(*(uint32_t*)prop->data);
>> -}
>> -
>> -/**
>> - * device_tree_for_each_node - iterate over all device tree sub-nodes
>> - * @fdt: flat device tree.
>> - * @node: parent node to start the search from
>> - * @func: function to call for each sub-node.
>> - * @data: data to pass to @func.
>> - *
>> - * Any nodes nested at DEVICE_TREE_MAX_DEPTH or deeper are ignored.
>> - *
>> - * Returns 0 if all nodes were iterated over successfully.  If @func
>> - * returns a value different from 0, that value is returned immediately.
>> - */
>> -int __init device_tree_for_each_node(const void *fdt, int node,
>> -                                     device_tree_node_func func,
>> -                                     void *data)
>> -{
>> -    /*
>> -     * We only care about relative depth increments, assume depth of
>> -     * node is 0 for simplicity.
>> -     */
>> -    int depth = 0;
>> -    const int first_node = node;
>> -    u32 address_cells[DEVICE_TREE_MAX_DEPTH];
>> -    u32 size_cells[DEVICE_TREE_MAX_DEPTH];
>> -    int ret;
>> -
>> -    do {
>> -        const char *name = fdt_get_name(fdt, node, NULL);
>> -        u32 as, ss;
>> -
>> -        if ( depth >= DEVICE_TREE_MAX_DEPTH )
>> -        {
>> -            printk("Warning: device tree node `%s' is nested too deep\n",
>> -                   name);
>> -            continue;
>> -        }
>> -
>> -        as = depth > 0 ? address_cells[depth-1] : 
>> DT_ROOT_NODE_ADDR_CELLS_DEFAULT;
>> -        ss = depth > 0 ? size_cells[depth-1] : 
>> DT_ROOT_NODE_SIZE_CELLS_DEFAULT;
>> -
>> -        address_cells[depth] = device_tree_get_u32(fdt, node,
>> -                                                   "#address-cells", as);
>> -        size_cells[depth] = device_tree_get_u32(fdt, node,
>> -                                                "#size-cells", ss);
>> -
>> -        /* skip the first node */
>> -        if ( node != first_node )
>> -        {
>> -            ret = func(fdt, node, name, depth, as, ss, data);
>> -            if ( ret != 0 )
>> -                return ret;
>> -        }
>> -
>> -        node = fdt_next_node(fdt, node, &depth);
>> -    } while ( node >= 0 && depth > 0 );
>> -
>> -    return 0;
>> -}
>> -
>>  static int __init process_memory_node(const void *fdt, int node,
>>                                        const char *name, int depth,
>>                                        u32 address_cells, u32 size_cells,
>> --
>> 2.43.0
>> 
>> 

As I answered to Daniel, as I'd rather not introduce functional changes in
code motion commits. I draw the line at trivial retyping (e.g: u32->uint32_t)

Otherwise it's just very confusing to future readers.

Cheers,
Alejandro