| Message ID | 20220812131304.1674484-1-philipp.tomsich@vrull.eu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] target/riscv: fence.i: update decode pattern | expand |
Please use a cover-letter for multi-patch patch series. On Fri, Aug 12, 2022 at 03:13:03PM +0200, Philipp Tomsich wrote: > The RISC-V specification specifies imm12, rs1 and rd to be all-zeros, > so we can't ignore these bits when decoding into fence.i. > > Update the decode pattern to reflect the specification. I got hung-up on this for a bit since there isn't any "must-be-0" fields, only ignored fields, but the next patch gives a clue which helped me make sense of this. The encoding of these instructions with ignored fields set to anything except zero gets into reserved instruction territory, and QEMU may legally raise an illegal-instruction in that case, which this patch will start doing. It'd be nice to have a bit more text in this commit message to make that clear. > > Signed-off-by: Philipp Tomsich <philipp.tomsich@vrull.eu> > --- > > target/riscv/insn32.decode | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/target/riscv/insn32.decode b/target/riscv/insn32.decode > index 014127d066..089128c3dc 100644 > --- a/target/riscv/insn32.decode > +++ b/target/riscv/insn32.decode > @@ -151,7 +151,7 @@ sra 0100000 ..... ..... 101 ..... 0110011 @r > or 0000000 ..... ..... 110 ..... 0110011 @r > and 0000000 ..... ..... 111 ..... 0110011 @r > fence ---- pred:4 succ:4 ----- 000 ----- 0001111 > -fence_i ---- ---- ---- ----- 001 ----- 0001111 > +fence_i 000000000000 00000 001 00000 0001111 ^ need two more spaces here to line up with fence. > csrrw ............ ..... 001 ..... 1110011 @csr > csrrs ............ ..... 010 ..... 1110011 @csr > csrrc ............ ..... 011 ..... 1110011 @csr > -- > 2.34.1 > > Thanks, drew
On Fri, 12 Aug 2022 at 16:01, Andrew Jones <ajones@ventanamicro.com> wrote: > > > Update the decode pattern to reflect the specification. > > I got hung-up on this for a bit since there isn't any "must-be-0" fields, Please refer to '“Zifencei” Instruction-Fetch Fence, Version 2.0' in the specification. The encoding diagram clearly states 0 for imm[11:0], 0 for rs1 and 0 for rd. However, there is an explanatory paragraph below (unfortunately, it is not clear whether this is normative or informative): > The unused fields in the FENCE.I instruction, imm[11:0], rs1, and rd, are reserved for finer-grain fences in future extensions. For forward compatibility, base implementations shall ignore these fields, and standard software shall zero these fields. Strictly speaking, this patch may be too restrictive (it violates the "for forward-compatibility" part — which I consider informative only, though). Thanks, Philipp.
On Fri, 12 Aug 2022 at 15:11, Philipp Tomsich <philipp.tomsich@vrull.eu> wrote: > > On Fri, 12 Aug 2022 at 16:01, Andrew Jones <ajones@ventanamicro.com> wrote: > > > > > Update the decode pattern to reflect the specification. > > > > I got hung-up on this for a bit since there isn't any "must-be-0" fields, > > Please refer to '“Zifencei” Instruction-Fetch Fence, Version 2.0' in > the specification. > The encoding diagram clearly states 0 for imm[11:0], 0 for rs1 and 0 for rd. > > However, there is an explanatory paragraph below (unfortunately, it is > not clear whether this is normative or informative): > > The unused fields in the FENCE.I instruction, imm[11:0], rs1, and rd, are reserved for finer-grain fences in future extensions. For forward compatibility, base implementations shall ignore these fields, and standard software shall zero these fields. That's pretty clear that this patch is wrong, then -- QEMU is an implementation, and so we must ignore these fields. Otherwise when a future version of the spec defines a finer-grain fence instruction in this part of the encoding space, older QEMU will incorrectly make software that uses it crash. If you think the spec is insufficiently clear about whether that is normative then that would be something to raise with the spec authors, preferably before anybody builds hardware that enforces must-be-zeroes on these fields... thanks -- PMM
diff --git a/target/riscv/insn32.decode b/target/riscv/insn32.decode index 014127d066..089128c3dc 100644 --- a/target/riscv/insn32.decode +++ b/target/riscv/insn32.decode @@ -151,7 +151,7 @@ sra 0100000 ..... ..... 101 ..... 0110011 @r or 0000000 ..... ..... 110 ..... 0110011 @r and 0000000 ..... ..... 111 ..... 0110011 @r fence ---- pred:4 succ:4 ----- 000 ----- 0001111 -fence_i ---- ---- ---- ----- 001 ----- 0001111 +fence_i 000000000000 00000 001 00000 0001111 csrrw ............ ..... 001 ..... 1110011 @csr csrrs ............ ..... 010 ..... 1110011 @csr csrrc ............ ..... 011 ..... 1110011 @csr
The RISC-V specification specifies imm12, rs1 and rd to be all-zeros, so we can't ignore these bits when decoding into fence.i. Update the decode pattern to reflect the specification. Signed-off-by: Philipp Tomsich <philipp.tomsich@vrull.eu> --- target/riscv/insn32.decode | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)