Re: [PATCH] libfdt: Remove special handling for unaligned reads

[David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>]

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

On Fri, Jan 17, 2020 at 10:31:06AM -0500, Tom Rini wrote:
> 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.
> 
> Ultimately however, these helpers should not exist.  Unaligned access
> only occurs when images are forced out of alignment by the user.  This
> unalignment is not supported and introduces problems later on as other
> parts of the system image are unaligned and they too require alignment.
> 
> Revert both of these changes.
> 
> Signed-off-by: Tom Rini <trini-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
> ---
> By way of a little more explanation, looking at the archives it seems
> that the initial bug reporter said that they had a platform that was
> using U-Boot and had the "fdt_high=0xffffffff" set in the environment.
> What that does is to tell U-Boot to not do any of the sanity checks and
> relocation to ensure alignment that it would normally do because the
> user knows best.  This later came up on the U-Boot list as once the DTB
> was loaded, Linux is unhappy because it demands correct alignment.
> 
> I only realized libfdt had introduced changes here when it was reported
> that boot time had gotten much slower once we merged this change in.  It
> would be best to just drop it.

Hmm.  I'm not sure about this.  The commit message makes a case for
why the unaligned handling isn't necessary, but not a case for why
it's bad.  Even if handling an unaligned tree isn't a common case,
isn't it better to be able to than not?

I gather from the previous discussion that there's a significant
performance impact, but that rationale needs to go into the commit
message for posterity.

[snip]
> diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
> index fc4c4962a01c..d4ebe915cf46 100644
> --- a/libfdt/libfdt.h
> +++ b/libfdt/libfdt.h
> @@ -117,23 +117,6 @@ 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
> - */
> -
> -static inline uint32_t fdt32_ld(const fdt32_t *p)
> -{
> -	const uint8_t *bp = (const uint8_t *)p;
> -
> -	return ((uint32_t)bp[0] << 24)
> -		| ((uint32_t)bp[1] << 16)
> -		| ((uint32_t)bp[2] << 8)
> -		| bp[3];
> -}

In particular, I definitely think removing the helpers entirely is a
no go.  They're now part of the published interface of the library.
Even if they're not used for reading the internal tags, they can be
used to load integers from within particular properties.  Those are
frequently unaligned, since properties generally have packed
representations.

How much of the performance loss would we get back if we put an actual
conditional on an aligned address in the helpers?

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