[U-Boot] [PATCH 1/2] fdt: introduce fdt_verify_alias_address() and fdt_get_base_address()

[U-Boot] [PATCH 1/2] fdt: introduce fdt_verify_alias_address() and fdt_get_base_address()

[Timur Tabi]

Introduce two functions, fdt_verify_alias_address() and fdt_get_base_address(),
which can be used to verify the physical address of a device in a device tree.

fdt_get_base_address() returns the base address of an SOC or PCI node.

fdt_verify_alias_address() prints a message if the address of a node specified
by an alias does not match the given physical address.

Signed-off-by: Timur Tabi <timur@freescale.com>
---
 common/fdt_support.c  |   54 +++++++++++++++++++++++++++++++++++++++++++++++++
 include/fdt_support.h |    4 +++
 2 files changed, 58 insertions(+), 0 deletions(-)

diff --git a/common/fdt_support.c b/common/fdt_support.c
index 496040b..4075ade 100644
--- a/common/fdt_support.c
+++ b/common/fdt_support.c
@@ -1223,3 +1223,57 @@ err_size:
 	return ret;
 }
 #endif
+
+/*
+ * Verify the physical address of device tree node for a given alias
+ *
+ * This function locates the device tree node of a given alias, and then
+ * verifies that the physical address of that device matches the given
+ * parameter.  It displays a message if there is a mismatch.
+ */
+void fdt_verify_alias_address(void *fdt, int anode, const char *alias, u64 addr)
+{
+	const char *path;
+	const u32 *reg;
+	int node, len;
+	u64 dt_addr;
+
+	path = fdt_getprop(fdt, anode, alias, NULL);
+	if (!path)
+		return;
+
+	node = fdt_path_offset(fdt, path);
+	if (node < 0)
+		return;
+
+	reg = fdt_getprop(fdt, node, "reg", &len);
+	if (!reg)
+		return;
+
+	dt_addr = fdt_translate_address(fdt, node, reg);
+	if (addr != dt_addr) {
+		printf("Warning: U-Boot configured device %s@address %llx,\n"
+		       "but the device tree has it address %llx.\n",
+		       alias, addr, dt_addr);
+	}
+}
+
+/*
+ * Returns the base address of an SOC or PCI node
+ */
+u64 fdt_get_base_address(void *fdt, int node)
+{
+	int size;
+	u32 naddr;
+	const u32 *prop;
+
+	prop = fdt_getprop(fdt, node, "#address-cells", &size);
+	if (prop && size == 4)
+		naddr = *prop;
+	else
+		naddr = 2;
+
+	prop = fdt_getprop(fdt, node, "ranges", &size);
+
+	return prop ? fdt_translate_address(fdt, node, prop + naddr) : 0;
+}
diff --git a/include/fdt_support.h b/include/fdt_support.h
index ce6817b..3709615 100644
--- a/include/fdt_support.h
+++ b/include/fdt_support.h
@@ -90,5 +90,9 @@ int fdt_node_offset_by_compat_reg(void *blob, const char *compat,
 int fdt_alloc_phandle(void *blob);
 int fdt_add_edid(void *blob, const char *compat, unsigned char *buf);
 
+void fdt_verify_alias_address(void *fdt, int anode, const char *alias,
+			      u64 addr);
+u64 fdt_get_base_address(void *fdt, int node);
+
 #endif /* ifdef CONFIG_OF_LIBFDT */
 #endif /* ifndef __FDT_SUPPORT_H */
-- 
1.7.3.4

[U-Boot] [PATCH 2/2] powerpc/85xx: verify the device tree before booting Linux

[Timur Tabi]

Introduce ft_verify_fdt(), a function that is called after the device tree
has been fixed up, to display warning messages if there is a mismatch between
the physical addresses of some devices that U-Boot has configured with what
the device tree says the addresses are.

This is a particular problem if when booting a 36-bit device tree from a 32-bit
U-Boot (or vice versa), because the physical address of CCSR and the serial
devices are wrong in the device tree.  When the operating system boots, no
messages are displayed, the so the user generally has no idea what's wrong.

Signed-off-by: Timur Tabi <timur@freescale.com>
---

At Kumar's request, this patch replaces "[v3] powerpc/85xx: introduce
'fdt verify' command".  The PCI checking has been dropped, because Kumar wants
PCI addresses to be fixed up, not verified (TBD in a future patch).  Also,
this code is now run on every boot via a weak function, rather than an 'fdt'
command.  It never really made much sense to make this a command-line command,
because running fdt commands is clunky.

 arch/powerpc/cpu/mpc85xx/fdt.c |   67 ++++++++++++++++++++++++++++++++++++++++
 arch/powerpc/lib/bootm.c       |   19 +++++++++++
 2 files changed, 86 insertions(+), 0 deletions(-)

diff --git a/arch/powerpc/cpu/mpc85xx/fdt.c b/arch/powerpc/cpu/mpc85xx/fdt.c
index 6e909b5..841f32c 100644
--- a/arch/powerpc/cpu/mpc85xx/fdt.c
+++ b/arch/powerpc/cpu/mpc85xx/fdt.c
@@ -497,3 +497,70 @@ void ft_cpu_setup(void *blob, bd_t *bd)
 	do_fixup_by_compat_u32(blob, "fsl,gianfar-ptp-timer",
 			"timer-frequency", gd->bus_clk/2, 1);
 }
