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>
---
v2:
* Remove the u32 identifiers in the device_tree_get_u32() implementation
* Fix an existing const-stripping case.
---
xen/arch/arm/include/asm/setup.h | 6 ----
xen/common/device-tree/bootfdt.c | 8 ++---
xen/include/xen/bootfdt.h | 62 ++++++++++++++++++++++++++++++++
xen/include/xen/device_tree.h | 34 +-----------------
4 files changed, 67 insertions(+), 43 deletions(-)
diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h
index 6cf272c160..2b58549c1a 100644
--- a/xen/arch/arm/include/asm/setup.h
+++ b/xen/arch/arm/include/asm/setup.h
@@ -53,12 +53,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/common/device-tree/bootfdt.c b/xen/common/device-tree/bootfdt.c
index 9daea06e57..ab449db8d6 100644
--- a/xen/common/device-tree/bootfdt.c
+++ b/xen/common/device-tree/bootfdt.c
@@ -202,16 +202,16 @@ 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)
+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) )
+ if ( !prop || prop->len < sizeof(uint32_t) )
return dflt;
- return fdt32_to_cpu(*(uint32_t*)prop->data);
+ return fdt32_to_cpu(*(const uint32_t*)prop->data);
}
/**
diff --git a/xen/include/xen/bootfdt.h b/xen/include/xen/bootfdt.h
index 19d2ff0f0c..1b89986e10 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,
@@ -261,4 +307,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);
+
+/*
+ * 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 6dc1fb5159..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>";
--
2.43.0On 05/06/2025 21:48, 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> > --- > v2: > * Remove the u32 identifiers in the device_tree_get_u32() implementation I don't understand the reasoning behind changing u32->uint32_t only for one function in this patch while leaving others unmodified. Also what about u64? Either don't change any or change all. > * Fix an existing const-stripping case. These points should be mentioned in the commit msg. ~Michal
On Fri Jun 6, 2025 at 10:59 AM CEST, Michal Orzel wrote: > > > On 05/06/2025 21:48, 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> >> --- >> v2: >> * Remove the u32 identifiers in the device_tree_get_u32() implementation > I don't understand the reasoning behind changing u32->uint32_t only for one > function in this patch while leaving others unmodified. Also what about u64? > Either don't change any or change all. Sure. Let's call the original u32->uint32_t change a misplaced mutation and move on. The point is the motion, not these cleanups on top. > >> * Fix an existing const-stripping case. > These points should be mentioned in the commit msg. Sure. > > ~Michal Cheers, Alejandro
On Fri, 6 Jun 2025, Alejandro Vallejo wrote: > On Fri Jun 6, 2025 at 10:59 AM CEST, Michal Orzel wrote: > > > > > > On 05/06/2025 21:48, 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> > >> --- > >> v2: > >> * Remove the u32 identifiers in the device_tree_get_u32() implementation > > I don't understand the reasoning behind changing u32->uint32_t only for one > > function in this patch while leaving others unmodified. Also what about u64? > > Either don't change any or change all. > > Sure. Let's call the original u32->uint32_t change a misplaced mutation and > move on. The point is the motion, not these cleanups on top. Yes I agree. I know from past experience that Jan doesn't mind changes during code movements, but for me it is important that changes and code movement are separate. That is because I have almost automatic ways to check that code movement is correct if there are no changes. It saves me a lot of time during review. Then I can look at the individual changes separately.
On Fri Jun 6, 2025 at 9:59 PM CEST, Stefano Stabellini wrote: > On Fri, 6 Jun 2025, Alejandro Vallejo wrote: >> On Fri Jun 6, 2025 at 10:59 AM CEST, Michal Orzel wrote: >> > >> > >> > On 05/06/2025 21:48, 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> >> >> --- >> >> v2: >> >> * Remove the u32 identifiers in the device_tree_get_u32() implementation >> > I don't understand the reasoning behind changing u32->uint32_t only for one >> > function in this patch while leaving others unmodified. Also what about u64? >> > Either don't change any or change all. >> >> Sure. Let's call the original u32->uint32_t change a misplaced mutation and >> move on. The point is the motion, not these cleanups on top. > > Yes I agree. I know from past experience that Jan doesn't mind changes > during code movements, but for me it is important that changes and code > movement are separate. That is because I have almost automatic ways to > check that code movement is correct if there are no changes. It saves me > a lot of time during review. Then I can look at the individual changes > separately. That's interesting. Could you please share the runes? That's one side of review I still struggle with. Cheers, Alejandro
On Mon, 9 Jun 2025, Alejandro Vallejo wrote: > On Fri Jun 6, 2025 at 9:59 PM CEST, Stefano Stabellini wrote: > > On Fri, 6 Jun 2025, Alejandro Vallejo wrote: > >> On Fri Jun 6, 2025 at 10:59 AM CEST, Michal Orzel wrote: > >> > > >> > > >> > On 05/06/2025 21:48, 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> > >> >> --- > >> >> v2: > >> >> * Remove the u32 identifiers in the device_tree_get_u32() implementation > >> > I don't understand the reasoning behind changing u32->uint32_t only for one > >> > function in this patch while leaving others unmodified. Also what about u64? > >> > Either don't change any or change all. > >> > >> Sure. Let's call the original u32->uint32_t change a misplaced mutation and > >> move on. The point is the motion, not these cleanups on top. > > > > Yes I agree. I know from past experience that Jan doesn't mind changes > > during code movements, but for me it is important that changes and code > > movement are separate. That is because I have almost automatic ways to > > check that code movement is correct if there are no changes. It saves me > > a lot of time during review. Then I can look at the individual changes > > separately. > > That's interesting. Could you please share the runes? That's one side of > review I still struggle with. I use vim to generate two source files, one with the code corresponding to the - lines (with the - prefix) and one with the code corresponding to the + lines (without the + prefix). Then I diff the two files. This method is a bit crude but very effective and managed to spot issues this way more than once.
© 2016 - 2025 Red Hat, Inc.