[PATCH] arm64: make sure fdt has #address-cells and #size-cells properties

[PATCH] arm64: make sure fdt has #address-cells and #size-cells properties

[Javier Martinez Canillas]

From: Mark Salter <msalter@redhat.com>

Recent upstream changes to kexec-tools relies on #address-cells
and #size-cells properties in the FDT. If grub2 needs to create
a chosen node, it is likely because firmware did not provide one.
In that case, set #address-cells and #size-cells properties to
make sure they exist.

Signed-off-by: Mark Salter <msalter@redhat.com>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---

 grub-core/loader/arm64/linux.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git grub-core/loader/arm64/linux.c grub-core/loader/arm64/linux.c
index ef3e9f9444c..24d73732d89 100644
--- grub-core/loader/arm64/linux.c
+++ grub-core/loader/arm64/linux.c
@@ -78,7 +78,21 @@ finalize_params_linux (void)
 
   node = grub_fdt_find_subnode (fdt, 0, "chosen");
   if (node < 0)
-    node = grub_fdt_add_subnode (fdt, 0, "chosen");
+    {
+      /*
+       * If we have to create a chosen node, Make sure we
+       * have #address-cells and #size-cells properties.
+       */
+      retval = grub_fdt_set_prop32(fdt, 0, "#address-cells", 2);
+      if (retval)
+        goto failure;
+
+      retval = grub_fdt_set_prop32(fdt, 0, "#size-cells", 2);
+      if (retval)
+        goto failure;
+
+      node = grub_fdt_add_subnode (fdt, 0, "chosen");
+    }
 
   if (node < 1)
     goto failure;
-- 
2.21.0

Re: [PATCH] arm64: make sure fdt has #address-cells and #size-cells properties

