[PATCH v15 06/16] of/fdt: add helper functions for handling properties

[PATCH v15 06/16] of/fdt: add helper functions for handling properties

[AKASHI Takahiro]

These functions will be used later to handle kexec-specific properties
in arm64's kexec_file implementation.

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
---
 drivers/of/fdt.c       | 56 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/of_fdt.h |  4 +++
 2 files changed, 60 insertions(+)

diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index 800ad252cf9c..c65c31562ccb 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -25,6 +25,7 @@
 #include <linux/debugfs.h>
 #include <linux/serial_core.h>
 #include <linux/sysfs.h>
+#include <linux/types.h>
 
 #include <asm/setup.h>  /* for COMMAND_LINE_SIZE */
 #include <asm/page.h>
@@ -1323,3 +1324,58 @@ late_initcall(of_fdt_raw_init);
 #endif
 
 #endif /* CONFIG_OF_EARLY_FLATTREE */
+
+#define FDT_ALIGN(x, a)	(((x) + (a) - 1) & ~((a) - 1))
+#define FDT_TAGALIGN(x)	(FDT_ALIGN((x), FDT_TAGSIZE))
+
+int fdt_prop_len(const char *prop_name, int len)
+{
+	return (strlen(prop_name) + 1) +
+		sizeof(struct fdt_property) +
+		FDT_TAGALIGN(len);
+}
+
+static void fill_property(void *buf, u64 val64, int cells)
+{
+	__be32 val32;
+
+	while (cells) {
+		val32 = cpu_to_fdt32((val64 >> (32 * (--cells))) & U32_MAX);
+		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))
+		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;
+
+	fill_property(prop, addr, addr_cells);
+	prop += addr_cells * sizeof(u32);
+
+	fill_property(prop, size, size_cells);
+
+	return fdt_setprop(fdt, nodeoffset, name, buf, buf_size);
+}
diff --git a/include/linux/of_fdt.h b/include/linux/of_fdt.h
index b9cd9ebdf9b9..842af6ea92ea 100644
--- a/include/linux/of_fdt.h
+++ b/include/linux/of_fdt.h
@@ -108,5 +108,9 @@ static inline void unflatten_device_tree(void) {}
 static inline void unflatten_and_copy_device_tree(void) {}
 #endif /* CONFIG_OF_EARLY_FLATTREE */
 
+int fdt_prop_len(const char *prop_name, int len);
+int fdt_setprop_reg(void *fdt, int nodeoffset, const char *name,
+						u64 addr, u64 size);
+
 #endif /* __ASSEMBLY__ */
 #endif /* _LINUX_OF_FDT_H */
-- 
2.19.0

Re: [PATCH v15 06/16] of/fdt: add helper functions for handling properties

[kbuild test robot]

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

