On Thu, Jan 23, 2020 at 07:16:50AM -0500, Tom Rini wrote:
> 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".
The initial report isn't the only relevant thing here. Although it
prompted the change, it wasn't the only consideration in making it.
There's also two separate questions here:
1) Do we want byteswapping integer load helpers?
2) Should those handle unaligned accesses?
In working out how to address the (as it turns out, non existent)
problem, I realized an abstraction for loading big-endian integers
from the blob would be a useful thing in its own right. I also
realized that it's a useful thing not just inside the libfdt code, but
as an external interface. Callers have always needed to interpret the
contents of individual dt properties, and loading integers from them
is often a part of that.
I know of people using those fdtXX_ld() helpers right now, so I'm not
going to take them away.
For the case of external users we absolutely do need to handle
unaligned accesses. There are a number of existing bindings that mix
strings and integers in packed format, in a single encoded property.
So regardless of the alignment of the whole property, we can easily
get unaligned integers in there, and I don't want to expose multiple
different helpers for different cases.
Now, we don't *have* to use the same helpers for internal use. We
could open code the internal loads, or use a special aligned-only
version inside. But using the existing external helpers is the
obvious and simple choice.
So, we're back to: I need a case for changing this now, not just a
case for claiming it wasn't needed in the first place.
--
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
