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@xxxxxxxxxxxx>
> ---
> 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:
signature.asc
Description: PGP signature
