[v16,06/16] lib: fdt: add a helper function for handling memory range property

Message ID 20181115055254.2812-7-takahiro.akashi@linaro.org
State New
Headers show
Series
  • arm64: kexec: add kexec_file_load() support
Related show

Commit Message

Takahiro Akashi Nov. 15, 2018, 5:52 a.m.
Added function, fdt_setprop_reg(), will be used later to handle
kexec-specific property in arm64's kexec_file implementation.
It will possibly be merged into libfdt in the future.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>

Cc: Rob Herring <robh+dt@kernel.org>
Cc: Frank Rowand <frowand.list@gmail.com>
Cc: devicetree@vger.kernel.org
---
 include/linux/libfdt.h | 26 ++++++++++++++++++++
 lib/Makefile           |  2 +-
 lib/fdt_addresses.c    | 56 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 83 insertions(+), 1 deletion(-)
 create mode 100644 lib/fdt_addresses.c

-- 
2.19.0

Comments

Will Deacon Nov. 30, 2018, 1:21 p.m. | #1
[moving some DT people to TO:]

On Thu, Nov 15, 2018 at 02:52:45PM +0900, AKASHI Takahiro wrote:
> Added function, fdt_setprop_reg(), will be used later to handle

> kexec-specific property in arm64's kexec_file implementation.

> It will possibly be merged into libfdt in the future.

> 

> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>

> Cc: Rob Herring <robh+dt@kernel.org>

> Cc: Frank Rowand <frowand.list@gmail.com>

> Cc: devicetree@vger.kernel.org

> ---

>  include/linux/libfdt.h | 26 ++++++++++++++++++++

>  lib/Makefile           |  2 +-

>  lib/fdt_addresses.c    | 56 ++++++++++++++++++++++++++++++++++++++++++

>  3 files changed, 83 insertions(+), 1 deletion(-)

>  create mode 100644 lib/fdt_addresses.c


We need an ack from the DT side before we can merge this series, please.

Will

> diff --git a/include/linux/libfdt.h b/include/linux/libfdt.h

> index 90ed4ebfa692..47c4dc9e135c 100644

> --- a/include/linux/libfdt.h

> +++ b/include/linux/libfdt.h

> @@ -5,4 +5,30 @@

>  #include <linux/libfdt_env.h>

>  #include "../../scripts/dtc/libfdt/libfdt.h"

>  

> +/**

> + * fdt_setprop_reg - add/set a memory region property

> + * @fdt: pointer to the device tree blob

> + * @nodeoffset: offset of the node to add a property at

> + * @name: name of property

> + * @addr: physical start address

> + * @size: size of region

> + *

> + * returns:

> + *	0, on success

> + *      -FDT_ERR_BADLAYOUT,

> + *	-FDT_ERR_BADMAGIC,

> + *	-FDT_ERR_BADNCELLS, if the node has a badly formatted or invalid

> + *		#address-cells property

> + *      -FDT_ERR_BADOFFSET, nodeoffset did not point to FDT_BEGIN_NODE tag

> + *	-FDT_ERR_BADSTATE,

> + *	-FDT_ERR_BADSTRUCTURE,

> + *	-FDT_ERR_BADVERSION,

> + *	-FDT_ERR_BADVALUE, addr or size doesn't fit to respective cells size

> + *      -FDT_ERR_NOSPACE, there is insufficient free space in the blob to

> + *              contain a new property

> + *	-FDT_ERR_TRUNCATED, standard meanings

> + */

> +int fdt_setprop_reg(void *fdt, int nodeoffset, const char *name,

> +					       u64 addr, u64 size);

> +

>  #endif /* _INCLUDE_LIBFDT_H_ */

> diff --git a/lib/Makefile b/lib/Makefile

> index db06d1237898..2a96cb05e15d 100644

> --- a/lib/Makefile

> +++ b/lib/Makefile

> @@ -205,7 +205,7 @@ KASAN_SANITIZE_stackdepot.o := n

>  KCOV_INSTRUMENT_stackdepot.o := n

>  

>  libfdt_files = fdt.o fdt_ro.o fdt_wip.o fdt_rw.o fdt_sw.o fdt_strerror.o \

