[PATCH v5] libfdt: add helpers to read address and size from reg

[PATCH v5] libfdt: add helpers to read address and size from reg

[Andrew F. Davis]

From: Benjamin Fair <b-fair-l0cyMroinI0@public.gmane.org>

This patch extends the capability of libfdt to parse the contents of device
trees in a similar manner to fdt_address_cells and fdt_size_cells.

It adds a helper function which reads the address and size of a device from
the reg property and performs basic sanity checks.

It does not perform translation to a physical address using the ranges
properties of parents, but this enhancement may be added as a separate
function in the future.

Signed-off-by: Benjamin Fair <b-fair-l0cyMroinI0@public.gmane.org>
Reviewed-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Signed-off-by: Dave Gerlach <d-gerlach-l0cyMroinI0@public.gmane.org>
Signed-off-by: Andrew F. Davis <afd-l0cyMroinI0@public.gmane.org>
---

Changes from v4:
 - Removed unneeded #defines
 - Return status in parameter to avoid unsafe conversion to unsigned
 - Added error code translation for NOTFOUND -> BADNCELLS
 - Check for cell sizes being larger than the types they go into
 - Added tests for failure cases

The only comment from the last posting I didn't agree on was
converting the type of the parameters into uint64_t. This
helper function is designed to be used to get the size and
address for instant use. If we have to cast them later we need
extra checks for size correctness in the caller, who can't do
that as they would then have to check the cells sizes themselves,
which defeats the whole point of this function.

To make this intentional behavior more clear we add checks for
miss-sized types and return appropriate errors for that. We even
add some test cases for platforms with different sized types to
check the same.

 libfdt/fdt_addresses.c | 72 ++++++++++++++++++++++++++++++++++++++
 libfdt/libfdt.h        | 33 ++++++++++++++++++
 libfdt/version.lds     |  1 +
 tests/.gitignore       |  1 +
 tests/Makefile.tests   |  2 +-
 tests/addr_size.c      | 93 ++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/addresses.dts    | 20 +++++++++++
 tests/run_tests.sh     |  1 +
 8 files changed, 222 insertions(+), 1 deletion(-)
 create mode 100644 tests/addr_size.c

diff --git a/libfdt/fdt_addresses.c b/libfdt/fdt_addresses.c
index eff4dbc..41d3c5d 100644
--- a/libfdt/fdt_addresses.c
+++ b/libfdt/fdt_addresses.c
@@ -94,3 +94,75 @@ int fdt_size_cells(const void *fdt, int nodeoffset)
 
 	return val;
 }
+
+static uint64_t _fdt_read_cells(const fdt32_t *cells, unsigned int n, int *res)
+{
+	int i;
+	uint64_t value;
+
+	if (n > 2) {
+		*res = -FDT_ERR_BADNCELLS;
+		return 0;
+	}
+
+	value = 0;
+	for (i = 0; i < n; i++) {
+		value <<= (sizeof(*cells) * 8);
+		value |= fdt32_to_cpu(cells[i]);
+	}
+
+	*res = 0;
+	return value;
+}
+
+int fdt_simple_addr_size(const void *fdt, int nodeoffset, int idx,
+			 uintptr_t *addrp, size_t *sizep)
+{
+	int parent;
+	int ac, sc, reg_stride;
+	int res;
+	const fdt32_t *reg;
+
+	reg = fdt_getprop(fdt, nodeoffset, "reg", &res);
+	if (res < 0)
+		return res;
+
+	parent = fdt_parent_offset(fdt, nodeoffset);
+	if (parent == -FDT_ERR_NOTFOUND) /* an node without a parent does
+					  * not have _any_ number of cells */
+		return -FDT_ERR_BADNCELLS;
+	if (parent < 0)
+		return parent;
+
+	ac = fdt_address_cells(fdt, parent);
+	if (ac < 0)
+		return ac;
+	if (ac * sizeof(fdt32_t) > sizeof(*addrp))
+		return -FDT_ERR_BADNCELLS;
+
+	sc = fdt_size_cells(fdt, parent);
+	if (sc < 0)
+		return sc;
+	if (sc * sizeof(fdt32_t) > sizeof(*sizep))
+		return -FDT_ERR_BADNCELLS;
+
+	reg_stride = ac + sc;
+
+	/* res is the number of bytes read and must be an even multiple of the
+	 * sum of address cells and size cells */
+	if ((res % (reg_stride * sizeof(*reg))) != 0)
+		return -FDT_ERR_BADVALUE;
+
+	if (addrp) {
+		*addrp = (uintptr_t) _fdt_read_cells(&reg[reg_stride * idx], ac, &res);
+		if (res < 0)
+			return res;
+	}
+	if (sizep) {
+		*sizep = (size_t) _fdt_read_cells(&reg[ac + reg_stride * idx], sc, &res);
+		if (res < 0)
+			return res;
+	}
+
+	return 0;
+}
diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
index c8c00fa..73f2fea 100644
--- a/libfdt/libfdt.h
+++ b/libfdt/libfdt.h
@@ -1101,6 +1101,39 @@ int fdt_address_cells(const void *fdt, int nodeoffset);
  */
 int fdt_size_cells(const void *fdt, int nodeoffset);
 
+/**
+ *
+ * fdt_simple_addr_size - read address and/or size from the reg property of a
+ *                        device node.
+ * @fdt: pointer to the device tree blob
+ * @nodeoffset: offset of the node to find the address and/or size from
+ * @idx: which address/size pair to read
+ * @addrp: pointer to where address will be stored (will be overwritten) or NULL
+ * @sizep: pointer to where size will be stored (will be overwritten) or NULL
+ *
+ * When the node has a valid reg property, returns the address and/or size
+ * values stored there. It does not perform any type of translation based on
+ * the parent bus(es).
+ *
+ * NOTE: This function is expensive, as it must scan the device tree
+ * structure from the start to nodeoffset, *twice*, with fdt_parent_offset.
+ *
+ * returns:
+ *	0, on success
+ *	-FDT_ERR_BADVALUE, if there is an unexpected number of entries in the
+ *		reg property
+ *	-FDT_ERR_NOTFOUND, if the node does not have a reg property
+ *	-FDT_ERR_BADNCELLS, if the number of address or size cells is invalid
+ *		or greater than 2 (which is the maximum currently supported)
+ *	-FDT_ERR_BADMAGIC,
+ *	-FDT_ERR_BADSTATE,
+ *	-FDT_ERR_BADSTRUCTURE,
+ *	-FDT_ERR_BADVERSION,
+ *	-FDT_ERR_TRUNCATED, standard meanings
+ */
+
+int fdt_simple_addr_size(const void *fdt, int nodeoffset, int idx,
+			 uintptr_t *addrp, size_t *sizep);
 
 /**********************************************************************/
 /* Write-in-place functions                                           */
diff --git a/libfdt/version.lds b/libfdt/version.lds
index 18fb69f..1f19341 100644
--- a/libfdt/version.lds
+++ b/libfdt/version.lds
@@ -59,6 +59,7 @@ LIBFDT_1.2 {
 		fdt_next_subnode;
 		fdt_address_cells;
 		fdt_size_cells;
+		fdt_simple_addr_size;
 		fdt_stringlist_contains;
 		fdt_stringlist_count;
 		fdt_stringlist_search;
diff --git a/tests/.gitignore b/tests/.gitignore
index 9e209d5..acb9335 100644
--- a/tests/.gitignore
+++ b/tests/.gitignore
@@ -43,6 +43,7 @@ tmp.*
 /path_offset
 /path_offset_aliases
 /phandle_format
+/addr_size
 /property_iterate
 /propname_escapes
 /references
diff --git a/tests/Makefile.tests b/tests/Makefile.tests
index 262944a..1e326ea 100644
--- a/tests/Makefile.tests
+++ b/tests/Makefile.tests
@@ -8,7 +8,7 @@ LIB_TESTS_L = get_mem_rsv \
 	char_literal \
 	sized_cells \
 	notfound \
-	addr_size_cells \
+	addr_size_cells addr_size \
 	stringlist \
 	setprop_inplace nop_property nop_node \
 	sw_tree1 \
diff --git a/tests/addr_size.c b/tests/addr_size.c
new file mode 100644
index 0000000..10275f4
--- /dev/null
+++ b/tests/addr_size.c
@@ -0,0 +1,93 @@
+/*
+ * libfdt - Flat Device Tree manipulation
+ *	Testcase for address and size handling
+ * Copyright (C) 2016-2018 Texas Instruments Incorporated - http://www.ti.com/
+ *
+ * Based on addr_size_cells.c by David Gibson, <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU Lesser General Public License as published by
+ * the Free Software Foundation; either version 2.1 of the License, or (at
+ * your option) any later version.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any kind,
+ * whether express or implied; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ */
+
+#include <stdlib.h>
+#include <stdio.h>
+#include <string.h>
+#include <stdint.h>
+
+#include <libfdt.h>
+
+#include "tests.h"
+#include "testdata.h"
+
+static void check_node(const void *fdt, const char *path, int err,
+		       int idx, uintptr_t addr, size_t size)
+{
+	int offset, res;
+	uintptr_t xaddr;
+	size_t xsize;
+
+	offset = fdt_path_offset(fdt, path);
+	if (offset < 0)
+		FAIL("Couldn't find path %s", path);
+
+	res = fdt_simple_addr_size(fdt, offset, idx, &xaddr, &xsize);
+	if (res != err)
+		FAIL("fdt_simple_addr_size returned %s instead of %s",
+		     fdt_strerror(res), fdt_strerror(err));
+	if (res < 0)
+		return;
+
+	if (xaddr != addr)
+		FAIL("Physical address for %s is %p instead of %p\n",
+		     path, (void *) xaddr, (void *) addr);
+
+	if (xsize != size)
+		FAIL("Size for %s is %zx instead of %zx\n",
+		     path, xsize, size);
+}
+
+int main(int argc, char *argv[])
+{
+	void *fdt;
+
+	if (argc != 2)
+		CONFIG("Usage: %s <dtb file>\n", argv[0]);
+
+	test_init(argc, argv);
+	fdt = load_blob(argv[1]);
+
+	if (sizeof(uintptr_t) == 4 && sizeof(size_t) == 4) {
+		/* 32-bit address , 32-bit size */
+		check_node(fdt, "/identity-bus@0/id-device@400",
+			   -FDT_ERR_BADNCELLS, 0, 0x0, 0x0);
+		check_node(fdt, "/simple-bus@1000000/sb-device@8000000800",
+			   -FDT_ERR_BADNCELLS, 0, 0x0, 0x0);
+	}
+	else if (sizeof(uintptr_t) == 8 && sizeof(size_t) == 8) {
+		/* 64-bit uintptr_t , 64-bit size_t */
+		check_node(fdt, "/identity-bus@0/id-device@400",
+			   0, 0,0x400, 0x100);
+		check_node(fdt, "/simple-bus@1000000/sb-device@8000000800",
+			   0, 0, 0x8000000800, 0x200);
+		check_node(fdt, "/identity-bus@0/id-device@400",
+			   0, 1, 0x400000000, 0x100000030);
+		check_node(fdt, "/simple-bus@1000000/sb-device@8000000800",
+			   0, 1, 0x70000000, 0x700);
+		check_node(fdt, "/simple-bus@1000000/sb-device@8000000800",
+			   0, 2, 0x1050000000, 0x20);
+	}
+
+	check_node(fdt, "/identity-bus@0",
+		   -FDT_ERR_NOTFOUND, 0, 0x0, 0x0);
+	check_node(fdt, "/narrow-bus@2000000/sb-device@80000000",
+		   0, 0, 0x00000800, 0x200);
+
+	PASS();
+}
diff --git a/tests/addresses.dts b/tests/addresses.dts
index a2faaf5..747c291 100644
--- a/tests/addresses.dts
+++ b/tests/addresses.dts
@@ -6,10 +6,30 @@
 	#size-cells = <2>;
 
 	identity-bus@0 {
+		#address-cells = <2>;
+		#size-cells = <2>;
+		id-device@400 {
+			reg = <0x0 0x00000400 0x0 0x00000100>,
+			      <0x4 0x00000000 0x1 0x00000030>;
+		};
 	};
 
 	simple-bus@1000000 {
 		#address-cells = <2>;
 		#size-cells = <1>;
+		sb-device@8000000800 {
+			reg = <0x80 0x00000800 0x200>,
+			      <0x00 0x70000000 0x700>,
+			      <0x10 0x50000000 0x020>;
+		};
+	};
+
+	narrow-bus@2000000 {
+		#address-cells = <1>;
+		#size-cells = <1>;
+		sb-device@80000000 {
+			reg = <0x00000800 0x200>,
+			      <0x50000000 0x020>;
+		};
 	};
 };
diff --git a/tests/run_tests.sh b/tests/run_tests.sh
index 0d30edf..c195d0d 100755
--- a/tests/run_tests.sh
+++ b/tests/run_tests.sh
@@ -311,6 +311,7 @@ libfdt_tests () {
 
     run_dtc_test -I dts -O dtb -o addresses.test.dtb addresses.dts
     run_test addr_size_cells addresses.test.dtb
+    run_test addr_size addresses.test.dtb
 
     run_dtc_test -I dts -O dtb -o stringlist.test.dtb stringlist.dts
     run_test stringlist stringlist.test.dtb
-- 
2.16.1

Re: [PATCH v5] libfdt: add helpers to read address and size from reg

[Rob Herring]

On Thu, Feb 1, 2018 at 5:04 PM, Andrew F. Davis <afd-l0cyMroinI0@public.gmane.org> wrote:
> From: Benjamin Fair <b-fair-l0cyMroinI0@public.gmane.org>
>
> This patch extends the capability of libfdt to parse the contents of device
> trees in a similar manner to fdt_address_cells and fdt_size_cells.
>
> It adds a helper function which reads the address and size of a device from
> the reg property and performs basic sanity checks.
>
> It does not perform translation to a physical address using the ranges
> properties of parents, but this enhancement may be added as a separate
> function in the future.

I'm concerned about merging this without translation support. First,
I'd question its usefulness without it. Second, it may encourage
people to write their DTs without any ranges just so this will work.

I tried to upstream the kernel and u-boot implementations to libfdt in
April 2014 (I couldn't find an archive). We've got to get it
relicensed and then David didn't like the implementation. He said he
was about to post his version, but then it derailed into PCI handling.
There's another implementation in the kernel powerpc boot code that
may be easier to re-license.

Rob

Re: [PATCH v5] libfdt: add helpers to read address and size from reg

[Andrew F. Davis]

On 02/01/2018 10:35 PM, Rob Herring wrote:
> On Thu, Feb 1, 2018 at 5:04 PM, Andrew F. Davis <afd-l0cyMroinI0@public.gmane.org> wrote:
>> From: Benjamin Fair <b-fair-l0cyMroinI0@public.gmane.org>
>>
>> This patch extends the capability of libfdt to parse the contents of device
>> trees in a similar manner to fdt_address_cells and fdt_size_cells.
>>
>> It adds a helper function which reads the address and size of a device from
>> the reg property and performs basic sanity checks.
>>
>> It does not perform translation to a physical address using the ranges
>> properties of parents, but this enhancement may be added as a separate
>> function in the future.
> 
> I'm concerned about merging this without translation support. First,
> I'd question its usefulness without it. Second, it may encourage
> people to write their DTs without any ranges just so this will work.
> 

It can be useful for nodes where ranges is not appropriate for whatever
reason, or more likely as a base for other helpers that do perform
translation (hence the "simple" in this ones name), it will need to
still be extended for many use-cases.

> I tried to upstream the kernel and u-boot implementations to libfdt in
> April 2014 (I couldn't find an archive). We've got to get it
> relicensed and then David didn't like the implementation. He said he
> was about to post his version, but then it derailed into PCI handling.
> There's another implementation in the kernel powerpc boot code that
> may be easier to re-license.
> 

My worry with adding translation is this kind of feature creep. PCI and
other odd cases will block this effort with little gain for the users
where this simple helper is sufficient.

> Rob
> 

Re: [PATCH v5] libfdt: add helpers to read address and size from reg

[Andre Przywara]

Hi,

thanks for picking this up!

On 01/02/18 23:04, Andrew F. Davis (by way of Andre Przywara
<andre-s5aTT1PPrfyzQB+pC5nmwQ@public.gmane.org>) wrote:
> From: Benjamin Fair <b-fair-l0cyMroinI0@public.gmane.org>
> 
> This patch extends the capability of libfdt to parse the contents of device
> trees in a similar manner to fdt_address_cells and fdt_size_cells.
> 
> It adds a helper function which reads the address and size of a device from
> the reg property and performs basic sanity checks.
> 
> It does not perform translation to a physical address using the ranges
> properties of parents, but this enhancement may be added as a separate
> function in the future.

Ignoring Rob's answer for a moment, there is something I noticed when I
quickly hacked an older version of this into the ARM port
of the Xen related Mini-OS, see below ...

> Signed-off-by: Benjamin Fair <b-fair-l0cyMroinI0@public.gmane.org>
> Reviewed-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> Signed-off-by: Dave Gerlach <d-gerlach-l0cyMroinI0@public.gmane.org>
> Signed-off-by: Andrew F. Davis <afd-l0cyMroinI0@public.gmane.org>
> ---
> 
> Changes from v4:
>  - Removed unneeded #defines
>  - Return status in parameter to avoid unsafe conversion to unsigned
>  - Added error code translation for NOTFOUND -> BADNCELLS
>  - Check for cell sizes being larger than the types they go into
>  - Added tests for failure cases
> 
> The only comment from the last posting I didn't agree on was
> converting the type of the parameters into uint64_t. This
> helper function is designed to be used to get the size and
> address for instant use. If we have to cast them later we need
> extra checks for size correctness in the caller, who can't do
> that as they would then have to check the cells sizes themselves,
> which defeats the whole point of this function.
> 
> To make this intentional behavior more clear we add checks for
> miss-sized types and return appropriate errors for that. We even
> add some test cases for platforms with different sized types to
> check the same.
> 
>  libfdt/fdt_addresses.c | 72 ++++++++++++++++++++++++++++++++++++++
>  libfdt/libfdt.h        | 33 ++++++++++++++++++
>  libfdt/version.lds     |  1 +
>  tests/.gitignore       |  1 +
>  tests/Makefile.tests   |  2 +-
>  tests/addr_size.c      | 93 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  tests/addresses.dts    | 20 +++++++++++
>  tests/run_tests.sh     |  1 +
>  8 files changed, 222 insertions(+), 1 deletion(-)
>  create mode 100644 tests/addr_size.c
> 
> diff --git a/libfdt/fdt_addresses.c b/libfdt/fdt_addresses.c
> index eff4dbc..41d3c5d 100644
> --- a/libfdt/fdt_addresses.c
> +++ b/libfdt/fdt_addresses.c
> @@ -94,3 +94,75 @@ int fdt_size_cells(const void *fdt, int nodeoffset)
>  
>  	return val;
>  }
> +
> +static uint64_t _fdt_read_cells(const fdt32_t *cells, unsigned int n, int *res)
> +{
> +	int i;
> +	uint64_t value;
> +
> +	if (n > 2) {
> +		*res = -FDT_ERR_BADNCELLS;
> +		return 0;
> +	}
> +
> +	value = 0;
> +	for (i = 0; i < n; i++) {
> +		value <<= (sizeof(*cells) * 8);
> +		value |= fdt32_to_cpu(cells[i]);
> +	}
> +
> +	*res = 0;
> +	return value;
> +}
> +
> +int fdt_simple_addr_size(const void *fdt, int nodeoffset, int idx,
> +			 uintptr_t *addrp, size_t *sizep)

I believe this was mentioned in previous reviews:
I don't think that those types are the right ones to use here.
There is no correlation between the address size used in a DT and the
(current) size of pointers or addressable memory.
So you might end up in 32-bit code reading a DT with 64-bit addresses.
MMIO addresses might be under 4GB underway, or you are running using
LPAE, where physical addresses can be 40 bits long.

So I think we should use uint64_t* here, this would also avoid the
checks below.

Cheers,
Andre.

> +{
> +	int parent;
> +	int ac, sc, reg_stride;
> +	int res;
> +	const fdt32_t *reg;
> +
> +	reg = fdt_getprop(fdt, nodeoffset, "reg", &res);
> +	if (res < 0)
> +		return res;
> +
> +	parent = fdt_parent_offset(fdt, nodeoffset);
> +	if (parent == -FDT_ERR_NOTFOUND) /* an node without a parent does
> +					  * not have _any_ number of cells */
> +		return -FDT_ERR_BADNCELLS;
> +	if (parent < 0)
> +		return parent;
> +
> +	ac = fdt_address_cells(fdt, parent);
> +	if (ac < 0)
> +		return ac;
> +	if (ac * sizeof(fdt32_t) > sizeof(*addrp))
> +		return -FDT_ERR_BADNCELLS;
> +
> +	sc = fdt_size_cells(fdt, parent);
> +	if (sc < 0)
> +		return sc;
> +	if (sc * sizeof(fdt32_t) > sizeof(*sizep))
> +		return -FDT_ERR_BADNCELLS;
> +
> +	reg_stride = ac + sc;
> +
> +	/* res is the number of bytes read and must be an even multiple of the
> +	 * sum of address cells and size cells */
> +	if ((res % (reg_stride * sizeof(*reg))) != 0)
> +		return -FDT_ERR_BADVALUE;
> +
> +	if (addrp) {
> +		*addrp = (uintptr_t) _fdt_read_cells(&reg[reg_stride * idx], ac, &res);
> +		if (res < 0)
> +			return res;
> +	}
> +	if (sizep) {
> +		*sizep = (size_t) _fdt_read_cells(&reg[ac + reg_stride * idx], sc, &res);
> +		if (res < 0)
> +			return res;
> +	}
> +
> +	return 0;
> +}
> diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
> index c8c00fa..73f2fea 100644
> --- a/libfdt/libfdt.h
> +++ b/libfdt/libfdt.h
> @@ -1101,6 +1101,39 @@ int fdt_address_cells(const void *fdt, int nodeoffset);
>   */
>  int fdt_size_cells(const void *fdt, int nodeoffset);
>  
> +/**
> + *
> + * fdt_simple_addr_size - read address and/or size from the reg property of a
> + *                        device node.
> + * @fdt: pointer to the device tree blob
> + * @nodeoffset: offset of the node to find the address and/or size from
> + * @idx: which address/size pair to read
> + * @addrp: pointer to where address will be stored (will be overwritten) or NULL
> + * @sizep: pointer to where size will be stored (will be overwritten) or NULL
> + *
> + * When the node has a valid reg property, returns the address and/or size
> + * values stored there. It does not perform any type of translation based on
> + * the parent bus(es).
> + *
> + * NOTE: This function is expensive, as it must scan the device tree
> + * structure from the start to nodeoffset, *twice*, with fdt_parent_offset.
> + *
> + * returns:
> + *	0, on success
> + *	-FDT_ERR_BADVALUE, if there is an unexpected number of entries in the
> + *		reg property
> + *	-FDT_ERR_NOTFOUND, if the node does not have a reg property
> + *	-FDT_ERR_BADNCELLS, if the number of address or size cells is invalid
> + *		or greater than 2 (which is the maximum currently supported)
> + *	-FDT_ERR_BADMAGIC,
> + *	-FDT_ERR_BADSTATE,
> + *	-FDT_ERR_BADSTRUCTURE,
> + *	-FDT_ERR_BADVERSION,
> + *	-FDT_ERR_TRUNCATED, standard meanings
> + */
> +
> +int fdt_simple_addr_size(const void *fdt, int nodeoffset, int idx,
> +			 uintptr_t *addrp, size_t *sizep);
>  
>  /**********************************************************************/
>  /* Write-in-place functions                                           */
> diff --git a/libfdt/version.lds b/libfdt/version.lds
> index 18fb69f..1f19341 100644
> --- a/libfdt/version.lds
> +++ b/libfdt/version.lds
> @@ -59,6 +59,7 @@ LIBFDT_1.2 {
>  		fdt_next_subnode;
>  		fdt_address_cells;
>  		fdt_size_cells;
> +		fdt_simple_addr_size;
>  		fdt_stringlist_contains;
>  		fdt_stringlist_count;
>  		fdt_stringlist_search;
> diff --git a/tests/.gitignore b/tests/.gitignore
> index 9e209d5..acb9335 100644
> --- a/tests/.gitignore
> +++ b/tests/.gitignore
> @@ -43,6 +43,7 @@ tmp.*
>  /path_offset
>  /path_offset_aliases
>  /phandle_format
> +/addr_size
>  /property_iterate
>  /propname_escapes
>  /references
> diff --git a/tests/Makefile.tests b/tests/Makefile.tests
> index 262944a..1e326ea 100644
> --- a/tests/Makefile.tests
> +++ b/tests/Makefile.tests
> @@ -8,7 +8,7 @@ LIB_TESTS_L = get_mem_rsv \
>  	char_literal \
>  	sized_cells \
>  	notfound \
> -	addr_size_cells \
> +	addr_size_cells addr_size \
>  	stringlist \
>  	setprop_inplace nop_property nop_node \
>  	sw_tree1 \
> diff --git a/tests/addr_size.c b/tests/addr_size.c
> new file mode 100644
> index 0000000..10275f4
> --- /dev/null
> +++ b/tests/addr_size.c
> @@ -0,0 +1,93 @@
> +/*
> + * libfdt - Flat Device Tree manipulation
> + *	Testcase for address and size handling
> + * Copyright (C) 2016-2018 Texas Instruments Incorporated - http://www.ti.com/
> + *
> + * Based on addr_size_cells.c by David Gibson, <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU Lesser General Public License as published by
> + * the Free Software Foundation; either version 2.1 of the License, or (at
> + * your option) any later version.
> + *
> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any kind,
> + * whether express or implied; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + */
> +
> +#include <stdlib.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <stdint.h>
> +
> +#include <libfdt.h>
> +
> +#include "tests.h"
> +#include "testdata.h"
> +
> +static void check_node(const void *fdt, const char *path, int err,
> +		       int idx, uintptr_t addr, size_t size)
> +{
> +	int offset, res;
> +	uintptr_t xaddr;
> +	size_t xsize;
> +
> +	offset = fdt_path_offset(fdt, path);
> +	if (offset < 0)
> +		FAIL("Couldn't find path %s", path);
> +
> +	res = fdt_simple_addr_size(fdt, offset, idx, &xaddr, &xsize);
> +	if (res != err)
> +		FAIL("fdt_simple_addr_size returned %s instead of %s",
> +		     fdt_strerror(res), fdt_strerror(err));
> +	if (res < 0)
> +		return;
> +
> +	if (xaddr != addr)
> +		FAIL("Physical address for %s is %p instead of %p\n",
> +		     path, (void *) xaddr, (void *) addr);
> +
> +	if (xsize != size)
> +		FAIL("Size for %s is %zx instead of %zx\n",
> +		     path, xsize, size);
> +}
> +
> +int main(int argc, char *argv[])
> +{
> +	void *fdt;
> +
> +	if (argc != 2)
> +		CONFIG("Usage: %s <dtb file>\n", argv[0]);
> +
> +	test_init(argc, argv);
> +	fdt = load_blob(argv[1]);
> +
> +	if (sizeof(uintptr_t) == 4 && sizeof(size_t) == 4) {
> +		/* 32-bit address , 32-bit size */
> +		check_node(fdt, "/identity-bus@0/id-device@400",
> +			   -FDT_ERR_BADNCELLS, 0, 0x0, 0x0);
> +		check_node(fdt, "/simple-bus@1000000/sb-device@8000000800",
> +			   -FDT_ERR_BADNCELLS, 0, 0x0, 0x0);
> +	}
> +	else if (sizeof(uintptr_t) == 8 && sizeof(size_t) == 8) {
> +		/* 64-bit uintptr_t , 64-bit size_t */
> +		check_node(fdt, "/identity-bus@0/id-device@400",
> +			   0, 0,0x400, 0x100);
> +		check_node(fdt, "/simple-bus@1000000/sb-device@8000000800",
> +			   0, 0, 0x8000000800, 0x200);
> +		check_node(fdt, "/identity-bus@0/id-device@400",
> +			   0, 1, 0x400000000, 0x100000030);
> +		check_node(fdt, "/simple-bus@1000000/sb-device@8000000800",
> +			   0, 1, 0x70000000, 0x700);
> +		check_node(fdt, "/simple-bus@1000000/sb-device@8000000800",
> +			   0, 2, 0x1050000000, 0x20);
> +	}
> +
> +	check_node(fdt, "/identity-bus@0",
> +		   -FDT_ERR_NOTFOUND, 0, 0x0, 0x0);
> +	check_node(fdt, "/narrow-bus@2000000/sb-device@80000000",
> +		   0, 0, 0x00000800, 0x200);
> +
> +	PASS();
> +}
> diff --git a/tests/addresses.dts b/tests/addresses.dts
> index a2faaf5..747c291 100644
> --- a/tests/addresses.dts
> +++ b/tests/addresses.dts
> @@ -6,10 +6,30 @@
>  	#size-cells = <2>;
>  
>  	identity-bus@0 {
> +		#address-cells = <2>;
> +		#size-cells = <2>;
> +		id-device@400 {
> +			reg = <0x0 0x00000400 0x0 0x00000100>,
> +			      <0x4 0x00000000 0x1 0x00000030>;
> +		};
>  	};
>  
>  	simple-bus@1000000 {
>  		#address-cells = <2>;
>  		#size-cells = <1>;
> +		sb-device@8000000800 {
> +			reg = <0x80 0x00000800 0x200>,
> +			      <0x00 0x70000000 0x700>,
> +			      <0x10 0x50000000 0x020>;
> +		};
> +	};
> +
> +	narrow-bus@2000000 {
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		sb-device@80000000 {
> +			reg = <0x00000800 0x200>,
> +			      <0x50000000 0x020>;
> +		};
>  	};
>  };
> diff --git a/tests/run_tests.sh b/tests/run_tests.sh
> index 0d30edf..c195d0d 100755
> --- a/tests/run_tests.sh
> +++ b/tests/run_tests.sh
> @@ -311,6 +311,7 @@ libfdt_tests () {
>  
>      run_dtc_test -I dts -O dtb -o addresses.test.dtb addresses.dts
>      run_test addr_size_cells addresses.test.dtb
> +    run_test addr_size addresses.test.dtb
>  
>      run_dtc_test -I dts -O dtb -o stringlist.test.dtb stringlist.dts
>      run_test stringlist stringlist.test.dtb
> 

Re: [PATCH v5] libfdt: add helpers to read address and size from reg

[Andrew F. Davis]

On 02/05/2018 11:11 AM, Andre Przywara wrote:
> Hi,
> 
> thanks for picking this up!
> 
> On 01/02/18 23:04, Andrew F. Davis (by way of Andre Przywara
> <andre-s5aTT1PPrfyzQB+pC5nmwQ@public.gmane.org>) wrote:
>> From: Benjamin Fair <b-fair-l0cyMroinI0@public.gmane.org>
>>
>> This patch extends the capability of libfdt to parse the contents of device
>> trees in a similar manner to fdt_address_cells and fdt_size_cells.
>>
>> It adds a helper function which reads the address and size of a device from
>> the reg property and performs basic sanity checks.
>>
>> It does not perform translation to a physical address using the ranges
>> properties of parents, but this enhancement may be added as a separate
>> function in the future.
> 
> Ignoring Rob's answer for a moment, there is something I noticed when I
> quickly hacked an older version of this into the ARM port
> of the Xen related Mini-OS, see below ...
> 
>> Signed-off-by: Benjamin Fair <b-fair-l0cyMroinI0@public.gmane.org>
>> Reviewed-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>> Signed-off-by: Dave Gerlach <d-gerlach-l0cyMroinI0@public.gmane.org>
>> Signed-off-by: Andrew F. Davis <afd-l0cyMroinI0@public.gmane.org>
>> ---
>>
>> Changes from v4:
>>  - Removed unneeded #defines
>>  - Return status in parameter to avoid unsafe conversion to unsigned
>>  - Added error code translation for NOTFOUND -> BADNCELLS
>>  - Check for cell sizes being larger than the types they go into
>>  - Added tests for failure cases
>>
>> The only comment from the last posting I didn't agree on was
>> converting the type of the parameters into uint64_t. This
>> helper function is designed to be used to get the size and
>> address for instant use. If we have to cast them later we need
>> extra checks for size correctness in the caller, who can't do
>> that as they would then have to check the cells sizes themselves,
>> which defeats the whole point of this function.
>>
>> To make this intentional behavior more clear we add checks for
>> miss-sized types and return appropriate errors for that. We even
>> add some test cases for platforms with different sized types to
>> check the same.
>>
>>  libfdt/fdt_addresses.c | 72 ++++++++++++++++++++++++++++++++++++++
>>  libfdt/libfdt.h        | 33 ++++++++++++++++++
>>  libfdt/version.lds     |  1 +
>>  tests/.gitignore       |  1 +
>>  tests/Makefile.tests   |  2 +-
>>  tests/addr_size.c      | 93 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>  tests/addresses.dts    | 20 +++++++++++
>>  tests/run_tests.sh     |  1 +
>>  8 files changed, 222 insertions(+), 1 deletion(-)
>>  create mode 100644 tests/addr_size.c
>>
>> diff --git a/libfdt/fdt_addresses.c b/libfdt/fdt_addresses.c
>> index eff4dbc..41d3c5d 100644
>> --- a/libfdt/fdt_addresses.c
>> +++ b/libfdt/fdt_addresses.c
>> @@ -94,3 +94,75 @@ int fdt_size_cells(const void *fdt, int nodeoffset)
>>  
>>  	return val;
>>  }
>> +
>> +static uint64_t _fdt_read_cells(const fdt32_t *cells, unsigned int n, int *res)
>> +{
>> +	int i;
>> +	uint64_t value;
>> +
>> +	if (n > 2) {
>> +		*res = -FDT_ERR_BADNCELLS;
>> +		return 0;
>> +	}
>> +
>> +	value = 0;
>> +	for (i = 0; i < n; i++) {
>> +		value <<= (sizeof(*cells) * 8);
>> +		value |= fdt32_to_cpu(cells[i]);
>> +	}
>> +
>> +	*res = 0;
>> +	return value;
>> +}
>> +
>> +int fdt_simple_addr_size(const void *fdt, int nodeoffset, int idx,
>> +			 uintptr_t *addrp, size_t *sizep)
> 
> I believe this was mentioned in previous reviews:
> I don't think that those types are the right ones to use here.
> There is no correlation between the address size used in a DT and the
> (current) size of pointers or addressable memory.
> So you might end up in 32-bit code reading a DT with 64-bit addresses.
> MMIO addresses might be under 4GB underway, or you are running using
> LPAE, where physical addresses can be 40 bits long.
> 

This function checks for these mismatches and returns an error. The
point of this helper is for the system about to use these pointers, if
you want to manipulate them on a different system or use them later then
this helper is not correct for you.

> So I think we should use uint64_t* here, this would also avoid the
> checks below.
> 

What will happen if I am on a 32-bit big-endian system, and I only want
to read in a 32-bit address, but the DT node is 64-bit? How will I know
that it read in 64-bits? How will I know where the MSB is? Do we care
about 128-bit? Etc.. I don't think this can be made generic for all the
combinations, and so for simplicity the address pointer and size must
match the system. For other more complex uses you will have to implement
your own handling.

> Cheers,
> Andre.
> 
>> +{
>> +	int parent;
>> +	int ac, sc, reg_stride;
>> +	int res;
>> +	const fdt32_t *reg;
>> +
>> +	reg = fdt_getprop(fdt, nodeoffset, "reg", &res);
>> +	if (res < 0)
>> +		return res;
>> +
>> +	parent = fdt_parent_offset(fdt, nodeoffset);
>> +	if (parent == -FDT_ERR_NOTFOUND) /* an node without a parent does
>> +					  * not have _any_ number of cells */
>> +		return -FDT_ERR_BADNCELLS;
>> +	if (parent < 0)
>> +		return parent;
>> +
>> +	ac = fdt_address_cells(fdt, parent);
>> +	if (ac < 0)
>> +		return ac;
>> +	if (ac * sizeof(fdt32_t) > sizeof(*addrp))
>> +		return -FDT_ERR_BADNCELLS;
>> +
>> +	sc = fdt_size_cells(fdt, parent);
>> +	if (sc < 0)
>> +		return sc;
>> +	if (sc * sizeof(fdt32_t) > sizeof(*sizep))
>> +		return -FDT_ERR_BADNCELLS;
>> +
>> +	reg_stride = ac + sc;
>> +
>> +	/* res is the number of bytes read and must be an even multiple of the
>> +	 * sum of address cells and size cells */
>> +	if ((res % (reg_stride * sizeof(*reg))) != 0)
>> +		return -FDT_ERR_BADVALUE;
>> +
>> +	if (addrp) {
>> +		*addrp = (uintptr_t) _fdt_read_cells(&reg[reg_stride * idx], ac, &res);
>> +		if (res < 0)
>> +			return res;
>> +	}
>> +	if (sizep) {
>> +		*sizep = (size_t) _fdt_read_cells(&reg[ac + reg_stride * idx], sc, &res);
>> +		if (res < 0)
>> +			return res;
>> +	}
>> +
>> +	return 0;
>> +}
>> diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
>> index c8c00fa..73f2fea 100644
>> --- a/libfdt/libfdt.h
>> +++ b/libfdt/libfdt.h
>> @@ -1101,6 +1101,39 @@ int fdt_address_cells(const void *fdt, int nodeoffset);
>>   */
>>  int fdt_size_cells(const void *fdt, int nodeoffset);
>>  
>> +/**
>> + *
>> + * fdt_simple_addr_size - read address and/or size from the reg property of a
>> + *                        device node.
>> + * @fdt: pointer to the device tree blob
>> + * @nodeoffset: offset of the node to find the address and/or size from
>> + * @idx: which address/size pair to read
>> + * @addrp: pointer to where address will be stored (will be overwritten) or NULL
>> + * @sizep: pointer to where size will be stored (will be overwritten) or NULL
>> + *
>> + * When the node has a valid reg property, returns the address and/or size
>> + * values stored there. It does not perform any type of translation based on
>> + * the parent bus(es).
>> + *
>> + * NOTE: This function is expensive, as it must scan the device tree
>> + * structure from the start to nodeoffset, *twice*, with fdt_parent_offset.
>> + *
>> + * returns:
>> + *	0, on success
>> + *	-FDT_ERR_BADVALUE, if there is an unexpected number of entries in the
>> + *		reg property
>> + *	-FDT_ERR_NOTFOUND, if the node does not have a reg property
>> + *	-FDT_ERR_BADNCELLS, if the number of address or size cells is invalid
>> + *		or greater than 2 (which is the maximum currently supported)
>> + *	-FDT_ERR_BADMAGIC,
>> + *	-FDT_ERR_BADSTATE,
>> + *	-FDT_ERR_BADSTRUCTURE,
>> + *	-FDT_ERR_BADVERSION,
>> + *	-FDT_ERR_TRUNCATED, standard meanings
>> + */
>> +
>> +int fdt_simple_addr_size(const void *fdt, int nodeoffset, int idx,
>> +			 uintptr_t *addrp, size_t *sizep);
>>  
>>  /**********************************************************************/
>>  /* Write-in-place functions                                           */
>> diff --git a/libfdt/version.lds b/libfdt/version.lds
>> index 18fb69f..1f19341 100644
>> --- a/libfdt/version.lds
>> +++ b/libfdt/version.lds
>> @@ -59,6 +59,7 @@ LIBFDT_1.2 {
>>  		fdt_next_subnode;
>>  		fdt_address_cells;
>>  		fdt_size_cells;
>> +		fdt_simple_addr_size;
>>  		fdt_stringlist_contains;
>>  		fdt_stringlist_count;
>>  		fdt_stringlist_search;
>> diff --git a/tests/.gitignore b/tests/.gitignore
>> index 9e209d5..acb9335 100644
>> --- a/tests/.gitignore
>> +++ b/tests/.gitignore
>> @@ -43,6 +43,7 @@ tmp.*
>>  /path_offset
>>  /path_offset_aliases
>>  /phandle_format
>> +/addr_size
>>  /property_iterate
>>  /propname_escapes
>>  /references
>> diff --git a/tests/Makefile.tests b/tests/Makefile.tests
>> index 262944a..1e326ea 100644
>> --- a/tests/Makefile.tests
>> +++ b/tests/Makefile.tests
>> @@ -8,7 +8,7 @@ LIB_TESTS_L = get_mem_rsv \
>>  	char_literal \
>>  	sized_cells \
>>  	notfound \
>> -	addr_size_cells \
>> +	addr_size_cells addr_size \
>>  	stringlist \
>>  	setprop_inplace nop_property nop_node \
>>  	sw_tree1 \
>> diff --git a/tests/addr_size.c b/tests/addr_size.c
>> new file mode 100644
>> index 0000000..10275f4
>> --- /dev/null
>> +++ b/tests/addr_size.c
>> @@ -0,0 +1,93 @@
>> +/*
>> + * libfdt - Flat Device Tree manipulation
>> + *	Testcase for address and size handling
>> + * Copyright (C) 2016-2018 Texas Instruments Incorporated - http://www.ti.com/
>> + *
>> + * Based on addr_size_cells.c by David Gibson, <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU Lesser General Public License as published by
>> + * the Free Software Foundation; either version 2.1 of the License, or (at
>> + * your option) any later version.
>> + *
>> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any kind,
>> + * whether express or implied; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> + * Lesser General Public License for more details.
>> + */
>> +
>> +#include <stdlib.h>
>> +#include <stdio.h>
>> +#include <string.h>
>> +#include <stdint.h>
>> +
>> +#include <libfdt.h>
>> +
>> +#include "tests.h"
>> +#include "testdata.h"
>> +
>> +static void check_node(const void *fdt, const char *path, int err,
>> +		       int idx, uintptr_t addr, size_t size)
>> +{
>> +	int offset, res;
>> +	uintptr_t xaddr;
>> +	size_t xsize;
>> +
>> +	offset = fdt_path_offset(fdt, path);
>> +	if (offset < 0)
>> +		FAIL("Couldn't find path %s", path);
>> +
>> +	res = fdt_simple_addr_size(fdt, offset, idx, &xaddr, &xsize);
>> +	if (res != err)
>> +		FAIL("fdt_simple_addr_size returned %s instead of %s",
>> +		     fdt_strerror(res), fdt_strerror(err));
>> +	if (res < 0)
>> +		return;
>> +
>> +	if (xaddr != addr)
>> +		FAIL("Physical address for %s is %p instead of %p\n",
>> +		     path, (void *) xaddr, (void *) addr);
>> +
>> +	if (xsize != size)
>> +		FAIL("Size for %s is %zx instead of %zx\n",
>> +		     path, xsize, size);
>> +}
>> +
>> +int main(int argc, char *argv[])
>> +{
>> +	void *fdt;
>> +
>> +	if (argc != 2)
>> +		CONFIG("Usage: %s <dtb file>\n", argv[0]);
>> +
>> +	test_init(argc, argv);
>> +	fdt = load_blob(argv[1]);
>> +
>> +	if (sizeof(uintptr_t) == 4 && sizeof(size_t) == 4) {
>> +		/* 32-bit address , 32-bit size */
>> +		check_node(fdt, "/identity-bus@0/id-device@400",
>> +			   -FDT_ERR_BADNCELLS, 0, 0x0, 0x0);
>> +		check_node(fdt, "/simple-bus@1000000/sb-device@8000000800",
>> +			   -FDT_ERR_BADNCELLS, 0, 0x0, 0x0);
>> +	}
>> +	else if (sizeof(uintptr_t) == 8 && sizeof(size_t) == 8) {
>> +		/* 64-bit uintptr_t , 64-bit size_t */
>> +		check_node(fdt, "/identity-bus@0/id-device@400",
>> +			   0, 0,0x400, 0x100);
>> +		check_node(fdt, "/simple-bus@1000000/sb-device@8000000800",
>> +			   0, 0, 0x8000000800, 0x200);
>> +		check_node(fdt, "/identity-bus@0/id-device@400",
>> +			   0, 1, 0x400000000, 0x100000030);
>> +		check_node(fdt, "/simple-bus@1000000/sb-device@8000000800",
>> +			   0, 1, 0x70000000, 0x700);
>> +		check_node(fdt, "/simple-bus@1000000/sb-device@8000000800",
>> +			   0, 2, 0x1050000000, 0x20);
>> +	}
>> +
>> +	check_node(fdt, "/identity-bus@0",
>> +		   -FDT_ERR_NOTFOUND, 0, 0x0, 0x0);
>> +	check_node(fdt, "/narrow-bus@2000000/sb-device@80000000",
>> +		   0, 0, 0x00000800, 0x200);
>> +
>> +	PASS();
>> +}
>> diff --git a/tests/addresses.dts b/tests/addresses.dts
>> index a2faaf5..747c291 100644
>> --- a/tests/addresses.dts
>> +++ b/tests/addresses.dts
>> @@ -6,10 +6,30 @@
>>  	#size-cells = <2>;
>>  
>>  	identity-bus@0 {
>> +		#address-cells = <2>;
>> +		#size-cells = <2>;
>> +		id-device@400 {
>> +			reg = <0x0 0x00000400 0x0 0x00000100>,
>> +			      <0x4 0x00000000 0x1 0x00000030>;
>> +		};
>>  	};
>>  
>>  	simple-bus@1000000 {
>>  		#address-cells = <2>;
>>  		#size-cells = <1>;
>> +		sb-device@8000000800 {
>> +			reg = <0x80 0x00000800 0x200>,
>> +			      <0x00 0x70000000 0x700>,
>> +			      <0x10 0x50000000 0x020>;
>> +		};
>> +	};
>> +
>> +	narrow-bus@2000000 {
>> +		#address-cells = <1>;
>> +		#size-cells = <1>;
>> +		sb-device@80000000 {
>> +			reg = <0x00000800 0x200>,
>> +			      <0x50000000 0x020>;
>> +		};
>>  	};
>>  };
>> diff --git a/tests/run_tests.sh b/tests/run_tests.sh
>> index 0d30edf..c195d0d 100755
>> --- a/tests/run_tests.sh
>> +++ b/tests/run_tests.sh
>> @@ -311,6 +311,7 @@ libfdt_tests () {
>>  
>>      run_dtc_test -I dts -O dtb -o addresses.test.dtb addresses.dts
>>      run_test addr_size_cells addresses.test.dtb
>> +    run_test addr_size addresses.test.dtb
>>  
>>      run_dtc_test -I dts -O dtb -o stringlist.test.dtb stringlist.dts
>>      run_test stringlist stringlist.test.dtb
>>

Re: [PATCH v5] libfdt: add helpers to read address and size from reg

[André Przywara]

On 05/02/18 20:10, Andrew F. Davis wrote:
> On 02/05/2018 11:11 AM, Andre Przywara wrote:
>> Hi,
>>
>> thanks for picking this up!
>>
>> On 01/02/18 23:04, Andrew F. Davis (by way of Andre Przywara
>> <andre-s5aTT1PPrfyzQB+pC5nmwQ@public.gmane.org>) wrote:
>>> From: Benjamin Fair <b-fair-l0cyMroinI0@public.gmane.org>

....

>>> +
>>> +int fdt_simple_addr_size(const void *fdt, int nodeoffset, int idx,
>>> +			 uintptr_t *addrp, size_t *sizep)
>>
>> I believe this was mentioned in previous reviews:
>> I don't think that those types are the right ones to use here.
>> There is no correlation between the address size used in a DT and the
>> (current) size of pointers or addressable memory.
>> So you might end up in 32-bit code reading a DT with 64-bit addresses.
>> MMIO addresses might be under 4GB underway, or you are running using
>> LPAE, where physical addresses can be 40 bits long.

> This function checks for these mismatches and returns an error. The
> point of this helper is for the system about to use these pointers, if
> you want to manipulate them on a different system or use them later then
> this helper is not correct for you.

Imagine you have a TI(!) Keystone SoC, the kernel DT tells me the
address and size are 64 bit, though the A15 cores will always have
uintptr_t and size_t at 32 bits. So you can never use this function
there, even if you would have the MMU using long descriptors, and even
if you just want to program the GIC to allow non-secure access in a
small secure firmware shim using libfdt:
                compatible = "arm,gic-400", "arm,cortex-a15-gic";
		reg = <0x0 0x02561000 0x0 0x1000>, ....
That would be perfectly accessible with the MMU off.
Looking further into the .dtsi actually underlines Rob's point of not
supporting translation being much less useful (though it would happen to
work in this case, more or less by chance).

>> So I think we should use uint64_t* here, this would also avoid the
>> checks below.
>>
> 
> What will happen if I am on a 32-bit big-endian system, and I only want
> to read in a 32-bit address, but the DT node is 64-bit? How will I know
> that it read in 64-bits?

if ((base_addr & (uintptr_t)~0) != base_addr)
	return -EFBIG;

> How will I know where the MSB is?

DT is always big endian. libfdt takes care of that.

> Do we care about 128-bit?

No. I am not aware of such a system, especially not one using DT.
Keystones are real, though. DTs for ARMv8 systems running in AArch32 are
real as well.

> Etc.. I don't think this can be made generic for all the
> combinations, and so for simplicity the address pointer and size must
> match the system. For other more complex uses you will have to implement
> your own handling.

We don't need to be super generic, but at least should care about
existing systems. I ran into this issue with 32 bit guests on an 64-bit
hypervisor. The HV tooling generates the same DT and the guest needs to
deal with that.

Please note, I actually like this patch and am not asking for a complete
rewrite: We just change the pointer types and drop the size check: done.

Or if a range-aware version isn't too far away, we use that.

Cheers,
Andre.

>>
>>> +{
>>> +	int parent;
>>> +	int ac, sc, reg_stride;
>>> +	int res;
>>> +	const fdt32_t *reg;
>>> +
>>> +	reg = fdt_getprop(fdt, nodeoffset, "reg", &res);
>>> +	if (res < 0)
>>> +		return res;
>>> +
>>> +	parent = fdt_parent_offset(fdt, nodeoffset);
>>> +	if (parent == -FDT_ERR_NOTFOUND) /* an node without a parent does
>>> +					  * not have _any_ number of cells */
>>> +		return -FDT_ERR_BADNCELLS;
>>> +	if (parent < 0)
>>> +		return parent;
>>> +
>>> +	ac = fdt_address_cells(fdt, parent);
>>> +	if (ac < 0)
>>> +		return ac;
>>> +	if (ac * sizeof(fdt32_t) > sizeof(*addrp))
>>> +		return -FDT_ERR_BADNCELLS;
>>> +
>>> +	sc = fdt_size_cells(fdt, parent);
>>> +	if (sc < 0)
>>> +		return sc;
>>> +	if (sc * sizeof(fdt32_t) > sizeof(*sizep))
>>> +		return -FDT_ERR_BADNCELLS;
>>> +
>>> +	reg_stride = ac + sc;
>>> +
>>> +	/* res is the number of bytes read and must be an even multiple of the
>>> +	 * sum of address cells and size cells */
>>> +	if ((res % (reg_stride * sizeof(*reg))) != 0)
>>> +		return -FDT_ERR_BADVALUE;
>>> +
>>> +	if (addrp) {
>>> +		*addrp = (uintptr_t) _fdt_read_cells(&reg[reg_stride * idx], ac, &res);
>>> +		if (res < 0)
>>> +			return res;
>>> +	}
>>> +	if (sizep) {
>>> +		*sizep = (size_t) _fdt_read_cells(&reg[ac + reg_stride * idx], sc, &res);
>>> +		if (res < 0)
>>> +			return res;
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
>>> index c8c00fa..73f2fea 100644
>>> --- a/libfdt/libfdt.h
>>> +++ b/libfdt/libfdt.h
>>> @@ -1101,6 +1101,39 @@ int fdt_address_cells(const void *fdt, int nodeoffset);
>>>   */
>>>  int fdt_size_cells(const void *fdt, int nodeoffset);
>>>  
>>> +/**
>>> + *
>>> + * fdt_simple_addr_size - read address and/or size from the reg property of a
>>> + *                        device node.
>>> + * @fdt: pointer to the device tree blob
>>> + * @nodeoffset: offset of the node to find the address and/or size from
>>> + * @idx: which address/size pair to read
>>> + * @addrp: pointer to where address will be stored (will be overwritten) or NULL
>>> + * @sizep: pointer to where size will be stored (will be overwritten) or NULL
>>> + *
>>> + * When the node has a valid reg property, returns the address and/or size
>>> + * values stored there. It does not perform any type of translation based on
>>> + * the parent bus(es).
>>> + *
>>> + * NOTE: This function is expensive, as it must scan the device tree
>>> + * structure from the start to nodeoffset, *twice*, with fdt_parent_offset.
>>> + *
>>> + * returns:
>>> + *	0, on success
>>> + *	-FDT_ERR_BADVALUE, if there is an unexpected number of entries in the
>>> + *		reg property
>>> + *	-FDT_ERR_NOTFOUND, if the node does not have a reg property
>>> + *	-FDT_ERR_BADNCELLS, if the number of address or size cells is invalid
>>> + *		or greater than 2 (which is the maximum currently supported)
>>> + *	-FDT_ERR_BADMAGIC,
>>> + *	-FDT_ERR_BADSTATE,
>>> + *	-FDT_ERR_BADSTRUCTURE,
>>> + *	-FDT_ERR_BADVERSION,
>>> + *	-FDT_ERR_TRUNCATED, standard meanings
>>> + */
>>> +
>>> +int fdt_simple_addr_size(const void *fdt, int nodeoffset, int idx,
>>> +			 uintptr_t *addrp, size_t *sizep);
>>>  
>>>  /**********************************************************************/
>>>  /* Write-in-place functions                                           */
>>> diff --git a/libfdt/version.lds b/libfdt/version.lds
>>> index 18fb69f..1f19341 100644
>>> --- a/libfdt/version.lds
>>> +++ b/libfdt/version.lds
>>> @@ -59,6 +59,7 @@ LIBFDT_1.2 {
>>>  		fdt_next_subnode;
>>>  		fdt_address_cells;
>>>  		fdt_size_cells;
>>> +		fdt_simple_addr_size;
>>>  		fdt_stringlist_contains;
>>>  		fdt_stringlist_count;
>>>  		fdt_stringlist_search;
>>> diff --git a/tests/.gitignore b/tests/.gitignore
>>> index 9e209d5..acb9335 100644
>>> --- a/tests/.gitignore
>>> +++ b/tests/.gitignore
>>> @@ -43,6 +43,7 @@ tmp.*
>>>  /path_offset
>>>  /path_offset_aliases
>>>  /phandle_format
>>> +/addr_size
>>>  /property_iterate
>>>  /propname_escapes
>>>  /references
>>> diff --git a/tests/Makefile.tests b/tests/Makefile.tests
>>> index 262944a..1e326ea 100644
>>> --- a/tests/Makefile.tests
>>> +++ b/tests/Makefile.tests
>>> @@ -8,7 +8,7 @@ LIB_TESTS_L = get_mem_rsv \
>>>  	char_literal \
>>>  	sized_cells \
>>>  	notfound \
>>> -	addr_size_cells \
>>> +	addr_size_cells addr_size \
>>>  	stringlist \
>>>  	setprop_inplace nop_property nop_node \
>>>  	sw_tree1 \
>>> diff --git a/tests/addr_size.c b/tests/addr_size.c
>>> new file mode 100644
>>> index 0000000..10275f4
>>> --- /dev/null
>>> +++ b/tests/addr_size.c
>>> @@ -0,0 +1,93 @@
>>> +/*
>>> + * libfdt - Flat Device Tree manipulation
>>> + *	Testcase for address and size handling
>>> + * Copyright (C) 2016-2018 Texas Instruments Incorporated - http://www.ti.com/
>>> + *
>>> + * Based on addr_size_cells.c by David Gibson, <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify it
>>> + * under the terms of the GNU Lesser General Public License as published by
>>> + * the Free Software Foundation; either version 2.1 of the License, or (at
>>> + * your option) any later version.
>>> + *
>>> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any kind,
>>> + * whether express or implied; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>>> + * Lesser General Public License for more details.
>>> + */
>>> +
>>> +#include <stdlib.h>
>>> +#include <stdio.h>
>>> +#include <string.h>
>>> +#include <stdint.h>
>>> +
>>> +#include <libfdt.h>
>>> +
>>> +#include "tests.h"
>>> +#include "testdata.h"
>>> +
>>> +static void check_node(const void *fdt, const char *path, int err,
>>> +		       int idx, uintptr_t addr, size_t size)
>>> +{
>>> +	int offset, res;
>>> +	uintptr_t xaddr;
>>> +	size_t xsize;
>>> +
>>> +	offset = fdt_path_offset(fdt, path);
>>> +	if (offset < 0)
>>> +		FAIL("Couldn't find path %s", path);
>>> +
>>> +	res = fdt_simple_addr_size(fdt, offset, idx, &xaddr, &xsize);
>>> +	if (res != err)
>>> +		FAIL("fdt_simple_addr_size returned %s instead of %s",
>>> +		     fdt_strerror(res), fdt_strerror(err));
>>> +	if (res < 0)
>>> +		return;
>>> +
>>> +	if (xaddr != addr)
>>> +		FAIL("Physical address for %s is %p instead of %p\n",
>>> +		     path, (void *) xaddr, (void *) addr);
>>> +
>>> +	if (xsize != size)
>>> +		FAIL("Size for %s is %zx instead of %zx\n",
>>> +		     path, xsize, size);
>>> +}
>>> +
>>> +int main(int argc, char *argv[])
>>> +{
>>> +	void *fdt;
>>> +
>>> +	if (argc != 2)
>>> +		CONFIG("Usage: %s <dtb file>\n", argv[0]);
>>> +
>>> +	test_init(argc, argv);
>>> +	fdt = load_blob(argv[1]);
>>> +
>>> +	if (sizeof(uintptr_t) == 4 && sizeof(size_t) == 4) {
>>> +		/* 32-bit address , 32-bit size */
>>> +		check_node(fdt, "/identity-bus@0/id-device@400",
>>> +			   -FDT_ERR_BADNCELLS, 0, 0x0, 0x0);
>>> +		check_node(fdt, "/simple-bus@1000000/sb-device@8000000800",
>>> +			   -FDT_ERR_BADNCELLS, 0, 0x0, 0x0);
>>> +	}
>>> +	else if (sizeof(uintptr_t) == 8 && sizeof(size_t) == 8) {
>>> +		/* 64-bit uintptr_t , 64-bit size_t */
>>> +		check_node(fdt, "/identity-bus@0/id-device@400",
>>> +			   0, 0,0x400, 0x100);
>>> +		check_node(fdt, "/simple-bus@1000000/sb-device@8000000800",
>>> +			   0, 0, 0x8000000800, 0x200);
>>> +		check_node(fdt, "/identity-bus@0/id-device@400",
>>> +			   0, 1, 0x400000000, 0x100000030);
>>> +		check_node(fdt, "/simple-bus@1000000/sb-device@8000000800",
>>> +			   0, 1, 0x70000000, 0x700);
>>> +		check_node(fdt, "/simple-bus@1000000/sb-device@8000000800",
>>> +			   0, 2, 0x1050000000, 0x20);
>>> +	}
>>> +
>>> +	check_node(fdt, "/identity-bus@0",
>>> +		   -FDT_ERR_NOTFOUND, 0, 0x0, 0x0);
>>> +	check_node(fdt, "/narrow-bus@2000000/sb-device@80000000",
>>> +		   0, 0, 0x00000800, 0x200);
>>> +
>>> +	PASS();
>>> +}
>>> diff --git a/tests/addresses.dts b/tests/addresses.dts
>>> index a2faaf5..747c291 100644
>>> --- a/tests/addresses.dts
>>> +++ b/tests/addresses.dts
>>> @@ -6,10 +6,30 @@
>>>  	#size-cells = <2>;
>>>  
>>>  	identity-bus@0 {
>>> +		#address-cells = <2>;
>>> +		#size-cells = <2>;
>>> +		id-device@400 {
>>> +			reg = <0x0 0x00000400 0x0 0x00000100>,
>>> +			      <0x4 0x00000000 0x1 0x00000030>;
>>> +		};
>>>  	};
>>>  
>>>  	simple-bus@1000000 {
>>>  		#address-cells = <2>;
>>>  		#size-cells = <1>;
>>> +		sb-device@8000000800 {
>>> +			reg = <0x80 0x00000800 0x200>,
>>> +			      <0x00 0x70000000 0x700>,
>>> +			      <0x10 0x50000000 0x020>;
>>> +		};
>>> +	};
>>> +
>>> +	narrow-bus@2000000 {
>>> +		#address-cells = <1>;
>>> +		#size-cells = <1>;
>>> +		sb-device@80000000 {
>>> +			reg = <0x00000800 0x200>,
>>> +			      <0x50000000 0x020>;
>>> +		};
>>>  	};
>>>  };
>>> diff --git a/tests/run_tests.sh b/tests/run_tests.sh
>>> index 0d30edf..c195d0d 100755
>>> --- a/tests/run_tests.sh
>>> +++ b/tests/run_tests.sh
>>> @@ -311,6 +311,7 @@ libfdt_tests () {
>>>  
>>>      run_dtc_test -I dts -O dtb -o addresses.test.dtb addresses.dts
>>>      run_test addr_size_cells addresses.test.dtb
>>> +    run_test addr_size addresses.test.dtb
>>>  
>>>      run_dtc_test -I dts -O dtb -o stringlist.test.dtb stringlist.dts
>>>      run_test stringlist stringlist.test.dtb
>>>

Re: [PATCH v5] libfdt: add helpers to read address and size from reg

[Andrew F. Davis]

On 02/05/2018 04:47 PM, André Przywara wrote:
> On 05/02/18 20:10, Andrew F. Davis wrote:
>> On 02/05/2018 11:11 AM, Andre Przywara wrote:
>>> Hi,
>>>
>>> thanks for picking this up!
>>>
>>> On 01/02/18 23:04, Andrew F. Davis (by way of Andre Przywara
>>> <andre-s5aTT1PPrfyzQB+pC5nmwQ@public.gmane.org>) wrote:
>>>> From: Benjamin Fair <b-fair-l0cyMroinI0@public.gmane.org>
> 
> ....
> 
>>>> +
>>>> +int fdt_simple_addr_size(const void *fdt, int nodeoffset, int idx,
>>>> +			 uintptr_t *addrp, size_t *sizep)
>>>
>>> I believe this was mentioned in previous reviews:
>>> I don't think that those types are the right ones to use here.
>>> There is no correlation between the address size used in a DT and the
>>> (current) size of pointers or addressable memory.
>>> So you might end up in 32-bit code reading a DT with 64-bit addresses.
>>> MMIO addresses might be under 4GB underway, or you are running using
>>> LPAE, where physical addresses can be 40 bits long.
> 
>> This function checks for these mismatches and returns an error. The
>> point of this helper is for the system about to use these pointers, if
>> you want to manipulate them on a different system or use them later then
>> this helper is not correct for you.
> 
> Imagine you have a TI(!) Keystone SoC, the kernel DT tells me the
> address and size are 64 bit, though the A15 cores will always have
> uintptr_t and size_t at 32 bits. So you can never use this function
> there, even if you would have the MMU using long descriptors, and even
> if you just want to program the GIC to allow non-secure access in a
> small secure firmware shim using libfdt:
>                 compatible = "arm,gic-400", "arm,cortex-a15-gic";
> 		reg = <0x0 0x02561000 0x0 0x1000>, ....
> That would be perfectly accessible with the MMU off.
> Looking further into the .dtsi actually underlines Rob's point of not
> supporting translation being much less useful (though it would happen to
> work in this case, more or less by chance).
> 
>>> So I think we should use uint64_t* here, this would also avoid the
>>> checks below.
>>>
>>
>> What will happen if I am on a 32-bit big-endian system, and I only want
>> to read in a 32-bit address, but the DT node is 64-bit? How will I know
>> that it read in 64-bits?
> 
> if ((base_addr & (uintptr_t)~0) != base_addr)
> 	return -EFBIG;
> 
>> How will I know where the MSB is?
> 
> DT is always big endian. libfdt takes care of that.
> 
>> Do we care about 128-bit?
> 
> No. I am not aware of such a system, especially not one using DT.
> Keystones are real, though. DTs for ARMv8 systems running in AArch32 are
> real as well.
> 
>> Etc.. I don't think this can be made generic for all the
>> combinations, and so for simplicity the address pointer and size must
>> match the system. For other more complex uses you will have to implement
>> your own handling.
> 
> We don't need to be super generic, but at least should care about
> existing systems. I ran into this issue with 32 bit guests on an 64-bit
> hypervisor. The HV tooling generates the same DT and the guest needs to
> deal with that.
> 
> Please note, I actually like this patch and am not asking for a complete
> rewrite: We just change the pointer types and drop the size check: done.
> 


Fair enough, I think we just had different use-case intentions in mind
but I can adapt the returned values outside of this helper for my
specific use.

I'll post a v2 with the changes here shortly.

Thanks,
Andrew


> Or if a range-aware version isn't too far away, we use that.
> 
> Cheers,
> Andre.
> 
>>>
>>>> +{
>>>> +	int parent;
>>>> +	int ac, sc, reg_stride;
>>>> +	int res;
>>>> +	const fdt32_t *reg;
>>>> +
>>>> +	reg = fdt_getprop(fdt, nodeoffset, "reg", &res);
>>>> +	if (res < 0)
>>>> +		return res;
>>>> +
>>>> +	parent = fdt_parent_offset(fdt, nodeoffset);
>>>> +	if (parent == -FDT_ERR_NOTFOUND) /* an node without a parent does
>>>> +					  * not have _any_ number of cells */
>>>> +		return -FDT_ERR_BADNCELLS;
>>>> +	if (parent < 0)
>>>> +		return parent;
>>>> +
>>>> +	ac = fdt_address_cells(fdt, parent);
>>>> +	if (ac < 0)
>>>> +		return ac;
>>>> +	if (ac * sizeof(fdt32_t) > sizeof(*addrp))
>>>> +		return -FDT_ERR_BADNCELLS;
>>>> +
>>>> +	sc = fdt_size_cells(fdt, parent);
>>>> +	if (sc < 0)
>>>> +		return sc;
>>>> +	if (sc * sizeof(fdt32_t) > sizeof(*sizep))
>>>> +		return -FDT_ERR_BADNCELLS;
>>>> +
>>>> +	reg_stride = ac + sc;
>>>> +
>>>> +	/* res is the number of bytes read and must be an even multiple of the
>>>> +	 * sum of address cells and size cells */
>>>> +	if ((res % (reg_stride * sizeof(*reg))) != 0)
>>>> +		return -FDT_ERR_BADVALUE;
>>>> +
>>>> +	if (addrp) {
>>>> +		*addrp = (uintptr_t) _fdt_read_cells(&reg[reg_stride * idx], ac, &res);
>>>> +		if (res < 0)
>>>> +			return res;
>>>> +	}
>>>> +	if (sizep) {
>>>> +		*sizep = (size_t) _fdt_read_cells(&reg[ac + reg_stride * idx], sc, &res);
>>>> +		if (res < 0)
>>>> +			return res;
>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +}
>>>> diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
>>>> index c8c00fa..73f2fea 100644
>>>> --- a/libfdt/libfdt.h
>>>> +++ b/libfdt/libfdt.h
>>>> @@ -1101,6 +1101,39 @@ int fdt_address_cells(const void *fdt, int nodeoffset);
>>>>   */
>>>>  int fdt_size_cells(const void *fdt, int nodeoffset);
>>>>  
>>>> +/**
>>>> + *
>>>> + * fdt_simple_addr_size - read address and/or size from the reg property of a
>>>> + *                        device node.
>>>> + * @fdt: pointer to the device tree blob
>>>> + * @nodeoffset: offset of the node to find the address and/or size from
>>>> + * @idx: which address/size pair to read
>>>> + * @addrp: pointer to where address will be stored (will be overwritten) or NULL
>>>> + * @sizep: pointer to where size will be stored (will be overwritten) or NULL
>>>> + *
>>>> + * When the node has a valid reg property, returns the address and/or size
>>>> + * values stored there. It does not perform any type of translation based on
>>>> + * the parent bus(es).
>>>> + *
>>>> + * NOTE: This function is expensive, as it must scan the device tree
>>>> + * structure from the start to nodeoffset, *twice*, with fdt_parent_offset.
>>>> + *
>>>> + * returns:
>>>> + *	0, on success
>>>> + *	-FDT_ERR_BADVALUE, if there is an unexpected number of entries in the
>>>> + *		reg property
>>>> + *	-FDT_ERR_NOTFOUND, if the node does not have a reg property
>>>> + *	-FDT_ERR_BADNCELLS, if the number of address or size cells is invalid
>>>> + *		or greater than 2 (which is the maximum currently supported)
>>>> + *	-FDT_ERR_BADMAGIC,
>>>> + *	-FDT_ERR_BADSTATE,
>>>> + *	-FDT_ERR_BADSTRUCTURE,
>>>> + *	-FDT_ERR_BADVERSION,
>>>> + *	-FDT_ERR_TRUNCATED, standard meanings
>>>> + */
>>>> +
>>>> +int fdt_simple_addr_size(const void *fdt, int nodeoffset, int idx,
>>>> +			 uintptr_t *addrp, size_t *sizep);
>>>>  
>>>>  /**********************************************************************/
>>>>  /* Write-in-place functions                                           */
>>>> diff --git a/libfdt/version.lds b/libfdt/version.lds
>>>> index 18fb69f..1f19341 100644
>>>> --- a/libfdt/version.lds
>>>> +++ b/libfdt/version.lds
>>>> @@ -59,6 +59,7 @@ LIBFDT_1.2 {
>>>>  		fdt_next_subnode;
>>>>  		fdt_address_cells;
>>>>  		fdt_size_cells;
>>>> +		fdt_simple_addr_size;
>>>>  		fdt_stringlist_contains;
>>>>  		fdt_stringlist_count;
>>>>  		fdt_stringlist_search;
>>>> diff --git a/tests/.gitignore b/tests/.gitignore
>>>> index 9e209d5..acb9335 100644
>>>> --- a/tests/.gitignore
>>>> +++ b/tests/.gitignore
>>>> @@ -43,6 +43,7 @@ tmp.*
>>>>  /path_offset
>>>>  /path_offset_aliases
>>>>  /phandle_format
>>>> +/addr_size
>>>>  /property_iterate
>>>>  /propname_escapes
>>>>  /references
>>>> diff --git a/tests/Makefile.tests b/tests/Makefile.tests
>>>> index 262944a..1e326ea 100644
>>>> --- a/tests/Makefile.tests
>>>> +++ b/tests/Makefile.tests
>>>> @@ -8,7 +8,7 @@ LIB_TESTS_L = get_mem_rsv \
>>>>  	char_literal \
>>>>  	sized_cells \
>>>>  	notfound \
>>>> -	addr_size_cells \
>>>> +	addr_size_cells addr_size \
>>>>  	stringlist \
>>>>  	setprop_inplace nop_property nop_node \
>>>>  	sw_tree1 \
>>>> diff --git a/tests/addr_size.c b/tests/addr_size.c
>>>> new file mode 100644
>>>> index 0000000..10275f4
>>>> --- /dev/null
>>>> +++ b/tests/addr_size.c
>>>> @@ -0,0 +1,93 @@
>>>> +/*
>>>> + * libfdt - Flat Device Tree manipulation
>>>> + *	Testcase for address and size handling
>>>> + * Copyright (C) 2016-2018 Texas Instruments Incorporated - http://www.ti.com/
>>>> + *
>>>> + * Based on addr_size_cells.c by David Gibson, <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>
>>>> + *
>>>> + * This program is free software; you can redistribute it and/or modify it
>>>> + * under the terms of the GNU Lesser General Public License as published by
>>>> + * the Free Software Foundation; either version 2.1 of the License, or (at
>>>> + * your option) any later version.
>>>> + *
>>>> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any kind,
>>>> + * whether express or implied; without even the implied warranty of
>>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>>>> + * Lesser General Public License for more details.
>>>> + */
>>>> +
>>>> +#include <stdlib.h>
>>>> +#include <stdio.h>
>>>> +#include <string.h>
>>>> +#include <stdint.h>
>>>> +
>>>> +#include <libfdt.h>
>>>> +
>>>> +#include "tests.h"
>>>> +#include "testdata.h"
>>>> +
>>>> +static void check_node(const void *fdt, const char *path, int err,
>>>> +		       int idx, uintptr_t addr, size_t size)
>>>> +{
>>>> +	int offset, res;
>>>> +	uintptr_t xaddr;
>>>> +	size_t xsize;
>>>> +
>>>> +	offset = fdt_path_offset(fdt, path);
>>>> +	if (offset < 0)
>>>> +		FAIL("Couldn't find path %s", path);
>>>> +
>>>> +	res = fdt_simple_addr_size(fdt, offset, idx, &xaddr, &xsize);
>>>> +	if (res != err)
>>>> +		FAIL("fdt_simple_addr_size returned %s instead of %s",
>>>> +		     fdt_strerror(res), fdt_strerror(err));
>>>> +	if (res < 0)
>>>> +		return;
>>>> +
>>>> +	if (xaddr != addr)
>>>> +		FAIL("Physical address for %s is %p instead of %p\n",
>>>> +		     path, (void *) xaddr, (void *) addr);
>>>> +
>>>> +	if (xsize != size)
>>>> +		FAIL("Size for %s is %zx instead of %zx\n",
>>>> +		     path, xsize, size);
>>>> +}
>>>> +
>>>> +int main(int argc, char *argv[])
>>>> +{
>>>> +	void *fdt;
>>>> +
>>>> +	if (argc != 2)
>>>> +		CONFIG("Usage: %s <dtb file>\n", argv[0]);
>>>> +
>>>> +	test_init(argc, argv);
>>>> +	fdt = load_blob(argv[1]);
>>>> +
>>>> +	if (sizeof(uintptr_t) == 4 && sizeof(size_t) == 4) {
>>>> +		/* 32-bit address , 32-bit size */
>>>> +		check_node(fdt, "/identity-bus@0/id-device@400",
>>>> +			   -FDT_ERR_BADNCELLS, 0, 0x0, 0x0);
>>>> +		check_node(fdt, "/simple-bus@1000000/sb-device@8000000800",
>>>> +			   -FDT_ERR_BADNCELLS, 0, 0x0, 0x0);
>>>> +	}
>>>> +	else if (sizeof(uintptr_t) == 8 && sizeof(size_t) == 8) {
>>>> +		/* 64-bit uintptr_t , 64-bit size_t */
>>>> +		check_node(fdt, "/identity-bus@0/id-device@400",
>>>> +			   0, 0,0x400, 0x100);
>>>> +		check_node(fdt, "/simple-bus@1000000/sb-device@8000000800",
>>>> +			   0, 0, 0x8000000800, 0x200);
>>>> +		check_node(fdt, "/identity-bus@0/id-device@400",
>>>> +			   0, 1, 0x400000000, 0x100000030);
>>>> +		check_node(fdt, "/simple-bus@1000000/sb-device@8000000800",
>>>> +			   0, 1, 0x70000000, 0x700);
>>>> +		check_node(fdt, "/simple-bus@1000000/sb-device@8000000800",
>>>> +			   0, 2, 0x1050000000, 0x20);
>>>> +	}
>>>> +
>>>> +	check_node(fdt, "/identity-bus@0",
>>>> +		   -FDT_ERR_NOTFOUND, 0, 0x0, 0x0);
>>>> +	check_node(fdt, "/narrow-bus@2000000/sb-device@80000000",
>>>> +		   0, 0, 0x00000800, 0x200);
>>>> +
>>>> +	PASS();
>>>> +}
>>>> diff --git a/tests/addresses.dts b/tests/addresses.dts
>>>> index a2faaf5..747c291 100644
>>>> --- a/tests/addresses.dts
>>>> +++ b/tests/addresses.dts
>>>> @@ -6,10 +6,30 @@
>>>>  	#size-cells = <2>;
>>>>  
>>>>  	identity-bus@0 {
>>>> +		#address-cells = <2>;
>>>> +		#size-cells = <2>;
>>>> +		id-device@400 {
>>>> +			reg = <0x0 0x00000400 0x0 0x00000100>,
>>>> +			      <0x4 0x00000000 0x1 0x00000030>;
>>>> +		};
>>>>  	};
>>>>  
>>>>  	simple-bus@1000000 {
>>>>  		#address-cells = <2>;
>>>>  		#size-cells = <1>;
>>>> +		sb-device@8000000800 {
>>>> +			reg = <0x80 0x00000800 0x200>,
>>>> +			      <0x00 0x70000000 0x700>,
>>>> +			      <0x10 0x50000000 0x020>;
>>>> +		};
>>>> +	};
>>>> +
>>>> +	narrow-bus@2000000 {
>>>> +		#address-cells = <1>;
>>>> +		#size-cells = <1>;
>>>> +		sb-device@80000000 {
>>>> +			reg = <0x00000800 0x200>,
>>>> +			      <0x50000000 0x020>;
>>>> +		};
>>>>  	};
>>>>  };
>>>> diff --git a/tests/run_tests.sh b/tests/run_tests.sh
>>>> index 0d30edf..c195d0d 100755
>>>> --- a/tests/run_tests.sh
>>>> +++ b/tests/run_tests.sh
>>>> @@ -311,6 +311,7 @@ libfdt_tests () {
>>>>  
>>>>      run_dtc_test -I dts -O dtb -o addresses.test.dtb addresses.dts
>>>>      run_test addr_size_cells addresses.test.dtb
>>>> +    run_test addr_size addresses.test.dtb
>>>>  
>>>>      run_dtc_test -I dts -O dtb -o stringlist.test.dtb stringlist.dts
>>>>      run_test stringlist stringlist.test.dtb
>>>>
> 

Re: [PATCH v5] libfdt: add helpers to read address and size from reg

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3119 bytes --]

On Mon, Feb 05, 2018 at 09:24:34AM -0600, Andrew F. Davis wrote:
> On 02/01/2018 10:35 PM, Rob Herring wrote:
> > On Thu, Feb 1, 2018 at 5:04 PM, Andrew F. Davis <afd-l0cyMroinI0@public.gmane.org> wrote:
> >> From: Benjamin Fair <b-fair-l0cyMroinI0@public.gmane.org>
> >>
> >> This patch extends the capability of libfdt to parse the contents of device
> >> trees in a similar manner to fdt_address_cells and fdt_size_cells.
> >>
> >> It adds a helper function which reads the address and size of a device from
> >> the reg property and performs basic sanity checks.
> >>
> >> It does not perform translation to a physical address using the ranges
> >> properties of parents, but this enhancement may be added as a separate
> >> function in the future.
> > 
> > I'm concerned about merging this without translation support. First,
> > I'd question its usefulness without it. Second, it may encourage
> > people to write their DTs without any ranges just so this will work.
> 
> It can be useful for nodes where ranges is not appropriate for whatever
> reason, or more likely as a base for other helpers that do perform
> translation (hence the "simple" in this ones name), it will need to
> still be extended for many use-cases.

Yeah, I have very mixed feelings about this.  If I'd just seen the
patches without the threads associated with them, I'd have been happy
enough.  Although it certainly doesn't cover anything, there are
definitely cases where an untranslated retrieval is useful.  Not least
as something for a translating helper to use underneath, but also
because this can be useful for buses which have no direct mapping into
MMIO space (/cpus, i2c amongst others).

However a number of the comments on versions of the patch - and indeed
the function types on this revision - seem to specifically imply this
is intended for the case where an untranslated fetch is somewhere
between fragile and dead wrong.  Like Rob, I'm concerned that will
encourage poor ways of doing things.

Perhaps a version of this which will error if the parent node has any
'ranges' property (including empty)?  That restricts it to the case of
buses that are not mapped into their parent address space.

> > I tried to upstream the kernel and u-boot implementations to libfdt in
> > April 2014 (I couldn't find an archive). We've got to get it
> > relicensed and then David didn't like the implementation. He said he
> > was about to post his version, but then it derailed into PCI handling.

Ugh.. sorry.  Clearly I completely lost that train of thought.

> > There's another implementation in the kernel powerpc boot code that
> > may be easier to re-license.
> > 
> 
> My worry with adding translation is this kind of feature creep. PCI and
> other odd cases will block this effort with little gain for the users
> where this simple helper is sufficient.
> 
> > Rob
> > 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]