Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200722152422.72532-1-kernel@esmil.dk> <20200722152422.72532-2-kernel@esmil.dk>
In-Reply-To: <20200722152422.72532-2-kernel@esmil.dk>
From:   Luke Nelson <lukenels@cs.washington.edu>
Date:   Thu, 30 Jul 2020 11:53:06 -0700
Message-ID: <CADasFoDfRwcfFHM_Sa-HzwSDyXEr2PNu3Wfe5riwZJM9XsELBQ@mail.gmail.com>
Subject: Re: [PATCH v1 2/2] riscv: Clean up module relocations
To:     Emil Renner Berthing <kernel@esmil.dk>
Cc:     linux-riscv@lists.infradead.org,
        Palmer Dabbelt <palmer@dabbelt.com>,
        Paul Walmsley <paul.walmsley@sifive.com>,
        =?UTF-8?B?QmrDtnJuIFTDtnBlbA==?= <bjorn.topel@gmail.com>,
        Zong Li <zong@andestech.com>, Andreas Schwab <schwab@suse.de>,
        LKML <linux-kernel@vger.kernel.org>, Xi Wang <xi.wang@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Thanks for the patch!

> Also RISC-V has a number of instruction pairs to
> generate 32bit immediates or jump/call offsets. Eg.:
>
> lui   rd, hi20
> addi  rd, rd, lo12

On RV64, both hi20 from lui and lo12 from addi are sign-extended to 64 bits=
.
This means that there are some 32-bit signed offsets (in the range
[2^31-2^11, 2^31-1])
that are not encodable using (lui+addi), (auipc+jalr), etc. (see
discussion at [1]).

The following note is from the ISA manual:
>>> Note that the set of address offsets that can be formed by pairing LUI =
with LD,
>>> AUIPC with JALR, etc. in RV64I is [=E2=88=922^31=E2=88=922^11, 2^31=E2=
=88=922^11=E2=88=921].

The existing code and the new code both seem buggy if the offset happens to
be a 32-bit int but falls outside of the encodable range.

> +       if (offset !=3D (s32)offset) {
> [...]
> +       if (offset !=3D (s32)offset) {
> [...]

These checks should probably be replaced with something similar to
what's used in the RV64 BPF JIT here: [2],
except that this code should check if using RV32 or RV64, since the
encodable range differs for each.

> My hope is that we can eventually factor out the code to generate
> immediates and instructions so it can be reused both here, in the
> jump-label code and in the bpf-jit code, but let's take it
> one step at a time.

This sounds great! Having fewer copies of RISC-V encoding logic around will
hopefully decrease the likelihood of bugs :) Some other archs already
have shared
infrastructure for doing instruction encoding (e.g., in
arch/arm64/kernel/insn.c);
we should consider doing something similar for RISC-V.

- Luke Nelson

[1]: https://groups.google.com/a/groups.riscv.org/forum/#!topic/isa-dev/bwW=
FhBnnZFQ
[2]: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf.git/commit/?id=
=3D489553dd13a88d8a882db10622ba8b9b58582ce4