> -	       fdt_empty_tree.o

> +	       fdt_empty_tree.o fdt_addresses.o

>  $(foreach file, $(libfdt_files), \

>  	$(eval CFLAGS_$(file) = -I$(src)/../scripts/dtc/libfdt))

>  lib-$(CONFIG_LIBFDT) += $(libfdt_files)

> diff --git a/lib/fdt_addresses.c b/lib/fdt_addresses.c

> new file mode 100644

> index 000000000000..97ddd5a5cc10

> --- /dev/null

> +++ b/lib/fdt_addresses.c

> @@ -0,0 +1,56 @@

> +// SPDX-License-Identifier: GPL-2.0

> +#include <linux/libfdt_env.h>

> +#include <linux/types.h>

> +#include "../scripts/dtc/libfdt/fdt_addresses.c"

> +

> +/*

> + * helper functions for arm64 kexec

> + * Those functions may be merged into libfdt in the future.

> + */

> +

> +/* This function assumes that cells is 1 or 2 */

> +static void cpu64_to_fdt_cells(void *buf, u64 val64, int cells)

> +{

> +	__be32 val32;

> +

> +	while (cells) {

> +		val32 = cpu_to_fdt32(val64 >> (32 * (--cells)));

> +		memcpy(buf, &val32, sizeof(val32));

> +		buf += sizeof(val32);

> +	}

> +}

> +

> +int fdt_setprop_reg(void *fdt, int nodeoffset, const char *name,

> +						u64 addr, u64 size)

> +{

> +	int addr_cells, size_cells;

> +	char buf[sizeof(__be32) * 2 * 2];

> +		/* assume dt_root_[addr|size]_cells <= 2 */

> +	void *prop;

> +	size_t buf_size;

> +

> +	addr_cells = fdt_address_cells(fdt, 0);

> +	if (addr_cells < 0)

> +		return addr_cells;

> +	size_cells = fdt_size_cells(fdt, 0);

> +	if (size_cells < 0)

> +		return size_cells;

> +

> +	/* if *_cells >= 2, cells can hold 64-bit values anyway */

> +	if ((addr_cells == 1) && ((addr > U32_MAX) ||

> +				  ((addr + size) > U32_MAX)))

> +		return -FDT_ERR_BADVALUE;

> +

> +	if ((size_cells == 1) && (size > U32_MAX))

> +		return -FDT_ERR_BADVALUE;

> +

> +	buf_size = (addr_cells + size_cells) * sizeof(u32);

> +	prop = buf;

> +

> +	cpu64_to_fdt_cells(prop, addr, addr_cells);

> +	prop += addr_cells * sizeof(u32);

> +

> +	cpu64_to_fdt_cells(prop, size, size_cells);

> +

> +	return fdt_setprop(fdt, nodeoffset, name, buf, buf_size);

> +}

> -- 

> 2.19.0

>
James Morse Dec. 7, 2018, 10:12 a.m. | #2
Hi Akashi, Will,

On 06/12/2018 15:54, Will Deacon wrote:
> On Thu, Dec 06, 2018 at 08:47:04AM -0600, Rob Herring wrote:

>> On Wed, Nov 14, 2018 at 11:52 PM AKASHI Takahiro

>> <takahiro.akashi@linaro.org> wrote:

>>>

>>> Added function, fdt_setprop_reg(), will be used later to handle

>>> kexec-specific property in arm64's kexec_file implementation.

>>> It will possibly be merged into libfdt in the future.

>>

>> You generally can't modify libfdt files. Any changes will be blown

