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

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

[Julien Grall]

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
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 <julien.grall@linaro.org>
---
 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
+   * #address-cells and #size-cells accordingly.
+   */
+  retval = grub_fdt_set_prop32 (xen_boot_fdt, chosen_node, "#address-cells", 2);
+  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);
+  if (retval)
+    return grub_error (GRUB_ERR_IO, "failed to set #size-cells");
+
   grub_dprintf ("xen_loader",
 		"Xen Hypervisor cmdline : %s @ %p size:%d\n",
 		xen_hypervisor->cmdline, xen_hypervisor->cmdline,
-- 
2.11.0

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

[Daniel Kiper]

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.

> 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 <julien.grall@linaro.org>
> ---
>  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...

> +  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.

Daniel

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[Daniel Kiper]

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.

> 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 <julien.grall@linaro.org>
> ---
>  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...

> +  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.

Daniel

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 <julien.grall@linaro.org>
>> ---
>>   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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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 <julien.grall@linaro.org>
>> ---
>>   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

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

[Daniel Kiper]

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.

> >>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 <julien.grall@linaro.org>
> >>---
> >>  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.

Daniel

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[Daniel Kiper]

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.

> >>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 <julien.grall@linaro.org>
> >>---
> >>  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.

Daniel

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

[Julien Grall]

On 30 November 2017 at 21:22, Daniel Kiper <daniel.kiper@oracle.com> 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 <julien.grall@linaro.org>
>> >>---
>> >>  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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[Julien Grall]

On 30 November 2017 at 21:22, Daniel Kiper <daniel.kiper@oracle.com> 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 <julien.grall@linaro.org>
>> >>---
>> >>  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

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

[Szyfelbein, Wojciech M]

-----Original Message-----
From: Grub-devel [mailto:grub-devel-bounces+wojciech.m.szyfelbein=intel.com@gnu.org] On Behalf Of Daniel Kiper
Sent: Thursday, November 30, 2017 10:23 PM
To: Julien Grall <julien.grall@linaro.org>
Cc: grub-devel@gnu.org; fu.wei@linaro.org; xen-devel@lists.xen.org
Subject: Re: [PATCH] arm64/xen: Add missing #address-cells and #size-cells properties

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.

> >>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 <julien.grall@linaro.org>
> >>---
> >>  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.

Daniel

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
--------------------------------------------------------------------

Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.

Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek
przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by
others is strictly prohibited.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[Szyfelbein, Wojciech M]

-----Original Message-----
From: Grub-devel [mailto:grub-devel-bounces+wojciech.m.szyfelbein=intel.com@gnu.org] On Behalf Of Daniel Kiper
Sent: Thursday, November 30, 2017 10:23 PM
To: Julien Grall <julien.grall@linaro.org>
Cc: grub-devel@gnu.org; fu.wei@linaro.org; xen-devel@lists.xen.org
Subject: Re: [PATCH] arm64/xen: Add missing #address-cells and #size-cells properties

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.

> >>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 <julien.grall@linaro.org>
> >>---
> >>  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.

Daniel

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
--------------------------------------------------------------------

Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.

Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek
przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by
others is strictly prohibited.

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

[Daniel Kiper]

On Thu, Nov 30, 2017 at 10:22:57PM +0100, Daniel Kiper wrote:

[...]

> Understood! I will apply this patch next week.

Pushed!

Thanks,

Daniel

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[Daniel Kiper]

On Thu, Nov 30, 2017 at 10:22:57PM +0100, Daniel Kiper wrote:

[...]

> Understood! I will apply this patch next week.

Pushed!

Thanks,

Daniel

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

[Julien Grall]

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
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 <julien.grall@linaro.org>
---
 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
+   * #address-cells and #size-cells accordingly.
+   */
+  retval = grub_fdt_set_prop32 (xen_boot_fdt, chosen_node, "#address-cells", 2);
+  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);
+  if (retval)
+    return grub_error (GRUB_ERR_IO, "failed to set #size-cells");
+
   grub_dprintf ("xen_loader",
 		"Xen Hypervisor cmdline : %s @ %p size:%d\n",
 		xen_hypervisor->cmdline, xen_hypervisor->cmdline,
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel