Re: [PATCH] xen/arm, device-tree: Make static-mem use #{address,size}-cells

[Julien Grall <julien@xxxxxxx>]

Hi,

On 08/09/2022 11:54, Henry Wang wrote:
> > -----Original Message-----
> > From: Michal Orzel <michal.orzel@xxxxxxx>
> > > @@ -362,14 +362,13 @@ device-tree:
> > >
> > >       / {
> > >           chosen {
> > > +            #address-cells = <0x1>;
> > > +            #size-cells = <0x1>;
> > > +            ...
> > >               domU1 {
> > >                   compatible = "xen,domain";
> > > -                #address-cells = <0x2>;
> > > -                #size-cells = <0x2>;
> > Why did you remove this set if it relates to the childs of domU1 (e.g. kernel,
> > ramdisk) and not to domU1 itself?
> Well, I think here the example is just how we setup the static memory, so we 
> just
> want to emphasize the related part. I agree users can add another #address-cells
> and #size-cells for domU1 node for the parts that you mentioned, but that is
> not reflected by the current example (I can't find anything related to kernel,
> ramdisk, etc. in current example). I might get it wrong but having two 
> #address-cells
> and #size-cells in that case would be quite misleading from my understanding.
I agree with that. As this is only a small part of the DT we want to 
focus on what is necessary for the current section.
> So I decided to remove it.
I would mention it in the commit message because the change seems 
unrelated otherwise.
The same apply for replacing adding extra "====". But TBH, this change 
feels completely unrelated to this patch. So I think it is better to 
have a separate patch.
[...]

> > > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> > > index ec81a45de9..cd264793d5 100644
> > > --- a/xen/arch/arm/bootfdt.c
> > > +++ b/xen/arch/arm/bootfdt.c
> > > @@ -352,11 +352,6 @@ static int __init process_domain_node(const void
> > *fdt, int node,
> > >           /* No "xen,static-mem" present. */
> > >           return 0;
> > >
> > > -    address_cells = device_tree_get_u32(fdt, node,
> > > -                                        "#xen,static-mem-address-cells", 0);
> > > -    size_cells = device_tree_get_u32(fdt, node,
> > > -                                     "#xen,static-mem-size-cells", 0);
> > > -
> > >       return device_tree_get_meminfo(fdt, node, "xen,static-mem",
> > address_cells,
> > >                                      size_cells, &bootinfo.reserved_mem, true);
> > >   }
> > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > > index b76a84e8f5..258d74699d 100644
> > > --- a/xen/arch/arm/domain_build.c
> > > +++ b/xen/arch/arm/domain_build.c
> > > @@ -563,21 +563,9 @@ static int __init parse_static_mem_prop(const
> > struct dt_device_node *node,
> > >       const struct dt_property *prop;
> > >
> > >       prop = dt_find_property(node, "xen,static-mem", NULL);
> > > -    if ( !dt_property_read_u32(node, "#xen,static-mem-address-cells",
> > > -                               addr_cells) )
> > > -    {
> > > -        printk(XENLOG_ERR
> > > -               "failed to read \"#xen,static-mem-address-cells\".\n");
> > > -        return -EINVAL;
> > > -    }
> > >
> > > -    if ( !dt_property_read_u32(node, "#xen,static-mem-size-cells",
> > > -                               size_cells) )
> > > -    {
> > > -        printk(XENLOG_ERR
> > > -               "failed to read \"#xen,static-mem-size-cells\".\n");
> > > -        return -EINVAL;
> > > -    }
> > > +    *addr_cells = dt_n_addr_cells(node);
> > > +    *size_cells = dt_n_size_cells(node);
> > There is a type mismatch here as e.g. addr_cells is u32 and dt_n_addr_cells
> > return type is int.
> > But I don't think this can be harmful and also it's strange for me that
> > dt_n_addr_cells
> > is defined to return int given that it either returns 2 or be32_to_cpup, which
> > means it should return u32.
> Yeah. I agree. I did a git blame here and found this function is introduced 9
> years ago in "dbd1243 xen/arm: Add helpers to use the device tree". So I think
> probably it would be easier to ask the author for the following action directly 
> :))
The code was imported from Linux where it seems to be more common to use 
"int" rather than "unsigned".
> @Julien, what do you think? Shall we modify the return type of these two
> functions?
I think it would be good to be consistent. However, there are other 
users of d_n_addr_cells() (some are expecting 'int'). So if you switch 
to a different type then this use will be consistent but not the others.
I would only suggest to look at it if you have if you have copious time 
and fancy going down the rabbit hole :).
As to which type to chose, we are phasing out use of uXX in new code. So 
this should be 'uint32_t'. I would also be fine to use 'unsigned int' 
for the outside interface.
I don't have a strong opinion either way.

Cheers,

--
Julien Grall