>> away with the next dtc sync (there's one in -next now). Though here

>> you are creating a new location with fdt code. lib/ is just a shim to

>> the actual libfdt code. Don't put any implementation there. You can

>> add this to drivers/of/fdt_address.c for the short term, but it still

>> needs to go upstream.

>>

>> Otherwise, the implementation looks fine to me.

> 

> I agree, but I don't think there's a real need for us to hack

> drivers/of/fdt_address.c in the meantime -- let's just target upstream

> and not carry this in the kernel.

> 

> Akashi -- for now, I'll drop the kdump parts of this series which rely

> on this helper. The majority of the series is actually independent and

> can go in as-is.

> 

> I've pushed out a kexec branch to the arm64 tree for you to take a look

> at:

> 

> https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git/log/?h=kexec


I gave this a quick spin. Without the elfcorehdr/usable-memory-range arm64 needs
to explicitly forbid kdump via kexec_file_load. (like powerpc does already).
Without this kdump works, but the second kernel overwrites the first as those DT
properties are missing.

I'll post a patch momentarily,


Thanks,

James
Takahiro Akashi Dec. 11, 2018, 6:17 a.m. | #3
James,

On Fri, Dec 07, 2018 at 10:12:47AM +0000, James Morse wrote:
> Hi Akashi, Will,

> 

> On 06/12/2018 15:54, Will Deacon wrote:

> > On Thu, Dec 06, 2018 at 08:47:04AM -0600, Rob Herring wrote:

> >> On Wed, Nov 14, 2018 at 11:52 PM AKASHI Takahiro

> >> <takahiro.akashi@linaro.org> wrote:

> >>>

> >>> Added function, fdt_setprop_reg(), will be used later to handle

> >>> kexec-specific property in arm64's kexec_file implementation.

> >>> It will possibly be merged into libfdt in the future.

> >>

> >> You generally can't modify libfdt files. Any changes will be blown

> >> away with the next dtc sync (there's one in -next now). Though here

> >> you are creating a new location with fdt code. lib/ is just a shim to

> >> the actual libfdt code. Don't put any implementation there. You can

> >> add this to drivers/of/fdt_address.c for the short term, but it still

> >> needs to go upstream.

> >>

> >> Otherwise, the implementation looks fine to me.

> > 

> > I agree, but I don't think there's a real need for us to hack

> > drivers/of/fdt_address.c in the meantime -- let's just target upstream

> > and not carry this in the kernel.

> > 

> > Akashi -- for now, I'll drop the kdump parts of this series which rely

> > on this helper. The majority of the series is actually independent and

> > can go in as-is.

> > 

> > I've pushed out a kexec branch to the arm64 tree for you to take a look

> > at:

> > 

> > https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git/log/?h=kexec

> 

> I gave this a quick spin. Without the elfcorehdr/usable-memory-range arm64 needs

> to explicitly forbid kdump via kexec_file_load. (like powerpc does already).


Thank you for pointing this out.

> Without this kdump works, but the second kernel overwrites the first as those DT

> properties are missing.

> 

> I'll post a patch momentarily,


Fine, but anyhow I'm going to submit a new version (*without* kdump),
I will fix the issue along with others.

-Takahiro Akashi


> 

> Thanks,

> 

> James

>
James Morse Dec. 11, 2018, 10:09 a.m. | #4
Hi Akashi,

On 11/12/2018 06:17, AKASHI, Takahiro wrote:
> On Fri, Dec 07, 2018 at 10:12:47AM +0000, James Morse wrote:

>> On 06/12/2018 15:54, Will Deacon wrote:

>>> On Thu, Dec 06, 2018 at 08:47:04AM -0600, Rob Herring wrote:

>>>> On Wed, Nov 14, 2018 at 11:52 PM AKASHI Takahiro

>>>> <takahiro.akashi@linaro.org> wrote:

>>>>>

>>>>> Added function, fdt_setprop_reg(), will be used later to handle

>>>>> kexec-specific property in arm64's kexec_file implementation.

>>>>> It will possibly be merged into libfdt in the future.

>>>>

>>>> You generally can't modify libfdt files. Any changes will be blown

>>>> away with the next dtc sync (there's one in -next now). Though here

>>>> you are creating a new location with fdt code. lib/ is just a shim to

>>>> the actual libfdt code. Don't put any implementation there. You can

>>>> add this to drivers/of/fdt_address.c for the short term, but it still

>>>> needs to go upstream.

>>>>

>>>> Otherwise, the implementation looks fine to me.

>>>

>>> I agree, but I don't think there's a real need for us to hack

>>> drivers/of/fdt_address.c in the meantime -- let's just target upstream

>>> and not carry this in the kernel.

>>>

>>> Akashi -- for now, I'll drop the kdump parts of this series which rely

>>> on this helper. The majority of the series is actually independent and

>>> can go in as-is.

>>>

>>> I've pushed out a kexec branch to the arm64 tree for you to take a look

>>> at:

>>>

>>> https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git/log/?h=kexec

>>

>> I gave this a quick spin. Without the elfcorehdr/usable-memory-range arm64 needs

>> to explicitly forbid kdump via kexec_file_load. (like powerpc does already).

> 

> Thank you for pointing this out.

> 

>> Without this kdump works, but the second kernel overwrites the first as those DT

>> properties are missing.

>>

>> I'll post a patch momentarily,

> 

> Fine, but anyhow I'm going to submit a new version (*without* kdump),

> I will fix the issue along with others.


I had a quick look at the arm64 for-next/core branch. Will has queued the
non-kdump parts of this series.

If you have changes, they need to be against the arm64 tree.


Thanks,

James
Takahiro Akashi Dec. 12, 2018, 1:28 a.m. | #5
On Tue, Dec 11, 2018 at 10:09:07AM +0000, James Morse wrote:
> Hi Akashi,

> 

> On 11/12/2018 06:17, AKASHI, Takahiro wrote:

> > On Fri, Dec 07, 2018 at 10:12:47AM +0000, James Morse wrote:

> >> On 06/12/2018 15:54, Will Deacon wrote:

> >>> On Thu, Dec 06, 2018 at 08:47:04AM -0600, Rob Herring wrote:

> >>>> On Wed, Nov 14, 2018 at 11:52 PM AKASHI Takahiro

> >>>> <takahiro.akashi@linaro.org> wrote:

> >>>>>

> >>>>> Added function, fdt_setprop_reg(), will be used later to handle

> >>>>> kexec-specific property in arm64's kexec_file implementation.

> >>>>> It will possibly be merged into libfdt in the future.

> >>>>

> >>>> You generally can't modify libfdt files. Any changes will be blown

> >>>> away with the next dtc sync (there's one in -next now). Though here

> >>>> you are creating a new location with fdt code. lib/ is just a shim to

> >>>> the actual libfdt code. Don't put any implementation there. You can

> >>>> add this to drivers/of/fdt_address.c for the short term, but it still

> >>>> needs to go upstream.

> >>>>

> >>>> Otherwise, the implementation looks fine to me.

> >>>

> >>> I agree, but I don't think there's a real need for us to hack

> >>> drivers/of/fdt_address.c in the meantime -- let's just target upstream

> >>> and not carry this in the kernel.

> >>>

> >>> Akashi -- for now, I'll drop the kdump parts of this series which rely

> >>> on this helper. The majority of the series is actually independent and

> >>> can go in as-is.

> >>>

> >>> I've pushed out a kexec branch to the arm64 tree for you to take a look

> >>> at:

> >>>

> >>> https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git/log/?h=kexec

> >>

> >> I gave this a quick spin. Without the elfcorehdr/usable-memory-range arm64 needs

> >> to explicitly forbid kdump via kexec_file_load. (like powerpc does already).

> > 

> > Thank you for pointing this out.

> > 

> >> Without this kdump works, but the second kernel overwrites the first as those DT

> >> properties are missing.

> >>

> >> I'll post a patch momentarily,

> > 

> > Fine, but anyhow I'm going to submit a new version (*without* kdump),

> > I will fix the issue along with others.

> 

> I had a quick look at the arm64 for-next/core branch. Will has queued the

> non-kdump parts of this series.

> 

> If you have changes, they need to be against the arm64 tree.


Okay!

-Takahiro Akashi

> 

> Thanks,

> 

> James

Patch

diff --git a/include/linux/libfdt.h b/include/linux/libfdt.h
index 90ed4ebfa692..47c4dc9e135c 100644
--- a/include/linux/libfdt.h
+++ b/include/linux/libfdt.h
@@ -5,4 +5,30 @@ 
 #include <linux/libfdt_env.h>
 #include "../../scripts/dtc/libfdt/libfdt.h"
 
+/**
+ * fdt_setprop_reg - add/set a memory region property
+ * @fdt: pointer to the device tree blob
+ * @nodeoffset: offset of the node to add a property at
+ * @name: name of property
+ * @addr: physical start address
+ * @size: size of region
+ *
+ * returns:
+ *	0, on success
+ *      -FDT_ERR_BADLAYOUT,
+ *	-FDT_ERR_BADMAGIC,
+ *	-FDT_ERR_BADNCELLS, if the node has a badly formatted or invalid
+ *		#address-cells property
+ *      -FDT_ERR_BADOFFSET, nodeoffset did not point to FDT_BEGIN_NODE tag
+ *	-FDT_ERR_BADSTATE,
+ *	-FDT_ERR_BADSTRUCTURE,
+ *	-FDT_ERR_BADVERSION,
+ *	-FDT_ERR_BADVALUE, addr or size doesn't fit to respective cells size
+ *      -FDT_ERR_NOSPACE, there is insufficient free space in the blob to
+ *              contain a new property
+ *	-FDT_ERR_TRUNCATED, standard meanings
+ */
+int fdt_setprop_reg(void *fdt, int nodeoffset, const char *name,
+					       u64 addr, u64 size);
+
 #endif /* _INCLUDE_LIBFDT_H_ */
diff --git a/lib/Makefile b/lib/Makefile
index db06d1237898..2a96cb05e15d 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -205,7 +205,7 @@  KASAN_SANITIZE_stackdepot.o := n
 KCOV_INSTRUMENT_stackdepot.o := n
 
 libfdt_files = fdt.o fdt_ro.o fdt_wip.o fdt_rw.o fdt_sw.o fdt_strerror.o \
-	       fdt_empty_tree.o
+	       fdt_empty_tree.o fdt_addresses.o
 $(foreach file, $(libfdt_files), \
 	$(eval CFLAGS_$(file) = -I$(src)/../scripts/dtc/libfdt))
 lib-$(CONFIG_LIBFDT) += $(libfdt_files)
diff --git a/lib/fdt_addresses.c b/lib/fdt_addresses.c
new file mode 100644
index 000000000000..97ddd5a5cc10
--- /dev/null
+++ b/lib/fdt_addresses.c
@@ -0,0 +1,56 @@ 
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/libfdt_env.h>
+#include <linux/types.h>
+#include "../scripts/dtc/libfdt/fdt_addresses.c"
+
+/*
+ * helper functions for arm64 kexec
+ * Those functions may be merged into libfdt in the future.
+ */
+
+/* This function assumes that cells is 1 or 2 */
+static void cpu64_to_fdt_cells(void *buf, u64 val64, int cells)
+{
+	__be32 val32;
+
+	while (cells) {
+		val32 = cpu_to_fdt32(val64 >> (32 * (--cells)));
+		memcpy(buf, &val32, sizeof(val32));
+		buf += sizeof(val32);
+	}
+}
+
+int fdt_setprop_reg(void *fdt, int nodeoffset, const char *name,
+						u64 addr, u64 size)
+{
+	int addr_cells, size_cells;
+	char buf[sizeof(__be32) * 2 * 2];
+		/* assume dt_root_[addr|size]_cells <= 2 */
+	void *prop;
+	size_t buf_size;
+
+	addr_cells = fdt_address_cells(fdt, 0);
+	if (addr_cells < 0)
+		return addr_cells;
+	size_cells = fdt_size_cells(fdt, 0);
+	if (size_cells < 0)
+		return size_cells;
+
+	/* if *_cells >= 2, cells can hold 64-bit values anyway */
+	if ((addr_cells == 1) && ((addr > U32_MAX) ||
+				  ((addr + size) > U32_MAX)))
+		return -FDT_ERR_BADVALUE;
+
+	if ((size_cells == 1) && (size > U32_MAX))
+		return -FDT_ERR_BADVALUE;
+
+	buf_size = (addr_cells + size_cells) * sizeof(u32);
+	prop = buf;
+
+	cpu64_to_fdt_cells(prop, addr, addr_cells);
+	prop += addr_cells * sizeof(u32);
+
+	cpu64_to_fdt_cells(prop, size, size_cells);
+
+	return fdt_setprop(fdt, nodeoffset, name, buf, buf_size);
+}