linux-kernel - Re: [PATCH 01/10] RISC-V: Expand instruction definitions

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20230804-barterer-heritage-ed191081bc47@wendy>
Date:   Fri, 4 Aug 2023 08:59:24 +0100
From:   Conor Dooley <conor.dooley@...rochip.com>
To:     Charlie Jenkins <charlie@...osinc.com>
CC:     <linux-riscv@...ts.infradead.org>, <linux-kernel@...r.kernel.org>,
        <kvm@...r.kernel.org>, <kvm-riscv@...ts.infradead.org>,
        <bpf@...r.kernel.org>, Paul Walmsley <paul.walmsley@...ive.com>,
        Palmer Dabbelt <palmer@...belt.com>,
        Albert Ou <aou@...s.berkeley.edu>,
        Peter Zijlstra <peterz@...radead.org>,
        Josh Poimboeuf <jpoimboe@...nel.org>,
        Jason Baron <jbaron@...mai.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Ard Biesheuvel <ardb@...nel.org>,
        Anup Patel <anup@...infault.org>,
        Atish Patra <atishp@...shpatra.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Martin KaFai Lau <martin.lau@...ux.dev>,
        Song Liu <song@...nel.org>, Yonghong Song <yhs@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...nel.org>,
        Stanislav Fomichev <sdf@...gle.com>,
        Hao Luo <haoluo@...gle.com>, Jiri Olsa <jolsa@...nel.org>,
        Björn Töpel <bjorn@...nel.org>,
        Luke Nelson <luke.r.nels@...il.com>,
        Xi Wang <xi.wang@...il.com>, Nam Cao <namcaov@...il.com>
Subject: Re: [PATCH 01/10] RISC-V: Expand instruction definitions

On Thu, Aug 03, 2023 at 07:10:26PM -0700, Charlie Jenkins wrote:
> There are many systems across the kernel that rely on directly creating
> and modifying instructions. In order to unify them, create shared
> definitions for instructions and registers.
> 
> Signed-off-by: Charlie Jenkins <charlie@...osinc.com>
> ---
>  arch/riscv/include/asm/insn.h            | 2742 +++++++++++++++++++++++++++---

"I did a lot of copy-pasting from the RISC-V spec"

How is anyone supposed to cross check this when there's 1000s of lines
of a diff here? We've had some subtle bugs in some of the definitions in
the past, so I would like to be able to check at this opportune moment
that things are correct.

>  arch/riscv/include/asm/reg.h             |   88 +
>  arch/riscv/kernel/kgdb.c                 |    4 +-
>  arch/riscv/kernel/probes/simulate-insn.c |   39 +-
>  arch/riscv/kernel/vector.c               |    2 +-

You need to at least split this up. I doubt a 2742 change diff for
insn.h was required to make the changes in these 4 files.

Then after that, it would be so much easier to reason about these
changes if the additions to insn.h happened at the same time as the
removals from the affected locations.

I would probably split this so that things are done in more stages,
with the larger patches split between changes that require no new
definitions and changes that require moving things to insn.h

>  5 files changed, 2629 insertions(+), 246 deletions(-)

What you would want to see if this arrived in your inbox as a reviewer?

Don't get me wrong, I do like what you are doing here, the BPF JIT
especially is filled with "uhh okay, I guess those offsets are right",
so I don't mean to be discouraging.

Thanks,
Conor.

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)