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

[Tom Rini <trini-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>]

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

On Thu, Nov 05, 2020 at 10:27:17AM -0600, Rob Herring wrote:
> On Wed, Nov 4, 2020 at 4:29 PM 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, 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.
> >
> > Signed-off-by: Tom Rini <trini-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
> > ---
> >  fdtget.c        |  2 +-
> >  libfdt/fdt_ro.c | 20 ++++++++++----------
> >  libfdt/libfdt.h | 23 +++++++++++++++++++----
> >  3 files changed, 30 insertions(+), 15 deletions(-)
> >
> > diff --git a/fdtget.c b/fdtget.c
> > index 777582e2d45f..0f643eb20a2e 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_ld((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..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..d5f11d6df48c 100644
> > --- a/libfdt/libfdt.h
> > +++ b/libfdt/libfdt.h
> > @@ -126,12 +126,27 @@ 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
> > + * 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)
> 
> As internal helpers, they should go in libfdt_internal.h.
> 
> With that,
> 
> Reviewed-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

So, fdtget.c doesn't grab libfdt_internal.h.  Since we're talking a user
space program, I'm just going to leave it with the API callers instead.

-- 
Tom

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