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