* [PATCH] arm64: make sure fdt has #address-cells and #size-cells properties @ 2019-10-31 10:37 Javier Martinez Canillas 2019-10-31 11:56 ` Vladimir 'phcoder' Serbinenko 2019-11-07 16:40 ` Leif Lindholm 0 siblings, 2 replies; 7+ messages in thread From: Javier Martinez Canillas @ 2019-10-31 10:37 UTC (permalink / raw) To: grub-devel; +Cc: Mark Salter, 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] arm64: make sure fdt has #address-cells and #size-cells properties 2019-10-31 10:37 [PATCH] arm64: make sure fdt has #address-cells and #size-cells properties Javier Martinez Canillas @ 2019-10-31 11:56 ` Vladimir 'phcoder' Serbinenko 2019-10-31 15:59 ` Javier Martinez Canillas 2019-11-07 16:40 ` Leif Lindholm 1 sibling, 1 reply; 7+ messages in thread From: Vladimir 'phcoder' Serbinenko @ 2019-10-31 11:56 UTC (permalink / raw) To: The development of GNU GRUB [-- 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 --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] arm64: make sure fdt has #address-cells and #size-cells properties 2019-10-31 11:56 ` Vladimir 'phcoder' Serbinenko @ 2019-10-31 15:59 ` Javier Martinez Canillas 2019-10-31 17:24 ` Mark Salter 0 siblings, 1 reply; 7+ messages in thread From: Javier Martinez Canillas @ 2019-10-31 15:59 UTC (permalink / raw) To: The development of GNU GRUB, Vladimir 'phcoder' Serbinenko Cc: Mark Salter 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] arm64: make sure fdt has #address-cells and #size-cells properties 2019-10-31 15:59 ` Javier Martinez Canillas @ 2019-10-31 17:24 ` Mark Salter 2019-11-05 14:47 ` Daniel Kiper 0 siblings, 1 reply; 7+ messages in thread From: Mark Salter @ 2019-10-31 17:24 UTC (permalink / raw) To: Javier Martinez Canillas, The development of GNU GRUB, Vladimir 'phcoder' Serbinenko 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, ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] arm64: make sure fdt has #address-cells and #size-cells properties 2019-10-31 17:24 ` Mark Salter @ 2019-11-05 14:47 ` Daniel Kiper 0 siblings, 0 replies; 7+ messages in thread From: Daniel Kiper @ 2019-11-05 14:47 UTC (permalink / raw) To: Mark Salter, leif.lindholm, agraf Cc: Javier Martinez Canillas, The development of GNU GRUB, Vladimir 'phcoder' Serbinenko 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, ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] arm64: make sure fdt has #address-cells and #size-cells properties 2019-10-31 10:37 [PATCH] arm64: make sure fdt has #address-cells and #size-cells properties Javier Martinez Canillas 2019-10-31 11:56 ` Vladimir 'phcoder' Serbinenko @ 2019-11-07 16:40 ` Leif Lindholm 2019-11-07 17:39 ` Javier Martinez Canillas 1 sibling, 1 reply; 7+ messages in thread From: Leif Lindholm @ 2019-11-07 16:40 UTC (permalink / raw) To: The development of GNU GRUB Cc: Mark Salter, Javier Martinez Canillas, Daniel Kiper 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] arm64: make sure fdt has #address-cells and #size-cells properties 2019-11-07 16:40 ` Leif Lindholm @ 2019-11-07 17:39 ` Javier Martinez Canillas 0 siblings, 0 replies; 7+ messages in thread From: Javier Martinez Canillas @ 2019-11-07 17:39 UTC (permalink / raw) To: Leif Lindholm, The development of GNU GRUB; +Cc: Mark Salter, Daniel Kiper 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-11-07 17:39 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-10-31 10:37 [PATCH] arm64: make sure fdt has #address-cells and #size-cells properties Javier Martinez Canillas 2019-10-31 11:56 ` Vladimir 'phcoder' Serbinenko 2019-10-31 15:59 ` Javier Martinez Canillas 2019-10-31 17:24 ` Mark Salter 2019-11-05 14:47 ` Daniel Kiper 2019-11-07 16:40 ` Leif Lindholm 2019-11-07 17:39 ` Javier Martinez Canillas
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.