Re: [PATCH 12/19] xen/dt: Move bootfdt functions to xen/bootfdt.h

[dmkhn@xxxxxxxxx]

On Mon, Jun 02, 2025 at 09:29:16AM +0200, Orzel, Michal wrote:
> 
> 
> On 31/05/2025 02:35, dmkhn@xxxxxxxxx wrote:
> > On Fri, May 30, 2025 at 02:02:20PM +0200, Alejandro Vallejo wrote:
> >> Part of an unpicking process to extract bootfdt contents independent of 
> >> bootinfo
> >> to a separate file for x86 to take.
> >>
> >> Move functions required for early FDT parsing from device_tree.h and arm's
> >> setup.h onto bootfdt.h
> >>
> >> Declaration motion only. Not a functional change.
> >>
> >> Signed-off-by: Alejandro Vallejo <agarciav@xxxxxxx>
> >> ---
> >>  xen/arch/arm/include/asm/setup.h |  6 ----
> >>  xen/include/xen/bootfdt.h        | 62 ++++++++++++++++++++++++++++++++
> >>  xen/include/xen/device_tree.h    | 34 +-----------------
> >>  3 files changed, 63 insertions(+), 39 deletions(-)
> >>
> >> diff --git a/xen/arch/arm/include/asm/setup.h 
> >> b/xen/arch/arm/include/asm/setup.h
> >> index 0f9e531a34..32308837a9 100644
> >> --- a/xen/arch/arm/include/asm/setup.h
> >> +++ b/xen/arch/arm/include/asm/setup.h
> >> @@ -55,12 +55,6 @@ void setup_mm(void);
> >>  extern uint32_t hyp_traps_vector[];
> >>  void init_traps(void);
> >>
> >> -void device_tree_get_reg(const __be32 **cell, uint32_t address_cells,
> >> -                         uint32_t size_cells, paddr_t *start, paddr_t 
> >> *size);
> >> -
> >> -u32 device_tree_get_u32(const void *fdt, int node,
> >> -                        const char *prop_name, u32 dflt);
> >> -
> >>  int handle_device(struct domain *d, struct dt_device_node *dev, 
> >> p2m_type_t p2mt,
> >>                    struct rangeset *iomem_ranges, struct rangeset 
> >> *irq_ranges);
> >>
> >> diff --git a/xen/include/xen/bootfdt.h b/xen/include/xen/bootfdt.h
> >> index fa65e8fcf4..079259c719 100644
> >> --- a/xen/include/xen/bootfdt.h
> >> +++ b/xen/include/xen/bootfdt.h
> >> @@ -2,6 +2,7 @@
> >>  #ifndef XEN_BOOTFDT_H
> >>  #define XEN_BOOTFDT_H
> >>
> >> +#include <xen/byteorder.h>
> >>  #include <xen/types.h>
> >>  #include <xen/kernel.h>
> >>  #include <xen/macros.h>
> >> @@ -16,8 +17,53 @@
> >>  #define NR_MEM_BANKS 256
> >>  #define NR_SHMEM_BANKS 32
> >>
> >> +/* Default #address and #size cells */
> >> +#define DT_ROOT_NODE_ADDR_CELLS_DEFAULT 2
> >> +#define DT_ROOT_NODE_SIZE_CELLS_DEFAULT 1
> >> +
> >>  #define MAX_MODULES 32 /* Current maximum useful modules */
> >>
> >> +#define DEVICE_TREE_MAX_DEPTH 16
> >> +
> >> +/* Helper to read a big number; size is in cells (not bytes) */
> >> +static inline u64 dt_read_number(const __be32 *cell, int size)
> >> +{
> >> +    u64 r = 0;
> >> +
> >> +    while ( size-- )
> >> +        r = (r << 32) | be32_to_cpu(*(cell++));
> >> +    return r;
> >> +}
> >> +
> >> +static inline u64 dt_next_cell(int s, const __be32 **cellp)
> >> +{
> >> +    const __be32 *p = *cellp;
> >> +
> >> +    *cellp = p + s;
> >> +    return dt_read_number(p, s);
> >> +}
> >> +
> >> +typedef int (*device_tree_node_func)(const void *fdt,
> >> +                                     int node, const char *name, int 
> >> depth,
> >> +                                     u32 address_cells, u32 size_cells,
> >> +                                     void *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 device_tree_for_each_node(const void *fdt, int node,
> >> +                              device_tree_node_func func,
> >> +                              void *data);
> >> +
> >>  typedef enum {
> >>      BOOTMOD_XEN,
> >>      BOOTMOD_FDT,
> >> @@ -246,4 +292,20 @@ static inline struct membanks 
> >> *membanks_xzalloc(unsigned int nr,
> >>      return banks;
> >>  }
> >>
> >> +/*
> >> + * Interpret the property `prop_name` of `node` as a u32.
> >> + *
> >> + * Returns the property value on success; otherwise returns `dflt`.
> >> + */
> >> +uint32_t device_tree_get_u32(const void *fdt, int node,
> >> +                             const char *prop_name, uint32_t dflt);
> >
> > Suggest using `dt_` prefix (or any other good prefix) for all functions
> > in this header for consistency: e.g. there's dt_read_number() but also
> > device_tree_get_u32().
> Not really. AFAIR device_tree_ prefix is for functions operating on flattened 
> DT
> (usually calling libfdt functions inside) and dt_ otherwise. Let's not mix 
> them.

I see, thanks for explanation.
I would add a brief comment in this header to distinguish such difference then.

> 
> ~Michal
>