Hi AKASHI,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on arm64/for-next/core]
[also build test ERROR on v4.19-rc5 next-20180928]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/AKASHI-Takahiro/arm64-kexec-add-kexec_file_load-support/20180928-151042
base:   https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-next/core
config: i386-randconfig-s0-201838 (attached as .config)
compiler: gcc-6 (Debian 6.4.0-9) 6.4.0 20171026
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   drivers/of/fdt.o: In function `fdt_setprop_reg':
>> drivers/of/fdt.c:1358: undefined reference to `fdt_address_cells'
>> drivers/of/fdt.c:1361: undefined reference to `fdt_size_cells'

vim +1358 drivers/of/fdt.c

  1348	
  1349	int fdt_setprop_reg(void *fdt, int nodeoffset, const char *name,
  1350							u64 addr, u64 size)
  1351	{
  1352		int addr_cells, size_cells;
  1353		char buf[sizeof(__be32) * 2 * 2];
  1354			/* assume dt_root_[addr|size]_cells <= 2 */
  1355		void *prop;
  1356		size_t buf_size;
  1357	
> 1358		addr_cells = fdt_address_cells(fdt, 0);
  1359		if (addr_cells < 0)
  1360			return addr_cells;
> 1361		size_cells = fdt_size_cells(fdt, 0);

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 31880 bytes --]

Re: [PATCH v15 06/16] of/fdt: add helper functions for handling properties

[Rob Herring]

+David Gibson

On Fri, Sep 28, 2018 at 1:48 AM AKASHI Takahiro
<takahiro.akashi@linaro.org> wrote:
>
> These functions will be used later to handle kexec-specific properties
> in arm64's kexec_file implementation.
>
> 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
> ---
>  drivers/of/fdt.c       | 56 ++++++++++++++++++++++++++++++++++++++++++
>  include/linux/of_fdt.h |  4 +++
>  2 files changed, 60 insertions(+)
>
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index 800ad252cf9c..c65c31562ccb 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -25,6 +25,7 @@
>  #include <linux/debugfs.h>
>  #include <linux/serial_core.h>
>  #include <linux/sysfs.h>
> +#include <linux/types.h>
>
>  #include <asm/setup.h>  /* for COMMAND_LINE_SIZE */
>  #include <asm/page.h>
> @@ -1323,3 +1324,58 @@ late_initcall(of_fdt_raw_init);
>  #endif
>
>  #endif /* CONFIG_OF_EARLY_FLATTREE */
> +
> +#define FDT_ALIGN(x, a)        (((x) + (a) - 1) & ~((a) - 1))
> +#define FDT_TAGALIGN(x)        (FDT_ALIGN((x), FDT_TAGSIZE))
> +
> +int fdt_prop_len(const char *prop_name, int len)
> +{
> +       return (strlen(prop_name) + 1) +
> +               sizeof(struct fdt_property) +
> +               FDT_TAGALIGN(len);

Looks like you are using this to calculate how much space you need to
allocate in addition to the current DTB for a couple of new or
replaced properties. I'm not sure that this calculation is completely
accurate. And it is strange there doesn't seem to be any libfdt
function for this already. It would be simpler to just add some fixed
additional amount.

Maybe David G has comments on this?

> +}
> +

The rest of this should go in drivers/of/fdt_address.c. Ultimately, it
should go into libfdt, but I'm fine with having it in the kernel for
now.

> +static void fill_property(void *buf, u64 val64, int cells)
> +{
> +       __be32 val32;
> +
> +       while (cells) {
> +               val32 = cpu_to_fdt32((val64 >> (32 * (--cells))) & U32_MAX);
> +               memcpy(buf, &val32, sizeof(val32));
> +               buf += sizeof(val32);

This is kind of hard to read. I would copy u-boot's fdt_pack_reg function.

BTW, for purposes of moving to libfdt, we'll need the authors'
(Masahiro Yamada and Hans de Goede) permission to dual license.

> +       }
> +}
> +
> +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))
> +               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;
> +
> +       fill_property(prop, addr, addr_cells);
> +       prop += addr_cells * sizeof(u32);
> +
> +       fill_property(prop, size, size_cells);
> +
> +       return fdt_setprop(fdt, nodeoffset, name, buf, buf_size);
> +}
> diff --git a/include/linux/of_fdt.h b/include/linux/of_fdt.h
> index b9cd9ebdf9b9..842af6ea92ea 100644
> --- a/include/linux/of_fdt.h
> +++ b/include/linux/of_fdt.h
> @@ -108,5 +108,9 @@ static inline void unflatten_device_tree(void) {}
>  static inline void unflatten_and_copy_device_tree(void) {}
>  #endif /* CONFIG_OF_EARLY_FLATTREE */
>
> +int fdt_prop_len(const char *prop_name, int len);
> +int fdt_setprop_reg(void *fdt, int nodeoffset, const char *name,
> +                                               u64 addr, u64 size);
> +
>  #endif /* __ASSEMBLY__ */
>  #endif /* _LINUX_OF_FDT_H */
> --
> 2.19.0
>

Re: [PATCH v15 06/16] of/fdt: add helper functions for handling properties

[Frank Rowand]

On 09/28/18 06:44, Rob Herring wrote:
> +David Gibson
> 
> On Fri, Sep 28, 2018 at 1:48 AM AKASHI Takahiro
> <takahiro.akashi@linaro.org> wrote:
>>
>> These functions will be used later to handle kexec-specific properties
>> in arm64's kexec_file implementation.

As I requested in version 14:

  The intent of the helper functions is related to properties whose values are
   tuples of the same format as the "reg" property of the "/memory" nodes.  For
   example, the "linux,usable-memory-range" and "linux,elfcoredhr" properties of
   the "/chosen" node.

   The patch header and the function names should be updated to reflect this intent.


>>
>> 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
>> ---

Missing list of changes since version 14.


>>  drivers/of/fdt.c       | 56 ++++++++++++++++++++++++++++++++++++++++++
>>  include/linux/of_fdt.h |  4 +++
>>  2 files changed, 60 insertions(+)
>>
>> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
>> index 800ad252cf9c..c65c31562ccb 100644
>> --- a/drivers/of/fdt.c
>> +++ b/drivers/of/fdt.c
>> @@ -25,6 +25,7 @@
>>  #include <linux/debugfs.h>
>>  #include <linux/serial_core.h>
>>  #include <linux/sysfs.h>
>> +#include <linux/types.h>
>>
>>  #include <asm/setup.h>  /* for COMMAND_LINE_SIZE */
>>  #include <asm/page.h>
>> @@ -1323,3 +1324,58 @@ late_initcall(of_fdt_raw_init);
>>  #endif
>>
>>  #endif /* CONFIG_OF_EARLY_FLATTREE */

In v14 I requested:

   Please add comment:

   /* helper functions for arm64 kexec */


>> +
>> +#define FDT_ALIGN(x, a)        (((x) + (a) - 1) & ~((a) - 1))
>> +#define FDT_TAGALIGN(x)        (FDT_ALIGN((x), FDT_TAGSIZE))
>> +
>> +int fdt_prop_len(const char *prop_name, int len)

In v14, I requested:

   Please rename as fdt_len_added_prop()

I'm not really happy with my suggested name, but do not have a
better one yet.  As Rob notes, maybe David G will have a helpful
comment.


>> +{
>> +       return (strlen(prop_name) + 1) +
>> +               sizeof(struct fdt_property) +
>> +               FDT_TAGALIGN(len);
> 
> Looks like you are using this to calculate how much space you need to
> allocate in addition to the current DTB for a couple of new or
> replaced properties. I'm not sure that this calculation is completely
> accurate. And it is strange there doesn't seem to be any libfdt
> function for this already. It would be simpler to just add some fixed
> additional amount.
> 
> Maybe David G has comments on this?
> 
>> +}
>> +
> 
> The rest of this should go in drivers/of/fdt_address.c. Ultimately, it
> should go into libfdt, but I'm fine with having it in the kernel for
> now.
> 
>> +static void fill_property(void *buf, u64 val64, int cells)

In v14 I requested:

Please rename as cpu64_to_fdt_cells()


>> +{
>> +       __be32 val32;
>> +
>> +       while (cells) {
>> +               val32 = cpu_to_fdt32((val64 >> (32 * (--cells))) & U32_MAX);
>> +               memcpy(buf, &val32, sizeof(val32));
>> +               buf += sizeof(val32);
> 
> This is kind of hard to read. I would copy u-boot's fdt_pack_reg function.
> 
> BTW, for purposes of moving to libfdt, we'll need the authors'
> (Masahiro Yamada and Hans de Goede) permission to dual license.
> 
>> +       }
>> +}
>> +
>> +int fdt_setprop_reg(void *fdt, int nodeoffset, const char *name,
>> +                                               u64 addr, u64 size)
>> +{
>> +       int addr_cells, size_cells;

unsigned


>> +       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))
>> +               return -FDT_ERR_BADVALUE;
>> +
>> +       if ((size_cells == 1) && (size > U32_MAX))
>> +               return -FDT_ERR_BADVALUE;

In v14 I requested:

   Should also check that base + size does not wrap around.


>> +
>> +       buf_size = (addr_cells + size_cells) * sizeof(u32);
>> +       prop = buf;
>> +
>> +       fill_property(prop, addr, addr_cells);
>> +       prop += addr_cells * sizeof(u32);
>> +
>> +       fill_property(prop, size, size_cells);
>> +
>> +       return fdt_setprop(fdt, nodeoffset, name, buf, buf_size);
>> +}
>> diff --git a/include/linux/of_fdt.h b/include/linux/of_fdt.h
>> index b9cd9ebdf9b9..842af6ea92ea 100644
>> --- a/include/linux/of_fdt.h
>> +++ b/include/linux/of_fdt.h
>> @@ -108,5 +108,9 @@ static inline void unflatten_device_tree(void) {}
>>  static inline void unflatten_and_copy_device_tree(void) {}
>>  #endif /* CONFIG_OF_EARLY_FLATTREE */
>>
>> +int fdt_prop_len(const char *prop_name, int len);
>> +int fdt_setprop_reg(void *fdt, int nodeoffset, const char *name,
>> +                                               u64 addr, u64 size);
>> +
>>  #endif /* __ASSEMBLY__ */
>>  #endif /* _LINUX_OF_FDT_H */
>> --
>> 2.19.0
>>
> 

Re: [PATCH v15 06/16] of/fdt: add helper functions for handling properties

[AKASHI, Takahiro]

Frank,

# I haven't reply to your comments.

On Fri, Sep 28, 2018 at 02:13:58PM -0700, Frank Rowand wrote:
> On 09/28/18 06:44, Rob Herring wrote:
> > +David Gibson
> > 
> > On Fri, Sep 28, 2018 at 1:48 AM AKASHI Takahiro
> > <takahiro.akashi@linaro.org> wrote:
> >>
> >> These functions will be used later to handle kexec-specific properties
> >> in arm64's kexec_file implementation.
> 
> As I requested in version 14:
> 
>   The intent of the helper functions is related to properties whose values are
>    tuples of the same format as the "reg" property of the "/memory" nodes.  For
>    example, the "linux,usable-memory-range" and "linux,elfcoredhr" properties of
>    the "/chosen" node.
> 
>    The patch header and the function names should be updated to reflect this intent.

I agree regarding the patch header.

> 
> >>
> >> 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
> >> ---
> 
> Missing list of changes since version 14.

Sorry for the inconvenience, but a whole change list goes into
the cover letter, not individual patches.

> 
> >>  drivers/of/fdt.c       | 56 ++++++++++++++++++++++++++++++++++++++++++
> >>  include/linux/of_fdt.h |  4 +++
> >>  2 files changed, 60 insertions(+)
> >>
> >> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> >> index 800ad252cf9c..c65c31562ccb 100644
> >> --- a/drivers/of/fdt.c
> >> +++ b/drivers/of/fdt.c
> >> @@ -25,6 +25,7 @@
> >>  #include <linux/debugfs.h>
> >>  #include <linux/serial_core.h>
> >>  #include <linux/sysfs.h>
> >> +#include <linux/types.h>
> >>
> >>  #include <asm/setup.h>  /* for COMMAND_LINE_SIZE */
> >>  #include <asm/page.h>
> >> @@ -1323,3 +1324,58 @@ late_initcall(of_fdt_raw_init);
> >>  #endif
> >>
> >>  #endif /* CONFIG_OF_EARLY_FLATTREE */
> 
> In v14 I requested:
> 
>    Please add comment:
> 
>    /* helper functions for arm64 kexec */

Okay.

> 
> >> +
> >> +#define FDT_ALIGN(x, a)        (((x) + (a) - 1) & ~((a) - 1))
> >> +#define FDT_TAGALIGN(x)        (FDT_ALIGN((x), FDT_TAGSIZE))
> >> +
> >> +int fdt_prop_len(const char *prop_name, int len)
> 
> In v14, I requested:
> 
>    Please rename as fdt_len_added_prop()

Anyhow, I will drop this function, preferring to new
fdt_[address|size]_cells().

> I'm not really happy with my suggested name, but do not have a
> better one yet.  As Rob notes, maybe David G will have a helpful
> comment.
> 
> >> +{
> >> +       return (strlen(prop_name) + 1) +
> >> +               sizeof(struct fdt_property) +
> >> +               FDT_TAGALIGN(len);
> > 
> > Looks like you are using this to calculate how much space you need to
> > allocate in addition to the current DTB for a couple of new or
> > replaced properties. I'm not sure that this calculation is completely
> > accurate. And it is strange there doesn't seem to be any libfdt
> > function for this already. It would be simpler to just add some fixed
> > additional amount.
> > 
> > Maybe David G has comments on this?

I'm not quit sure why it's not that accurate, but as I said in a reply to
David's comment, I will take your suggestion.

> >> +}
> >> +
> > 
> > The rest of this should go in drivers/of/fdt_address.c. Ultimately, it
> > should go into libfdt, but I'm fine with having it in the kernel for
> > now.
> > 
> >> +static void fill_property(void *buf, u64 val64, int cells)
> 
> In v14 I requested:
> 
> Please rename as cpu64_to_fdt_cells()

I don't mind, but this function may be dropped if Rob sticks to
u-boot's fdt_pack_reg() over my fdt_setprop_reg().

> 
> >> +{
> >> +       __be32 val32;
> >> +
> >> +       while (cells) {
> >> +               val32 = cpu_to_fdt32((val64 >> (32 * (--cells))) & U32_MAX);
> >> +               memcpy(buf, &val32, sizeof(val32));
> >> +               buf += sizeof(val32);
> > 
> > This is kind of hard to read. I would copy u-boot's fdt_pack_reg function.
> > 
> > BTW, for purposes of moving to libfdt, we'll need the authors'
> > (Masahiro Yamada and Hans de Goede) permission to dual license.
> > 
> >> +       }
> >> +}
> >> +
> >> +int fdt_setprop_reg(void *fdt, int nodeoffset, const char *name,
> >> +                                               u64 addr, u64 size)
> >> +{
> >> +       int addr_cells, size_cells;
> 
> unsigned

fdt_[address|size]_cell() returns an int.

> 
> >> +       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))
> >> +               return -FDT_ERR_BADVALUE;
> >> +
> >> +       if ((size_cells == 1) && (size > U32_MAX))
> >> +               return -FDT_ERR_BADVALUE;
> 
> In v14 I requested:
> 
>    Should also check that base + size does not wrap around.

Okay, I will start discussion, as you have suggested, in devicetree-spec ML.

Thanks,
-Takahiro Akashi

> 
> >> +
> >> +       buf_size = (addr_cells + size_cells) * sizeof(u32);
> >> +       prop = buf;
> >> +
> >> +       fill_property(prop, addr, addr_cells);
> >> +       prop += addr_cells * sizeof(u32);
> >> +
> >> +       fill_property(prop, size, size_cells);
> >> +
> >> +       return fdt_setprop(fdt, nodeoffset, name, buf, buf_size);
> >> +}
> >> diff --git a/include/linux/of_fdt.h b/include/linux/of_fdt.h
> >> index b9cd9ebdf9b9..842af6ea92ea 100644
> >> --- a/include/linux/of_fdt.h
> >> +++ b/include/linux/of_fdt.h
> >> @@ -108,5 +108,9 @@ static inline void unflatten_device_tree(void) {}
> >>  static inline void unflatten_and_copy_device_tree(void) {}
> >>  #endif /* CONFIG_OF_EARLY_FLATTREE */
> >>
> >> +int fdt_prop_len(const char *prop_name, int len);
> >> +int fdt_setprop_reg(void *fdt, int nodeoffset, const char *name,
> >> +                                               u64 addr, u64 size);
> >> +
> >>  #endif /* __ASSEMBLY__ */
> >>  #endif /* _LINUX_OF_FDT_H */
> >> --
> >> 2.19.0
> >>
> > 
> 

Re: [PATCH v15 06/16] of/fdt: add helper functions for handling properties

[David Gibson]

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

On Fri, Oct 05, 2018 at 02:06:29PM +0900, AKASHI, Takahiro wrote:
[snip]
> > >> +{
> > >> +       return (strlen(prop_name) + 1) +
> > >> +               sizeof(struct fdt_property) +
> > >> +               FDT_TAGALIGN(len);
> > > 
> > > Looks like you are using this to calculate how much space you need to
> > > allocate in addition to the current DTB for a couple of new or
> > > replaced properties. I'm not sure that this calculation is completely
> > > accurate. And it is strange there doesn't seem to be any libfdt
> > > function for this already. It would be simpler to just add some fixed
> > > additional amount.
> > > 
> > > Maybe David G has comments on this?
> 
> I'm not quit sure why it's not that accurate, but as I said in a reply to
> David's comment, I will take your suggestion.

I think it is accurate, but it is not robust.  It might not be
accurate if we ever make further tweaks to the libfdt format.  libfdt
is supposed to abstract those changes away, but if you make
assumptions like this, you break that abstraction.

-- 
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 --]

Re: [PATCH v15 06/16] of/fdt: add helper functions for handling properties

[Frank Rowand]

On 10/04/18 22:06, AKASHI, Takahiro wrote:
> Frank,
> 
> # I haven't reply to your comments.
> 
> On Fri, Sep 28, 2018 at 02:13:58PM -0700, Frank Rowand wrote:
>> On 09/28/18 06:44, Rob Herring wrote:
>>> +David Gibson
>>>
>>> On Fri, Sep 28, 2018 at 1:48 AM AKASHI Takahiro
>>> <takahiro.akashi@linaro.org> wrote:
>>>>
>>>> These functions will be used later to handle kexec-specific properties
>>>> in arm64's kexec_file implementation.
>>
>> As I requested in version 14:
>>
>>   The intent of the helper functions is related to properties whose values are
>>    tuples of the same format as the "reg" property of the "/memory" nodes.  For
>>    example, the "linux,usable-memory-range" and "linux,elfcoredhr" properties of
>>    the "/chosen" node.
>>
>>    The patch header and the function names should be updated to reflect this intent.
> 
> I agree regarding the patch header.
> 
>>
>>>>
>>>> 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
>>>> ---
>>
>> Missing list of changes since version 14.
> 
> Sorry for the inconvenience, but a whole change list goes into
> the cover letter, not individual patches.

That works for me, but please add us to the distro list for the cover letter.


>>>>  drivers/of/fdt.c       | 56 ++++++++++++++++++++++++++++++++++++++++++
>>>>  include/linux/of_fdt.h |  4 +++
>>>>  2 files changed, 60 insertions(+)
>>>>
>>>> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
>>>> index 800ad252cf9c..c65c31562ccb 100644
>>>> --- a/drivers/of/fdt.c
>>>> +++ b/drivers/of/fdt.c
>>>> @@ -25,6 +25,7 @@
>>>>  #include <linux/debugfs.h>
>>>>  #include <linux/serial_core.h>
>>>>  #include <linux/sysfs.h>
>>>> +#include <linux/types.h>
>>>>
>>>>  #include <asm/setup.h>  /* for COMMAND_LINE_SIZE */
>>>>  #include <asm/page.h>
>>>> @@ -1323,3 +1324,58 @@ late_initcall(of_fdt_raw_init);
>>>>  #endif
>>>>
>>>>  #endif /* CONFIG_OF_EARLY_FLATTREE */
>>
>> In v14 I requested:
>>
>>    Please add comment:
>>
>>    /* helper functions for arm64 kexec */
> 
> Okay.
> 
>>
>>>> +
>>>> +#define FDT_ALIGN(x, a)        (((x) + (a) - 1) & ~((a) - 1))
>>>> +#define FDT_TAGALIGN(x)        (FDT_ALIGN((x), FDT_TAGSIZE))
>>>> +
>>>> +int fdt_prop_len(const char *prop_name, int len)
>>
>> In v14, I requested:
>>
>>    Please rename as fdt_len_added_prop()
> 
> Anyhow, I will drop this function, preferring to new
> fdt_[address|size]_cells().
> 
>> I'm not really happy with my suggested name, but do not have a
>> better one yet.  As Rob notes, maybe David G will have a helpful
>> comment.
>>
>>>> +{
>>>> +       return (strlen(prop_name) + 1) +
>>>> +               sizeof(struct fdt_property) +
>>>> +               FDT_TAGALIGN(len);
>>>
>>> Looks like you are using this to calculate how much space you need to
>>> allocate in addition to the current DTB for a couple of new or
>>> replaced properties. I'm not sure that this calculation is completely
>>> accurate. And it is strange there doesn't seem to be any libfdt
>>> function for this already. It would be simpler to just add some fixed
>>> additional amount.
>>>
>>> Maybe David G has comments on this?
> 
> I'm not quit sure why it's not that accurate, but as I said in a reply to
> David's comment, I will take your suggestion.
> 
>>>> +}
>>>> +
>>>
>>> The rest of this should go in drivers/of/fdt_address.c. Ultimately, it
>>> should go into libfdt, but I'm fine with having it in the kernel for
>>> now.
>>>
>>>> +static void fill_property(void *buf, u64 val64, int cells)
>>
>> In v14 I requested:
>>
>> Please rename as cpu64_to_fdt_cells()
> 
> I don't mind, but this function may be dropped if Rob sticks to
> u-boot's fdt_pack_reg() over my fdt_setprop_reg().

I have another comment a couple of minutes ago in the longer thread
about this function.


>>
>>>> +{
>>>> +       __be32 val32;
>>>> +
>>>> +       while (cells) {
>>>> +               val32 = cpu_to_fdt32((val64 >> (32 * (--cells))) & U32_MAX);
>>>> +               memcpy(buf, &val32, sizeof(val32));
>>>> +               buf += sizeof(val32);
>>>
>>> This is kind of hard to read. I would copy u-boot's fdt_pack_reg function.
>>>
>>> BTW, for purposes of moving to libfdt, we'll need the authors'
>>> (Masahiro Yamada and Hans de Goede) permission to dual license.
>>>
>>>> +       }
>>>> +}
>>>> +
>>>> +int fdt_setprop_reg(void *fdt, int nodeoffset, const char *name,
>>>> +                                               u64 addr, u64 size)
>>>> +{
>>>> +       int addr_cells, size_cells;
>>
>> unsigned
> 
> fdt_[address|size]_cell() returns an int.

I stand corrected.  They take advantage of the fact that valid values
are in the range 0..4 and return a negative value for error.


>>>> +       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))
>>>> +               return -FDT_ERR_BADVALUE;
>>>> +
>>>> +       if ((size_cells == 1) && (size > U32_MAX))
>>>> +               return -FDT_ERR_BADVALUE;
>>
>> In v14 I requested:
>>
>>    Should also check that base + size does not wrap around.
> 
> Okay, I will start discussion, as you have suggested, in devicetree-spec ML.
> 
> Thanks,
> -Takahiro Akashi
> 
>>
>>>> +
>>>> +       buf_size = (addr_cells + size_cells) * sizeof(u32);
>>>> +       prop = buf;
>>>> +
>>>> +       fill_property(prop, addr, addr_cells);
>>>> +       prop += addr_cells * sizeof(u32);
>>>> +
>>>> +       fill_property(prop, size, size_cells);
>>>> +
>>>> +       return fdt_setprop(fdt, nodeoffset, name, buf, buf_size);
>>>> +}
>>>> diff --git a/include/linux/of_fdt.h b/include/linux/of_fdt.h
>>>> index b9cd9ebdf9b9..842af6ea92ea 100644
>>>> --- a/include/linux/of_fdt.h
>>>> +++ b/include/linux/of_fdt.h
>>>> @@ -108,5 +108,9 @@ static inline void unflatten_device_tree(void) {}
>>>>  static inline void unflatten_and_copy_device_tree(void) {}
>>>>  #endif /* CONFIG_OF_EARLY_FLATTREE */
>>>>
>>>> +int fdt_prop_len(const char *prop_name, int len);
>>>> +int fdt_setprop_reg(void *fdt, int nodeoffset, const char *name,
>>>> +                                               u64 addr, u64 size);
>>>> +
>>>>  #endif /* __ASSEMBLY__ */
>>>>  #endif /* _LINUX_OF_FDT_H */
>>>> --
>>>> 2.19.0
>>>>
>>>
>>
> 

Re: [PATCH v15 06/16] of/fdt: add helper functions for handling properties

[David Gibson]

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

On Tue, Oct 09, 2018 at 11:02:29AM -0700, Frank Rowand wrote:
> On 10/04/18 22:06, AKASHI, Takahiro wrote:
[snip]
> >>>> +int fdt_setprop_reg(void *fdt, int nodeoffset, const char *name,
> >>>> +                                               u64 addr, u64 size)
> >>>> +{
> >>>> +       int addr_cells, size_cells;
> >>
> >> unsigned
> > 
> > fdt_[address|size]_cell() returns an int.
> 
> I stand corrected.  They take advantage of the fact that valid values
> are in the range 0..4 and return a negative value for error.

I'm not sure if the spec formally defines that these values will fit
in a positive 'int'.  But if addresses are 8GiB each, you're going to
have much bigger problems putting it in a device tree than this
function.

-- 
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 --]

Re: [PATCH v15 06/16] of/fdt: add helper functions for handling properties

[AKASHI, Takahiro]

Frank,

On Tue, Oct 09, 2018 at 11:02:29AM -0700, Frank Rowand wrote:
> On 10/04/18 22:06, AKASHI, Takahiro wrote:
> > Frank,
> > 
> > # I haven't reply to your comments.
> > 
> > On Fri, Sep 28, 2018 at 02:13:58PM -0700, Frank Rowand wrote:
> >> On 09/28/18 06:44, Rob Herring wrote:
> >>> +David Gibson
> >>>
> >>> On Fri, Sep 28, 2018 at 1:48 AM AKASHI Takahiro
> >>> <takahiro.akashi@linaro.org> wrote:
> >>>>
> >>>> These functions will be used later to handle kexec-specific properties
> >>>> in arm64's kexec_file implementation.
> >>
> >> As I requested in version 14:
> >>
> >>   The intent of the helper functions is related to properties whose values are
> >>    tuples of the same format as the "reg" property of the "/memory" nodes.  For
> >>    example, the "linux,usable-memory-range" and "linux,elfcoredhr" properties of
> >>    the "/chosen" node.
> >>
> >>    The patch header and the function names should be updated to reflect this intent.
> > 
> > I agree regarding the patch header.
> > 
> >>
> >>>>
> >>>> 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
> >>>> ---
> >>
> >> Missing list of changes since version 14.
> > 
> > Sorry for the inconvenience, but a whole change list goes into
> > the cover letter, not individual patches.
> 
> That works for me, but please add us to the distro list for the cover letter.

Sure, I will try to do so.

> 
> >>>>  drivers/of/fdt.c       | 56 ++++++++++++++++++++++++++++++++++++++++++
> >>>>  include/linux/of_fdt.h |  4 +++
> >>>>  2 files changed, 60 insertions(+)
> >>>>
> >>>> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> >>>> index 800ad252cf9c..c65c31562ccb 100644
> >>>> --- a/drivers/of/fdt.c
> >>>> +++ b/drivers/of/fdt.c
> >>>> @@ -25,6 +25,7 @@
> >>>>  #include <linux/debugfs.h>
> >>>>  #include <linux/serial_core.h>
> >>>>  #include <linux/sysfs.h>
> >>>> +#include <linux/types.h>
> >>>>
> >>>>  #include <asm/setup.h>  /* for COMMAND_LINE_SIZE */
> >>>>  #include <asm/page.h>
> >>>> @@ -1323,3 +1324,58 @@ late_initcall(of_fdt_raw_init);
> >>>>  #endif
> >>>>
> >>>>  #endif /* CONFIG_OF_EARLY_FLATTREE */
> >>
> >> In v14 I requested:
> >>
> >>    Please add comment:
> >>
> >>    /* helper functions for arm64 kexec */
> > 
> > Okay.
> > 
> >>
> >>>> +
> >>>> +#define FDT_ALIGN(x, a)        (((x) + (a) - 1) & ~((a) - 1))
> >>>> +#define FDT_TAGALIGN(x)        (FDT_ALIGN((x), FDT_TAGSIZE))
> >>>> +
> >>>> +int fdt_prop_len(const char *prop_name, int len)
> >>
> >> In v14, I requested:
> >>
> >>    Please rename as fdt_len_added_prop()
> > 
> > Anyhow, I will drop this function, preferring to new
> > fdt_[address|size]_cells().
> > 
> >> I'm not really happy with my suggested name, but do not have a
> >> better one yet.  As Rob notes, maybe David G will have a helpful
> >> comment.
> >>
> >>>> +{
> >>>> +       return (strlen(prop_name) + 1) +
> >>>> +               sizeof(struct fdt_property) +
> >>>> +               FDT_TAGALIGN(len);
> >>>
> >>> Looks like you are using this to calculate how much space you need to
> >>> allocate in addition to the current DTB for a couple of new or
> >>> replaced properties. I'm not sure that this calculation is completely
> >>> accurate. And it is strange there doesn't seem to be any libfdt
> >>> function for this already. It would be simpler to just add some fixed
> >>> additional amount.
> >>>
> >>> Maybe David G has comments on this?
> > 
> > I'm not quit sure why it's not that accurate, but as I said in a reply to
> > David's comment, I will take your suggestion.
> > 
> >>>> +}
> >>>> +
> >>>
> >>> The rest of this should go in drivers/of/fdt_address.c. Ultimately, it
> >>> should go into libfdt, but I'm fine with having it in the kernel for
> >>> now.
> >>>
> >>>> +static void fill_property(void *buf, u64 val64, int cells)
> >>
> >> In v14 I requested:
> >>
> >> Please rename as cpu64_to_fdt_cells()
> > 
> > I don't mind, but this function may be dropped if Rob sticks to
> > u-boot's fdt_pack_reg() over my fdt_setprop_reg().
> 
> I have another comment a couple of minutes ago in the longer thread
> about this function.

Okay.

-Takahiro Akashi

> 
> >>
> >>>> +{
> >>>> +       __be32 val32;
> >>>> +
> >>>> +       while (cells) {
> >>>> +               val32 = cpu_to_fdt32((val64 >> (32 * (--cells))) & U32_MAX);
> >>>> +               memcpy(buf, &val32, sizeof(val32));
> >>>> +               buf += sizeof(val32);
> >>>
> >>> This is kind of hard to read. I would copy u-boot's fdt_pack_reg function.
> >>>
> >>> BTW, for purposes of moving to libfdt, we'll need the authors'
> >>> (Masahiro Yamada and Hans de Goede) permission to dual license.
> >>>
> >>>> +       }
> >>>> +}
> >>>> +
> >>>> +int fdt_setprop_reg(void *fdt, int nodeoffset, const char *name,
> >>>> +                                               u64 addr, u64 size)
> >>>> +{
> >>>> +       int addr_cells, size_cells;
> >>
> >> unsigned
> > 
> > fdt_[address|size]_cell() returns an int.
> 
> I stand corrected.  They take advantage of the fact that valid values
> are in the range 0..4 and return a negative value for error.
> 
> 
> >>>> +       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))
> >>>> +               return -FDT_ERR_BADVALUE;
> >>>> +
> >>>> +       if ((size_cells == 1) && (size > U32_MAX))
> >>>> +               return -FDT_ERR_BADVALUE;
> >>
> >> In v14 I requested:
> >>
> >>    Should also check that base + size does not wrap around.
> > 
> > Okay, I will start discussion, as you have suggested, in devicetree-spec ML.
> > 
> > Thanks,
> > -Takahiro Akashi
> > 
> >>
> >>>> +
> >>>> +       buf_size = (addr_cells + size_cells) * sizeof(u32);
> >>>> +       prop = buf;
> >>>> +
> >>>> +       fill_property(prop, addr, addr_cells);
> >>>> +       prop += addr_cells * sizeof(u32);
> >>>> +
> >>>> +       fill_property(prop, size, size_cells);
> >>>> +
> >>>> +       return fdt_setprop(fdt, nodeoffset, name, buf, buf_size);
> >>>> +}
> >>>> diff --git a/include/linux/of_fdt.h b/include/linux/of_fdt.h
> >>>> index b9cd9ebdf9b9..842af6ea92ea 100644
> >>>> --- a/include/linux/of_fdt.h
> >>>> +++ b/include/linux/of_fdt.h
> >>>> @@ -108,5 +108,9 @@ static inline void unflatten_device_tree(void) {}
> >>>>  static inline void unflatten_and_copy_device_tree(void) {}
> >>>>  #endif /* CONFIG_OF_EARLY_FLATTREE */
> >>>>
> >>>> +int fdt_prop_len(const char *prop_name, int len);
> >>>> +int fdt_setprop_reg(void *fdt, int nodeoffset, const char *name,
> >>>> +                                               u64 addr, u64 size);
> >>>> +
> >>>>  #endif /* __ASSEMBLY__ */
> >>>>  #endif /* _LINUX_OF_FDT_H */
> >>>> --
> >>>> 2.19.0
> >>>>
> >>>
> >>
> > 
> 

Re: [PATCH v15 06/16] of/fdt: add helper functions for handling properties

[David Gibson]

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

On Fri, Sep 28, 2018 at 08:44:42AM -0500, Rob Herring wrote:
> +David Gibson
> 
> On Fri, Sep 28, 2018 at 1:48 AM AKASHI Takahiro
> <takahiro.akashi@linaro.org> wrote:
> >
> > These functions will be used later to handle kexec-specific properties
> > in arm64's kexec_file implementation.
> >
> > 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
> > ---
> >  drivers/of/fdt.c       | 56 ++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/of_fdt.h |  4 +++
> >  2 files changed, 60 insertions(+)
> >
> > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> > index 800ad252cf9c..c65c31562ccb 100644
> > --- a/drivers/of/fdt.c
> > +++ b/drivers/of/fdt.c
> > @@ -25,6 +25,7 @@
> >  #include <linux/debugfs.h>
> >  #include <linux/serial_core.h>
> >  #include <linux/sysfs.h>
> > +#include <linux/types.h>
> >
> >  #include <asm/setup.h>  /* for COMMAND_LINE_SIZE */
> >  #include <asm/page.h>
> > @@ -1323,3 +1324,58 @@ late_initcall(of_fdt_raw_init);
> >  #endif
> >
> >  #endif /* CONFIG_OF_EARLY_FLATTREE */
> > +
> > +#define FDT_ALIGN(x, a)        (((x) + (a) - 1) & ~((a) - 1))
> > +#define FDT_TAGALIGN(x)        (FDT_ALIGN((x), FDT_TAGSIZE))
> > +
> > +int fdt_prop_len(const char *prop_name, int len)
> > +{
> > +       return (strlen(prop_name) + 1) +
> > +               sizeof(struct fdt_property) +
> > +               FDT_TAGALIGN(len);
> 
> Looks like you are using this to calculate how much space you need to
> allocate in addition to the current DTB for a couple of new or
> replaced properties. I'm not sure that this calculation is completely
> accurate. And it is strange there doesn't seem to be any libfdt
> function for this already. It would be simpler to just add some fixed
> additional amount.
> 
> Maybe David G has comments on this?

Hrm.  Yeah, the approach I'd suggest would be to just add a fixed
amount, and if your functions return -FDT_ERR_NOSPACE, add some more.
That's pretty much the expected way of working with fdts in write
mode.

If that's really not suitable, we could look at adding a helper to
estimate this in libfdt.  Or, actually, I suspect taking an existing
libfdt internal helper, polishing and exporting it.

> 
> > +}
> > +
> 
> The rest of this should go in drivers/of/fdt_address.c. Ultimately, it
> should go into libfdt, but I'm fine with having it in the kernel for
> now.
> 
> > +static void fill_property(void *buf, u64 val64, int cells)
> > +{
> > +       __be32 val32;
> > +
> > +       while (cells) {
> > +               val32 = cpu_to_fdt32((val64 >> (32 * (--cells))) & U32_MAX);
> > +               memcpy(buf, &val32, sizeof(val32));
> > +               buf += sizeof(val32);
> 
> This is kind of hard to read. I would copy u-boot's fdt_pack_reg function.
> 
> BTW, for purposes of moving to libfdt, we'll need the authors'
> (Masahiro Yamada and Hans de Goede) permission to dual license.
> 
> > +       }
> > +}
> > +
> > +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))
> > +               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;
> > +
> > +       fill_property(prop, addr, addr_cells);
> > +       prop += addr_cells * sizeof(u32);
> > +
> > +       fill_property(prop, size, size_cells);
> > +
> > +       return fdt_setprop(fdt, nodeoffset, name, buf, buf_size);
> > +}
> > diff --git a/include/linux/of_fdt.h b/include/linux/of_fdt.h
> > index b9cd9ebdf9b9..842af6ea92ea 100644
> > --- a/include/linux/of_fdt.h
> > +++ b/include/linux/of_fdt.h
> > @@ -108,5 +108,9 @@ static inline void unflatten_device_tree(void) {}
> >  static inline void unflatten_and_copy_device_tree(void) {}
> >  #endif /* CONFIG_OF_EARLY_FLATTREE */
> >
> > +int fdt_prop_len(const char *prop_name, int len);
> > +int fdt_setprop_reg(void *fdt, int nodeoffset, const char *name,
> > +                                               u64 addr, u64 size);
> > +
> >  #endif /* __ASSEMBLY__ */
> >  #endif /* _LINUX_OF_FDT_H */
> >
> 

-- 
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 --]

Re: [PATCH v15 06/16] of/fdt: add helper functions for handling properties

[AKASHI, Takahiro]

On Tue, Oct 02, 2018 at 02:47:10PM +1000, David Gibson wrote:
> On Fri, Sep 28, 2018 at 08:44:42AM -0500, Rob Herring wrote:
> > +David Gibson
> > 
> > On Fri, Sep 28, 2018 at 1:48 AM AKASHI Takahiro
> > <takahiro.akashi@linaro.org> wrote:
> > >
> > > These functions will be used later to handle kexec-specific properties
> > > in arm64's kexec_file implementation.
> > >
> > > 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
> > > ---
> > >  drivers/of/fdt.c       | 56 ++++++++++++++++++++++++++++++++++++++++++
> > >  include/linux/of_fdt.h |  4 +++
> > >  2 files changed, 60 insertions(+)
> > >
> > > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> > > index 800ad252cf9c..c65c31562ccb 100644
> > > --- a/drivers/of/fdt.c
> > > +++ b/drivers/of/fdt.c
> > > @@ -25,6 +25,7 @@
> > >  #include <linux/debugfs.h>
> > >  #include <linux/serial_core.h>
> > >  #include <linux/sysfs.h>
> > > +#include <linux/types.h>
> > >
> > >  #include <asm/setup.h>  /* for COMMAND_LINE_SIZE */
> > >  #include <asm/page.h>
> > > @@ -1323,3 +1324,58 @@ late_initcall(of_fdt_raw_init);
> > >  #endif
> > >
> > >  #endif /* CONFIG_OF_EARLY_FLATTREE */
> > > +
> > > +#define FDT_ALIGN(x, a)        (((x) + (a) - 1) & ~((a) - 1))
> > > +#define FDT_TAGALIGN(x)        (FDT_ALIGN((x), FDT_TAGSIZE))
> > > +
> > > +int fdt_prop_len(const char *prop_name, int len)
> > > +{
> > > +       return (strlen(prop_name) + 1) +
> > > +               sizeof(struct fdt_property) +
> > > +               FDT_TAGALIGN(len);
> > 
> > Looks like you are using this to calculate how much space you need to
> > allocate in addition to the current DTB for a couple of new or
> > replaced properties. I'm not sure that this calculation is completely
> > accurate. And it is strange there doesn't seem to be any libfdt
> > function for this already. It would be simpler to just add some fixed
> > additional amount.
> > 
> > Maybe David G has comments on this?
> 
> Hrm.  Yeah, the approach I'd suggest would be to just add a fixed
> amount, and if your functions return -FDT_ERR_NOSPACE, add some more.
> That's pretty much the expected way of working with fdts in write
> mode.

I will try to follow this approach.

Thanks,
-Takahiro Akashi

> If that's really not suitable, we could look at adding a helper to
> estimate this in libfdt.  Or, actually, I suspect taking an existing
> libfdt internal helper, polishing and exporting it.
> 
> > 
> > > +}
> > > +
> > 
> > The rest of this should go in drivers/of/fdt_address.c. Ultimately, it
> > should go into libfdt, but I'm fine with having it in the kernel for
> > now.
> > 
> > > +static void fill_property(void *buf, u64 val64, int cells)
> > > +{
> > > +       __be32 val32;
> > > +
> > > +       while (cells) {
> > > +               val32 = cpu_to_fdt32((val64 >> (32 * (--cells))) & U32_MAX);
> > > +               memcpy(buf, &val32, sizeof(val32));
> > > +               buf += sizeof(val32);
> > 
> > This is kind of hard to read. I would copy u-boot's fdt_pack_reg function.
> > 
> > BTW, for purposes of moving to libfdt, we'll need the authors'
> > (Masahiro Yamada and Hans de Goede) permission to dual license.
> > 
> > > +       }
> > > +}
> > > +
> > > +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))
> > > +               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;
> > > +
> > > +       fill_property(prop, addr, addr_cells);
> > > +       prop += addr_cells * sizeof(u32);
> > > +
> > > +       fill_property(prop, size, size_cells);
> > > +
> > > +       return fdt_setprop(fdt, nodeoffset, name, buf, buf_size);
> > > +}
> > > diff --git a/include/linux/of_fdt.h b/include/linux/of_fdt.h
> > > index b9cd9ebdf9b9..842af6ea92ea 100644
> > > --- a/include/linux/of_fdt.h
> > > +++ b/include/linux/of_fdt.h
> > > @@ -108,5 +108,9 @@ static inline void unflatten_device_tree(void) {}
> > >  static inline void unflatten_and_copy_device_tree(void) {}
> > >  #endif /* CONFIG_OF_EARLY_FLATTREE */
> > >
> > > +int fdt_prop_len(const char *prop_name, int len);
> > > +int fdt_setprop_reg(void *fdt, int nodeoffset, const char *name,
> > > +                                               u64 addr, u64 size);
> > > +
> > >  #endif /* __ASSEMBLY__ */
> > >  #endif /* _LINUX_OF_FDT_H */
> > >
> > 
> 
> -- 
> 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

Re: [PATCH v15 06/16] of/fdt: add helper functions for handling properties

[AKASHI, Takahiro]

Rob,

# I haven't replied to this comment yet.

On Fri, Sep 28, 2018 at 08:44:42AM -0500, Rob Herring wrote:
> +David Gibson
> 
> On Fri, Sep 28, 2018 at 1:48 AM AKASHI Takahiro
> <takahiro.akashi@linaro.org> wrote:
> >
> > These functions will be used later to handle kexec-specific properties
> > in arm64's kexec_file implementation.
> >
> > 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
> > ---
> >  drivers/of/fdt.c       | 56 ++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/of_fdt.h |  4 +++
> >  2 files changed, 60 insertions(+)
> >
> > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> > index 800ad252cf9c..c65c31562ccb 100644
> > --- a/drivers/of/fdt.c
> > +++ b/drivers/of/fdt.c
> > @@ -25,6 +25,7 @@
> >  #include <linux/debugfs.h>
> >  #include <linux/serial_core.h>
> >  #include <linux/sysfs.h>
> > +#include <linux/types.h>
> >
> >  #include <asm/setup.h>  /* for COMMAND_LINE_SIZE */
> >  #include <asm/page.h>
> > @@ -1323,3 +1324,58 @@ late_initcall(of_fdt_raw_init);
> >  #endif
> >
> >  #endif /* CONFIG_OF_EARLY_FLATTREE */
> > +
> > +#define FDT_ALIGN(x, a)        (((x) + (a) - 1) & ~((a) - 1))
> > +#define FDT_TAGALIGN(x)        (FDT_ALIGN((x), FDT_TAGSIZE))
> > +
> > +int fdt_prop_len(const char *prop_name, int len)
> > +{
> > +       return (strlen(prop_name) + 1) +
> > +               sizeof(struct fdt_property) +
> > +               FDT_TAGALIGN(len);
> 
> Looks like you are using this to calculate how much space you need to
> allocate in addition to the current DTB for a couple of new or
> replaced properties. I'm not sure that this calculation is completely
> accurate. And it is strange there doesn't seem to be any libfdt
> function for this already. It would be simpler to just add some fixed
> additional amount.
> 
> Maybe David G has comments on this?
> 
> > +}
> > +
> 
> The rest of this should go in drivers/of/fdt_address.c. Ultimately, it
> should go into libfdt, but I'm fine with having it in the kernel for
> now.

I'd like to have this function in the kernel for now.

> > +static void fill_property(void *buf, u64 val64, int cells)
> > +{
> > +       __be32 val32;
> > +
> > +       while (cells) {
> > +               val32 = cpu_to_fdt32((val64 >> (32 * (--cells))) & U32_MAX);
> > +               memcpy(buf, &val32, sizeof(val32));
> > +               buf += sizeof(val32);
> 
> This is kind of hard to read. I would copy u-boot's fdt_pack_reg function.

Are you sure?
I originally implemented this function in a similar way that fdt_pack_reg()
was, but, you suggested, in your past comment[1], that we'd be better to
have of_read_number()-like implementation.

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2018-May/579118.html

-Takahiro Akashi


> BTW, for purposes of moving to libfdt, we'll need the authors'
> (Masahiro Yamada and Hans de Goede) permission to dual license.
> 
> > +       }
> > +}
> > +
> > +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))
> > +               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;
> > +
> > +       fill_property(prop, addr, addr_cells);
> > +       prop += addr_cells * sizeof(u32);
> > +
> > +       fill_property(prop, size, size_cells);
> > +
> > +       return fdt_setprop(fdt, nodeoffset, name, buf, buf_size);
> > +}
> > diff --git a/include/linux/of_fdt.h b/include/linux/of_fdt.h
> > index b9cd9ebdf9b9..842af6ea92ea 100644
> > --- a/include/linux/of_fdt.h
> > +++ b/include/linux/of_fdt.h
> > @@ -108,5 +108,9 @@ static inline void unflatten_device_tree(void) {}
> >  static inline void unflatten_and_copy_device_tree(void) {}
> >  #endif /* CONFIG_OF_EARLY_FLATTREE */
> >
> > +int fdt_prop_len(const char *prop_name, int len);
> > +int fdt_setprop_reg(void *fdt, int nodeoffset, const char *name,
> > +                                               u64 addr, u64 size);
> > +
> >  #endif /* __ASSEMBLY__ */
> >  #endif /* _LINUX_OF_FDT_H */
> > --
> > 2.19.0
> >

Re: [PATCH v15 06/16] of/fdt: add helper functions for handling properties

[Rob Herring]

On Thu, Oct 4, 2018 at 10:07 PM AKASHI, Takahiro
<takahiro.akashi@linaro.org> wrote:
>
> Rob,
>
> # I haven't replied to this comment yet.
>
> On Fri, Sep 28, 2018 at 08:44:42AM -0500, Rob Herring wrote:
> > +David Gibson
> >
> > On Fri, Sep 28, 2018 at 1:48 AM AKASHI Takahiro
> > <takahiro.akashi@linaro.org> wrote:
> > >
> > > These functions will be used later to handle kexec-specific properties
> > > in arm64's kexec_file implementation.
> > >
> > > 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
> > > ---
> > >  drivers/of/fdt.c       | 56 ++++++++++++++++++++++++++++++++++++++++++
> > >  include/linux/of_fdt.h |  4 +++
> > >  2 files changed, 60 insertions(+)
> > >
> > > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> > > index 800ad252cf9c..c65c31562ccb 100644
> > > --- a/drivers/of/fdt.c
> > > +++ b/drivers/of/fdt.c
> > > @@ -25,6 +25,7 @@
> > >  #include <linux/debugfs.h>
> > >  #include <linux/serial_core.h>
> > >  #include <linux/sysfs.h>
> > > +#include <linux/types.h>
> > >
> > >  #include <asm/setup.h>  /* for COMMAND_LINE_SIZE */
> > >  #include <asm/page.h>
> > > @@ -1323,3 +1324,58 @@ late_initcall(of_fdt_raw_init);
> > >  #endif
> > >
> > >  #endif /* CONFIG_OF_EARLY_FLATTREE */
> > > +
> > > +#define FDT_ALIGN(x, a)        (((x) + (a) - 1) & ~((a) - 1))
> > > +#define FDT_TAGALIGN(x)        (FDT_ALIGN((x), FDT_TAGSIZE))
> > > +
> > > +int fdt_prop_len(const char *prop_name, int len)
> > > +{
> > > +       return (strlen(prop_name) + 1) +
> > > +               sizeof(struct fdt_property) +
> > > +               FDT_TAGALIGN(len);
> >
> > Looks like you are using this to calculate how much space you need to
> > allocate in addition to the current DTB for a couple of new or
> > replaced properties. I'm not sure that this calculation is completely
> > accurate. And it is strange there doesn't seem to be any libfdt
> > function for this already. It would be simpler to just add some fixed
> > additional amount.
> >
> > Maybe David G has comments on this?
> >
> > > +}
> > > +
> >
> > The rest of this should go in drivers/of/fdt_address.c. Ultimately, it
> > should go into libfdt, but I'm fine with having it in the kernel for
> > now.
>
> I'd like to have this function in the kernel for now.
>
> > > +static void fill_property(void *buf, u64 val64, int cells)
> > > +{
> > > +       __be32 val32;
> > > +
> > > +       while (cells) {
> > > +               val32 = cpu_to_fdt32((val64 >> (32 * (--cells))) & U32_MAX);
> > > +               memcpy(buf, &val32, sizeof(val32));
> > > +               buf += sizeof(val32);
> >
> > This is kind of hard to read. I would copy u-boot's fdt_pack_reg function.
>
> Are you sure?
> I originally implemented this function in a similar way that fdt_pack_reg()
> was, but, you suggested, in your past comment[1], that we'd be better to
> have of_read_number()-like implementation.
>
> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2018-May/579118.html

Yeah, you're right. Plus, I'm not sure the u-boot one would work for
unaligned accesses with armv5 and earlier h/w.

My only comment then is I think you can drop the U32_MAX masking.

Rob

Re: [PATCH v15 06/16] of/fdt: add helper functions for handling properties

[AKASHI, Takahiro]

On Fri, Oct 05, 2018 at 08:23:57AM -0500, Rob Herring wrote:
> On Thu, Oct 4, 2018 at 10:07 PM AKASHI, Takahiro
> <takahiro.akashi@linaro.org> wrote:
> >
> > Rob,
> >
> > # I haven't replied to this comment yet.
> >
> > On Fri, Sep 28, 2018 at 08:44:42AM -0500, Rob Herring wrote:
> > > +David Gibson
> > >
> > > On Fri, Sep 28, 2018 at 1:48 AM AKASHI Takahiro
> > > <takahiro.akashi@linaro.org> wrote:
> > > >
> > > > These functions will be used later to handle kexec-specific properties
> > > > in arm64's kexec_file implementation.
> > > >
> > > > 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
> > > > ---
> > > >  drivers/of/fdt.c       | 56 ++++++++++++++++++++++++++++++++++++++++++
> > > >  include/linux/of_fdt.h |  4 +++
> > > >  2 files changed, 60 insertions(+)
> > > >
> > > > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> > > > index 800ad252cf9c..c65c31562ccb 100644
> > > > --- a/drivers/of/fdt.c
> > > > +++ b/drivers/of/fdt.c
> > > > @@ -25,6 +25,7 @@
> > > >  #include <linux/debugfs.h>
> > > >  #include <linux/serial_core.h>
> > > >  #include <linux/sysfs.h>
> > > > +#include <linux/types.h>
> > > >
> > > >  #include <asm/setup.h>  /* for COMMAND_LINE_SIZE */
> > > >  #include <asm/page.h>
> > > > @@ -1323,3 +1324,58 @@ late_initcall(of_fdt_raw_init);
> > > >  #endif
> > > >
> > > >  #endif /* CONFIG_OF_EARLY_FLATTREE */
> > > > +
> > > > +#define FDT_ALIGN(x, a)        (((x) + (a) - 1) & ~((a) - 1))
> > > > +#define FDT_TAGALIGN(x)        (FDT_ALIGN((x), FDT_TAGSIZE))
> > > > +
> > > > +int fdt_prop_len(const char *prop_name, int len)
> > > > +{
> > > > +       return (strlen(prop_name) + 1) +
> > > > +               sizeof(struct fdt_property) +
> > > > +               FDT_TAGALIGN(len);
> > >
> > > Looks like you are using this to calculate how much space you need to
> > > allocate in addition to the current DTB for a couple of new or
> > > replaced properties. I'm not sure that this calculation is completely
> > > accurate. And it is strange there doesn't seem to be any libfdt
> > > function for this already. It would be simpler to just add some fixed
> > > additional amount.
> > >
> > > Maybe David G has comments on this?
> > >
> > > > +}
> > > > +
> > >
> > > The rest of this should go in drivers/of/fdt_address.c. Ultimately, it
> > > should go into libfdt, but I'm fine with having it in the kernel for
> > > now.
> >
> > I'd like to have this function in the kernel for now.
> >
> > > > +static void fill_property(void *buf, u64 val64, int cells)
> > > > +{
> > > > +       __be32 val32;
> > > > +
> > > > +       while (cells) {
> > > > +               val32 = cpu_to_fdt32((val64 >> (32 * (--cells))) & U32_MAX);
> > > > +               memcpy(buf, &val32, sizeof(val32));
> > > > +               buf += sizeof(val32);
> > >
> > > This is kind of hard to read. I would copy u-boot's fdt_pack_reg function.
> >
> > Are you sure?
> > I originally implemented this function in a similar way that fdt_pack_reg()
> > was, but, you suggested, in your past comment[1], that we'd be better to
> > have of_read_number()-like implementation.
> >
> > [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2018-May/579118.html
> 
> Yeah, you're right. Plus, I'm not sure the u-boot one would work for
> unaligned accesses with armv5 and earlier h/w.
> 
> My only comment then is I think you can drop the U32_MAX masking.

Okay, then I will leave this function, yet renaming it to
cpu64_to_fdt_cells() after Frank's comment.

Thanks,
-Takahiro Akashi
> 
> Rob

Re: [PATCH v15 06/16] of/fdt: add helper functions for handling properties

[Frank Rowand]

On 10/08/18 17:37, AKASHI, Takahiro wrote:
> On Fri, Oct 05, 2018 at 08:23:57AM -0500, Rob Herring wrote:
>> On Thu, Oct 4, 2018 at 10:07 PM AKASHI, Takahiro
>> <takahiro.akashi@linaro.org> wrote:
>>>
>>> Rob,
>>>
>>> # I haven't replied to this comment yet.
>>>
>>> On Fri, Sep 28, 2018 at 08:44:42AM -0500, Rob Herring wrote:
>>>> +David Gibson
>>>>
>>>> On Fri, Sep 28, 2018 at 1:48 AM AKASHI Takahiro
>>>> <takahiro.akashi@linaro.org> wrote:
>>>>>
>>>>> These functions will be used later to handle kexec-specific properties
>>>>> in arm64's kexec_file implementation.
>>>>>
>>>>> 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
>>>>> ---
>>>>>  drivers/of/fdt.c       | 56 ++++++++++++++++++++++++++++++++++++++++++
>>>>>  include/linux/of_fdt.h |  4 +++
>>>>>  2 files changed, 60 insertions(+)
>>>>>
>>>>> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
>>>>> index 800ad252cf9c..c65c31562ccb 100644
>>>>> --- a/drivers/of/fdt.c
>>>>> +++ b/drivers/of/fdt.c
>>>>> @@ -25,6 +25,7 @@
>>>>>  #include <linux/debugfs.h>
>>>>>  #include <linux/serial_core.h>
>>>>>  #include <linux/sysfs.h>
>>>>> +#include <linux/types.h>
>>>>>
>>>>>  #include <asm/setup.h>  /* for COMMAND_LINE_SIZE */
>>>>>  #include <asm/page.h>
>>>>> @@ -1323,3 +1324,58 @@ late_initcall(of_fdt_raw_init);
>>>>>  #endif
>>>>>
>>>>>  #endif /* CONFIG_OF_EARLY_FLATTREE */
>>>>> +
>>>>> +#define FDT_ALIGN(x, a)        (((x) + (a) - 1) & ~((a) - 1))
>>>>> +#define FDT_TAGALIGN(x)        (FDT_ALIGN((x), FDT_TAGSIZE))
>>>>> +
>>>>> +int fdt_prop_len(const char *prop_name, int len)
>>>>> +{
>>>>> +       return (strlen(prop_name) + 1) +
>>>>> +               sizeof(struct fdt_property) +
>>>>> +               FDT_TAGALIGN(len);
>>>>
>>>> Looks like you are using this to calculate how much space you need to
>>>> allocate in addition to the current DTB for a couple of new or
>>>> replaced properties. I'm not sure that this calculation is completely
>>>> accurate. And it is strange there doesn't seem to be any libfdt
>>>> function for this already. It would be simpler to just add some fixed
>>>> additional amount.
>>>>
>>>> Maybe David G has comments on this?
>>>>
>>>>> +}
>>>>> +
>>>>
>>>> The rest of this should go in drivers/of/fdt_address.c. Ultimately, it
>>>> should go into libfdt, but I'm fine with having it in the kernel for
>>>> now.
>>>
>>> I'd like to have this function in the kernel for now.
>>>
>>>>> +static void fill_property(void *buf, u64 val64, int cells)
>>>>> +{
>>>>> +       __be32 val32;
>>>>> +
>>>>> +       while (cells) {
>>>>> +               val32 = cpu_to_fdt32((val64 >> (32 * (--cells))) & U32_MAX);
>>>>> +               memcpy(buf, &val32, sizeof(val32));
>>>>> +               buf += sizeof(val32);
>>>>
>>>> This is kind of hard to read. I would copy u-boot's fdt_pack_reg function.
>>>
>>> Are you sure?
>>> I originally implemented this function in a similar way that fdt_pack_reg()
>>> was, but, you suggested, in your past comment[1], that we'd be better to
>>> have of_read_number()-like implementation.
>>>
>>> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2018-May/579118.html
>>
>> Yeah, you're right. Plus, I'm not sure the u-boot one would work for
>> unaligned accesses with armv5 and earlier h/w.
>>
>> My only comment then is I think you can drop the U32_MAX masking.
> 
> Okay, then I will leave this function, yet renaming it to
> cpu64_to_fdt_cells() after Frank's comment.

I have second guessed myself and do not like the name I suggested
because what the function really does is either cpu32 to be32 or
cpu64 to be64.

I agree with Rob that readability is important here.  Instead of
having a fill_property() function, how about having inline code,
something like (untested even for thinkos):

	prop = buf;

	if (addr_cells == 1) {
		*(__be32 *)prop = cpu32_to_be32(addr);
		prop += 4;
	} else {
		*(__be64 *)prop = cpu64_to_be64(addr);
		prop += 8;
	}

	if (size_cells == 1)
		*(__be32 *)prop = cpu32_to_be32(size);
	else
		*(__be64 *)prop = cpu64_to_be64(size);

You might want to also give Rob a chance to bike shed on this
suggestion.

-Frank

> 
> Thanks,
> -Takahiro Akashi
>>
>> Rob
> 

Re: [PATCH v15 06/16] of/fdt: add helper functions for handling properties

[AKASHI, Takahiro]

Frank, Rob,

On Tue, Oct 09, 2018 at 10:47:15AM -0700, Frank Rowand wrote:
> On 10/08/18 17:37, AKASHI, Takahiro wrote:
> > On Fri, Oct 05, 2018 at 08:23:57AM -0500, Rob Herring wrote:
> >> On Thu, Oct 4, 2018 at 10:07 PM AKASHI, Takahiro
> >> <takahiro.akashi@linaro.org> wrote:
> >>>
> >>> Rob,
> >>>
> >>> # I haven't replied to this comment yet.
> >>>
> >>> On Fri, Sep 28, 2018 at 08:44:42AM -0500, Rob Herring wrote:
> >>>> +David Gibson
> >>>>
> >>>> On Fri, Sep 28, 2018 at 1:48 AM AKASHI Takahiro
> >>>> <takahiro.akashi@linaro.org> wrote:
> >>>>>
> >>>>> These functions will be used later to handle kexec-specific properties
> >>>>> in arm64's kexec_file implementation.
> >>>>>
> >>>>> 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
> >>>>> ---
> >>>>>  drivers/of/fdt.c       | 56 ++++++++++++++++++++++++++++++++++++++++++
> >>>>>  include/linux/of_fdt.h |  4 +++
> >>>>>  2 files changed, 60 insertions(+)
> >>>>>
> >>>>> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> >>>>> index 800ad252cf9c..c65c31562ccb 100644
> >>>>> --- a/drivers/of/fdt.c
> >>>>> +++ b/drivers/of/fdt.c
> >>>>> @@ -25,6 +25,7 @@
> >>>>>  #include <linux/debugfs.h>
> >>>>>  #include <linux/serial_core.h>
> >>>>>  #include <linux/sysfs.h>
> >>>>> +#include <linux/types.h>
> >>>>>
> >>>>>  #include <asm/setup.h>  /* for COMMAND_LINE_SIZE */
> >>>>>  #include <asm/page.h>
> >>>>> @@ -1323,3 +1324,58 @@ late_initcall(of_fdt_raw_init);
> >>>>>  #endif
> >>>>>
> >>>>>  #endif /* CONFIG_OF_EARLY_FLATTREE */
> >>>>> +
> >>>>> +#define FDT_ALIGN(x, a)        (((x) + (a) - 1) & ~((a) - 1))
> >>>>> +#define FDT_TAGALIGN(x)        (FDT_ALIGN((x), FDT_TAGSIZE))
> >>>>> +
> >>>>> +int fdt_prop_len(const char *prop_name, int len)
> >>>>> +{
> >>>>> +       return (strlen(prop_name) + 1) +
> >>>>> +               sizeof(struct fdt_property) +
> >>>>> +               FDT_TAGALIGN(len);
> >>>>
> >>>> Looks like you are using this to calculate how much space you need to
> >>>> allocate in addition to the current DTB for a couple of new or
> >>>> replaced properties. I'm not sure that this calculation is completely
> >>>> accurate. And it is strange there doesn't seem to be any libfdt
> >>>> function for this already. It would be simpler to just add some fixed
> >>>> additional amount.
> >>>>
> >>>> Maybe David G has comments on this?
> >>>>
> >>>>> +}
> >>>>> +
> >>>>
> >>>> The rest of this should go in drivers/of/fdt_address.c. Ultimately, it
> >>>> should go into libfdt, but I'm fine with having it in the kernel for
> >>>> now.
> >>>
> >>> I'd like to have this function in the kernel for now.
> >>>
> >>>>> +static void fill_property(void *buf, u64 val64, int cells)
> >>>>> +{
> >>>>> +       __be32 val32;
> >>>>> +
> >>>>> +       while (cells) {
> >>>>> +               val32 = cpu_to_fdt32((val64 >> (32 * (--cells))) & U32_MAX);
> >>>>> +               memcpy(buf, &val32, sizeof(val32));
> >>>>> +               buf += sizeof(val32);
> >>>>
> >>>> This is kind of hard to read. I would copy u-boot's fdt_pack_reg function.
> >>>
> >>> Are you sure?
> >>> I originally implemented this function in a similar way that fdt_pack_reg()
> >>> was, but, you suggested, in your past comment[1], that we'd be better to
> >>> have of_read_number()-like implementation.
> >>>
> >>> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2018-May/579118.html
> >>
> >> Yeah, you're right. Plus, I'm not sure the u-boot one would work for
> >> unaligned accesses with armv5 and earlier h/w.
> >>
> >> My only comment then is I think you can drop the U32_MAX masking.
> > 
> > Okay, then I will leave this function, yet renaming it to
> > cpu64_to_fdt_cells() after Frank's comment.
> 
> I have second guessed myself and do not like the name I suggested
> because what the function really does is either cpu32 to be32 or
> cpu64 to be64.

Okay.

> I agree with Rob that readability is important here.  Instead of
> having a fill_property() function, how about having inline code,
> something like (untested even for thinkos):
> 
> 	prop = buf;
> 
> 	if (addr_cells == 1) {
> 		*(__be32 *)prop = cpu32_to_be32(addr);
> 		prop += 4;
> 	} else {
> 		*(__be64 *)prop = cpu64_to_be64(addr);
> 		prop += 8;
> 	}
> 
> 	if (size_cells == 1)
> 		*(__be32 *)prop = cpu32_to_be32(size);
> 	else
> 		*(__be64 *)prop = cpu64_to_be64(size);
> 
> You might want to also give Rob a chance to bike shed on this
> suggestion.

Basically, I don't care either way, but what Rob suggested
is that some architecture(s) might not handle correctly
unaligned memory access here.

I just want to stay tuned with Rob.

Thanks,
-Takahiro Akashi

> -Frank
> 
> > 
> > Thanks,
> > -Takahiro Akashi
> >>
> >> Rob
> > 
> 

Re: [PATCH v15 06/16] of/fdt: add helper functions for handling properties

[Frank Rowand]

On 10/09/18 18:14, AKASHI, Takahiro wrote:
> Frank, Rob,
> 
> On Tue, Oct 09, 2018 at 10:47:15AM -0700, Frank Rowand wrote:
>> On 10/08/18 17:37, AKASHI, Takahiro wrote:
>>> On Fri, Oct 05, 2018 at 08:23:57AM -0500, Rob Herring wrote:
>>>> On Thu, Oct 4, 2018 at 10:07 PM AKASHI, Takahiro
>>>> <takahiro.akashi@linaro.org> wrote:
>>>>>
>>>>> Rob,
>>>>>
>>>>> # I haven't replied to this comment yet.
>>>>>
>>>>> On Fri, Sep 28, 2018 at 08:44:42AM -0500, Rob Herring wrote:
>>>>>> +David Gibson
>>>>>>
>>>>>> On Fri, Sep 28, 2018 at 1:48 AM AKASHI Takahiro
>>>>>> <takahiro.akashi@linaro.org> wrote:
>>>>>>>
>>>>>>> These functions will be used later to handle kexec-specific properties
>>>>>>> in arm64's kexec_file implementation.
>>>>>>>
>>>>>>> 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
>>>>>>> ---
>>>>>>>  drivers/of/fdt.c       | 56 ++++++++++++++++++++++++++++++++++++++++++
>>>>>>>  include/linux/of_fdt.h |  4 +++
>>>>>>>  2 files changed, 60 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
>>>>>>> index 800ad252cf9c..c65c31562ccb 100644
>>>>>>> --- a/drivers/of/fdt.c
>>>>>>> +++ b/drivers/of/fdt.c
>>>>>>> @@ -25,6 +25,7 @@
>>>>>>>  #include <linux/debugfs.h>
>>>>>>>  #include <linux/serial_core.h>
>>>>>>>  #include <linux/sysfs.h>
>>>>>>> +#include <linux/types.h>
>>>>>>>
>>>>>>>  #include <asm/setup.h>  /* for COMMAND_LINE_SIZE */
>>>>>>>  #include <asm/page.h>
>>>>>>> @@ -1323,3 +1324,58 @@ late_initcall(of_fdt_raw_init);
>>>>>>>  #endif
>>>>>>>
>>>>>>>  #endif /* CONFIG_OF_EARLY_FLATTREE */
>>>>>>> +
>>>>>>> +#define FDT_ALIGN(x, a)        (((x) + (a) - 1) & ~((a) - 1))
>>>>>>> +#define FDT_TAGALIGN(x)        (FDT_ALIGN((x), FDT_TAGSIZE))
>>>>>>> +
>>>>>>> +int fdt_prop_len(const char *prop_name, int len)
>>>>>>> +{
>>>>>>> +       return (strlen(prop_name) + 1) +
>>>>>>> +               sizeof(struct fdt_property) +
>>>>>>> +               FDT_TAGALIGN(len);
>>>>>>
>>>>>> Looks like you are using this to calculate how much space you need to
>>>>>> allocate in addition to the current DTB for a couple of new or
>>>>>> replaced properties. I'm not sure that this calculation is completely
>>>>>> accurate. And it is strange there doesn't seem to be any libfdt
>>>>>> function for this already. It would be simpler to just add some fixed
>>>>>> additional amount.
>>>>>>
>>>>>> Maybe David G has comments on this?
>>>>>>
>>>>>>> +}
>>>>>>> +
>>>>>>
>>>>>> The rest of this should go in drivers/of/fdt_address.c. Ultimately, it
>>>>>> should go into libfdt, but I'm fine with having it in the kernel for
>>>>>> now.
>>>>>
>>>>> I'd like to have this function in the kernel for now.
>>>>>
>>>>>>> +static void fill_property(void *buf, u64 val64, int cells)
>>>>>>> +{
>>>>>>> +       __be32 val32;
>>>>>>> +
>>>>>>> +       while (cells) {
>>>>>>> +               val32 = cpu_to_fdt32((val64 >> (32 * (--cells))) & U32_MAX);
>>>>>>> +               memcpy(buf, &val32, sizeof(val32));
>>>>>>> +               buf += sizeof(val32);
>>>>>>
>>>>>> This is kind of hard to read. I would copy u-boot's fdt_pack_reg function.
>>>>>
>>>>> Are you sure?
>>>>> I originally implemented this function in a similar way that fdt_pack_reg()
>>>>> was, but, you suggested, in your past comment[1], that we'd be better to
>>>>> have of_read_number()-like implementation.
>>>>>
>>>>> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2018-May/579118.html
>>>>
>>>> Yeah, you're right. Plus, I'm not sure the u-boot one would work for
>>>> unaligned accesses with armv5 and earlier h/w.
>>>>
>>>> My only comment then is I think you can drop the U32_MAX masking.
>>>
>>> Okay, then I will leave this function, yet renaming it to
>>> cpu64_to_fdt_cells() after Frank's comment.
>>
>> I have second guessed myself and do not like the name I suggested
>> because what the function really does is either cpu32 to be32 or
>> cpu64 to be64.
> 
> Okay.
> 
>> I agree with Rob that readability is important here.  Instead of
>> having a fill_property() function, how about having inline code,
>> something like (untested even for thinkos):
>>
>> 	prop = buf;
>>
>> 	if (addr_cells == 1) {
>> 		*(__be32 *)prop = cpu32_to_be32(addr);
>> 		prop += 4;
>> 	} else {
>> 		*(__be64 *)prop = cpu64_to_be64(addr);
>> 		prop += 8;
>> 	}
>>
>> 	if (size_cells == 1)
>> 		*(__be32 *)prop = cpu32_to_be32(size);
>> 	else
>> 		*(__be64 *)prop = cpu64_to_be64(size);
>>
>> You might want to also give Rob a chance to bike shed on this
>> suggestion.
> 
> Basically, I don't care either way, but what Rob suggested
> is that some architecture(s) might not handle correctly
> unaligned memory access here.

Good point about unaligned memory access.  That rules out my
suggestion above.  But if using the fill_property() implementation,
either come up with a name that better describes what the function
does or add a function header comment that gives the reader a
clue as to what the function accomplishes.

-Frank


> I just want to stay tuned with Rob.
> 
> Thanks,
> -Takahiro Akashi
> 
>> -Frank
>>
>>>
>>> Thanks,
>>> -Takahiro Akashi
>>>>
>>>> Rob
>>>
>>
> 

[PATCH v15 12/16] arm64: kexec_file: add crash dump support

[AKASHI Takahiro]

Enabling crash dump (kdump) includes
* prepare contents of ELF header of a core dump file, /proc/vmcore,
  using crash_prepare_elf64_headers(), and
* add two device tree properties, "linux,usable-memory-range" and
  "linux,elfcorehdr", which represent respectively a memory range
  to be used by crash dump kernel and the header's location

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Reviewed-by: James Morse <james.morse@arm.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Frank Rowand <frowand.list@gmail.com>
Cc: devicetree@vger.kernel.org
---
 arch/arm64/include/asm/kexec.h         |   4 +
 arch/arm64/kernel/machine_kexec_file.c | 109 ++++++++++++++++++++++++-
 2 files changed, 110 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/include/asm/kexec.h b/arch/arm64/include/asm/kexec.h
index 5e673481b3a3..1b2c27026ae0 100644
--- a/arch/arm64/include/asm/kexec.h
+++ b/arch/arm64/include/asm/kexec.h
@@ -99,6 +99,10 @@ static inline void crash_post_resume(void) {}
 struct kimage_arch {
 	void *dtb;
 	unsigned long dtb_mem;
+	/* Core ELF header buffer */
+	void *elf_headers;
+	unsigned long elf_headers_mem;
+	unsigned long elf_headers_sz;
 };
 
 /**
diff --git a/arch/arm64/kernel/machine_kexec_file.c b/arch/arm64/kernel/machine_kexec_file.c
index 05fb2d4e6fef..4b5ea256c84c 100644
--- a/arch/arm64/kernel/machine_kexec_file.c
+++ b/arch/arm64/kernel/machine_kexec_file.c
@@ -16,10 +16,14 @@
 #include <linux/libfdt.h>
 #include <linux/memblock.h>
 #include <linux/of_fdt.h>
+#include <linux/slab.h>
 #include <linux/types.h>
+#include <linux/vmalloc.h>
 #include <asm/byteorder.h>
 
 /* relevant device tree properties */
+#define FDT_PSTR_KEXEC_ELFHDR	"linux,elfcorehdr"
+#define FDT_PSTR_MEM_RANGE	"linux,usable-memory-range"
 #define FDT_PSTR_INITRD_STA	"linux,initrd-start"
 #define FDT_PSTR_INITRD_END	"linux,initrd-end"
 #define FDT_PSTR_BOOTARGS	"bootargs"
@@ -34,6 +38,10 @@ int arch_kimage_file_post_load_cleanup(struct kimage *image)
 	vfree(image->arch.dtb);
 	image->arch.dtb = NULL;
 
+	vfree(image->arch.elf_headers);
+	image->arch.elf_headers = NULL;
+	image->arch.elf_headers_sz = 0;
+
 	return kexec_image_post_load_cleanup_default(image);
 }
 
@@ -43,12 +51,25 @@ static int setup_dtb(struct kimage *image,
 		void **dtb_buf, unsigned long *dtb_buf_len)
 {
 	void *buf = NULL;
-	size_t buf_size;
+	int addr_cells, size_cells;
+	size_t buf_size, range_size;
 	int nodeoffset;
 	int ret;
 
 	/* duplicate dt blob */
 	buf_size = fdt_totalsize(initial_boot_params);
+	addr_cells = fdt_address_cells(initial_boot_params, 0);
+	if (addr_cells < 0)
+		return -EINVAL;
+	size_cells = fdt_size_cells(initial_boot_params, 0);
+	if (size_cells < 0)
+		return -EINVAL;
+	range_size = (addr_cells + size_cells) * sizeof(u32);
+
+	if (image->type == KEXEC_TYPE_CRASH) {
+		buf_size += fdt_prop_len(FDT_PSTR_KEXEC_ELFHDR, range_size);
+		buf_size += fdt_prop_len(FDT_PSTR_MEM_RANGE, range_size);
+	}
 
 	if (initrd_load_addr) {
 		/* can be redundant, but trimmed at the end */
@@ -78,6 +99,22 @@ static int setup_dtb(struct kimage *image,
 		goto out_err;
 	}
 
+	if (image->type == KEXEC_TYPE_CRASH) {
+		/* add linux,elfcorehdr */
+		ret = fdt_setprop_reg(buf, nodeoffset, FDT_PSTR_KEXEC_ELFHDR,
+				image->arch.elf_headers_mem,
+				image->arch.elf_headers_sz);
+		if (ret)
+			goto out_err;
+
+		/* add linux,usable-memory-range */
+		ret = fdt_setprop_reg(buf, nodeoffset, FDT_PSTR_MEM_RANGE,
+				crashk_res.start,
+				crashk_res.end - crashk_res.start + 1);
+		if (ret)
+			goto out_err;
+	}
+
 	/* add bootargs */
 	if (cmdline) {
 		ret = fdt_setprop_string(buf, nodeoffset, FDT_PSTR_BOOTARGS,
@@ -135,6 +172,43 @@ static int setup_dtb(struct kimage *image,
 	return ret;
 }
 
+static int prepare_elf_headers(void **addr, unsigned long *sz)
+{
+	struct crash_mem *cmem;
+	unsigned int nr_ranges;
+	int ret;
+	u64 i;
+	phys_addr_t start, end;
+
+	nr_ranges = 1; /* for exclusion of crashkernel region */
+	for_each_mem_range(i, &memblock.memory, NULL, NUMA_NO_NODE,
+					MEMBLOCK_NONE, &start, &end, NULL)
+		nr_ranges++;
+
+	cmem = kmalloc(sizeof(struct crash_mem) +
+			sizeof(struct crash_mem_range) * nr_ranges, GFP_KERNEL);
+	if (!cmem)
+		return -ENOMEM;
+
+	cmem->max_nr_ranges = nr_ranges;
+	cmem->nr_ranges = 0;
+	for_each_mem_range(i, &memblock.memory, NULL, NUMA_NO_NODE,
+					MEMBLOCK_NONE, &start, &end, NULL) {
+		cmem->ranges[cmem->nr_ranges].start = start;
+		cmem->ranges[cmem->nr_ranges].end = end - 1;
+		cmem->nr_ranges++;
+	}
+
+	/* Exclude crashkernel region */
+	ret = crash_exclude_mem_range(cmem, crashk_res.start, crashk_res.end);
+
+	if (!ret)
+		ret =  crash_prepare_elf64_headers(cmem, true, addr, sz);
+
+	kfree(cmem);
+	return ret;
+}
+
 int load_other_segments(struct kimage *image,
 			unsigned long kernel_load_addr,
 			unsigned long kernel_size,
@@ -142,14 +216,43 @@ int load_other_segments(struct kimage *image,
 			char *cmdline, unsigned long cmdline_len)
 {
 	struct kexec_buf kbuf;
-	void *dtb = NULL;
-	unsigned long initrd_load_addr = 0, dtb_len;
+	void *headers, *dtb = NULL;
+	unsigned long headers_sz, initrd_load_addr = 0, dtb_len;
 	int ret = 0;
 
 	kbuf.image = image;
 	/* not allocate anything below the kernel */
 	kbuf.buf_min = kernel_load_addr + kernel_size;
 
+	/* load elf core header */
+	if (image->type == KEXEC_TYPE_CRASH) {
+		ret = prepare_elf_headers(&headers, &headers_sz);
+		if (ret) {
+			pr_err("Preparing elf core header failed\n");
+			goto out_err;
+		}
+
+		kbuf.buffer = headers;
+		kbuf.bufsz = headers_sz;
+		kbuf.mem = 0;
+		kbuf.memsz = headers_sz;
+		kbuf.buf_align = SZ_64K; /* largest supported page size */
+		kbuf.buf_max = ULONG_MAX;
+		kbuf.top_down = true;
+
+		ret = kexec_add_buffer(&kbuf);
+		if (ret) {
+			vfree(headers);
+			goto out_err;
+		}
+		image->arch.elf_headers = headers;
+		image->arch.elf_headers_mem = kbuf.mem;
+		image->arch.elf_headers_sz = headers_sz;
+
+		pr_debug("Loaded elf core header at 0x%lx bufsz=0x%lx memsz=0x%lx\n",
+			 image->arch.elf_headers_mem, headers_sz, headers_sz);
+	}
+
 	/* load initrd */
 	if (initrd) {
 		kbuf.buffer = initrd;
-- 
2.19.0