[PATCHv2] libfdt: Internally perform potentially unaligned loads

[PATCHv2] libfdt: Internally perform potentially unaligned loads

[Tom Rini]

Commits 6dcb8ba4 "libfdt: Add helpers for accessing unaligned words"
introduced changes to support unaligned reads for ARM platforms and
11738cf01f15 "libfdt: Don't use memcpy to handle unaligned reads on ARM"
improved the performance of these helpers.

On further discussion, while there are potential cases where we could be
used on platforms that do not fixup unaligned reads for us, making this
choice the default is very expensive in terms of binary size and access
time.  To address this, introduce and use new _fdt{32,64}_ld functions
that call fdt{32,64}_to_cpu() as was done prior to the above mentioned
commits.  Leave the existing load functions as unaligned-safe and
include comments in both cases.

Reviewed-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Signed-off-by: Tom Rini <trini-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
---
Changes in v2:
- Move the new internal functions to libfdt_internal.h (per Rob).
- Drop change to fdtget.c as it shouldn't use internal helpers, only
  exposed API.
- Add Rob's Reviewed-by
---
 libfdt/fdt_ro.c          | 20 ++++++++++----------
 libfdt/libfdt.h          |  8 +++-----
 libfdt/libfdt_internal.h | 17 +++++++++++++++++
 3 files changed, 30 insertions(+), 15 deletions(-)

diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
index 91cc6fefe374..414b417362a8 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_ld(&re->address);
+	*size = _fdt64_ld(&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_ld(&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_ld(&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_ld(&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_ld(&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_ld(&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_ld(&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_ld(&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_ld(php);
 }
 
 const char *fdt_get_alias_namelen(const void *fdt,
diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
index b600c8d6dd41..3f36ed6d690f 100644
--- a/libfdt/libfdt.h
+++ b/libfdt/libfdt.h
@@ -126,12 +126,10 @@ static inline void *fdt_offset_ptr_w(void *fdt, int offset, int checklen)
 uint32_t fdt_next_tag(const void *fdt, int offset, int *nextoffset);
 
 /*
- * Alignment helpers:
- *     These helpers access words from a device tree blob.  They're
- *     built to work even with unaligned pointers on platforms (ike
- *     ARM) that don't like unaligned loads and stores
+ * External helpers to access words from a device tree blob.  These functions
+ * work in a manner that is safe on platforms that do not handle unaligned
+ * memory accesses and need special care in those cases.
  */
-
 static inline uint32_t fdt32_ld(const fdt32_t *p)
 {
 	const uint8_t *bp = (const uint8_t *)p;
diff --git a/libfdt/libfdt_internal.h b/libfdt/libfdt_internal.h
index d4e0bd49c037..785b8b45ad1c 100644
--- a/libfdt/libfdt_internal.h
+++ b/libfdt/libfdt_internal.h
@@ -46,6 +46,23 @@ static inline struct fdt_reserve_entry *fdt_mem_rsv_w_(void *fdt, int n)
 	return (void *)(uintptr_t)fdt_mem_rsv_(fdt, n);
 }
 
+/*
+ * Internal helpers to access words from a device tree blob.  Assume that we
+ * are on a platform where unaligned memory reads will be handled in a graceful
+ * manner and that we do not need to ensure our reads are aligned.  If this is
+ * not the case there are _unaligned versions of these functions that follow
+ * and can be used.
+ */
+static inline uint32_t _fdt32_ld(const fdt32_t *p)
+{
+	return fdt32_to_cpu(*p);
+}
+
+static inline uint64_t _fdt64_ld(const fdt64_t *p)
+{
+	return fdt64_to_cpu(*p);
+}
+
 #define FDT_SW_MAGIC		(~FDT_MAGIC)
 
 /**********************************************************************/
-- 
2.17.1

Re: [PATCHv2] libfdt: Internally perform potentially unaligned loads

[Tom Rini]

On Thu, Nov 05, 2020 at 11:57:07AM -0500, Tom Rini wrote:

> Commits 6dcb8ba4 "libfdt: Add helpers for accessing unaligned words"
> introduced changes to support unaligned reads for ARM platforms and
> 11738cf01f15 "libfdt: Don't use memcpy to handle unaligned reads on ARM"
> improved the performance of these helpers.
> 
> On further discussion, while there are potential cases where we could be
> used on platforms that do not fixup unaligned reads for us, making this
> choice the default is very expensive in terms of binary size and access
> time.  To address this, introduce and use new _fdt{32,64}_ld functions
> that call fdt{32,64}_to_cpu() as was done prior to the above mentioned
> commits.  Leave the existing load functions as unaligned-safe and
> include comments in both cases.
> 
> Reviewed-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Signed-off-by: Tom Rini <trini-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
> ---
> Changes in v2:
> - Move the new internal functions to libfdt_internal.h (per Rob).
> - Drop change to fdtget.c as it shouldn't use internal helpers, only
>   exposed API.
> - Add Rob's Reviewed-by
> ---
>  libfdt/fdt_ro.c          | 20 ++++++++++----------
>  libfdt/libfdt.h          |  8 +++-----
>  libfdt/libfdt_internal.h | 17 +++++++++++++++++
>  3 files changed, 30 insertions(+), 15 deletions(-)
> 
> diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
> index 91cc6fefe374..414b417362a8 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_ld(&re->address);
> +	*size = _fdt64_ld(&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_ld(&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_ld(&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_ld(&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_ld(&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_ld(&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_ld(&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_ld(&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_ld(php);
>  }
>  
>  const char *fdt_get_alias_namelen(const void *fdt,
> diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
> index b600c8d6dd41..3f36ed6d690f 100644
> --- a/libfdt/libfdt.h
> +++ b/libfdt/libfdt.h
> @@ -126,12 +126,10 @@ static inline void *fdt_offset_ptr_w(void *fdt, int offset, int checklen)
>  uint32_t fdt_next_tag(const void *fdt, int offset, int *nextoffset);
>  
>  /*
> - * Alignment helpers:
> - *     These helpers access words from a device tree blob.  They're
> - *     built to work even with unaligned pointers on platforms (ike
> - *     ARM) that don't like unaligned loads and stores
> + * External helpers to access words from a device tree blob.  These functions
> + * work in a manner that is safe on platforms that do not handle unaligned
> + * memory accesses and need special care in those cases.
>   */
> -
>  static inline uint32_t fdt32_ld(const fdt32_t *p)
>  {
>  	const uint8_t *bp = (const uint8_t *)p;
> diff --git a/libfdt/libfdt_internal.h b/libfdt/libfdt_internal.h
> index d4e0bd49c037..785b8b45ad1c 100644
> --- a/libfdt/libfdt_internal.h
> +++ b/libfdt/libfdt_internal.h
> @@ -46,6 +46,23 @@ static inline struct fdt_reserve_entry *fdt_mem_rsv_w_(void *fdt, int n)
>  	return (void *)(uintptr_t)fdt_mem_rsv_(fdt, n);
>  }
>  
> +/*
> + * Internal helpers to access words from a device tree blob.  Assume that we
> + * are on a platform where unaligned memory reads will be handled in a graceful
> + * manner and that we do not need to ensure our reads are aligned.  If this is
> + * not the case there are _unaligned versions of these functions that follow
> + * and can be used.
> + */
> +static inline uint32_t _fdt32_ld(const fdt32_t *p)
> +{
> +	return fdt32_to_cpu(*p);
> +}
> +
> +static inline uint64_t _fdt64_ld(const fdt64_t *p)
> +{
> +	return fdt64_to_cpu(*p);
> +}
> +
>  #define FDT_SW_MAGIC		(~FDT_MAGIC)
>  
>  /**********************************************************************/

Any comments?  Thanks.

-- 
Tom

Re: [PATCHv2] libfdt: Internally perform potentially unaligned loads

[David Gibson]

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

Hi Tom,

Sorry I've taken so long to reply to this.  I was pretty busy, and
then on holiday away from my email.

Overall I think this looks good, but there are some nits and some
inaccuracies in comments.

On Thu, Nov 05, 2020 at 11:57:07AM -0500, Tom Rini wrote:
> Commits 6dcb8ba4 "libfdt: Add helpers for accessing unaligned words"
> introduced changes to support unaligned reads for ARM platforms and
> 11738cf01f15 "libfdt: Don't use memcpy to handle unaligned reads on ARM"
> improved the performance of these helpers.
> 
> On further discussion, while there are potential cases where we could be
> used on platforms that do not fixup unaligned reads for us, making this
> choice the default is very expensive in terms of binary size and access
> time.  To address this, introduce and use new _fdt{32,64}_ld functions
> that call fdt{32,64}_to_cpu() as was done prior to the above mentioned
> commits.  Leave the existing load functions as unaligned-safe and
> include comments in both cases.

So, leading underscore identifiers are off limits for libfdt - they're
reserved for the system libraries (the kernel can get away with it
because it doesn't use the system libraries, but we sometimes do).

The usual workaround is to use a trailing underscore instead
(e.g. fdt_add_property_(), fdt_splice_struct_()).  Although.. the
convention with those (similar to the kernel's) is that foo() is
usually a safer wrapper implemented in terms of foo_().  In this case
fdtXX_ld() is not, and cannot, be implemented in terms of fdtXX_ld_().

[snip]
>  const char *fdt_get_alias_namelen(const void *fdt,
> diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
> index b600c8d6dd41..3f36ed6d690f 100644
> --- a/libfdt/libfdt.h
> +++ b/libfdt/libfdt.h
> @@ -126,12 +126,10 @@ static inline void *fdt_offset_ptr_w(void *fdt, int offset, int checklen)
>  uint32_t fdt_next_tag(const void *fdt, int offset, int *nextoffset);
>  
>  /*
> - * Alignment helpers:
> - *     These helpers access words from a device tree blob.  They're
> - *     built to work even with unaligned pointers on platforms (ike
> - *     ARM) that don't like unaligned loads and stores
> + * External helpers to access words from a device tree blob.  These functions
> + * work in a manner that is safe on platforms that do not handle unaligned
> + * memory accesses and need special care in those cases.

I actually prefer the old wording for the most part.  Could you just
tweak it for the changes, rather than rewriting the whole thing?

>   */
> -
>  static inline uint32_t fdt32_ld(const fdt32_t *p)
>  {
>  	const uint8_t *bp = (const uint8_t *)p;
> diff --git a/libfdt/libfdt_internal.h b/libfdt/libfdt_internal.h
> index d4e0bd49c037..785b8b45ad1c 100644
> --- a/libfdt/libfdt_internal.h
> +++ b/libfdt/libfdt_internal.h
> @@ -46,6 +46,23 @@ static inline struct fdt_reserve_entry *fdt_mem_rsv_w_(void *fdt, int n)
>  	return (void *)(uintptr_t)fdt_mem_rsv_(fdt, n);
>  }
>  
> +/*
> + * Internal helpers to access words from a device tree blob.  Assume that we
> + * are on a platform where unaligned memory reads will be handled in a graceful
> + * manner and that we do not need to ensure our reads are aligned.  If this is
> + * not the case there are _unaligned versions of these functions that follow
> + * and can be used.

Can you adjust this to emphasise a couple of points:
 * These helpers are intended for structural elements of the DT,
   rather than for reading integers from within property values
 * These are safe if EITHER the platform handles unaligned loads OR
   they're given naturally aligned addresses.  Currently you only
   mention the first, but the second condition is really the one we
   rely on, since it should be true in practice for the users of these
   assuming a compliant dtb loaded at an 8-byte aligned address.

Finally, given the history, I'm just a little paranoid that somebody
in future is going to hit some weird platform with some new weird
alignment issue.  So, I'm rather tempted to tie this to a new ASSUME
flag (though I'd be willing to have it default to on).  I'd envision:
  1. You use new internal load wrappers
  2. If can_assume(ALIGNED) or whatever, those wrappers expand to the
     load and byteswap only
  3. if !can_assume(ALIGNED), they call the external, unaligned-safe
     helpers instead

> + */
> +static inline uint32_t _fdt32_ld(const fdt32_t *p)
> +{
> +	return fdt32_to_cpu(*p);
> +}
> +
> +static inline uint64_t _fdt64_ld(const fdt64_t *p)
> +{
> +	return fdt64_to_cpu(*p);
> +}
> +
>  #define FDT_SW_MAGIC		(~FDT_MAGIC)
>  
>  /**********************************************************************/

-- 
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: [PATCHv2] libfdt: Internally perform potentially unaligned loads

[Tom Rini]

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

On Tue, Nov 24, 2020 at 04:41:04PM +1100, David Gibson wrote:
> Hi Tom,
> 
> Sorry I've taken so long to reply to this.  I was pretty busy, and
> then on holiday away from my email.

Thanks for explaining.

> Overall I think this looks good, but there are some nits and some
> inaccuracies in comments.
> 
> On Thu, Nov 05, 2020 at 11:57:07AM -0500, Tom Rini wrote:
> > Commits 6dcb8ba4 "libfdt: Add helpers for accessing unaligned words"
> > introduced changes to support unaligned reads for ARM platforms and
> > 11738cf01f15 "libfdt: Don't use memcpy to handle unaligned reads on ARM"
> > improved the performance of these helpers.
> > 
> > On further discussion, while there are potential cases where we could be
> > used on platforms that do not fixup unaligned reads for us, making this
> > choice the default is very expensive in terms of binary size and access
> > time.  To address this, introduce and use new _fdt{32,64}_ld functions
> > that call fdt{32,64}_to_cpu() as was done prior to the above mentioned
> > commits.  Leave the existing load functions as unaligned-safe and
> > include comments in both cases.
> 
> So, leading underscore identifiers are off limits for libfdt - they're
> reserved for the system libraries (the kernel can get away with it
> because it doesn't use the system libraries, but we sometimes do).
> 
> The usual workaround is to use a trailing underscore instead
> (e.g. fdt_add_property_(), fdt_splice_struct_()).  Although.. the
> convention with those (similar to the kernel's) is that foo() is
> usually a safer wrapper implemented in terms of foo_().  In this case
> fdtXX_ld() is not, and cannot, be implemented in terms of fdtXX_ld_().

Alright, so not _foo().  foo__() to indicate it's special?

> [snip]
> >  const char *fdt_get_alias_namelen(const void *fdt,
> > diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
> > index b600c8d6dd41..3f36ed6d690f 100644
> > --- a/libfdt/libfdt.h
> > +++ b/libfdt/libfdt.h
> > @@ -126,12 +126,10 @@ static inline void *fdt_offset_ptr_w(void *fdt, int offset, int checklen)
> >  uint32_t fdt_next_tag(const void *fdt, int offset, int *nextoffset);
> >  
> >  /*
> > - * Alignment helpers:
> > - *     These helpers access words from a device tree blob.  They're
> > - *     built to work even with unaligned pointers on platforms (ike
> > - *     ARM) that don't like unaligned loads and stores
> > + * External helpers to access words from a device tree blob.  These functions
> > + * work in a manner that is safe on platforms that do not handle unaligned
> > + * memory accesses and need special care in those cases.
> 
> I actually prefer the old wording for the most part.  Could you just
> tweak it for the changes, rather than rewriting the whole thing?

I rewrote it because I didn't like the level of accuracy in the existing
comment.  As Rob detailed in the other thread, yes, ARM could be a
problem, but only if you don't enable the feature that's virtually
always enabled on cores that have it.   But, I don't feel so strongly
about it that it's worth delaying this either.  Do you prefer:

External helpers to access words from a device tree blob. They're built
to work even with unaligned pointers on platforms (like ARM) that don't
like unaligned loads and stores.

Or:

External helpers to access words from a device tree blob. They're built
to work even with unaligned pointers on platforms that don't like
unaligned loads and stores.

> >   */
> > -
> >  static inline uint32_t fdt32_ld(const fdt32_t *p)
> >  {
> >  	const uint8_t *bp = (const uint8_t *)p;
> > diff --git a/libfdt/libfdt_internal.h b/libfdt/libfdt_internal.h
> > index d4e0bd49c037..785b8b45ad1c 100644
> > --- a/libfdt/libfdt_internal.h
> > +++ b/libfdt/libfdt_internal.h
> > @@ -46,6 +46,23 @@ static inline struct fdt_reserve_entry *fdt_mem_rsv_w_(void *fdt, int n)
> >  	return (void *)(uintptr_t)fdt_mem_rsv_(fdt, n);
> >  }
> >  
> > +/*
> > + * Internal helpers to access words from a device tree blob.  Assume that we
> > + * are on a platform where unaligned memory reads will be handled in a graceful
> > + * manner and that we do not need to ensure our reads are aligned.  If this is
> > + * not the case there are _unaligned versions of these functions that follow
> > + * and can be used.
> 
> Can you adjust this to emphasise a couple of points:
>  * These helpers are intended for structural elements of the DT,
>    rather than for reading integers from within property values
>  * These are safe if EITHER the platform handles unaligned loads OR
>    they're given naturally aligned addresses.  Currently you only
>    mention the first, but the second condition is really the one we
>    rely on, since it should be true in practice for the users of these
>    assuming a compliant dtb loaded at an 8-byte aligned address.

OK.

> Finally, given the history, I'm just a little paranoid that somebody
> in future is going to hit some weird platform with some new weird
> alignment issue.  So, I'm rather tempted to tie this to a new ASSUME
> flag (though I'd be willing to have it default to on).  I'd envision:
>   1. You use new internal load wrappers
>   2. If can_assume(ALIGNED) or whatever, those wrappers expand to the
>      load and byteswap only
>   3. if !can_assume(ALIGNED), they call the external, unaligned-safe
>      helpers instead

But the entirety of the history of the problem is caught with the other
patch, fail directly if we're not 8 byte aligned.

That said, if we renumber the ASSUME values so that
ASSUME_SAFE_UNALIGNED_LOAD, I would hope that the compiler would do the
right thing and optimize things the way we need them over in U-Boot.
But I have to experiment first there to be sure.

-- 
Tom

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

Re: [PATCHv2] libfdt: Internally perform potentially unaligned loads

[David Gibson]

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

On Tue, Nov 24, 2020 at 07:25:38AM -0500, Tom Rini wrote:
> On Tue, Nov 24, 2020 at 04:41:04PM +1100, David Gibson wrote:
> > Hi Tom,
> > 
> > Sorry I've taken so long to reply to this.  I was pretty busy, and
> > then on holiday away from my email.
> 
> Thanks for explaining.
> 
> > Overall I think this looks good, but there are some nits and some
> > inaccuracies in comments.
> > 
> > On Thu, Nov 05, 2020 at 11:57:07AM -0500, Tom Rini wrote:
> > > Commits 6dcb8ba4 "libfdt: Add helpers for accessing unaligned words"
> > > introduced changes to support unaligned reads for ARM platforms and
> > > 11738cf01f15 "libfdt: Don't use memcpy to handle unaligned reads on ARM"
> > > improved the performance of these helpers.
> > > 
> > > On further discussion, while there are potential cases where we could be
> > > used on platforms that do not fixup unaligned reads for us, making this
> > > choice the default is very expensive in terms of binary size and access
> > > time.  To address this, introduce and use new _fdt{32,64}_ld functions
> > > that call fdt{32,64}_to_cpu() as was done prior to the above mentioned
> > > commits.  Leave the existing load functions as unaligned-safe and
> > > include comments in both cases.
> > 
> > So, leading underscore identifiers are off limits for libfdt - they're
> > reserved for the system libraries (the kernel can get away with it
> > because it doesn't use the system libraries, but we sometimes do).
> > 
> > The usual workaround is to use a trailing underscore instead
> > (e.g. fdt_add_property_(), fdt_splice_struct_()).  Although.. the
> > convention with those (similar to the kernel's) is that foo() is
> > usually a safer wrapper implemented in terms of foo_().  In this case
> > fdtXX_ld() is not, and cannot, be implemented in terms of fdtXX_ld_().
> 
> Alright, so not _foo().  foo__() to indicate it's special?

Just foo_() will be ok.  Two underscores doesn't really clarify
anything.  fdtXX_ld_aligned() would be clearer, but are also really
verbose.

> > [snip]
> > >  const char *fdt_get_alias_namelen(const void *fdt,
> > > diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
> > > index b600c8d6dd41..3f36ed6d690f 100644
> > > --- a/libfdt/libfdt.h
> > > +++ b/libfdt/libfdt.h
> > > @@ -126,12 +126,10 @@ static inline void *fdt_offset_ptr_w(void *fdt, int offset, int checklen)
> > >  uint32_t fdt_next_tag(const void *fdt, int offset, int *nextoffset);
> > >  
> > >  /*
> > > - * Alignment helpers:
> > > - *     These helpers access words from a device tree blob.  They're
> > > - *     built to work even with unaligned pointers on platforms (ike
> > > - *     ARM) that don't like unaligned loads and stores
> > > + * External helpers to access words from a device tree blob.  These functions
> > > + * work in a manner that is safe on platforms that do not handle unaligned
> > > + * memory accesses and need special care in those cases.
> > 
> > I actually prefer the old wording for the most part.  Could you just
> > tweak it for the changes, rather than rewriting the whole thing?
> 
> I rewrote it because I didn't like the level of accuracy in the existing
> comment.  As Rob detailed in the other thread, yes, ARM could be a
> problem, but only if you don't enable the feature that's virtually
> always enabled on cores that have it.

I'd be fine with adjusting that to say "like certain ARM
configurations" or something similar.

> But, I don't feel so strongly
> about it that it's worth delaying this either.  Do you prefer:
> 
> External helpers to access words from a device tree blob. They're built
> to work even with unaligned pointers on platforms (like ARM) that don't
> like unaligned loads and stores.
> 
> Or:
> 
> External helpers to access words from a device tree blob. They're built
> to work even with unaligned pointers on platforms that don't like
> unaligned loads and stores.

I'd prefer to give some example, but changing that example to be more
accurate would be welcome.
> 
> > >   */
> > > -
> > >  static inline uint32_t fdt32_ld(const fdt32_t *p)
> > >  {
> > >  	const uint8_t *bp = (const uint8_t *)p;
> > > diff --git a/libfdt/libfdt_internal.h b/libfdt/libfdt_internal.h
> > > index d4e0bd49c037..785b8b45ad1c 100644
> > > --- a/libfdt/libfdt_internal.h
> > > +++ b/libfdt/libfdt_internal.h
> > > @@ -46,6 +46,23 @@ static inline struct fdt_reserve_entry *fdt_mem_rsv_w_(void *fdt, int n)
> > >  	return (void *)(uintptr_t)fdt_mem_rsv_(fdt, n);
> > >  }
> > >  
> > > +/*
> > > + * Internal helpers to access words from a device tree blob.  Assume that we
> > > + * are on a platform where unaligned memory reads will be handled in a graceful
> > > + * manner and that we do not need to ensure our reads are aligned.  If this is
> > > + * not the case there are _unaligned versions of these functions that follow
> > > + * and can be used.
> > 
> > Can you adjust this to emphasise a couple of points:
> >  * These helpers are intended for structural elements of the DT,
> >    rather than for reading integers from within property values
> >  * These are safe if EITHER the platform handles unaligned loads OR
> >    they're given naturally aligned addresses.  Currently you only
> >    mention the first, but the second condition is really the one we
> >    rely on, since it should be true in practice for the users of these
> >    assuming a compliant dtb loaded at an 8-byte aligned address.
> 
> OK.
> 
> > Finally, given the history, I'm just a little paranoid that somebody
> > in future is going to hit some weird platform with some new weird
> > alignment issue.  So, I'm rather tempted to tie this to a new ASSUME
> > flag (though I'd be willing to have it default to on).  I'd envision:
> >   1. You use new internal load wrappers
> >   2. If can_assume(ALIGNED) or whatever, those wrappers expand to the
> >      load and byteswap only
> >   3. if !can_assume(ALIGNED), they call the external, unaligned-safe
> >      helpers instead
> 
> But the entirety of the history of the problem is caught with the other
> patch, fail directly if we're not 8 byte aligned.

Hmm... yes, I guess that's true.  Ok, let's leave out a new assume
flag for now, we can add it if we really do hit a problem in future.

> That said, if we renumber the ASSUME values so that
> ASSUME_SAFE_UNALIGNED_LOAD, I would hope that the compiler would do the
> right thing and optimize things the way we need them over in U-Boot.
> But I have to experiment first there to be sure.

All the ASSUME flags are intended to work that way - they're
written as runtime tests in the source, but the expectation is that
they will be resolved at compile time.  If that's not true, it needs
addressing.

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