| Message ID | 20210120171745.1657762-1-jthierry@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | arm64: Prepare instruction decoder for objtool | expand |
Hi Mark, On 2/2/21 11:17 AM, Mark Rutland wrote: > Hi Julien, > > 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). Or do you suggest that I still create the aarch64-insn.* files next to the insn.* files? > 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. Thanks,
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.
On 2/3/21 12:12 PM, Mark Rutland wrote: > 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. > Yes, that approach sounds good. I'll if the aarchxx-insn is necessary but as you say, all in the same file shouldn't cause trouble. Thanks,