[Vladimir 'phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 1778 bytes --]

This patch looks incomplete. What if "chosen" is there but does not contain
the relevant fields?

On Thu, 31 Oct 2019, 11:39 Javier Martinez Canillas, <javierm@redhat.com>
wrote:

> From: Mark Salter <msalter@redhat.com>
>
> Recent upstream changes to kexec-tools relies on #address-cells
> and #size-cells properties in the FDT. If grub2 needs to create
> a chosen node, it is likely because firmware did not provide one.
> In that case, set #address-cells and #size-cells properties to
> make sure they exist.
>
> Signed-off-by: Mark Salter <msalter@redhat.com>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
>
>  grub-core/loader/arm64/linux.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git grub-core/loader/arm64/linux.c grub-core/loader/arm64/linux.c
> index ef3e9f9444c..24d73732d89 100644
> --- grub-core/loader/arm64/linux.c
> +++ grub-core/loader/arm64/linux.c
> @@ -78,7 +78,21 @@ finalize_params_linux (void)
>
>    node = grub_fdt_find_subnode (fdt, 0, "chosen");
>    if (node < 0)
> -    node = grub_fdt_add_subnode (fdt, 0, "chosen");
> +    {
> +      /*
> +       * If we have to create a chosen node, Make sure we
> +       * have #address-cells and #size-cells properties.
> +       */
> +      retval = grub_fdt_set_prop32(fdt, 0, "#address-cells", 2);
> +      if (retval)
> +        goto failure;
> +
> +      retval = grub_fdt_set_prop32(fdt, 0, "#size-cells", 2);
> +      if (retval)
> +        goto failure;
> +
> +      node = grub_fdt_add_subnode (fdt, 0, "chosen");
> +    }
>
>    if (node < 1)
>      goto failure;
> --
> 2.21.0
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>

[-- Attachment #2: Type: text/html, Size: 2734 bytes --]

Re: [PATCH] arm64: make sure fdt has #address-cells and #size-cells properties

[Javier Martinez Canillas]

On 10/31/19 12:56 PM, Vladimir 'phcoder' Serbinenko wrote:
> This patch looks incomplete. What if "chosen" is there but does not contain
> the relevant fields?
>

My understanding is that if there's a chosen node already defined in the
Device Tree but without #address-cells and #size-cells properties, then
is a malformed DT and is not up to GRUB to correct it.

But if GRUB is adding a child node as is the case of the chosen node, then
is up to GRUB to make sure that there are #address-cells and #size-cells
properties also defined so the child nodes are addressed correctly.

But maybe I'm wrong on this, so I'll let Mark comment since he authored
the patch and knows better what's the problem that's fixing.

> On Thu, 31 Oct 2019, 11:39 Javier Martinez Canillas, <javierm@redhat.com>
> wrote:
> 
>> From: Mark Salter <msalter@redhat.com>
>>
>> Recent upstream changes to kexec-tools relies on #address-cells
>> and #size-cells properties in the FDT. If grub2 needs to create
>> a chosen node, it is likely because firmware did not provide one.
>> In that case, set #address-cells and #size-cells properties to
>> make sure they exist.
>>
>> Signed-off-by: Mark Salter <msalter@redhat.com>
>> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
>> ---
>>
>>  grub-core/loader/arm64/linux.c | 16 +++++++++++++++-
>>  1 file changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git grub-core/loader/arm64/linux.c grub-core/loader/arm64/linux.c
>> index ef3e9f9444c..24d73732d89 100644
>> --- grub-core/loader/arm64/linux.c
>> +++ grub-core/loader/arm64/linux.c
>> @@ -78,7 +78,21 @@ finalize_params_linux (void)
>>
>>    node = grub_fdt_find_subnode (fdt, 0, "chosen");
>>    if (node < 0)
>> -    node = grub_fdt_add_subnode (fdt, 0, "chosen");
>> +    {
>> +      /*
>> +       * If we have to create a chosen node, Make sure we
>> +       * have #address-cells and #size-cells properties.
>> +       */
>> +      retval = grub_fdt_set_prop32(fdt, 0, "#address-cells", 2);
>> +      if (retval)
>> +        goto failure;
>> +
>> +      retval = grub_fdt_set_prop32(fdt, 0, "#size-cells", 2);
>> +      if (retval)
>> +        goto failure;
>> +
>> +      node = grub_fdt_add_subnode (fdt, 0, "chosen");
>> +    }
>>
>>    if (node < 1)
>>      goto failure;
>> --
>> 2.21.0
>>
>>
 
Best regards,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat

Re: [PATCH] arm64: make sure fdt has #address-cells and #size-cells properties

[Mark Salter]

On Thu, 2019-10-31 at 16:59 +0100, Javier Martinez Canillas wrote:
> On 10/31/19 12:56 PM, Vladimir 'phcoder' Serbinenko wrote:
> > This patch looks incomplete. What if "chosen" is there but does not contain
> > the relevant fields?
> > 
> 
> My understanding is that if there's a chosen node already defined in the
> Device Tree but without #address-cells and #size-cells properties, then
> is a malformed DT and is not up to GRUB to correct it.

Right. IIRC, this was the case where firmware only has ACPI tables and grub
has to create a DT with just a chosen node.

> 
> But if GRUB is adding a child node as is the case of the chosen node, then
> is up to GRUB to make sure that there are #address-cells and #size-cells
> properties also defined so the child nodes are addressed correctly.
> 
> But maybe I'm wrong on this, so I'll let Mark comment since he authored
> the patch and knows better what's the problem that's fixing.
> 
> > On Thu, 31 Oct 2019, 11:39 Javier Martinez Canillas, <javierm@redhat.com>
> > wrote:
> > 
> > > From: Mark Salter <msalter@redhat.com>
> > > 
> > > Recent upstream changes to kexec-tools relies on #address-cells
> > > and #size-cells properties in the FDT. If grub2 needs to create
> > > a chosen node, it is likely because firmware did not provide one.
> > > In that case, set #address-cells and #size-cells properties to
> > > make sure they exist.
> > > 
> > > Signed-off-by: Mark Salter <msalter@redhat.com>
> > > Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> > > ---
> > > 
> > >  grub-core/loader/arm64/linux.c | 16 +++++++++++++++-
> > >  1 file changed, 15 insertions(+), 1 deletion(-)
> > > 
> > > diff --git grub-core/loader/arm64/linux.c grub-core/loader/arm64/linux.c
> > > index ef3e9f9444c..24d73732d89 100644
> > > --- grub-core/loader/arm64/linux.c
> > > +++ grub-core/loader/arm64/linux.c
> > > @@ -78,7 +78,21 @@ finalize_params_linux (void)
> > > 
> > >    node = grub_fdt_find_subnode (fdt, 0, "chosen");
> > >    if (node < 0)
> > > -    node = grub_fdt_add_subnode (fdt, 0, "chosen");
> > > +    {
> > > +      /*
> > > +       * If we have to create a chosen node, Make sure we
> > > +       * have #address-cells and #size-cells properties.
> > > +       */
> > > +      retval = grub_fdt_set_prop32(fdt, 0, "#address-cells", 2);
> > > +      if (retval)
> > > +        goto failure;
> > > +
> > > +      retval = grub_fdt_set_prop32(fdt, 0, "#size-cells", 2);
> > > +      if (retval)
> > > +        goto failure;
> > > +
> > > +      node = grub_fdt_add_subnode (fdt, 0, "chosen");
> > > +    }
> > > 
> > >    if (node < 1)
> > >      goto failure;
> > > --
> > > 2.21.0
> > > 
> > > 
>  
> Best regards,

Re: [PATCH] arm64: make sure fdt has #address-cells and #size-cells properties

[Daniel Kiper]

Adding Leif and Alex...

On Thu, Oct 31, 2019 at 01:24:01PM -0400, Mark Salter wrote:
> On Thu, 2019-10-31 at 16:59 +0100, Javier Martinez Canillas wrote:
> > On 10/31/19 12:56 PM, Vladimir 'phcoder' Serbinenko wrote:
> > > This patch looks incomplete. What if "chosen" is there but does not contain
> > > the relevant fields?
> > >
> >
> > My understanding is that if there's a chosen node already defined in the
> > Device Tree but without #address-cells and #size-cells properties, then
> > is a malformed DT and is not up to GRUB to correct it.
>
> Right. IIRC, this was the case where firmware only has ACPI tables and grub
> has to create a DT with just a chosen node.
>
> >
> > But if GRUB is adding a child node as is the case of the chosen node, then
> > is up to GRUB to make sure that there are #address-cells and #size-cells
> > properties also defined so the child nodes are addressed correctly.
> >
> > But maybe I'm wrong on this, so I'll let Mark comment since he authored
> > the patch and knows better what's the problem that's fixing.
> >
> > > On Thu, 31 Oct 2019, 11:39 Javier Martinez Canillas, <javierm@redhat.com>
> > > wrote:
> > >
> > > > From: Mark Salter <msalter@redhat.com>
> > > >
> > > > Recent upstream changes to kexec-tools relies on #address-cells
> > > > and #size-cells properties in the FDT. If grub2 needs to create
> > > > a chosen node, it is likely because firmware did not provide one.
> > > > In that case, set #address-cells and #size-cells properties to
> > > > make sure they exist.
> > > >
> > > > Signed-off-by: Mark Salter <msalter@redhat.com>
> > > > Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> > > > ---
> > > >
> > > >  grub-core/loader/arm64/linux.c | 16 +++++++++++++++-
> > > >  1 file changed, 15 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git grub-core/loader/arm64/linux.c grub-core/loader/arm64/linux.c
> > > > index ef3e9f9444c..24d73732d89 100644
> > > > --- grub-core/loader/arm64/linux.c
> > > > +++ grub-core/loader/arm64/linux.c
> > > > @@ -78,7 +78,21 @@ finalize_params_linux (void)
> > > >
> > > >    node = grub_fdt_find_subnode (fdt, 0, "chosen");
> > > >    if (node < 0)
> > > > -    node = grub_fdt_add_subnode (fdt, 0, "chosen");
> > > > +    {
> > > > +      /*
> > > > +       * If we have to create a chosen node, Make sure we
> > > > +       * have #address-cells and #size-cells properties.
> > > > +       */
> > > > +      retval = grub_fdt_set_prop32(fdt, 0, "#address-cells", 2);
> > > > +      if (retval)
> > > > +        goto failure;
> > > > +
> > > > +      retval = grub_fdt_set_prop32(fdt, 0, "#size-cells", 2);
> > > > +      if (retval)
> > > > +        goto failure;
> > > > +
> > > > +      node = grub_fdt_add_subnode (fdt, 0, "chosen");
> > > > +    }
> > > >
> > > >    if (node < 1)
> > > >      goto failure;
> > > > --
> > > > 2.21.0
> > > >
> > > >
> >
> > Best regards,

Re: [PATCH] arm64: make sure fdt has #address-cells and #size-cells properties

[Leif Lindholm]

On Thu, Oct 31, 2019 at 11:37:40AM +0100, Javier Martinez Canillas wrote:
> From: Mark Salter <msalter@redhat.com>
> 
> Recent upstream changes to kexec-tools relies on #address-cells
> and #size-cells properties in the FDT. If grub2 needs to create
> a chosen node, it is likely because firmware did not provide one.
> In that case, set #address-cells and #size-cells properties to
> make sure they exist.

I assumed we fixed that in 347210a5d5ce
("efi/fdt: Set address/size cells to 2 for empty tree")?

If we didn't, I still think the fix should be in
grub-core/loader/efi/fdt.c rather than here.

/
    Leif

> Signed-off-by: Mark Salter <msalter@redhat.com>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
> 
>  grub-core/loader/arm64/linux.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git grub-core/loader/arm64/linux.c grub-core/loader/arm64/linux.c
> index ef3e9f9444c..24d73732d89 100644
> --- grub-core/loader/arm64/linux.c
> +++ grub-core/loader/arm64/linux.c
> @@ -78,7 +78,21 @@ finalize_params_linux (void)
>  
>    node = grub_fdt_find_subnode (fdt, 0, "chosen");
>    if (node < 0)
> -    node = grub_fdt_add_subnode (fdt, 0, "chosen");
> +    {
> +      /*
> +       * If we have to create a chosen node, Make sure we
> +       * have #address-cells and #size-cells properties.
> +       */
> +      retval = grub_fdt_set_prop32(fdt, 0, "#address-cells", 2);
> +      if (retval)
> +        goto failure;
> +
> +      retval = grub_fdt_set_prop32(fdt, 0, "#size-cells", 2);
> +      if (retval)
> +        goto failure;
> +
> +      node = grub_fdt_add_subnode (fdt, 0, "chosen");
> +    }
>  
>    if (node < 1)
>      goto failure;
> -- 
> 2.21.0
> 
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] arm64: make sure fdt has #address-cells and #size-cells properties

[Javier Martinez Canillas]

Hello Leif,

On 11/7/19 5:40 PM, Leif Lindholm wrote:
> On Thu, Oct 31, 2019 at 11:37:40AM +0100, Javier Martinez Canillas wrote:
>> From: Mark Salter <msalter@redhat.com>
>>
>> Recent upstream changes to kexec-tools relies on #address-cells
>> and #size-cells properties in the FDT. If grub2 needs to create
>> a chosen node, it is likely because firmware did not provide one.
>> In that case, set #address-cells and #size-cells properties to
>> make sure they exist.
> 
> I assumed we fixed that in 347210a5d5ce
> ("efi/fdt: Set address/size cells to 2 for empty tree")?
> 

That indeed seems to be the case. I missed that was already fixed when
rebasing Mark's patch because the fix was a different one.

So it's another patch that can be dropped from our package, thanks a
lot for pointing it out.

Best regards,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat