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

From: Charlie Jenkins
Date: Fri Aug 04 2023 - 13:26:40 EST


On Fri, Aug 04, 2023 at 08:59:24AM +0100, Conor Dooley wrote:
> 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@xxxxxxxxxxxx>
> > ---
> > 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.
Yeah it is kind of a nightmare to look at, I will split it up.
>
> 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.