diff mbox series

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

Message ID 20171129170812.31951-1-julien.grall@linaro.org
State Accepted
Commit d34977cb662d9d3c74532dc175103758c47f552f
Headers show
Series arm64/xen: Add missing #address-cells and #size-cells properties | expand

Commit Message

Julien Grall Nov. 29, 2017, 5:08 p.m. UTC 
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(+)

-- 
2.11.0


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

Comments

Daniel Kiper Nov. 30, 2017, 1:06 p.m. UTC | #1 
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

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Julien Grall Nov. 30, 2017, 1:22 p.m. UTC | #2 
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

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Daniel Kiper Nov. 30, 2017, 9:22 p.m. UTC | #3 
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
Julien Grall Nov. 30, 2017, 10:06 p.m. UTC | #4 
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,
Szyfelbein, Wojciech M Dec. 4, 2017, 7:24 a.m. UTC | #5 
-----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.


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Daniel Kiper Dec. 6, 2017, 12:17 p.m. UTC | #6 
On Thu, Nov 30, 2017 at 10:22:57PM +0100, Daniel Kiper wrote:

[...]

> Understood! I will apply this patch next week.


Pushed!

Thanks,

Daniel

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

Patch

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,