Re: [PATCH v2 2/2] fdt: make fdt handling reusable across arch

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Daniel,

On 03/08/2023 12:44, Daniel P. Smith wrote:
> 
> 
> This refactors reusable code from Arm's bootfdt.c and device-tree.h that is
> general fdt handling code.  The Kconfig parameter CORE_DEVICE_TREE is
IIUC, you just try to untangle the code for fdt from dt (unflattened). 
CORE_DEVICE_TREE is
a bit ambiguous in my opinion, so maybe just CONFIG_FDT, especially since you 
use it to guard libfdt/?

> introduced for when the ability of parsing DTB files is needed by a capability
> such as hyperlaunch.
> 
> Signed-off-by: Daniel P. Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>
> ---
>  MAINTAINERS                      |   8 +-
>  xen/arch/arm/bootfdt.c           | 141 +---------------------------
>  xen/arch/arm/domain_build.c      |   1 +
>  xen/arch/arm/include/asm/setup.h |   6 --
>  xen/common/Kconfig               |   4 +
>  xen/common/Makefile              |   3 +-
>  xen/common/fdt.c                 | 153 +++++++++++++++++++++++++++++++
>  xen/include/xen/device_tree.h    |  50 +---------
>  xen/include/xen/fdt.h            |  79 ++++++++++++++++
>  9 files changed, 246 insertions(+), 199 deletions(-)
>  create mode 100644 xen/common/fdt.c
>  create mode 100644 xen/include/xen/fdt.h
> 
[...]

> diff --git a/xen/common/fdt.c b/xen/common/fdt.c
> new file mode 100644
> index 0000000000..8d7acaaa43
> --- /dev/null
> +++ b/xen/common/fdt.c
> @@ -0,0 +1,153 @@
> +/*
SPDX missing for a new file

> + * Flattened Device Tree
> + *
> + * Copyright (C) 2012-2014 Citrix Systems, Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +#include <xen/fdt.h>
> +#include <xen/init.h>
> +#include <xen/lib.h>
> +#include <xen/libfdt/libfdt.h>
> +#include <xen/types.h>
> +
> +bool __init device_tree_node_matches(
> +    const void *fdt, int node, const char *match)
FWICS, this code style (that you use for every function in this patch) is 
rather uncommon in Xen so maybe better to follow the generic style.
Also, this would avoid changing the style of functions/prototypes you move.

[...]

> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> index 1d79e23b28..82db38b140 100644
> --- a/xen/include/xen/device_tree.h
> +++ b/xen/include/xen/device_tree.h
> @@ -14,13 +14,12 @@
>  #include <asm/device.h>
>  #include <public/xen.h>
>  #include <public/device_tree_defs.h>
> +#include <xen/fdt.h>
>  #include <xen/kernel.h>
>  #include <xen/string.h>
>  #include <xen/types.h>
>  #include <xen/list.h>
> 
> -#define DEVICE_TREE_MAX_DEPTH 16
> -
>  /*
>   * Struct used for matching a device
>   */
> @@ -159,17 +158,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
>   *
> @@ -214,14 +204,6 @@ extern const struct dt_device_node 
> *dt_interrupt_controller;
>  struct dt_device_node *
>  dt_find_interrupt_controller(const struct dt_device_match *matches);
> 
> -#define dt_prop_cmp(s1, s2) strcmp((s1), (s2))
> -#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 )
> 
> @@ -231,16 +213,6 @@ dt_find_interrupt_controller(const struct 
> dt_device_match *matches);
>  #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)
Shouldn't this also go to fdt.h as it is just a wrapper for dt_read_number() 
you moved?

~Michal