Messages in this thread | | | From | Emil Renner Berthing <> | Date | Wed, 5 Aug 2020 19:28:29 +0200 | Subject | Re: [PATCH v1 2/2] riscv: Clean up module relocations |
| |
On Thu, 30 Jul 2020 at 20:53, Luke Nelson <lukenels@cs.washington.edu> wrote: > > 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 [−2^31−2^11, 2^31−2^11−1].
Good point!
> 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 != (s32)offset) { > > [...] > > + if (offset != (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.
Yeah, I decided to leave these checks as they are to not change everything at once, but I'll see if I can improve on those in the next version.
> > 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.
Exactly this. I've even tried to copy the naming from arm64 as well. I'll send a 2nd version of this after -rc1.
> > - Luke Nelson > > [1]: https://groups.google.com/a/groups.riscv.org/forum/#!topic/isa-dev/bwWFhBnnZFQ > [2]: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf.git/commit/?id=489553dd13a88d8a882db10622ba8b9b58582ce4
| |