| Message ID | 20141111144221.5FCDAC416AF@trevor.secretlab.ca (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 11 November 2014 15:42, Grant Likely <grant.likely@linaro.org> wrote: > On Mon, 10 Nov 2014 19:47:01 +0100 > , Ard Biesheuvel <ard.biesheuvel@linaro.org> > wrote: >> Create a new /sys entry '/sys/firmware/fdt' to export the FDT blob >> that was passed to the kernel by the bootloader. This allows userland >> applications such as kexec to access the raw binary. The blob needs to >> be preserved as early as possible by calling preserve_fdt(). >> >> The fact that this node does not reside under /sys/firmware/device-tree >> is deliberate: FDT is also used on arm64 UEFI/ACPI systems to >> communicate just the UEFI and ACPI entry points, but the FDT is never >> unflattened and used to configure the system. >> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > On further thought, nak. I want tree modifications to be treated as > bugs. Do a checksum CRC32 or check. It's simpler and it can be extended > to us an in-blob CRC when the file format gets upreved to include one. > Ah, right, I was under the impression that tree modifications were expected and allowed. In that case, yes, let me respin to calculate a CRC early on, and WARN() if it gets corrupted.
On Tue, Nov 11, 2014 at 8:42 AM, Grant Likely <grant.likely@linaro.org> wrote: > On Mon, 10 Nov 2014 19:47:01 +0100 > , Ard Biesheuvel <ard.biesheuvel@linaro.org> > wrote: >> Create a new /sys entry '/sys/firmware/fdt' to export the FDT blob >> that was passed to the kernel by the bootloader. This allows userland >> applications such as kexec to access the raw binary. The blob needs to >> be preserved as early as possible by calling preserve_fdt(). >> >> The fact that this node does not reside under /sys/firmware/device-tree >> is deliberate: FDT is also used on arm64 UEFI/ACPI systems to >> communicate just the UEFI and ACPI entry points, but the FDT is never >> unflattened and used to configure the system. >> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > On further thought, nak. I want tree modifications to be treated as > bugs. Do a checksum CRC32 or check. It's simpler and it can be extended > to us an in-blob CRC when the file format gets upreved to include one. Why? MIPS Octeon is full of them BTW. So the rule is any modifications or fixups have to be on the unflattened tree? For the CRC, we should add it into the entropy pool. Rob > > g. > > --- > > diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig > index be16ce2ffd69..adda0a16934f 100644 > --- a/drivers/of/Kconfig > +++ b/drivers/of/Kconfig > @@ -23,6 +23,7 @@ config OF_FLATTREE > bool > select DTC > select LIBFDT > + select CRC > > config OF_EARLY_FLATTREE > bool > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c > index 83a8e1154602..80db46a2eab9 100644 > --- a/drivers/of/fdt.c > +++ b/drivers/of/fdt.c > @@ -10,6 +10,7 @@ > */ > > #include <linux/kernel.h> > +#include <linux/crc32.h> > #include <linux/initrd.h> > #include <linux/memblock.h> > #include <linux/of.h> > @@ -420,6 +421,7 @@ int __initdata dt_root_addr_cells; > int __initdata dt_root_size_cells; > > void *initial_boot_params; > +u32 initial_boot_params_crc; > > #ifdef CONFIG_OF_EARLY_FLATTREE > > @@ -1003,6 +1005,9 @@ bool __init early_init_dt_verify(void *params) > > /* Setup flat device-tree pointer */ > initial_boot_params = params; > + initial_boot_params_crc = crc32_be(0, params, fdt_totalsize(params)); > + pr_info("FDT CRC: %x\n", initial_boot_params_crc); > + > return true; > } > > > > >> --- >> drivers/of/fdt.c | 34 ++++++++++++++++++++++++++++++++++ >> include/linux/of_fdt.h | 2 ++ >> 2 files changed, 36 insertions(+) >> >> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c >> index d1ffca8b34ea..e9ee3d5f7ea4 100644 >> --- a/drivers/of/fdt.c >> +++ b/drivers/of/fdt.c >> @@ -22,6 +22,7 @@ >> #include <linux/libfdt.h> >> #include <linux/debugfs.h> >> #include <linux/serial_core.h> >> +#include <linux/sysfs.h> >> >> #include <asm/setup.h> /* for COMMAND_LINE_SIZE */ >> #include <asm/page.h> >> @@ -1103,4 +1104,37 @@ static int __init of_flat_dt_debugfs_export_fdt(void) >> module_init(of_flat_dt_debugfs_export_fdt); >> #endif >> >> +static u8 *raw_fdt_copy; >> + >> +void __init preserve_fdt(void) >> +{ >> + u32 fdt_size; >> + >> + fdt_size = fdt_totalsize(initial_boot_params); >> + raw_fdt_copy = memcpy(__va(memblock_alloc(fdt_size, PAGE_SIZE)), >> + initial_boot_params, fdt_size); >> +} >> + >> +#ifdef CONFIG_SYSFS >> +static ssize_t of_fdt_raw_read(struct file *filp, struct kobject *kobj, >> + struct bin_attribute *bin_attr, >> + char *buf, loff_t off, size_t count) >> +{ >> + memcpy(buf, raw_fdt_copy + off, count); >> + return count; >> +} >> + >> +static int __init of_fdt_raw_init(void) >> +{ >> + static struct bin_attribute of_fdt_raw_attr = >> + __BIN_ATTR(fdt, S_IRUSR, of_fdt_raw_read, NULL, 0); >> + >> + if (!raw_fdt_copy) >> + return 0; >> + of_fdt_raw_attr.size = fdt_totalsize(raw_fdt_copy); >> + return sysfs_create_bin_file(firmware_kobj, &of_fdt_raw_attr); >> +} >> +module_init(of_fdt_raw_init); >> +#endif >> + >> #endif /* CONFIG_OF_EARLY_FLATTREE */ >> diff --git a/include/linux/of_fdt.h b/include/linux/of_fdt.h >> index 0ff360d5b3b3..7672a26305a5 100644 >> --- a/include/linux/of_fdt.h >> +++ b/include/linux/of_fdt.h >> @@ -83,6 +83,7 @@ extern const void *of_flat_dt_match_machine(const void *default_match, >> /* Other Prototypes */ >> extern void unflatten_device_tree(void); >> extern void unflatten_and_copy_device_tree(void); >> +extern void preserve_fdt(void); >> extern void early_init_devtree(void *); >> extern void early_get_first_memblock_info(void *, phys_addr_t *); >> extern u64 fdt_translate_address(const void *blob, int node_offset); >> @@ -92,6 +93,7 @@ static inline void early_init_fdt_scan_reserved_mem(void) {} >> static inline const char *of_flat_dt_get_machine_name(void) { return NULL; } >> static inline void unflatten_device_tree(void) {} >> static inline void unflatten_and_copy_device_tree(void) {} >> +static inline void preserve_fdt(void) {} >> #endif /* CONFIG_OF_FLATTREE */ >> >> #endif /* __ASSEMBLY__ */ >> -- >> 1.8.3.2 >> > > -- > To unsubscribe from this list: send the line "unsubscribe devicetree" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Nov 11, 2014 at 4:25 PM, Rob Herring <robherring2@gmail.com> wrote: > On Tue, Nov 11, 2014 at 8:42 AM, Grant Likely <grant.likely@linaro.org> wrote: >> On Mon, 10 Nov 2014 19:47:01 +0100 >> , Ard Biesheuvel <ard.biesheuvel@linaro.org> >> wrote: >>> Create a new /sys entry '/sys/firmware/fdt' to export the FDT blob >>> that was passed to the kernel by the bootloader. This allows userland >>> applications such as kexec to access the raw binary. The blob needs to >>> be preserved as early as possible by calling preserve_fdt(). >>> >>> The fact that this node does not reside under /sys/firmware/device-tree >>> is deliberate: FDT is also used on arm64 UEFI/ACPI systems to >>> communicate just the UEFI and ACPI entry points, but the FDT is never >>> unflattened and used to configure the system. >>> >>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> >> On further thought, nak. I want tree modifications to be treated as >> bugs. Do a checksum CRC32 or check. It's simpler and it can be extended >> to us an in-blob CRC when the file format gets upreved to include one. > > Why? MIPS Octeon is full of them BTW. So the rule is any modifications > or fixups have to be on the unflattened tree? That, or we require the fixups to explicitly clean up the CRC after themselves. That will force them to be visible. Anything using the fdt_*() write functions could potentially take care of it automatically. That would make it immediately discoverable where changes are being made in the tree. g.
On 11 November 2014 21:08, Grant Likely <grant.likely@linaro.org> wrote: > On Tue, Nov 11, 2014 at 4:25 PM, Rob Herring <robherring2@gmail.com> wrote: >> On Tue, Nov 11, 2014 at 8:42 AM, Grant Likely <grant.likely@linaro.org> wrote: >>> On Mon, 10 Nov 2014 19:47:01 +0100 >>> , Ard Biesheuvel <ard.biesheuvel@linaro.org> >>> wrote: >>>> Create a new /sys entry '/sys/firmware/fdt' to export the FDT blob >>>> that was passed to the kernel by the bootloader. This allows userland >>>> applications such as kexec to access the raw binary. The blob needs to >>>> be preserved as early as possible by calling preserve_fdt(). >>>> >>>> The fact that this node does not reside under /sys/firmware/device-tree >>>> is deliberate: FDT is also used on arm64 UEFI/ACPI systems to >>>> communicate just the UEFI and ACPI entry points, but the FDT is never >>>> unflattened and used to configure the system. >>>> >>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >>> >>> On further thought, nak. I want tree modifications to be treated as >>> bugs. Do a checksum CRC32 or check. It's simpler and it can be extended >>> to us an in-blob CRC when the file format gets upreved to include one. >> >> Why? MIPS Octeon is full of them BTW. So the rule is any modifications >> or fixups have to be on the unflattened tree? > > That, or we require the fixups to explicitly clean up the CRC after > themselves. That will force them to be visible. Anything using the > fdt_*() write functions could potentially take care of it > automatically. That would make it immediately discoverable where > changes are being made in the tree. > Doesn't that defeat the purpose of capturing the FDT as it was passed by the bootloader? Those fixups will be reapplied by the kexec'ed kernel anyway.
On Tue, Nov 11, 2014 at 8:34 PM, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > On 11 November 2014 21:08, Grant Likely <grant.likely@linaro.org> wrote: >> On Tue, Nov 11, 2014 at 4:25 PM, Rob Herring <robherring2@gmail.com> wrote: >>> On Tue, Nov 11, 2014 at 8:42 AM, Grant Likely <grant.likely@linaro.org> wrote: >>>> On Mon, 10 Nov 2014 19:47:01 +0100 >>>> , Ard Biesheuvel <ard.biesheuvel@linaro.org> >>>> wrote: >>>>> Create a new /sys entry '/sys/firmware/fdt' to export the FDT blob >>>>> that was passed to the kernel by the bootloader. This allows userland >>>>> applications such as kexec to access the raw binary. The blob needs to >>>>> be preserved as early as possible by calling preserve_fdt(). >>>>> >>>>> The fact that this node does not reside under /sys/firmware/device-tree >>>>> is deliberate: FDT is also used on arm64 UEFI/ACPI systems to >>>>> communicate just the UEFI and ACPI entry points, but the FDT is never >>>>> unflattened and used to configure the system. >>>>> >>>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >>>> >>>> On further thought, nak. I want tree modifications to be treated as >>>> bugs. Do a checksum CRC32 or check. It's simpler and it can be extended >>>> to us an in-blob CRC when the file format gets upreved to include one. >>> >>> Why? MIPS Octeon is full of them BTW. So the rule is any modifications >>> or fixups have to be on the unflattened tree? >> >> That, or we require the fixups to explicitly clean up the CRC after >> themselves. That will force them to be visible. Anything using the >> fdt_*() write functions could potentially take care of it >> automatically. That would make it immediately discoverable where >> changes are being made in the tree. >> > > Doesn't that defeat the purpose of capturing the FDT as it was passed > by the bootloader? Those fixups will be reapplied by the kexec'ed > kernel anyway. We're always going to be in a grey area here. There are some things that should not be passed over. For example, a framebuffer that is passed in from firmware will no longer be valid on kexec. Right now on arch/arm, the exynos platform fixes up the DT because the memory node is outright /corrupt/. In the MIPS case, they do a bunch of stuff in the kernel to figure out the hardware as configured by firmware. I don't agree with that approach, but I would be very surprised if the full original DT has any validity after the early boot fixups. Early boot fixups to the .dtb are always going to be oddball cases. g.
On 12 November 2014 12:45, Grant Likely <grant.likely@linaro.org> wrote: > On Tue, Nov 11, 2014 at 8:34 PM, Ard Biesheuvel > <ard.biesheuvel@linaro.org> wrote: >> On 11 November 2014 21:08, Grant Likely <grant.likely@linaro.org> wrote: >>> On Tue, Nov 11, 2014 at 4:25 PM, Rob Herring <robherring2@gmail.com> wrote: >>>> On Tue, Nov 11, 2014 at 8:42 AM, Grant Likely <grant.likely@linaro.org> wrote: >>>>> On Mon, 10 Nov 2014 19:47:01 +0100 >>>>> , Ard Biesheuvel <ard.biesheuvel@linaro.org> >>>>> wrote: >>>>>> Create a new /sys entry '/sys/firmware/fdt' to export the FDT blob >>>>>> that was passed to the kernel by the bootloader. This allows userland >>>>>> applications such as kexec to access the raw binary. The blob needs to >>>>>> be preserved as early as possible by calling preserve_fdt(). >>>>>> >>>>>> The fact that this node does not reside under /sys/firmware/device-tree >>>>>> is deliberate: FDT is also used on arm64 UEFI/ACPI systems to >>>>>> communicate just the UEFI and ACPI entry points, but the FDT is never >>>>>> unflattened and used to configure the system. >>>>>> >>>>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >>>>> >>>>> On further thought, nak. I want tree modifications to be treated as >>>>> bugs. Do a checksum CRC32 or check. It's simpler and it can be extended >>>>> to us an in-blob CRC when the file format gets upreved to include one. >>>> >>>> Why? MIPS Octeon is full of them BTW. So the rule is any modifications >>>> or fixups have to be on the unflattened tree? >>> >>> That, or we require the fixups to explicitly clean up the CRC after >>> themselves. That will force them to be visible. Anything using the >>> fdt_*() write functions could potentially take care of it >>> automatically. That would make it immediately discoverable where >>> changes are being made in the tree. >>> >> >> Doesn't that defeat the purpose of capturing the FDT as it was passed >> by the bootloader? Those fixups will be reapplied by the kexec'ed >> kernel anyway. > > We're always going to be in a grey area here. There are some things > that should not be passed over. For example, a framebuffer that is > passed in from firmware will no longer be valid on kexec. Right now on > arch/arm, the exynos platform fixes up the DT because the memory node > is outright /corrupt/. In the MIPS case, they do a bunch of stuff in > the kernel to figure out the hardware as configured by firmware. I > don't agree with that approach, but I would be very surprised if the > full original DT has any validity after the early boot fixups. > Well, yes, but those fixups will presumably be done on every boot, including the kexec ones. > Early boot fixups to the .dtb are always going to be oddball cases. > OK, so generally speaking, whether the original DTB is more suitable to be presented through /sys/firmware/fdt than the 'current' FDT (i.e., whatever is in memory at the time the sysfs node is accessed) is not obvious at all. So why not just do the latter?
On Wed, Nov 12, 2014 at 11:51 AM, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > On 12 November 2014 12:45, Grant Likely <grant.likely@linaro.org> wrote: >> On Tue, Nov 11, 2014 at 8:34 PM, Ard Biesheuvel >> <ard.biesheuvel@linaro.org> wrote: >>> On 11 November 2014 21:08, Grant Likely <grant.likely@linaro.org> wrote: >>>> On Tue, Nov 11, 2014 at 4:25 PM, Rob Herring <robherring2@gmail.com> wrote: >>>>> On Tue, Nov 11, 2014 at 8:42 AM, Grant Likely <grant.likely@linaro.org> wrote: >>>>>> On Mon, 10 Nov 2014 19:47:01 +0100 >>>>>> , Ard Biesheuvel <ard.biesheuvel@linaro.org> >>>>>> wrote: >>>>>>> Create a new /sys entry '/sys/firmware/fdt' to export the FDT blob >>>>>>> that was passed to the kernel by the bootloader. This allows userland >>>>>>> applications such as kexec to access the raw binary. The blob needs to >>>>>>> be preserved as early as possible by calling preserve_fdt(). >>>>>>> >>>>>>> The fact that this node does not reside under /sys/firmware/device-tree >>>>>>> is deliberate: FDT is also used on arm64 UEFI/ACPI systems to >>>>>>> communicate just the UEFI and ACPI entry points, but the FDT is never >>>>>>> unflattened and used to configure the system. >>>>>>> >>>>>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >>>>>> >>>>>> On further thought, nak. I want tree modifications to be treated as >>>>>> bugs. Do a checksum CRC32 or check. It's simpler and it can be extended >>>>>> to us an in-blob CRC when the file format gets upreved to include one. >>>>> >>>>> Why? MIPS Octeon is full of them BTW. So the rule is any modifications >>>>> or fixups have to be on the unflattened tree? >>>> >>>> That, or we require the fixups to explicitly clean up the CRC after >>>> themselves. That will force them to be visible. Anything using the >>>> fdt_*() write functions could potentially take care of it >>>> automatically. That would make it immediately discoverable where >>>> changes are being made in the tree. >>>> >>> >>> Doesn't that defeat the purpose of capturing the FDT as it was passed >>> by the bootloader? Those fixups will be reapplied by the kexec'ed >>> kernel anyway. >> >> We're always going to be in a grey area here. There are some things >> that should not be passed over. For example, a framebuffer that is >> passed in from firmware will no longer be valid on kexec. Right now on >> arch/arm, the exynos platform fixes up the DT because the memory node >> is outright /corrupt/. In the MIPS case, they do a bunch of stuff in >> the kernel to figure out the hardware as configured by firmware. I >> don't agree with that approach, but I would be very surprised if the >> full original DT has any validity after the early boot fixups. >> > > Well, yes, but those fixups will presumably be done on every boot, > including the kexec ones. > >> Early boot fixups to the .dtb are always going to be oddball cases. >> > > OK, so generally speaking, whether the original DTB is more suitable > to be presented through /sys/firmware/fdt than the 'current' FDT > (i.e., whatever is in memory at the time the sysfs node is accessed) > is not obvious at all. So why not just do the latter? We're now talking theoreticals. In the case you're working on (ACPI+FDT), the FDT is generated by Grub or the EFI stub as an intermediary format, and there isn't currently any code in your boot path that modifies the FDT. You're arguing for a future situation where the kernel makes a fixup that is not wanted in the second kernel. I'm arguing that 1) I want to /know/ when that happens because there are very few situations where it is a valid thing to do, and 2) the likelyhood of that happening in the FDT+ACPI case is somewhere between slim and none because you're working with an intermediary FDT, not a real hardware description. /If/ it ever becomes a problem we can revisit, until then I still prefer the checksum method. g.
On 12 November 2014 13:01, Grant Likely <grant.likely@linaro.org> wrote: > On Wed, Nov 12, 2014 at 11:51 AM, Ard Biesheuvel > <ard.biesheuvel@linaro.org> wrote: >> On 12 November 2014 12:45, Grant Likely <grant.likely@linaro.org> wrote: >>> On Tue, Nov 11, 2014 at 8:34 PM, Ard Biesheuvel >>> <ard.biesheuvel@linaro.org> wrote: >>>> On 11 November 2014 21:08, Grant Likely <grant.likely@linaro.org> wrote: >>>>> On Tue, Nov 11, 2014 at 4:25 PM, Rob Herring <robherring2@gmail.com> wrote: >>>>>> On Tue, Nov 11, 2014 at 8:42 AM, Grant Likely <grant.likely@linaro.org> wrote: >>>>>>> On Mon, 10 Nov 2014 19:47:01 +0100 >>>>>>> , Ard Biesheuvel <ard.biesheuvel@linaro.org> >>>>>>> wrote: >>>>>>>> Create a new /sys entry '/sys/firmware/fdt' to export the FDT blob >>>>>>>> that was passed to the kernel by the bootloader. This allows userland >>>>>>>> applications such as kexec to access the raw binary. The blob needs to >>>>>>>> be preserved as early as possible by calling preserve_fdt(). >>>>>>>> >>>>>>>> The fact that this node does not reside under /sys/firmware/device-tree >>>>>>>> is deliberate: FDT is also used on arm64 UEFI/ACPI systems to >>>>>>>> communicate just the UEFI and ACPI entry points, but the FDT is never >>>>>>>> unflattened and used to configure the system. >>>>>>>> >>>>>>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >>>>>>> >>>>>>> On further thought, nak. I want tree modifications to be treated as >>>>>>> bugs. Do a checksum CRC32 or check. It's simpler and it can be extended >>>>>>> to us an in-blob CRC when the file format gets upreved to include one. >>>>>> >>>>>> Why? MIPS Octeon is full of them BTW. So the rule is any modifications >>>>>> or fixups have to be on the unflattened tree? >>>>> >>>>> That, or we require the fixups to explicitly clean up the CRC after >>>>> themselves. That will force them to be visible. Anything using the >>>>> fdt_*() write functions could potentially take care of it >>>>> automatically. That would make it immediately discoverable where >>>>> changes are being made in the tree. >>>>> >>>> >>>> Doesn't that defeat the purpose of capturing the FDT as it was passed >>>> by the bootloader? Those fixups will be reapplied by the kexec'ed >>>> kernel anyway. >>> >>> We're always going to be in a grey area here. There are some things >>> that should not be passed over. For example, a framebuffer that is >>> passed in from firmware will no longer be valid on kexec. Right now on >>> arch/arm, the exynos platform fixes up the DT because the memory node >>> is outright /corrupt/. In the MIPS case, they do a bunch of stuff in >>> the kernel to figure out the hardware as configured by firmware. I >>> don't agree with that approach, but I would be very surprised if the >>> full original DT has any validity after the early boot fixups. >>> >> >> Well, yes, but those fixups will presumably be done on every boot, >> including the kexec ones. >> >>> Early boot fixups to the .dtb are always going to be oddball cases. >>> >> >> OK, so generally speaking, whether the original DTB is more suitable >> to be presented through /sys/firmware/fdt than the 'current' FDT >> (i.e., whatever is in memory at the time the sysfs node is accessed) >> is not obvious at all. So why not just do the latter? > > We're now talking theoreticals. In the case you're working on > (ACPI+FDT), the FDT is generated by Grub or the EFI stub as an > intermediary format, and there isn't currently any code in your boot > path that modifies the FDT. You're arguing for a future situation > where the kernel makes a fixup that is not wanted in the second > kernel. I'm arguing that 1) I want to /know/ when that happens because > there are very few situations where it is a valid thing to do, and 2) > the likelyhood of that happening in the FDT+ACPI case is somewhere > between slim and none because you're working with an intermediary FDT, > not a real hardware description. > But do you still want the warning? Or only if you access the sysfs node? Or just a pr_warn() perhaps rather than a full blown WARN() ? MIPS will start seeing the warning as well even if they don't care about what is in /sys/firmware/fdt
On Wed, Nov 12, 2014 at 12:08 PM, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > On 12 November 2014 13:01, Grant Likely <grant.likely@linaro.org> wrote: >> On Wed, Nov 12, 2014 at 11:51 AM, Ard Biesheuvel >> <ard.biesheuvel@linaro.org> wrote: >>> On 12 November 2014 12:45, Grant Likely <grant.likely@linaro.org> wrote: >>>> On Tue, Nov 11, 2014 at 8:34 PM, Ard Biesheuvel >>>> <ard.biesheuvel@linaro.org> wrote: >>>>> On 11 November 2014 21:08, Grant Likely <grant.likely@linaro.org> wrote: >>>>>> On Tue, Nov 11, 2014 at 4:25 PM, Rob Herring <robherring2@gmail.com> wrote: >>>>>>> On Tue, Nov 11, 2014 at 8:42 AM, Grant Likely <grant.likely@linaro.org> wrote: >>>>>>>> On Mon, 10 Nov 2014 19:47:01 +0100 >>>>>>>> , Ard Biesheuvel <ard.biesheuvel@linaro.org> >>>>>>>> wrote: >>>>>>>>> Create a new /sys entry '/sys/firmware/fdt' to export the FDT blob >>>>>>>>> that was passed to the kernel by the bootloader. This allows userland >>>>>>>>> applications such as kexec to access the raw binary. The blob needs to >>>>>>>>> be preserved as early as possible by calling preserve_fdt(). >>>>>>>>> >>>>>>>>> The fact that this node does not reside under /sys/firmware/device-tree >>>>>>>>> is deliberate: FDT is also used on arm64 UEFI/ACPI systems to >>>>>>>>> communicate just the UEFI and ACPI entry points, but the FDT is never >>>>>>>>> unflattened and used to configure the system. >>>>>>>>> >>>>>>>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >>>>>>>> >>>>>>>> On further thought, nak. I want tree modifications to be treated as >>>>>>>> bugs. Do a checksum CRC32 or check. It's simpler and it can be extended >>>>>>>> to us an in-blob CRC when the file format gets upreved to include one. >>>>>>> >>>>>>> Why? MIPS Octeon is full of them BTW. So the rule is any modifications >>>>>>> or fixups have to be on the unflattened tree? >>>>>> >>>>>> That, or we require the fixups to explicitly clean up the CRC after >>>>>> themselves. That will force them to be visible. Anything using the >>>>>> fdt_*() write functions could potentially take care of it >>>>>> automatically. That would make it immediately discoverable where >>>>>> changes are being made in the tree. >>>>>> >>>>> >>>>> Doesn't that defeat the purpose of capturing the FDT as it was passed >>>>> by the bootloader? Those fixups will be reapplied by the kexec'ed >>>>> kernel anyway. >>>> >>>> We're always going to be in a grey area here. There are some things >>>> that should not be passed over. For example, a framebuffer that is >>>> passed in from firmware will no longer be valid on kexec. Right now on >>>> arch/arm, the exynos platform fixes up the DT because the memory node >>>> is outright /corrupt/. In the MIPS case, they do a bunch of stuff in >>>> the kernel to figure out the hardware as configured by firmware. I >>>> don't agree with that approach, but I would be very surprised if the >>>> full original DT has any validity after the early boot fixups. >>>> >>> >>> Well, yes, but those fixups will presumably be done on every boot, >>> including the kexec ones. >>> >>>> Early boot fixups to the .dtb are always going to be oddball cases. >>>> >>> >>> OK, so generally speaking, whether the original DTB is more suitable >>> to be presented through /sys/firmware/fdt than the 'current' FDT >>> (i.e., whatever is in memory at the time the sysfs node is accessed) >>> is not obvious at all. So why not just do the latter? >> >> We're now talking theoreticals. In the case you're working on >> (ACPI+FDT), the FDT is generated by Grub or the EFI stub as an >> intermediary format, and there isn't currently any code in your boot >> path that modifies the FDT. You're arguing for a future situation >> where the kernel makes a fixup that is not wanted in the second >> kernel. I'm arguing that 1) I want to /know/ when that happens because >> there are very few situations where it is a valid thing to do, and 2) >> the likelyhood of that happening in the FDT+ACPI case is somewhere >> between slim and none because you're working with an intermediary FDT, >> not a real hardware description. >> > > But do you still want the warning? Or only if you access the sysfs > node? Or just a pr_warn() perhaps rather than a full blown WARN() ? > MIPS will start seeing the warning as well even if they don't care > about what is in /sys/firmware/fdt pr_warn() and then don't export the file in /sys/firmware/fdt. I'm okay with the check being done once at the end of initcalls. g.
diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig index be16ce2ffd69..adda0a16934f 100644 --- a/drivers/of/Kconfig +++ b/drivers/of/Kconfig @@ -23,6 +23,7 @@ config OF_FLATTREE bool select DTC select LIBFDT + select CRC config OF_EARLY_FLATTREE bool diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c index 83a8e1154602..80db46a2eab9 100644 --- a/drivers/of/fdt.c +++ b/drivers/of/fdt.c @@ -10,6 +10,7 @@ */ #include <linux/kernel.h> +#include <linux/crc32.h> #include <linux/initrd.h> #include <linux/memblock.h> #include <linux/of.h> @@ -420,6 +421,7 @@ int __initdata dt_root_addr_cells; int __initdata dt_root_size_cells; void *initial_boot_params; +u32 initial_boot_params_crc; #ifdef CONFIG_OF_EARLY_FLATTREE @@ -1003,6 +1005,9 @@ bool __init early_init_dt_verify(void *params) /* Setup flat device-tree pointer */ initial_boot_params = params; + initial_boot_params_crc = crc32_be(0, params, fdt_totalsize(params)); + pr_info("FDT CRC: %x\n", initial_boot_params_crc); + return true; }