Re: [PATCH] arm64/xen: Add missing #address-cells and #size-cells properties

[Julien Grall]

Hi Daniel,

On 30/11/17 13:06, Daniel Kiper wrote:
> On Wed, Nov 29, 2017 at 05:08:12PM +0000, Julien Grall wrote:
> > The properties #address-cells and #size-cells are used to know the
> > number of cells for ranges provided by "regs". If they don't exist, the
> > value are resp. 2 and 1.
> >
> > Currently, when multiboot nodes are created it is assumed that #address-cells
>
> IIRC ARM boot protocol is not related to Multiboot protocol in any way.
> So, calling it in that way is very confusing. Could you invent a better
> not confusion name. Or at least provide a spec. I am happy to see it
> in GRUB2 tree.

That's the name of the node see the MODULE_CUSTOM_COMPATIBLE in the 
code... But this it not the Linux Arm boot protocol, it is an extension 
currently only used by Xen. See [1].

> > and #size-cells are exactly 2. However, they are never set by GRUB and
> > will result to later failure when the device-tree is generated by GRUB
> > or contain different values.
> >
> > To prevent this failure, create the both properties in the chosen nodes.
> >
> > Signed-off-by: Julien Grall <address@hidden>
> > ---
> >   grub-core/loader/arm64/xen_boot.c | 11 +++++++++++
> >   1 file changed, 11 insertions(+)
> >
> > diff --git a/grub-core/loader/arm64/xen_boot.c 
> > b/grub-core/loader/arm64/xen_boot.c
> > index c95d6c5a8..6780b1f0c 100644
> > --- a/grub-core/loader/arm64/xen_boot.c
> > +++ b/grub-core/loader/arm64/xen_boot.c
> > @@ -115,6 +115,17 @@ prepare_xen_hypervisor_params (void *xen_boot_fdt)
> >     if (chosen_node < 1)
> >       return grub_error (GRUB_ERR_IO, "failed to get chosen node in FDT");
> >
> > +  /*
> > +   * The address and size are always written using 64-bits value. Set
>
> Here you say "64-bits value"...
>
> > +   * #address-cells and #size-cells accordingly.
> > +   */
> > +  retval = grub_fdt_set_prop32 (xen_boot_fdt, chosen_node, "#address-cells", 
> > 2);
>
> ...and then call grub_fdt_set_prop32(). I am confused...

#address-cells and #size-cells are property to know the number of cells 
per address/size.

The address and size are 64-bit (i.e 2 cells) as you can see the call to 
grub_fdt_set_reg64 in the prepare_xen_module_params.

> > +  if (retval)
> > +    return grub_error (GRUB_ERR_IO, "failed to set #address-cells");
> > +  retval = grub_fdt_set_prop32 (xen_boot_fdt, chosen_node, "#size-cells", 2);
>
> Ditto.

Cheers,

[1] 
https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=docs/misc/arm/device-tree/booting.txt

--
Julien Grall