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

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

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

On Thu, Jan 23, 2020 at 08:14:40PM +1100, David Gibson wrote:
> 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.

I wanted to emphasize that the code simply isn't ever needed, not that
it's a performance problem.  A performance problem implies that we would
keep this, if it was fast enough.  That's why people noticed it (it
slows things down to an unusable level).  But it's functionally wrong.

> [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.

Perhaps "mistakes were made" ?  I don't think we need to worry about
removing an interface here as projects are avoiding upgrading libfdt
now (TF-A, formerly ATF) or reverting the change (U-Boot).

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

I don't see the relevance.  Go back to the initial problem report.  It's
not "I have a new unique platform I'm using libfdt on and I have
problems".  It's "I keep jabbing myself with a rusty nail and now I have
problems".

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

I don't know but to be very clear, we don't ever need these on ARM.
Keep in mind we've been using device trees on ARM for over 10 years at
this point.

-- 
Tom

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