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

["Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>]

On 8/2/23 02:13, Henry Wang wrote:
> Hi Daniel,
Hey Henry!

> > -----Original Message-----
> > Subject: [PATCH 2/2] fdt: make fdt handling reusable across arch
> >
> > 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
> > 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                   |   2 +
> >   xen/arch/arm/bootfdt.c        | 141 +------------------------------
> >   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 ++++++++++++++++++
> >   7 files changed, 242 insertions(+), 190 deletions(-)
> >   create mode 100644 xen/common/fdt.c
> >   create mode 100644 xen/include/xen/fdt.h
> >
> > +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;
> > +}
> > +
> > +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);
> > +}
> When I tested this patch by our internal CI (I applied this patch on top
> of today's staging:
> c2026b88b5 xen/arm/IRQ: uniform irq_set_affinity() with x86 version),
> there are some build errors as below:
>
> aarch64-linux-gnu-gcc -MMD -MP -MF ./.asm-offsets.s.d -DBUILD_ID 
> -fno-strict-aliasing -std=gnu99 -Wall -Wstrict-prototypes 
> -Wdeclaration-after-statement -Wno-unused-but-set-variable 
> -Wno-unused-local-typedefs -O2 -fomit-frame-pointer -nostdinc -fno-builtin 
> -fno-common -Werror -Wredundant-decls -Wno-pointer-arith -Wvla -pipe -D__XEN__ 
> -include /jenkins/workspace/ais-xenbits-xen/layers/xen/xen/include/xen/config.h 
> -mcpu=generic -mgeneral-regs-only -mno-outline-atomics -I./include 
> -I./arch/arm/include 
> -I/jenkins/workspace/ais-xenbits-xen/layers/xen/xen/include 
> -I/jenkins/workspace/ais-xenbits-xen/layers/xen/xen/arch/arm/include -fno-pie 
> -fno-stack-protector -fno-exceptions -fno-asynchronous-unwind-tables 
> -Wnested-externs -S -g0 -o asm-offsets.s.new -MQ asm-offsets.s 
> /jenkins/workspace/ais-xenbits-xen/layers/xen/xen/arch/arm/arm64/asm-offsets.c
> In file included from 
> /jenkins/workspace/ais-xenbits-xen/layers/xen/xen/arch/arm/arm64/asm-offsets.c:13:
> /jenkins/workspace/ais-xenbits-xen/layers/xen/xen/arch/arm/include/asm/setup.h:162:6:
>  error: redundant redeclaration of 'device_tree_get_reg' 
> [-Werror=redundant-decls]
>    162 | void device_tree_get_reg(const __be32 **cell, uint32_t address_cells,
>        |      ^~~~~~~~~~~~~~~~~~~
> In file included from 
> /jenkins/workspace/ais-xenbits-xen/layers/xen/xen/include/xen/device_tree.h:17,
>                   from 
> /jenkins/workspace/ais-xenbits-xen/layers/xen/xen/arch/arm/include/asm/smp.h:6,
>                   from 
> /jenkins/workspace/ais-xenbits-xen/layers/xen/xen/include/xen/smp.h:4,
>                   from 
> /jenkins/workspace/ais-xenbits-xen/layers/xen/xen/include/xen/rwlock.h:6,
>                   from 
> /jenkins/workspace/ais-xenbits-xen/layers/xen/xen/include/xen/sched.h:7,
>                   from 
> /jenkins/workspace/ais-xenbits-xen/layers/xen/xen/arch/arm/arm64/asm-offsets.c:9:
> /jenkins/workspace/ais-xenbits-xen/layers/xen/xen/include/xen/fdt.h:64:13: 
> note: previous declaration of 'device_tree_get_reg' with type 'void(const 
> __be32 **, u32,  u32,  u64 *, u64 *)' {aka 'void(const unsigned int **, 
> unsigned int,  unsigned int,  long unsigned int *, long unsigned int *)'}
>     64 | void __init device_tree_get_reg(
>        |             ^~~~~~~~~~~~~~~~~~~
> In file included from 
> /jenkins/workspace/ais-xenbits-xen/layers/xen/xen/arch/arm/arm64/asm-offsets.c:13:
> /jenkins/workspace/ais-xenbits-xen/layers/xen/xen/arch/arm/include/asm/setup.h:165:5:
>  error: redundant redeclaration of 'device_tree_get_u32' 
> [-Werror=redundant-decls]
>    165 | u32 device_tree_get_u32(const void *fdt, int node,
>        |     ^~~~~~~~~~~~~~~~~~~
> In file included from 
> /jenkins/workspace/ais-xenbits-xen/layers/xen/xen/include/xen/device_tree.h:17,
>                   from 
> /jenkins/workspace/ais-xenbits-xen/layers/xen/xen/arch/arm/include/asm/smp.h:6,
>                   from 
> /jenkins/workspace/ais-xenbits-xen/layers/xen/xen/include/xen/smp.h:4,
>                   from 
> /jenkins/workspace/ais-xenbits-xen/layers/xen/xen/include/xen/rwlock.h:6,
>                   from 
> /jenkins/workspace/ais-xenbits-xen/layers/xen/xen/include/xen/sched.h:7,
>                   from 
> /jenkins/workspace/ais-xenbits-xen/layers/xen/xen/arch/arm/arm64/asm-offsets.c:9:
> /jenkins/workspace/ais-xenbits-xen/layers/xen/xen/include/xen/fdt.h:68:12: 
> note: previous declaration of 'device_tree_get_u32' with type 'u32(const void 
> *, int,  const char *, u32)' {aka 'unsigned int(const void *, int,  const char 
> *, unsigned int)'}
>     68 | u32 __init device_tree_get_u32(
>        |            ^~~~~~~~~~~~~~~~~~~
> cc1: all warnings being treated as errors
> make[2]: *** [/jenkins/workspace/ais-xenbits-xen/layers/xen/xen/./build.mk:45: 
> asm-offsets.s] Error 1
> make[1]: *** [/jenkins/workspace/ais-xenbits-xen/layers/xen/xen/Makefile:597: 
> xen] Error 2
> make[1]: Leaving directory 
> '/jenkins/workspace/ais-xenbits-xen/build/xen-fvp-base'
> make: *** [Makefile:183: __sub-make] Error 2
> make: Leaving directory '/jenkins/workspace/ais-xenbits-xen/layers/xen/xen'
>
> The staging itself passed the CI check. Did I miss some context to make
> this patch works? Could you please provide some hints? Thanks in advance!
Nope you are correct, I now need to go back and look if I sent patches 
out of the wrong branch or if I really did miss cleaning up the original 
declarations in the Arm tree. I fairly confident I made sure gitlab-ci 
passed before I cut the patches for sending, but since I had three 
different series in flight, I may have gotten jumbled. Apologies for the 
unnecessary churn here.
V/r,
Daniel P. Smith