Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220131182720.236065-1-kernel@esmil.dk> <mhng-8e6926b7-c690-4398-a70a-072b11d7e6fa@palmer-ri-x1c9>
In-Reply-To: <mhng-8e6926b7-c690-4398-a70a-072b11d7e6fa@palmer-ri-x1c9>
From:   Emil Renner Berthing <kernel@esmil.dk>
Date:   Wed, 23 Feb 2022 16:53:39 +0100
Message-ID: <CANBLGcxeO0uYutsAHP-TKRCjRqLqbUy-rvK1vcX02eDe0445Hg@mail.gmail.com>
Subject: Re: [PATCH v2 0/7] Module relocation fixes and asm/insn.h header
To:     Palmer Dabbelt <palmer@dabbelt.com>
Cc:     linux-riscv <linux-riscv@lists.infradead.org>,
        Paul Walmsley <paul.walmsley@sifive.com>,
        Albert Ou <aou@eecs.berkeley.edu>,
        Peter Zijlstra <peterz@infradead.org>,
        Josh Poimboeuf <jpoimboe@redhat.com>,
        Jason Baron <jbaron@akamai.com>,
        Steven Rostedt <rostedt@goodmis.org>,
        Ard Biesheuvel <ardb@kernel.org>,
        Alexandre Ghiti <alex@ghiti.fr>,
        Jisheng Zhang <jszhang@kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, 23 Feb 2022 at 00:15, Palmer Dabbelt <palmer@dabbelt.com> wrote:
> On Mon, 31 Jan 2022 10:27:13 PST (-0800), kernel@esmil.dk wrote:
> > Apologies! I messed up v1. Please consider this patch set only.
> >
> > The first patch removes a bunch of code from the asm/module.h which is
> > included in almost all drivers through linux/module.h. Next are two
> > patches to fix unaligned access when doing module relocations and do
> > proper range checks for auipc+jalr offsets.
> >
> > I'm a little less confident about the following patches, so consider
> > this more of an RFC for those. The idea is to consolidate the RISC-V
> > instruction generation and manipulation similar to arm64's asm/insn.h
> > header.
> >
> > /Emil
> >
> > Emil Renner Berthing (7):
> >   riscv: Remove unneeded definitions from asm/module.h
> >   riscv: Avoid unaligned access when relocating modules
> >   riscv: Fix auipc+jalr relocation range checks
> >   riscv: Add asm/insn.h header
> >   riscv: Use asm/insn.h for module relocations
> >   riscv: Use asm/insn.h to generate plt entries
> >   riscv: Use asm/insn.h for jump labels
> >
> >  arch/riscv/include/asm/insn.h       | 121 ++++++++++++++
> >  arch/riscv/include/asm/module.h     |  87 ----------
> >  arch/riscv/kernel/jump_label.c      |  12 +-
> >  arch/riscv/kernel/module-sections.c |  71 +++++++++
> >  arch/riscv/kernel/module.c          | 237 +++++++++++++---------------
> >  5 files changed, 306 insertions(+), 222 deletions(-)
> >  create mode 100644 arch/riscv/include/asm/insn.h
>
> These generally look good to me, though there's a lot of bit-field
> twiddling so I'll take another look before merging it.  There's a
> handful of minor issues:
>
> * There's a fix in here, mixed into the cleanups.  It's generally best
>   to split those out.

There are two fixes. The 32bit range check on rv64 and unaligned 32bit
access. The code has been like that for years so I was unsure if they
were worth splitting out and adding early. Since you only mention one
I guess that's the range check. I'll send that separately.

> * There's another copy of the insn patterns in our BPF JIT, it'd be nice
>   to clean that up too.  That can be a follow-on, though.
> * It's 2022, but there's some 2020 copyrights.  If this really is old
>   stuff that's OK, I just wanted to check.

Nice catch, but the year is actually correct. These patches have been
well aged in my local repo. The reason is exactly that I never got
around to doing the BPF conversion, so now I decided to just send them
and see if it was worth finishing.

> I'm usually OK just re-ordering patches myself, but I figured I'd have
> to ask about the copyright dates anyway.  LMK if you want to send a v2
> with the fix pulled to the front, and what you want me to do about the
> copyright dates (if you're going to send a v2 then just fix them, but if
> you're not then just telling me is OK).

Thank you. I'll send the range check separately and a v2 converting
the "if (IS_ENABLED(CONFIG_32BIT))" to an #ifdef to avoid the warning
the kernel test robot found.