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

[Julien Grall]

On 30 November 2017 at 21:22, Daniel Kiper <address@hidden> wrote:
> On Thu, Nov 30, 2017 at 01:22:37PM +0000, Julien Grall wrote:
>> 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].
>
> Oh, boy! It is unfortunate. Sadly it looks that we have to live with
> this right now... Too late for change... I would just add the comment
> to this file that this is not related to the Multiboot protocol family
> in any way.

I will send a patch for that.

>
>> >>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.
>
> Understood! I will apply this patch next week.

Thank you!

Cheers,

-- 
Julien Grall