| Message ID | 20191031103740.5400-1-javierm@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | arm64: make sure fdt has #address-cells and #size-cells properties | expand |
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 > <div dir="auto">This patch looks incomplete. What if "chosen" is there but does not contain the relevant fields?</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, 31 Oct 2019, 11:39 Javier Martinez Canillas, <<a href="mailto:javierm@redhat.com">javierm@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">From: Mark Salter <<a href="mailto:msalter@redhat.com" target="_blank" rel="noreferrer">msalter@redhat.com</a>><br> <br> Recent upstream changes to kexec-tools relies on #address-cells<br> and #size-cells properties in the FDT. If grub2 needs to create<br> a chosen node, it is likely because firmware did not provide one.<br> In that case, set #address-cells and #size-cells properties to<br> make sure they exist.<br> <br> Signed-off-by: Mark Salter <<a href="mailto:msalter@redhat.com" target="_blank" rel="noreferrer">msalter@redhat.com</a>><br> Signed-off-by: Javier Martinez Canillas <<a href="mailto:javierm@redhat.com" target="_blank" rel="noreferrer">javierm@redhat.com</a>><br> ---<br> <br> grub-core/loader/arm64/linux.c | 16 +++++++++++++++-<br> 1 file changed, 15 insertions(+), 1 deletion(-)<br> <br> diff --git grub-core/loader/arm64/linux.c grub-core/loader/arm64/linux.c<br> index ef3e9f9444c..24d73732d89 100644<br> --- grub-core/loader/arm64/linux.c<br> +++ grub-core/loader/arm64/linux.c<br> @@ -78,7 +78,21 @@ finalize_params_linux (void)<br> <br> node = grub_fdt_find_subnode (fdt, 0, "chosen");<br> if (node < 0)<br> - node = grub_fdt_add_subnode (fdt, 0, "chosen");<br> + {<br> + /*<br> + * If we have to create a chosen node, Make sure we<br> + * have #address-cells and #size-cells properties.<br> + */<br> + retval = grub_fdt_set_prop32(fdt, 0, "#address-cells", 2);<br> + if (retval)<br> + goto failure;<br> +<br> + retval = grub_fdt_set_prop32(fdt, 0, "#size-cells", 2);<br> + if (retval)<br> + goto failure;<br> +<br> + node = grub_fdt_add_subnode (fdt, 0, "chosen");<br> + }<br> <br> if (node < 1)<br> goto failure;<br> -- <br> 2.21.0<br> <br> <br> _______________________________________________<br> Grub-devel mailing list<br> <a href="mailto:Grub-devel@gnu.org" target="_blank" rel="noreferrer">Grub-devel@gnu.org</a><br> <a href="https://lists.gnu.org/mailman/listinfo/grub-devel" rel="noreferrer noreferrer" target="_blank">https://lists.gnu.org/mailman/listinfo/grub-devel</a><br> </blockquote></div>
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,
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,
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,
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
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,
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;