+
+/*
+ * For some CCSR devices, we only have the virtual address, not the physical
+ * address.  This is because we map CCSR as a whole, so we typically don't need
+ * a macro for the physical address of any device within CCSR.  In this case,
+ * we calculate the physical address of that device using it's the difference
+ * between the virtual address of the device and the virtual address of the
+ * beginning of CCSR.
+ */
+#define CCSR_VIRT_TO_PHYS(x) \
+	(CONFIG_SYS_CCSRBAR_PHYS + ((x) - CONFIG_SYS_CCSRBAR))
+
+/*
+ * Verify the device tree
+ *
+ * This function compares several CONFIG_xxx macros that contain physical
+ * addresses with the corresponding nodes in the device tree, to see if
+ * the physical addresses are all correct.  For example, if
+ * CONFIG_SYS_NS16550_COM1 is defined, then it contains the virtual address
+ * of the first UART.  We convert this to a physical address and compare
+ * that with the physical address of the first ns16550-compatible node
+ * in the device tree.  If they don't match, then we display a warning.
+ */
+void ft_verify_fdt(void *fdt)
+{
+	uint64_t ccsr = 0;
+	int aliases;
+	int off;
+
+	/* First check the CCSR base address */
+	off = fdt_node_offset_by_prop_value(fdt, -1, "device_type", "soc", 4);
+	if (off > 0)
+		ccsr = fdt_get_base_address(fdt, off);
+
+	if (!ccsr) {
+		printf("Warning: could not determine base CCSR address in "
+		       "device tree\n");
+		/* No point in checking anything else */
+		return;
+	}
+
+	if (ccsr != CONFIG_SYS_CCSRBAR_PHYS) {
+		printf("Warning: U-Boot configured CCSR at address %llx,\n"
+		       "but the device tree has it at %llx\n",
+		       (uint64_t) CONFIG_SYS_CCSRBAR_PHYS, ccsr);
+		/* No point in checking anything else */
+		return;
+	}
+
+	/*
+	 * Get the 'aliases' node.  If there isn't one, then there's nothing
+	 * left to do.
+	 */
+	aliases = fdt_path_offset(fdt, "/aliases");
+	if (aliases > 0) {
+#ifdef CONFIG_SYS_NS16550_COM1
+		fdt_verify_alias_address(fdt, aliases, "serial0",
+			CCSR_VIRT_TO_PHYS(CONFIG_SYS_NS16550_COM1));
+#endif
+
+#ifdef CONFIG_SYS_NS16550_COM2
+		fdt_verify_alias_address(fdt, aliases, "serial1",
+			CCSR_VIRT_TO_PHYS(CONFIG_SYS_NS16550_COM2));
+#endif
+	}
+
+}
diff --git a/arch/powerpc/lib/bootm.c b/arch/powerpc/lib/bootm.c
index e01787d..53b9b27 100644
--- a/arch/powerpc/lib/bootm.c
+++ b/arch/powerpc/lib/bootm.c
@@ -226,6 +226,23 @@ static int boot_bd_t_linux(bootm_headers_t *images)
 	return ret;
 }
 
