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

[dmkhn@proton.me]

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@amd.com>
> ---
>  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
> 
>