[PATCH] libfdt: Default to assuming aligned reads are OK

[PATCH] libfdt: Default to assuming aligned reads are OK

[Tom Rini]

Start with commit 6dcb8ba4 "libfdt: Add helpers for accessing unaligned
words" which introduced changes to support unaligned reads for ARM
platforms and 11738cf01f15 "libfdt: Don't use memcpy to handle unaligned
reads on ARM" to improve the performance of these helpers, libfdt has
defaulted to assuming that unaligned memory access could be a fatal
problem.

Upon further discussion on the mailing list, we leave the improved
unaligned-safe memory load functions available if needed, but go back to
using fdt{32,64}_to_cpu() for access as generally platforms handle
unaligned access safely and there is still a sizable performance and
size impact of using the always-safe helpers in all cases.

Signed-off-by: Tom Rini <trini-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
---
 fdtget.c        |  2 +-
 libfdt/fdt_ro.c | 20 ++++++++++----------
 libfdt/libfdt.h |  2 +-
 3 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/fdtget.c b/fdtget.c
index 777582e2d45f..7cee28718cbc 100644
--- a/fdtget.c
+++ b/fdtget.c
@@ -62,7 +62,7 @@ static int show_cell_list(struct display_info *disp, const char *data, int len,
 	for (i = 0; i < len; i += size, p += size) {
 		if (i)
 			printf(" ");
-		value = size == 4 ? fdt32_ld((const fdt32_t *)p) :
+		value = size == 4 ? fdt32_to_cpu(*(const fdt32_t *)p) :
 			size == 2 ? (*p << 8) | p[1] : *p;
 		printf(fmt, value);
 	}
diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
index 91cc6fefe374..5bc27ac97317 100644
--- a/libfdt/fdt_ro.c
+++ b/libfdt/fdt_ro.c
@@ -181,8 +181,8 @@ int fdt_get_mem_rsv(const void *fdt, int n, uint64_t *address, uint64_t *size)
 	if (!can_assume(VALID_INPUT) && !re)
 		return -FDT_ERR_BADOFFSET;
 
-	*address = fdt64_ld(&re->address);
-	*size = fdt64_ld(&re->size);
+	*address = fdt64_to_cpu(re->address);
+	*size = fdt64_to_cpu(re->size);
 	return 0;
 }
 
@@ -192,7 +192,7 @@ int fdt_num_mem_rsv(const void *fdt)
 	const struct fdt_reserve_entry *re;
 
 	for (i = 0; (re = fdt_mem_rsv(fdt, i)) != NULL; i++) {
-		if (fdt64_ld(&re->size) == 0)
+		if (fdt64_to_cpu(re->size) == 0)
 			return i;
 	}
 	return -FDT_ERR_TRUNCATED;
@@ -370,7 +370,7 @@ static const struct fdt_property *fdt_get_property_by_offset_(const void *fdt,
 	prop = fdt_offset_ptr_(fdt, offset);
 
 	if (lenp)
-		*lenp = fdt32_ld(&prop->len);
+		*lenp = fdt32_to_cpu(prop->len);
 
 	return prop;
 }
@@ -408,7 +408,7 @@ static const struct fdt_property *fdt_get_property_namelen_(const void *fdt,
 			offset = -FDT_ERR_INTERNAL;
 			break;
 		}
