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