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@amd.com>
---
v3:
  * Avoid mutations during code motion
---
 xen/include/xen/bootfdt.h     | 62 +++++++++++++++++++++++++++++++++++
 xen/include/xen/device_tree.h | 40 +---------------------
 2 files changed, 63 insertions(+), 39 deletions(-)

diff --git a/xen/include/xen/bootfdt.h b/xen/include/xen/bootfdt.h
index 8ea52290b7..b6ae7d6aa6 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,
@@ -260,4 +306,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`.
+ */
+u32 device_tree_get_u32(const void *fdt, int node,
+                        const char *prop_name, u32 dflt);
+
+/*
+ * Interpret the property `prop_name` of `node` as a "reg".
+ *
+ * Returns outputs in `start` and `size`.
+ */
+void device_tree_get_reg(const __be32 **cell, uint32_t address_cells,
+                         uint32_t size_cells, paddr_t *start, paddr_t *size);
+
 #endif /* XEN_BOOTFDT_H */
diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
index 75017e4266..0a22b1ba1d 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -10,6 +10,7 @@
 #ifndef __XEN_DEVICE_TREE_H__
 #define __XEN_DEVICE_TREE_H__
 
+#include <xen/bootfdt.h>
 #include <xen/byteorder.h>
 
 #include <asm/device.h>
@@ -22,8 +23,6 @@
 #include <xen/list.h>
 #include <xen/rwlock.h>
 
-#define DEVICE_TREE_MAX_DEPTH 16
-
 /*
  * Struct used for matching a device
  */
@@ -164,17 +163,8 @@ struct dt_raw_irq {
     u32 specifier[DT_MAX_IRQ_SPEC];
 };
 
-typedef int (*device_tree_node_func)(const void *fdt,
-                                     int node, const char *name, int depth,
-                                     u32 address_cells, u32 size_cells,
-                                     void *data);
-
 extern const void *device_tree_flattened;
 
-int device_tree_for_each_node(const void *fdt, int node,
-                              device_tree_node_func func,
-                              void *data);
-
 /**
  * dt_unflatten_host_device_tree - Unflatten the host device tree
  *
@@ -245,10 +235,6 @@ void intc_dt_preinit(void);
 #define dt_node_cmp(s1, s2) strcasecmp((s1), (s2))
 #define dt_compat_cmp(s1, s2) strcasecmp((s1), (s2))
 
-/* Default #address and #size cells */
-#define DT_ROOT_NODE_ADDR_CELLS_DEFAULT 2
-#define DT_ROOT_NODE_SIZE_CELLS_DEFAULT 1
-
 #define dt_for_each_property_node(dn, pp)                   \
     for ( pp = (dn)->properties; (pp) != NULL; pp = (pp)->next )
 
@@ -258,16 +244,6 @@ void intc_dt_preinit(void);
 #define dt_for_each_child_node(dt, dn)                      \
     for ( dn = (dt)->child; (dn) != NULL; dn = (dn)->sibling )
 
-/* 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;
-}
-
 /* Wrapper for dt_read_number() to return paddr_t (instead of uint64_t) */
 static inline paddr_t dt_read_paddr(const __be32 *cell, int size)
 {
@@ -307,14 +283,6 @@ static inline int dt_size_to_cells(int bytes)
     return (bytes / sizeof(u32));
 }
 
-static inline u64 dt_next_cell(int s, const __be32 **cellp)
-{
-    const __be32 *p = *cellp;
-
-    *cellp = p + s;
-    return dt_read_number(p, s);
-}
-
 static inline const char *dt_node_full_name(const struct dt_device_node *np)
 {
     return (np && np->full_name) ? np->full_name : "<no-node>";
@@ -949,12 +917,6 @@ int dt_get_pci_domain_nr(struct dt_device_node *node);
 
 struct dt_device_node *dt_find_node_by_phandle(dt_phandle handle);
 
-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);
-
 #ifdef CONFIG_DEVICE_TREE_DEBUG
 #define dt_dprintk(fmt, args...)  \
     printk(XENLOG_DEBUG fmt, ## args)
-- 
2.43.0

On Fri, 13 Jun 2025, 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@amd.com>
> ---
> v3:
>   * Avoid mutations during code motion
> ---
>  xen/include/xen/bootfdt.h     | 62 +++++++++++++++++++++++++++++++++++
>  xen/include/xen/device_tree.h | 40 +---------------------
>  2 files changed, 63 insertions(+), 39 deletions(-)
> 
> diff --git a/xen/include/xen/bootfdt.h b/xen/include/xen/bootfdt.h
> index 8ea52290b7..b6ae7d6aa6 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,
> @@ -260,4 +306,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`.
> + */
> +u32 device_tree_get_u32(const void *fdt, int node,
> +                        const char *prop_name, u32 dflt);
> +
> +/*
> + * Interpret the property `prop_name` of `node` as a "reg".
> + *
> + * Returns outputs in `start` and `size`.
> + */
> +void device_tree_get_reg(const __be32 **cell, uint32_t address_cells,
> +                         uint32_t size_cells, paddr_t *start, paddr_t *size);
> +
>  #endif /* XEN_BOOTFDT_H */
> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> index 75017e4266..0a22b1ba1d 100644
> --- a/xen/include/xen/device_tree.h
> +++ b/xen/include/xen/device_tree.h
> @@ -10,6 +10,7 @@
>  #ifndef __XEN_DEVICE_TREE_H__
>  #define __XEN_DEVICE_TREE_H__
>  
> +#include <xen/bootfdt.h>
>  #include <xen/byteorder.h>

