Re: [PATCH] libfdt: Default to assuming aligned reads are OK

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

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

On Mon, Nov 02, 2020 at 11:03:47AM -0500, Tom Rini wrote:
> On Mon, Nov 02, 2020 at 11:56:07AM +1100, David Gibson wrote:
> > On Fri, Oct 30, 2020 at 02:56:58PM -0400, Tom Rini wrote:
> > > Start with commit 6dcb8ba4 "libfdt: Add helpers for accessing unaligned
> > > words" which introduced changes to support unaligned reads for ARM
> > > platforms and 11738cf01f15 "libfdt: Don't use memcpy to handle unaligned
> > > reads on ARM" to improve the performance of these helpers, libfdt has
> > > defaulted to assuming that unaligned memory access could be a fatal
> > > problem.
> > > 
> > > Upon further discussion on the mailing list, we leave the improved
> > > unaligned-safe memory load functions available if needed, but go back to
> > > using fdt{32,64}_to_cpu() for access as generally platforms handle
> > > unaligned access safely and there is still a sizable performance and
> > > size impact of using the always-safe helpers in all cases.
> > 
> > I think the basic change is ok, but there's some details I'd like to
> > see changed.
> > 
> > First, I'd like to see an alignment check added to fdt_probe_ro_().
> 
> OK, that's not hard.  Do you want a new error code or FDT_ERR_INTERNAL
> or something else?

It definitely shouldn't be FDT_ERR_INTERNAL - that's always supposed
to indicate a bug within libfdt itself (basicaly anything that returns
FDT_ERR_INTERNAL wants to be an assert(), but actually using assert()
would pull in complex dependencies we don't want).

/me looks through the error list

Yeah, nothing there really suits, so we'll need to add a new one for
this.

> > > Signed-off-by: Tom Rini <trini-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
> > > ---
> > >  fdtget.c        |  2 +-
> > >  libfdt/fdt_ro.c | 20 ++++++++++----------
> > >  libfdt/libfdt.h |  2 +-
> > >  3 files changed, 12 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/fdtget.c b/fdtget.c
> > > index 777582e2d45f..7cee28718cbc 100644
> > > --- a/fdtget.c
> > > +++ b/fdtget.c
> > > @@ -62,7 +62,7 @@ static int show_cell_list(struct display_info *disp, const char *data, int len,
> > >  	for (i = 0; i < len; i += size, p += size) {
> > >  		if (i)
> > >  			printf(" ");
> > > -		value = size == 4 ? fdt32_ld((const fdt32_t *)p) :
> > > +		value = size == 4 ? fdt32_to_cpu(*(const fdt32_t *)p) :
> > 
> > Second, getting rid of these ugly open-coded constructions was an
> > additional reason I went to using fdt32_ld() everywhere.  So I'd like
> > to see both an assume-aligned and an unaligned-safe helper, rather
> > than open-coding the load-and-byteswap everywhere.
> 
> OK, I think I've got an idea.
> 



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