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;
Date:   Wed, 3 Feb 2021 11:12:40 +0000
From:   Mark Rutland <mark.rutland@arm.com>
To:     Julien Thierry <jthierry@redhat.com>
Cc:     linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        catalin.marinas@arm.com, will@kernel.org, broonie@kernel.org
Subject: Re: [RFC PATCH 1/5] arm64: Move instruction encoder/decoder under
 lib/
Message-ID: <20210203111240.GB55896@C02TD0UTHF1T.local>
References: <20210120171745.1657762-1-jthierry@redhat.com>
 <20210120171745.1657762-2-jthierry@redhat.com>
 <20210202101755.GB59049@C02TD0UTHF1T.local>
 <9c01f293-2c98-43e6-a497-89610fe5970e@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <9c01f293-2c98-43e6-a497-89610fe5970e@redhat.com>
Precedence: bulk

On Wed, Feb 03, 2021 at 09:26:45AM +0100, Julien Thierry wrote:
> On 2/2/21 11:17 AM, Mark Rutland wrote:
> > On Wed, Jan 20, 2021 at 06:17:41PM +0100, Julien Thierry wrote:
> > > Aarch64 instruction set encoding and decoding logic can prove useful
> > > for some features/tools both part of the kernel and outside the kernel.
> > > 
> > > Isolate the function dealing only with encoding/decoding instructions,
> > > with minimal dependency on kernel utilities in order to be able to reuse
> > > that code.
> > > 
> > > Code was only moved, no code should have been added, removed nor
> > > modifier.
> > > 
> > > Signed-off-by: Julien Thierry <jthierry@redhat.com>
> > 
> > This looks sound, but the diff is a little hard to check.
> 
> Yes, I was expecting this change to be hard to digest.
> 
> > Would it be possible to split this into two patches, where:
> > 
> > 1) Refactoring definitions into insn.h and insn.c, leaving those files
> >     in their current locations.
> 
> I'm not quite sure I understand the suggestions. After this patch insn.h and
> insn.c still contain some definitions that can't really be used outside of
> kernel code which is why I split them into insn.* and aarch64-insn.* (the
> latter containing what could be used by tools).

Sorry; I hadn't appreciated what was getting left behind. With the
series applied I see that's some kernel patching logic, and AArch32 insn
bits.

How about we invert the move, and split those bits out of insn.c first,
then move the rest in one go, i.e.

1) Factor the patching bits out into new patching.{c,h} files.

2) Move insn.c to arch/arm64/lib/

3) Split insn.* into aarch64-insn.* and aarch32-insn.*

... if that makes any sense?

We might not even need to split the aarch32 bits out given they all have
an aarch32_* prefix.

> > 2) Moving insn.h and insn.c out to arch/arm64/lib/, updating includes
> > > With that, the second patch might be easier for git to identify as a
> > rename (if using `git format-patch -M -C`), and it'd be easier to see
> > that there are no unintended changes.
> 
> But it is more a split than a rename (at least in this patch). But I'm happy
> to do this in another way.

I think the above suggestion might solve that, unless I've missed
something?

Thanks,
Mark.