+/*
+ * Verify the device tree.
+ *
+ * This function is called after all device tree fix-ups have been enacted,
+ * so that the final device tree can be verified.  The definition of "verified"
+ * is up to the specific implementation.  However, it generally means that the
+ * addresses of some of the devices in the device tree are compared with the
+ * actual addresses at which U-Boot has placed them.
+ *
+ * The function should display warning messages for any problems it finds.
+ * U-Boot will still attempt to boot the operating system, however.
+ */
+static void __ft_verify_fdt(void *fdt)
+{
+}
+__attribute__((weak, alias("__ft_verify_fdt"))) void ft_verify_fdt(void *fdt);
+
 static int boot_body_linux(bootm_headers_t *images)
 {
 	ulong rd_len;
@@ -296,6 +313,8 @@ static int boot_body_linux(bootm_headers_t *images)
 		/* fixup the initrd now that we know where it should be */
 		if (*initrd_start && *initrd_end)
 			fdt_initrd(*of_flat_tree, *initrd_start, *initrd_end, 1);
+
+		ft_verify_fdt(*of_flat_tree);
 	}
 #endif	/* CONFIG_OF_LIBFDT */
 	return 0;
-- 
1.7.3.4

[U-Boot] [PATCH 2/2] powerpc/85xx: verify the device tree before booting Linux

[Kumar Gala]

> diff --git a/arch/powerpc/lib/bootm.c b/arch/powerpc/lib/bootm.c
> index e01787d..53b9b27 100644
> --- a/arch/powerpc/lib/bootm.c
> +++ b/arch/powerpc/lib/bootm.c
> @@ -226,6 +226,23 @@ static int boot_bd_t_linux(bootm_headers_t *images)
> 	return ret;
> }
> 
> +/*
> + * Verify the device tree.
> + *
> + * This function is called after all device tree fix-ups have been enacted,
> + * so that the final device tree can be verified.  The definition of "verified"
> + * is up to the specific implementation.  However, it generally means that the
> + * addresses of some of the devices in the device tree are compared with the
> + * actual addresses at which U-Boot has placed them.
> + *
> + * The function should display warning messages for any problems it finds.
> + * U-Boot will still attempt to boot the operating system, however.
> + */
> +static void __ft_verify_fdt(void *fdt)
> +{
> +}
> +__attribute__((weak, alias("__ft_verify_fdt"))) void ft_verify_fdt(void *fdt);
> +
> static int boot_body_linux(bootm_headers_t *images)
> {
> 	ulong rd_len;
> @@ -296,6 +313,8 @@ static int boot_body_linux(bootm_headers_t *images)
> 		/* fixup the initrd now that we know where it should be */
> 		if (*initrd_start && *initrd_end)
> 			fdt_initrd(*of_flat_tree, *initrd_start, *initrd_end, 1);
> +
> +		ft_verify_fdt(*of_flat_tree);

Do we not want to error out here if verify fails?

> 	}
> #endif	/* CONFIG_OF_LIBFDT */
> 	return 0;
> -- 
> 1.7.3.4

[U-Boot] [PATCH 2/2] powerpc/85xx: verify the device tree before booting Linux

[Timur Tabi]

On May 3, 2011, at 9:21 AM, Kumar Gala <galak@kernel.crashing.org> wrote:

>> 
>> +
>> +        ft_verify_fdt(*of_flat_tree);
> 
> Do we not want to error out here if verify fails?

Maybe.  I didn't want a false negative to prevent booting.  If the DT is wrong, the kernel won't display any messages, so the warning will be the last thing the user sees anyway.

If the consensus is that failure should abort, then I can add that.  I don't have a strong preference either way.
> 

