On Fri, Sep 15, 2023 at 01:07:40PM +0200, Andrew Jones wrote: > On Fri, Sep 15, 2023 at 05:22:26AM -0300, Leonardo Bras wrote: > > On Thu, Sep 14, 2023 at 03:47:59PM +0200, Andrew Jones wrote: > > > On Sun, Sep 10, 2023 at 04:28:57AM -0400, guoren@xxxxxxxxxx wrote: > > > > From: Guo Ren <guoren@xxxxxxxxxxxxxxxxx> > ... > > > > diff --git a/arch/riscv/include/asm/insn-def.h b/arch/riscv/include/asm/insn-def.h > > > > index 6960beb75f32..dc590d331894 100644 > > > > --- a/arch/riscv/include/asm/insn-def.h > > > > +++ b/arch/riscv/include/asm/insn-def.h > > > > @@ -134,6 +134,7 @@ > > > > > > > > #define RV_OPCODE_MISC_MEM RV_OPCODE(15) > > > > #define RV_OPCODE_SYSTEM RV_OPCODE(115) > > > > +#define RV_OPCODE_PREFETCH RV_OPCODE(19) > > > > > > This should be named RV_OPCODE_OP_IMM and be placed in > > > numerical order with the others, i.e. above SYSTEM. > > > > > > > > > > > #define HFENCE_VVMA(vaddr, asid) \ > > > > INSN_R(OPCODE_SYSTEM, FUNC3(0), FUNC7(17), \ > > > > @@ -196,4 +197,8 @@ > > > > INSN_I(OPCODE_MISC_MEM, FUNC3(2), __RD(0), \ > > > > RS1(base), SIMM12(4)) > > > > > > > > +#define CBO_prefetchw(base) \ > > > > > > Please name this 'PREFETCH_w' and it should take an immediate parameter, > > > even if we intend to pass 0 for it. > > > > It makes sense. > > > > The mnemonic in the previously mentioned documentation is: > > > > prefetch.w offset(base) > > > > So yeah, makes sense to have both offset and base as parameters for > > CBO_prefetchw (or PREFETCH_w, I have no strong preference). > > I have a strong preference :-) > > PREFETCH_w is consistent with the naming we already have for e.g. > cbo.clean, which is CBO_clean. The instruction we're picking a name > for now is prefetch.w, not cbo.prefetchw. > > > > > > > > > > + INSN_R(OPCODE_PREFETCH, FUNC3(6), FUNC7(0), \ > > > > + RD(x0), RS1(base), RS2(x0)) > > > > > > prefetch.w is not an R-type instruction, it's an S-type. While the bit > > > shifts are the same, the names are different. We need to add S-type > > > names while defining this instruction. > > > > That is correct, it is supposed to look like a store instruction (S-type), > > even though documentation don't explicitly state that. > > > > Even though it works fine with the R-type definition, code documentation > > would be wrong, and future changes could break it. > > > > > Then, this define would be > > > > > > #define PREFETCH_w(base, imm) \ > > I should have suggested 'offset' instead of 'imm' for the second parameter > name. > > > > INSN_S(OPCODE_OP_IMM, FUNC3(6), IMM_11_5(imm), __IMM_4_0(0), \ > > > RS1(base), __RS2(3)) > > > > s/OPCODE_OP_IMM/OPCODE_PREFETCH > > 0x4 vs 0x13 > > There's no major opcode named "PREFETCH" and the spec says that the major > opcode used for prefetch instructions is OP-IMM. That's why we want to > name this OPCODE_OP_IMM. I'm not sure where the 0x4 you're referring to > comes from Oh, you are right. Sorry about this, I misinterpreted table 24.1 from the Unprivileged ISA (20191213). Yeap, everything make sense now, and the define below is not actually needed: > > > > +#define RV_OPCODE_PREFETCH RV_OPCODE(19) Thanks! Leo > . A 32-bit instruction has the lowest two bits set (figure 1.1 > of the unpriv spec) and table 27.1 of the unpriv spec shows OP-IMM is > 0b00100xx, so we have 0b0010011. Keeping the naming of the opcode macros > consistent with the spec also keeps them consistent with the .insn > directive where we could even use the names directly, i.e. > > .insn s OP_IMM, 6, x3, 0(a0) > > > > > > > When the assembler as insn_r I hope it will validate that > > I meant insn_s here, which would be the macro for '.insn s' > > > > (imm & 0xfe0) == imm > > I played with it. It won't do what we want for prefetch, only > what works for s-type instructions in general, i.e. it allows > +/-2047 offsets and fails for everything else. That's good enough. > We can just mask off the low 5 bits here in our macro > > #define PREFETCH_w(base, offset) \ > INSN_S(OPCODE_OP_IMM, FUNC3(6), IMM_11_5((offset) & ~0x1f), \ > __IMM_4_0(0), RS1(base), __RS2(3)) > > Thanks, > drew >
