Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210303193305.924384-1-paul@crapouillou.net> <CAL_JsqLfkjC4c4PYfm6yJLZMH-5WaKA_mr9ziJ1J63UohcgRCw@mail.gmail.com>
 <20210306084513.GA5453@alpha.franken.de> <CAL_JsqK0_M18gnoYFyTyf_OaQgbmbYYyoAr-WaFCzzsmFuFeFg@mail.gmail.com>
 <20210306225855.GA3574@alpha.franken.de>
In-Reply-To: <20210306225855.GA3574@alpha.franken.de>
From:   Rob Herring <robh@kernel.org>
Date:   Mon, 8 Mar 2021 10:04:15 -0700
Message-ID: <CAL_Jsq+m0uwgo_-phR_zAz6ZfiSAr=JMXMaFsW-tPv_kXV+3fA@mail.gmail.com>
Subject: Re: [PATCH] MIPS: boot/compressed: Copy DTB to aligned address
To:     Thomas Bogendoerfer <tsbogend@alpha.franken.de>
Cc:     Paul Cercueil <paul@crapouillou.net>, od@zcrc.me,
        "open list:MIPS" <linux-mips@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Sat, Mar 6, 2021 at 3:59 PM Thomas Bogendoerfer
<tsbogend@alpha.franken.de> wrote:
>
> On Sat, Mar 06, 2021 at 02:35:21PM -0700, Rob Herring wrote:
> > On Sat, Mar 6, 2021 at 1:45 AM Thomas Bogendoerfer
> > <tsbogend@alpha.franken.de> wrote:
> > >
> > > On Wed, Mar 03, 2021 at 02:37:55PM -0600, Rob Herring wrote:
> > > > On Wed, Mar 3, 2021 at 1:33 PM Paul Cercueil <paul@crapouillou.net> wrote:
> > > > >
> > > > > Since 5.12-rc1, the Device Tree blob must now be properly aligned.
> > > >
> > > > I had checked the other built-in cases as microblaze broke too, but
> > > > missed some of the many ways MIPS can have a dtb. Appended and
> > > > built-in DTBs were supposed to be temporary. :(
> > >
> > > and a fdt can also be provided by firmware. And according to spec
> > > there is no aligmnet requirement. So this whole change will break
> > > then. What was the reason for the whole churn ?

Actually, that is wrong. The spec defines the alignment (from
flattened format appendix):

"Alignment

For the data in the memory reservation and structure blocks to be used
without unaligned memory accesses, they shall lie at suitably aligned
memory addresses. Specifically, the memory reservation block shall be
aligned to an 8-byte boundary and the structure block to a 4-byte
boundary.

Furthermore, the devicetree blob as a whole can be relocated without
destroying the alignment of the subblocks.

As described in the previous sections, the structure and strings
blocks shall have aligned offsets from the beginning of the devicetree
blob. To ensure the in-memory alignment of the blocks, it is
sufficient to ensure that the devicetree as a whole is loaded at an
address aligned to the largest alignment of any of the subblocks, that
is, to an 8-byte boundary. A |spec| compliant boot program shall load
the devicetree blob at such an aligned address before passing it to
the client program. If an |spec| client program relocates the
devicetree blob in memory, it should only do so to another 8-byte
aligned address."


> > There was a long discussion on devicetree-compiler list a few months
> > ago. In summary, a while back libfdt switched to accessors from raw
> > pointer accesses to avoid any possible unaligned accesses (is MIPS
> > always okay with unaligned accesses?).
>
> no, it will trap unaligned accesses, that's the reason for Paul's problem.
>
> > This was determined to be a
> > performance regression and an overkill as the DT structure itself
> > should always be naturally aligned if the dtb is 64-bit aligned. I
> > think 32-bit aligned has some possible misaligned accesses.
>
> the access macros are using *(unsigned long long *), which isn't
> even nice for 32bit CPUs...

Where are those?

> > As part of this, a dtb alignment check was added. So worst case, we
> > could disable that if need be.
>
> yeah, or override fdt32/64_to_cpu, if I understood the code correctly.

No, fdt32/64_to_cpu don't dereference the pointer.

Rob