[U-Boot] [PATCH 2/2] powerpc/85xx: verify the device tree before booting Linux

[Kumar Gala]

On May 3, 2011, at 9:41 AM, Timur Tabi wrote:

> On May 3, 2011, at 9:21 AM, Kumar Gala <galak@kernel.crashing.org> wrote:
> 
>>> 
>>> +
>>> +        ft_verify_fdt(*of_flat_tree);
>> 
>> Do we not want to error out here if verify fails?
> 
> Maybe.  I didn't want a false negative to prevent booting.  If the DT is wrong, the kernel won't display any messages, so the warning will be the last thing the user sees anyway.
> 
> If the consensus is that failure should abort, then I can add that.  I don't have a strong preference either way.
>> 

I would think that a verify of this form should be considered as bad as not passing a checksum verification.

- k

[U-Boot] [PATCH 2/2] powerpc/85xx: verify the device tree before booting Linux

[Scott Wood]

On Tue, 3 May 2011 10:10:18 -0500
Kumar Gala <galak@kernel.crashing.org> wrote:

> 
> On May 3, 2011, at 9:41 AM, Timur Tabi wrote:
> 
> > On May 3, 2011, at 9:21 AM, Kumar Gala <galak@kernel.crashing.org> wrote:
> > 
> >>> 
> >>> +
> >>> +        ft_verify_fdt(*of_flat_tree);
> >> 
> >> Do we not want to error out here if verify fails?
> > 
> > Maybe.  I didn't want a false negative to prevent booting.  If the DT is wrong, the kernel won't display any messages, so the warning will be the last thing the user sees anyway.
> > 
> > If the consensus is that failure should abort, then I can add that.  I don't have a strong preference either way.
> >> 
> 
> I would think that a verify of this form should be considered as bad as not passing a checksum verification.

This seems to have a higher potential for false positives than a checksum.

-Scott

[U-Boot] [PATCH 2/2] powerpc/85xx: verify the device tree before booting Linux

[Timur Tabi]

Scott Wood wrote:
> This seems to have a higher potential for false positives than a checksum.

As a compromise, if the verify function thinks that there could be a false
positive, then it can display a warning and return success.

Frankly, though, if the there's a mismatch with the CCSR base address, then
there's not much room for false positives.

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] [PATCH 2/2] powerpc/85xx: verify the device tree before booting Linux

[Kumar Gala]

On May 3, 2011, at 5:20 PM, Scott Wood wrote:

> On Tue, 3 May 2011 10:10:18 -0500
> Kumar Gala <galak@kernel.crashing.org> wrote:
> 
>> 
>> On May 3, 2011, at 9:41 AM, Timur Tabi wrote:
>> 
>>> On May 3, 2011, at 9:21 AM, Kumar Gala <galak@kernel.crashing.org> wrote:
>>> 
>>>>> 
>>>>> +
>>>>> +        ft_verify_fdt(*of_flat_tree);
>>>> 
>>>> Do we not want to error out here if verify fails?
>>> 
>>> Maybe.  I didn't want a false negative to prevent booting.  If the DT is wrong, the kernel won't display any messages, so the warning will be the last thing the user sees anyway.
>>> 
>>> If the consensus is that failure should abort, then I can add that.  I don't have a strong preference either way.
>>>> 
>> 
>> I would think that a verify of this form should be considered as bad as not passing a checksum verification.
> 
> This seems to have a higher potential for false positives than a checksum.

My feeling is the feature should not have false positives, the goal should be to be as good as a checksum.

- k

[U-Boot] [PATCH 2/2] powerpc/85xx: verify the device tree before booting Linux

[Timur Tabi]

On May 4, 2011, at 8:40 AM, Kumar Gala <galak@kernel.crashing.org> wrote:

> 
>> 
>> This seems to have a higher potential for false positives than a checksum.
> 
> My feeling is the feature should not have false positives, the goal should be to be as good as a checksum.

I believe my v2 version of this patch satisfies this goal.  Someone else might try to add features that break that, however.


> 
> - k