Re: [XEN v2 04/11] xen/arm: Typecast the DT values into paddr_t

[Julien Grall <julien@xxxxxxx>]

Hi Ayan,

On 31/01/2023 10:51, Ayan Kumar Halder wrote:
> On 20/01/2023 10:16, Julien Grall wrote:
> > > > Last comment, maybe we could add fdt_get_mem_rsv_paddr to setup.h
> > > > instead of introducing xen/arch/arm/include/asm/device_tree.h, given
> > > > that we already have device tree definitions there
> > > > (device_tree_get_reg). I am OK either way.
> > >   Actually I noticed you also add dt_device_get_paddr to
> > > xen/arch/arm/include/asm/device_tree.h. So it sounds like it is a good
> > > idea to introduce xen/arch/arm/include/asm/device_tree.h, and we could
> > > also move the declarations of device_tree_get_reg, device_tree_get_u32
> > > there.
> > None of the helpers you mention sounds arm specific. So why should the 
> > be move an arch specific headers?
> The functions (fdt_get_mem_rsv_paddr(), device_tree_get_reg(), 
> device_tree_get_u32()) are currently used by Arm specific code only.
> So, I thought of implementing fdt_get_mem_rsv_paddr() in 
> xen/arch/arm/include/asm/device_tree.h.
> However, I see your point as well. So the alternative approach would be :-
>
> Approach 1) Implement fdt_get_mem_rsv_paddr() in 
> ./xen/include/xen/libfdt/libfdt.h.
> However libfdt is imported from external sources, so I am not sure if 
> this is the  best approach.
One way would be to introduce "libfdt_xen.h" which would contain all the 
implementation specific to Xen.
> Approach 2) Rename fdt_get_mem_rsv_paddr() to dt_get_mem_rsv_paddr() and 
> implement it in ./xen/include/xen/device_tree.h.
> However, this will be an odd one as it invokes fdt_get_mem_rsv() for 
> which we will have to include libfdt.h in device_tree.h.
You could implement the functions in the device_tree.c but see below.

> So, I am biased towards having xen/arch/arm/include/asm/device_tree.h in 
> which we can implement all the non-static fdt and dt functions (which 
> are required by xen/arch/arm/*).
> And then (as Stefano suggested), we can move the definitions of the 
> following to xen/arch/arm/include/asm/device_tree.h.
> device_tree_get_reg()
>
> device_tree_get_u32()
>
> device_tree_for_each_node()
Well none of them are truly arch specific as well. I could imagine 
RISC-v to use it in the future if they also care about checking the 
truncation.
I have a slight preference to introduce libfdt_xen.h over adding the 
implementation in device_tree.h so we can keep the unflatten API 
(device_tree.h) separated from the flatten API (libfdt.h).
But this is not a strong preference between the two. That said, I would 
strongly argue against adding the helper in asm/*.h because there is 
nothing Arm specific in them.
> > [...]
> >
> > > > > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> > > > > index 0085c28d74..f536a3f3ab 100644
> > > > > --- a/xen/arch/arm/bootfdt.c
> > > > > +++ b/xen/arch/arm/bootfdt.c
> > > > > @@ -11,9 +11,9 @@
> > > > >   #include <xen/efi.h>
> > > > >   #include <xen/device_tree.h>
> > > > >   #include <xen/lib.h>
> > > > > -#include <xen/libfdt/libfdt.h>
> > > > >   #include <xen/sort.h>
> > > > >   #include <xsm/xsm.h>
> > > > > +#include <asm/device_tree.h>
> > > > >   #include <asm/setup.h>
> > > > >     static bool __init device_tree_node_matches(const void *fdt, 
> > > > > int node,
> > > > > @@ -53,10 +53,15 @@ static bool __init 
> > > > > device_tree_node_compatible(const void *fdt, int node,
> > > > >   }
> > > > >     void __init device_tree_get_reg(const __be32 **cell, u32 
> > > > > address_cells,
> > > > > -                                u32 size_cells, u64 *start, u64 
> > > > > *size)
> > > > > +                                u32 size_cells, paddr_t *start, 
> > > > > paddr_t *size)
> > > > >   {
> > > > > -    *start = dt_next_cell(address_cells, cell);
> > > > > -    *size = dt_next_cell(size_cells, cell);
> > > > > +    /*
> > > > > +     * dt_next_cell will return u64 whereas paddr_t may be u64 or 
> > > > > u32. Thus, one
> > > > > +     * needs to cast paddr_t to u32. Note that we do not check for 
> > > > > truncation as
> > > > > +     * it is user's responsibility to provide the correct values 
> > > > > in the DT.
> > > > > +     */
> > > > > +    *start = (paddr_t) dt_next_cell(address_cells, cell);
> > > > > +    *size = (paddr_t) dt_next_cell(size_cells, cell);
> > There is no need for explicit cast here.
> Should we have it for the sake of documentation (that it casts u64 to 
> paddr_t) ?
You already have a comment on top of dt_next_cell() to explain the 
typecast. So I would rather not add the explicit casts.
Cheers,

--
Julien Grall