linux-kernel - Re: [RFC PATCH 1/5] arm64: Move instruction encoder/decoder under lib/

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <75b53229-f518-09f1-2449-ecac2352663c@redhat.com>
Date:   Wed, 3 Feb 2021 18:30:56 +0100
From:   Julien Thierry <jthierry@...hat.com>
To:     Mark Rutland <mark.rutland@....com>
Cc:     linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        catalin.marinas@....com, will@...nel.org, broonie@...nel.org
Subject: Re: [RFC PATCH 1/5] arm64: Move instruction encoder/decoder under
 lib/



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@...hat.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,

-- 
Julien Thierry