[PATCH] libfdt: Make fdt{32,64}_ld() default to assuming unaligned access is safe

[PATCH] libfdt: Make fdt{32,64}_ld() default to assuming unaligned access is safe

[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, suffix the existing fdt{32,64}_ld functions with
_unaligned and introduce new load functions that call
fdt{32,64}_to_cpu() as was done prior to the above mentioned commits.

Signed-off-by: Tom Rini <trini-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
---
 libfdt/libfdt.h | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
index b600c8d6dd41..307ba745c92f 100644
--- a/libfdt/libfdt.h
+++ b/libfdt/libfdt.h
@@ -126,13 +126,22 @@ 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
+ * Load functions.  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);
+}
+
+static inline uint32_t fdt32_ld_unaligned(const fdt32_t *p)
 {
 	const uint8_t *bp = (const uint8_t *)p;
 
@@ -152,7 +161,7 @@ static inline void fdt32_st(void *property, uint32_t value)
 	bp[3] = value & 0xff;
 }
 
-static inline uint64_t fdt64_ld(const fdt64_t *p)
+static inline uint64_t fdt64_ld_unaligned(const fdt64_t *p)
 {
 	const uint8_t *bp = (const uint8_t *)p;
 
-- 
2.17.1

Re: [PATCH] libfdt: Make fdt{32,64}_ld() default to assuming unaligned access is safe

[Rob Herring]

On Wed, Nov 4, 2020 at 7:46 AM Tom Rini <trini-OWPKS81ov/FWk0Htik3J/w@public.gmane.org> 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, suffix the existing fdt{32,64}_ld functions with
> _unaligned and introduce new load functions that call
> fdt{32,64}_to_cpu() as was done prior to the above mentioned commits.
>
> Signed-off-by: Tom Rini <trini-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
> ---
>  libfdt/libfdt.h | 21 +++++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)
>
> diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
> index b600c8d6dd41..307ba745c92f 100644
> --- a/libfdt/libfdt.h
> +++ b/libfdt/libfdt.h
> @@ -126,13 +126,22 @@ 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
> + * Load functions.  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);

This changes the public behavior of fdt32_ld() which is one of the
things David was against.

I think we want a _fdt32_ld or fdt32_ld_internal or ?? which is
internal only and doesn't create another ABI.

Rob

Re: [PATCH] libfdt: Make fdt{32,64}_ld() default to assuming unaligned access is safe

[Tom Rini]

On Wed, Nov 04, 2020 at 11:29:28AM -0600, Rob Herring wrote:
> On Wed, Nov 4, 2020 at 7:46 AM Tom Rini <trini-OWPKS81ov/FWk0Htik3J/w@public.gmane.org> 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, suffix the existing fdt{32,64}_ld functions with
> > _unaligned and introduce new load functions that call
> > fdt{32,64}_to_cpu() as was done prior to the above mentioned commits.
> >
> > Signed-off-by: Tom Rini <trini-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
> > ---
> >  libfdt/libfdt.h | 21 +++++++++++++++------
> >  1 file changed, 15 insertions(+), 6 deletions(-)
> >
> > diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
> > index b600c8d6dd41..307ba745c92f 100644
> > --- a/libfdt/libfdt.h
> > +++ b/libfdt/libfdt.h
> > @@ -126,13 +126,22 @@ 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
> > + * Load functions.  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);
> 
> This changes the public behavior of fdt32_ld() which is one of the
> things David was against.
> 
> I think we want a _fdt32_ld or fdt32_ld_internal or ?? which is
> internal only and doesn't create another ABI.

I thought it was minimal code change?  But anyhow, yes, I can easily
respin things.  Should I do that now or wait for David's comments?

-- 
Tom