This should not be needed?


>  #include <asm/device.h>
> @@ -22,8 +23,6 @@
>  #include <xen/list.h>
>  #include <xen/rwlock.h>
>  
> -#define DEVICE_TREE_MAX_DEPTH 16
> -
>  /*
>   * Struct used for matching a device
>   */
> @@ -164,17 +163,8 @@ struct dt_raw_irq {
>      u32 specifier[DT_MAX_IRQ_SPEC];
>  };
>  
> -typedef int (*device_tree_node_func)(const void *fdt,
> -                                     int node, const char *name, int depth,
> -                                     u32 address_cells, u32 size_cells,
> -                                     void *data);
> -
>  extern const void *device_tree_flattened;
>  
> -int device_tree_for_each_node(const void *fdt, int node,
> -                              device_tree_node_func func,
> -                              void *data);
> -
>  /**
>   * dt_unflatten_host_device_tree - Unflatten the host device tree
>   *
> @@ -245,10 +235,6 @@ void intc_dt_preinit(void);
>  #define dt_node_cmp(s1, s2) strcasecmp((s1), (s2))
>  #define dt_compat_cmp(s1, s2) strcasecmp((s1), (s2))
>  
> -/* Default #address and #size cells */
> -#define DT_ROOT_NODE_ADDR_CELLS_DEFAULT 2
> -#define DT_ROOT_NODE_SIZE_CELLS_DEFAULT 1
> -
>  #define dt_for_each_property_node(dn, pp)                   \
>      for ( pp = (dn)->properties; (pp) != NULL; pp = (pp)->next )
>  
> @@ -258,16 +244,6 @@ void intc_dt_preinit(void);
>  #define dt_for_each_child_node(dt, dn)                      \
>      for ( dn = (dt)->child; (dn) != NULL; dn = (dn)->sibling )
>  
> -/* 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;
> -}
> -
>  /* Wrapper for dt_read_number() to return paddr_t (instead of uint64_t) */
>  static inline paddr_t dt_read_paddr(const __be32 *cell, int size)
>  {
> @@ -307,14 +283,6 @@ static inline int dt_size_to_cells(int bytes)
>      return (bytes / sizeof(u32));
>  }
>  
> -static inline u64 dt_next_cell(int s, const __be32 **cellp)
> -{
> -    const __be32 *p = *cellp;
> -
> -    *cellp = p + s;
> -    return dt_read_number(p, s);
> -}
> -
>  static inline const char *dt_node_full_name(const struct dt_device_node *np)
>  {
>      return (np && np->full_name) ? np->full_name : "<no-node>";
> @@ -949,12 +917,6 @@ int dt_get_pci_domain_nr(struct dt_device_node *node);
>  
>  struct dt_device_node *dt_find_node_by_phandle(dt_phandle handle);
>  
> -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);
> -
>  #ifdef CONFIG_DEVICE_TREE_DEBUG
>  #define dt_dprintk(fmt, args...)  \
>      printk(XENLOG_DEBUG fmt, ## args)
> -- 
> 2.43.0
>

Hi,

On Sat Jun 14, 2025 at 3:16 AM CEST, Stefano Stabellini wrote:
> On Fri, 13 Jun 2025, Alejandro Vallejo wrote:
>> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
>> index 75017e4266..0a22b1ba1d 100644
>> --- a/xen/include/xen/device_tree.h
>> +++ b/xen/include/xen/device_tree.h
>> @@ -10,6 +10,7 @@
>>  #ifndef __XEN_DEVICE_TREE_H__
>>  #define __XEN_DEVICE_TREE_H__
>>  
>> +#include <xen/bootfdt.h>
>>  #include <xen/byteorder.h>
>
> This should not be needed?

dt_read_number() is still needed in dt_read_paddr() in device_tree.h

I can move that helper too (makes sense, as it's strictly a wrapper) and
add includes in whatever .c files require it (tentatively it looks like only
static-evtchn.c. Then that include can be dropped.

Cheers,
Alejandro

On Thu, 19 Jun 2025, Alejandro Vallejo wrote:
> Hi,
> 
> On Sat Jun 14, 2025 at 3:16 AM CEST, Stefano Stabellini wrote:
> > On Fri, 13 Jun 2025, Alejandro Vallejo wrote:
> >> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> >> index 75017e4266..0a22b1ba1d 100644
> >> --- a/xen/include/xen/device_tree.h
> >> +++ b/xen/include/xen/device_tree.h
> >> @@ -10,6 +10,7 @@
> >>  #ifndef __XEN_DEVICE_TREE_H__
> >>  #define __XEN_DEVICE_TREE_H__
> >>  
> >> +#include <xen/bootfdt.h>
> >>  #include <xen/byteorder.h>
> >
> > This should not be needed?
> 
> dt_read_number() is still needed in dt_read_paddr() in device_tree.h
> 
> I can move that helper too (makes sense, as it's strictly a wrapper) and
> add includes in whatever .c files require it (tentatively it looks like only
> static-evtchn.c. Then that include can be dropped.

Procede as you think is best. I thought it was unneeded by code
inspection, then I tried to remove it and Xen compiled okay. So I wrote
that comment. If it is needed it is also OK to keep it.

On Sat Jun 14, 2025 at 3:16 AM CEST, Stefano Stabellini wrote:
> On Fri, 13 Jun 2025, 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@amd.com>
>> ---
>> v3:
>>   * Avoid mutations during code motion
>> ---
>>  xen/include/xen/bootfdt.h     | 62 +++++++++++++++++++++++++++++++++++
>>  xen/include/xen/device_tree.h | 40 +---------------------
>>  2 files changed, 63 insertions(+), 39 deletions(-)
>> 
>> diff --git a/xen/include/xen/bootfdt.h b/xen/include/xen/bootfdt.h
>> index 8ea52290b7..b6ae7d6aa6 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,
>> @@ -260,4 +306,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`.
>> + */
>> +u32 device_tree_get_u32(const void *fdt, int node,
>> +                        const char *prop_name, u32 dflt);
>> +
>> +/*
>> + * Interpret the property `prop_name` of `node` as a "reg".
>> + *
>> + * Returns outputs in `start` and `size`.
>> + */
>> +void device_tree_get_reg(const __be32 **cell, uint32_t address_cells,
>> +                         uint32_t size_cells, paddr_t *start, paddr_t *size);
>> +
>>  #endif /* XEN_BOOTFDT_H */
>> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
>> index 75017e4266..0a22b1ba1d 100644
>> --- a/xen/include/xen/device_tree.h
>> +++ b/xen/include/xen/device_tree.h
>> @@ -10,6 +10,7 @@
>>  #ifndef __XEN_DEVICE_TREE_H__
>>  #define __XEN_DEVICE_TREE_H__
>>  
>> +#include <xen/bootfdt.h>
>>  #include <xen/byteorder.h>
>
> This should not be needed?

I wanted to avoid having to touch include sites. Let me check how many affected
places there are and fix up accordingly if any needs adjusting.

Cheers,
Alejandro