| Message ID | 1409215247-10195-1-git-send-email-mpe@ellerman.id.au (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | ad72a279a2b874828d1b5070ef01cf6ee6b1d62c |
| Delegated to: | Michael Ellerman |
| Headers | show |
On Thu, Aug 28, 2014 at 3:40 AM, Michael Ellerman <mpe@ellerman.id.au> wrote: > In commit e6a6928c3ea1 "of/fdt: Convert FDT functions to use libfdt", > the kernel stopped supporting old flat device tree formats. The minimum > supported version is now 0x10. Ugg. Is that something which needs to be supported? Supporting it at this point would mean adding support to libfdt. Rob > There was a checking function added, early_init_dt_verify(), but it's > not called on powerpc. > > The result is, if you boot with an old flat device tree, the kernel will > fail to parse it correctly, think you have no memory etc. and hilarity > ensues. > > We can't really fix it, but we can at least catch the fact that the > device tree is in an unsupported format and panic(). We can't call > BUG(), it's too early. > > Signed-off-by: Michael Ellerman <mpe@ellerman.id.au> > --- > arch/powerpc/kernel/prom.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c > index 4e139f8a69ef..bb777b255b1c 100644 > --- a/arch/powerpc/kernel/prom.c > +++ b/arch/powerpc/kernel/prom.c > @@ -641,6 +641,10 @@ void __init early_init_devtree(void *params) > > DBG(" -> early_init_devtree(%p)\n", params); > > + /* Too early to BUG_ON(), do it by hand */ > + if (!early_init_dt_verify(params)) > + panic("BUG: Failed verifying flat device tree, bad version?"); > + > /* Setup flat device-tree pointer */ > initial_boot_params = params; > > -- > 1.9.1 >
On Thu, 2014-08-28 at 09:27 -0500, Rob Herring wrote: > On Thu, Aug 28, 2014 at 3:40 AM, Michael Ellerman <mpe@ellerman.id.au> wrote: > > In commit e6a6928c3ea1 "of/fdt: Convert FDT functions to use libfdt", > > the kernel stopped supporting old flat device tree formats. The minimum > > supported version is now 0x10. > > Ugg. Is that something which needs to be supported? Supporting it at > this point would mean adding support to libfdt. Well it would have been nice to keep supporting v2, dropping it broke kexec-tools for example. But it's done now, so we'll just deal with the fallout. If you can CC linuxppc-dev in future on patches that fundamentally change the device tree parsing that would be appreciated :) cheers
On 29 Aug 2014 02:56, "Michael Ellerman" <mpe@ellerman.id.au> wrote: > > On Thu, 2014-08-28 at 09:27 -0500, Rob Herring wrote: > > On Thu, Aug 28, 2014 at 3:40 AM, Michael Ellerman <mpe@ellerman.id.au> wrote: > > > In commit e6a6928c3ea1 "of/fdt: Convert FDT functions to use libfdt", > > > the kernel stopped supporting old flat device tree formats. The minimum > > > supported version is now 0x10. > > > > Ugg. Is that something which needs to be supported? Supporting it at > > this point would mean adding support to libfdt. > > Well it would have been nice to keep supporting v2, dropping it broke > kexec-tools for example. But it's done now, so we'll just deal with the > fallout. Umm, so we broke userspace? That's not okay. At the very least we should investigate how much work is needed to bring v2 support into libfdt, or up-rev the flat tree at runtime before we parse it. We should be able to update it in-line. IIRC, the main difference between v2 and v0x10 is that only the leaf node name is encoded into the node instead of the full name. We can loop over the tree and truncate all the names while filling the unused bytes with FDT NOP tags. Should be simple. Something like the following: fixup_old_device_tree(blob) { for (offset = fdt_next_node(blob, -1, &depth); offset >= 0 && depth >= 0 && !rc; offset = fdt_next_node(blob, offset, &depth)) { char *pathp = fdt_get_name(blob, offset, NULL); if (*pathp == '/') { char *leaf = kbasename(pathp); int len = strlen(pathp); int newlen = strlen(leaf); strcpy(pathp, leaf); /* copying in place, need to check make sure this code is safe */ node->size = newlen /* fixme - this is just pseudocode */ newlen = FDT_TAGALIGN(newlen); while (newlen < len) { *(pathp + newlen) = FDT_NOP; newlen = FDT_TAGALIGN(newlen+1); } } } } There's probable some more elegance that can be put into the above block, but you get the idea. That could be run in-place without copying the tree to another location, and it could possibly be done in the boot wrapper. Then we'd also be able to get rid of the v2 compatibility code elsewhere in drivers/of/fdt.c because it would already be taken care of. g.
On Fri, Aug 29, 2014 at 6:13 AM, Grant Likely <grant.likely@linaro.org> wrote: > On 29 Aug 2014 02:56, "Michael Ellerman" <mpe@ellerman.id.au> wrote: >> >> On Thu, 2014-08-28 at 09:27 -0500, Rob Herring wrote: >> > On Thu, Aug 28, 2014 at 3:40 AM, Michael Ellerman <mpe@ellerman.id.au> wrote: >> > > In commit e6a6928c3ea1 "of/fdt: Convert FDT functions to use libfdt", >> > > the kernel stopped supporting old flat device tree formats. The minimum >> > > supported version is now 0x10. >> > >> > Ugg. Is that something which needs to be supported? Supporting it at >> > this point would mean adding support to libfdt. >> >> Well it would have been nice to keep supporting v2, dropping it broke >> kexec-tools for example. But it's done now, so we'll just deal with the >> fallout. > > Umm, so we broke userspace? That's not okay. At the very least we > should investigate how much work is needed to bring v2 support into > libfdt, or up-rev the flat tree at runtime before we parse it. Would up-reving work? kexec-tools is broken or booting a new kernel with kexec is broken? > We should be able to update it in-line. IIRC, the main difference > between v2 and v0x10 is that only the leaf node name is encoded into > the node instead of the full name. We can loop over the tree and > truncate all the names while filling the unused bytes with FDT NOP > tags. Should be simple. Something like the following: There is also the name property in v1-v3. I'm guessing linux ignores that already? > > fixup_old_device_tree(blob) > { > for (offset = fdt_next_node(blob, -1, &depth); > offset >= 0 && depth >= 0 && !rc; > offset = fdt_next_node(blob, offset, &depth)) { > char *pathp = fdt_get_name(blob, offset, NULL); > if (*pathp == '/') { > char *leaf = kbasename(pathp); > int len = strlen(pathp); > int newlen = strlen(leaf); > strcpy(pathp, leaf); /* copying in place, need > to check make sure this code is safe */ > node->size = newlen /* fixme - this is just > pseudocode */ > newlen = FDT_TAGALIGN(newlen); > while (newlen < len) { > *(pathp + newlen) = FDT_NOP; > newlen = FDT_TAGALIGN(newlen+1); > } > } > } > > } > > There's probable some more elegance that can be put into the above > block, but you get the idea. That could be run in-place without > copying the tree to another location, and it could possibly be done in > the boot wrapper. Then we'd also be able to get rid of the v2 > compatibility code elsewhere in drivers/of/fdt.c because it would > already be taken care of. That's what got us into this problem. ;) Rob
diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c index 4e139f8a69ef..bb777b255b1c 100644 --- a/arch/powerpc/kernel/prom.c +++ b/arch/powerpc/kernel/prom.c @@ -641,6 +641,10 @@ void __init early_init_devtree(void *params) DBG(" -> early_init_devtree(%p)\n", params); + /* Too early to BUG_ON(), do it by hand */ + if (!early_init_dt_verify(params)) + panic("BUG: Failed verifying flat device tree, bad version?"); + /* Setup flat device-tree pointer */ initial_boot_params = params;
In commit e6a6928c3ea1 "of/fdt: Convert FDT functions to use libfdt", the kernel stopped supporting old flat device tree formats. The minimum supported version is now 0x10. There was a checking function added, early_init_dt_verify(), but it's not called on powerpc. The result is, if you boot with an old flat device tree, the kernel will fail to parse it correctly, think you have no memory etc. and hilarity ensues. We can't really fix it, but we can at least catch the fact that the device tree is in an unsupported format and panic(). We can't call BUG(), it's too early. Signed-off-by: Michael Ellerman <mpe@ellerman.id.au> --- arch/powerpc/kernel/prom.c | 4 ++++ 1 file changed, 4 insertions(+)