-		if (fdt_string_eq_(fdt, fdt32_ld(&prop->nameoff),
+		if (fdt_string_eq_(fdt, fdt32_to_cpu(prop->nameoff),
 				   name, namelen)) {
 			if (poffset)
 				*poffset = offset;
@@ -461,7 +461,7 @@ const void *fdt_getprop_namelen(const void *fdt, int nodeoffset,
 
 	/* Handle realignment */
 	if (!can_assume(LATEST) && fdt_version(fdt) < 0x10 &&
-	    (poffset + sizeof(*prop)) % 8 && fdt32_ld(&prop->len) >= 8)
+	    (poffset + sizeof(*prop)) % 8 && fdt32_to_cpu(prop->len) >= 8)
 		return prop->data + 4;
 	return prop->data;
 }
@@ -479,7 +479,7 @@ const void *fdt_getprop_by_offset(const void *fdt, int offset,
 		int namelen;
 
 		if (!can_assume(VALID_INPUT)) {
-			name = fdt_get_string(fdt, fdt32_ld(&prop->nameoff),
+			name = fdt_get_string(fdt, fdt32_to_cpu(prop->nameoff),
 					      &namelen);
 			if (!name) {
 				if (lenp)
@@ -488,13 +488,13 @@ const void *fdt_getprop_by_offset(const void *fdt, int offset,
 			}
 			*namep = name;
 		} else {
-			*namep = fdt_string(fdt, fdt32_ld(&prop->nameoff));
+			*namep = fdt_string(fdt, fdt32_to_cpu(prop->nameoff));
 		}
 	}
 
 	/* Handle realignment */
 	if (!can_assume(LATEST) && fdt_version(fdt) < 0x10 &&
-	    (offset + sizeof(*prop)) % 8 && fdt32_ld(&prop->len) >= 8)
+	    (offset + sizeof(*prop)) % 8 && fdt32_to_cpu(prop->len) >= 8)
 		return prop->data + 4;
 	return prop->data;
 }
@@ -519,7 +519,7 @@ uint32_t fdt_get_phandle(const void *fdt, int nodeoffset)
 			return 0;
 	}
 
-	return fdt32_ld(php);
+	return fdt32_to_cpu(*php);
 }
 
 const char *fdt_get_alias_namelen(const void *fdt,
diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
index 544d3efff584..5004300e7ddc 100644
--- a/libfdt/libfdt.h
+++ b/libfdt/libfdt.h
@@ -236,7 +236,7 @@ int fdt_next_subnode(const void *fdt, int offset);
 /* General functions                                                  */
 /**********************************************************************/
 #define fdt_get_header(fdt, field) \
-	(fdt32_ld(&((const struct fdt_header *)(fdt))->field))
+	(fdt32_to_cpu(((const struct fdt_header *)(fdt))->field))
 #define fdt_magic(fdt)			(fdt_get_header(fdt, magic))
 #define fdt_totalsize(fdt)		(fdt_get_header(fdt, totalsize))
 #define fdt_off_dt_struct(fdt)		(fdt_get_header(fdt, off_dt_struct))
-- 
2.17.1

Re: [PATCH] libfdt: Default to assuming aligned reads are OK

[David Gibson]

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

On Fri, Oct 30, 2020 at 02:56:58PM -0400, Tom Rini wrote:
> Start with commit 6dcb8ba4 "libfdt: Add helpers for accessing unaligned
> words" which introduced changes to support unaligned reads for ARM
> platforms and 11738cf01f15 "libfdt: Don't use memcpy to handle unaligned
> reads on ARM" to improve the performance of these helpers, libfdt has
> defaulted to assuming that unaligned memory access could be a fatal
> problem.
> 
> Upon further discussion on the mailing list, we leave the improved
> unaligned-safe memory load functions available if needed, but go back to
> using fdt{32,64}_to_cpu() for access as generally platforms handle
> unaligned access safely and there is still a sizable performance and
> size impact of using the always-safe helpers in all cases.

I think the basic change is ok, but there's some details I'd like to
see changed.

First, I'd like to see an alignment check added to fdt_probe_ro_().

> 
> Signed-off-by: Tom Rini <trini-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
> ---
>  fdtget.c        |  2 +-
>  libfdt/fdt_ro.c | 20 ++++++++++----------
>  libfdt/libfdt.h |  2 +-
>  3 files changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/fdtget.c b/fdtget.c
> index 777582e2d45f..7cee28718cbc 100644
> --- a/fdtget.c
> +++ b/fdtget.c
> @@ -62,7 +62,7 @@ static int show_cell_list(struct display_info *disp, const char *data, int len,
>  	for (i = 0; i < len; i += size, p += size) {
>  		if (i)
>  			printf(" ");
> -		value = size == 4 ? fdt32_ld((const fdt32_t *)p) :
> +		value = size == 4 ? fdt32_to_cpu(*(const fdt32_t *)p) :

Second, getting rid of these ugly open-coded constructions was an
additional reason I went to using fdt32_ld() everywhere.  So I'd like
to see both an assume-aligned and an unaligned-safe helper, rather
than open-coding the load-and-byteswap everywhere.

>  			size == 2 ? (*p << 8) | p[1] : *p;
>  		printf(fmt, value);
>  	}
> diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
> index 91cc6fefe374..5bc27ac97317 100644
> --- a/libfdt/fdt_ro.c
> +++ b/libfdt/fdt_ro.c
> @@ -181,8 +181,8 @@ int fdt_get_mem_rsv(const void *fdt, int n, uint64_t *address, uint64_t *size)
>  	if (!can_assume(VALID_INPUT) && !re)
>  		return -FDT_ERR_BADOFFSET;
>  
> -	*address = fdt64_ld(&re->address);
> -	*size = fdt64_ld(&re->size);
> +	*address = fdt64_to_cpu(re->address);
> +	*size = fdt64_to_cpu(re->size);
>  	return 0;
>  }
>  
> @@ -192,7 +192,7 @@ int fdt_num_mem_rsv(const void *fdt)
>  	const struct fdt_reserve_entry *re;
>  
>  	for (i = 0; (re = fdt_mem_rsv(fdt, i)) != NULL; i++) {
> -		if (fdt64_ld(&re->size) == 0)
> +		if (fdt64_to_cpu(re->size) == 0)
>  			return i;
>  	}
>  	return -FDT_ERR_TRUNCATED;
> @@ -370,7 +370,7 @@ static const struct fdt_property *fdt_get_property_by_offset_(const void *fdt,
>  	prop = fdt_offset_ptr_(fdt, offset);
>  
>  	if (lenp)
> -		*lenp = fdt32_ld(&prop->len);
> +		*lenp = fdt32_to_cpu(prop->len);
>  
>  	return prop;
>  }
> @@ -408,7 +408,7 @@ static const struct fdt_property *fdt_get_property_namelen_(const void *fdt,
>  			offset = -FDT_ERR_INTERNAL;
>  			break;
>  		}
> -		if (fdt_string_eq_(fdt, fdt32_ld(&prop->nameoff),
> +		if (fdt_string_eq_(fdt, fdt32_to_cpu(prop->nameoff),
>  				   name, namelen)) {
>  			if (poffset)
>  				*poffset = offset;
> @@ -461,7 +461,7 @@ const void *fdt_getprop_namelen(const void *fdt, int nodeoffset,
>  
>  	/* Handle realignment */
>  	if (!can_assume(LATEST) && fdt_version(fdt) < 0x10 &&
> -	    (poffset + sizeof(*prop)) % 8 && fdt32_ld(&prop->len) >= 8)
> +	    (poffset + sizeof(*prop)) % 8 && fdt32_to_cpu(prop->len) >= 8)
>  		return prop->data + 4;
>  	return prop->data;
>  }
> @@ -479,7 +479,7 @@ const void *fdt_getprop_by_offset(const void *fdt, int offset,
>  		int namelen;
>  
>  		if (!can_assume(VALID_INPUT)) {
> -			name = fdt_get_string(fdt, fdt32_ld(&prop->nameoff),
> +			name = fdt_get_string(fdt, fdt32_to_cpu(prop->nameoff),
>  					      &namelen);
>  			if (!name) {
>  				if (lenp)
> @@ -488,13 +488,13 @@ const void *fdt_getprop_by_offset(const void *fdt, int offset,
>  			}
>  			*namep = name;
>  		} else {
> -			*namep = fdt_string(fdt, fdt32_ld(&prop->nameoff));
> +			*namep = fdt_string(fdt, fdt32_to_cpu(prop->nameoff));
>  		}
>  	}
>  
>  	/* Handle realignment */
>  	if (!can_assume(LATEST) && fdt_version(fdt) < 0x10 &&
> -	    (offset + sizeof(*prop)) % 8 && fdt32_ld(&prop->len) >= 8)
> +	    (offset + sizeof(*prop)) % 8 && fdt32_to_cpu(prop->len) >= 8)
>  		return prop->data + 4;
>  	return prop->data;
>  }
> @@ -519,7 +519,7 @@ uint32_t fdt_get_phandle(const void *fdt, int nodeoffset)
>  			return 0;
>  	}
>  
> -	return fdt32_ld(php);
> +	return fdt32_to_cpu(*php);
>  }
>  
>  const char *fdt_get_alias_namelen(const void *fdt,
> diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
> index 544d3efff584..5004300e7ddc 100644
> --- a/libfdt/libfdt.h
> +++ b/libfdt/libfdt.h
> @@ -236,7 +236,7 @@ int fdt_next_subnode(const void *fdt, int offset);
>  /* General functions                                                  */
>  /**********************************************************************/
>  #define fdt_get_header(fdt, field) \
> -	(fdt32_ld(&((const struct fdt_header *)(fdt))->field))
> +	(fdt32_to_cpu(((const struct fdt_header *)(fdt))->field))
>  #define fdt_magic(fdt)			(fdt_get_header(fdt, magic))
>  #define fdt_totalsize(fdt)		(fdt_get_header(fdt, totalsize))
>  #define fdt_off_dt_struct(fdt)		(fdt_get_header(fdt, off_dt_struct))

-- 
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] libfdt: Default to assuming aligned reads are OK

[Tom Rini]

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

On Mon, Nov 02, 2020 at 11:56:07AM +1100, David Gibson wrote:
> On Fri, Oct 30, 2020 at 02:56:58PM -0400, Tom Rini wrote:
> > Start with commit 6dcb8ba4 "libfdt: Add helpers for accessing unaligned
> > words" which introduced changes to support unaligned reads for ARM
> > platforms and 11738cf01f15 "libfdt: Don't use memcpy to handle unaligned
> > reads on ARM" to improve the performance of these helpers, libfdt has
> > defaulted to assuming that unaligned memory access could be a fatal
> > problem.
> > 
> > Upon further discussion on the mailing list, we leave the improved
> > unaligned-safe memory load functions available if needed, but go back to
> > using fdt{32,64}_to_cpu() for access as generally platforms handle
> > unaligned access safely and there is still a sizable performance and
> > size impact of using the always-safe helpers in all cases.
> 
> I think the basic change is ok, but there's some details I'd like to
> see changed.
> 
> First, I'd like to see an alignment check added to fdt_probe_ro_().

OK, that's not hard.  Do you want a new error code or FDT_ERR_INTERNAL
or something else?

> > Signed-off-by: Tom Rini <trini-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
> > ---
> >  fdtget.c        |  2 +-
> >  libfdt/fdt_ro.c | 20 ++++++++++----------
> >  libfdt/libfdt.h |  2 +-
> >  3 files changed, 12 insertions(+), 12 deletions(-)
> > 
> > diff --git a/fdtget.c b/fdtget.c
> > index 777582e2d45f..7cee28718cbc 100644
> > --- a/fdtget.c
> > +++ b/fdtget.c
> > @@ -62,7 +62,7 @@ static int show_cell_list(struct display_info *disp, const char *data, int len,
> >  	for (i = 0; i < len; i += size, p += size) {
> >  		if (i)
> >  			printf(" ");
> > -		value = size == 4 ? fdt32_ld((const fdt32_t *)p) :
> > +		value = size == 4 ? fdt32_to_cpu(*(const fdt32_t *)p) :
> 
> Second, getting rid of these ugly open-coded constructions was an
> additional reason I went to using fdt32_ld() everywhere.  So I'd like
> to see both an assume-aligned and an unaligned-safe helper, rather
> than open-coding the load-and-byteswap everywhere.

OK, I think I've got an idea.

-- 
Tom

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

Re: [PATCH] libfdt: Default to assuming aligned reads are OK

[David Gibson]

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

On Mon, Nov 02, 2020 at 11:03:47AM -0500, Tom Rini wrote:
> On Mon, Nov 02, 2020 at 11:56:07AM +1100, David Gibson wrote:
> > On Fri, Oct 30, 2020 at 02:56:58PM -0400, Tom Rini wrote:
> > > Start with commit 6dcb8ba4 "libfdt: Add helpers for accessing unaligned
> > > words" which introduced changes to support unaligned reads for ARM
> > > platforms and 11738cf01f15 "libfdt: Don't use memcpy to handle unaligned
> > > reads on ARM" to improve the performance of these helpers, libfdt has
> > > defaulted to assuming that unaligned memory access could be a fatal
> > > problem.
> > > 
> > > Upon further discussion on the mailing list, we leave the improved
> > > unaligned-safe memory load functions available if needed, but go back to
> > > using fdt{32,64}_to_cpu() for access as generally platforms handle
> > > unaligned access safely and there is still a sizable performance and
> > > size impact of using the always-safe helpers in all cases.
> > 
> > I think the basic change is ok, but there's some details I'd like to
> > see changed.
> > 
> > First, I'd like to see an alignment check added to fdt_probe_ro_().
> 
> OK, that's not hard.  Do you want a new error code or FDT_ERR_INTERNAL
> or something else?

It definitely shouldn't be FDT_ERR_INTERNAL - that's always supposed
to indicate a bug within libfdt itself (basicaly anything that returns
FDT_ERR_INTERNAL wants to be an assert(), but actually using assert()
would pull in complex dependencies we don't want).

/me looks through the error list

Yeah, nothing there really suits, so we'll need to add a new one for
this.

> > > Signed-off-by: Tom Rini <trini-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
> > > ---
> > >  fdtget.c        |  2 +-
> > >  libfdt/fdt_ro.c | 20 ++++++++++----------
> > >  libfdt/libfdt.h |  2 +-
> > >  3 files changed, 12 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/fdtget.c b/fdtget.c
> > > index 777582e2d45f..7cee28718cbc 100644
> > > --- a/fdtget.c
> > > +++ b/fdtget.c
> > > @@ -62,7 +62,7 @@ static int show_cell_list(struct display_info *disp, const char *data, int len,
> > >  	for (i = 0; i < len; i += size, p += size) {
> > >  		if (i)
> > >  			printf(" ");
> > > -		value = size == 4 ? fdt32_ld((const fdt32_t *)p) :
> > > +		value = size == 4 ? fdt32_to_cpu(*(const fdt32_t *)p) :
> > 
> > Second, getting rid of these ugly open-coded constructions was an
> > additional reason I went to using fdt32_ld() everywhere.  So I'd like
> > to see both an assume-aligned and an unaligned-safe helper, rather
> > than open-coding the load-and-byteswap everywhere.
> 
> OK, I think I've got an idea.
> 



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