[v8,03/14] target/riscv: slli.uw is only a valid encoding if shamt first in 64 bits

Commit Message

Philipp Tomsich Sept. 3, 2021, 1:14 p.m. UTC

For RV64, the shamt field in slli.uw is 6 bits wide. While the encoding
space currently reserves a wider shamt-field (for use is a future RV128
ISA), setting the additional bit to 1 will not map to slli.uw for RV64
and needs to be treated as an illegal instruction.

Note that this encoding being reserved for a future RV128 does not imply
that no other instructions for RV64-only could be added in this encoding
space in the future.

As the implementation is separate from the gen_shifti helpers, we keep
it that way and add the check for the shamt-width here.

Signed-off-by: Philipp Tomsich <[e-mail]>
Reviewed-by: Richard Henderson <[e-mail]>
Reviewed-by: Alistair Francis <[e-mail]>
---

(no changes since v3)

Changes in v3:
- Instead of defining a new decoding format, we treat slli.uw as if it
  had a 7bit-wide field for shamt (the 7th bit is reserved for RV128)
  and check for validity of the encoding in C code.

 target/riscv/insn_trans/trans_rvb.c.inc | 9 +++++++++
 1 file changed, 9 insertions(+)

Comments

Richard Henderson Sept. 3, 2021, 1:45 p.m. UTC | #1

On 9/3/21 3:14 PM, Philipp Tomsich wrote:
> For RV64, the shamt field in slli.uw is 6 bits wide. While the encoding
> space currently reserves a wider shamt-field (for use is a future RV128
> ISA), setting the additional bit to 1 will not map to slli.uw for RV64
> and needs to be treated as an illegal instruction.
> 
> Note that this encoding being reserved for a future RV128 does not imply
> that no other instructions for RV64-only could be added in this encoding
> space in the future.
> 
> As the implementation is separate from the gen_shifti helpers, we keep
> it that way and add the check for the shamt-width here.
> 
> Signed-off-by: Philipp Tomsich <[e-mail]>
> Reviewed-by: Richard Henderson <[e-mail]>
> Reviewed-by: Alistair Francis <[e-mail]>
> ---
> 
> (no changes since v3)
> 
> Changes in v3:
> - Instead of defining a new decoding format, we treat slli.uw as if it
>    had a 7bit-wide field for shamt (the 7th bit is reserved for RV128)
>    and check for validity of the encoding in C code.
> 
>   target/riscv/insn_trans/trans_rvb.c.inc | 9 +++++++++
>   1 file changed, 9 insertions(+)
> 
> diff --git a/target/riscv/insn_trans/trans_rvb.c.inc b/target/riscv/insn_trans/trans_rvb.c.inc
> index 3cdd70a2b9..dcc7b6893d 100644
> --- a/target/riscv/insn_trans/trans_rvb.c.inc
> +++ b/target/riscv/insn_trans/trans_rvb.c.inc
> @@ -430,6 +430,15 @@ static bool trans_slli_uw(DisasContext *ctx, arg_slli_uw *a)
>       REQUIRE_64BIT(ctx);
>       REQUIRE_ZBA(ctx);
>   
> +    /*
> +     * The shamt field is only 6 bits for RV64 (with the 7th bit
> +     * remaining reserved for RV128).  If the reserved bit is set
> +     * on RV64, the encoding is illegal.
> +     */
> +    if (a->shamt >= TARGET_LONG_BITS) {
> +        return false;
> +    }

So, now you need to rebase on qemu master, where the code you are patching does not exist, 
and has been properly fixed by using gen_shift_imm_fn.


r~

Patch

diff --git a/target/riscv/insn_trans/trans_rvb.c.inc b/target/riscv/insn_trans/trans_rvb.c.inc
index 3cdd70a2b9..dcc7b6893d 100644
--- a/target/riscv/insn_trans/trans_rvb.c.inc
+++ b/target/riscv/insn_trans/trans_rvb.c.inc
@@ -430,6 +430,15 @@  static bool trans_slli_uw(DisasContext *ctx, arg_slli_uw *a)
     REQUIRE_64BIT(ctx);
     REQUIRE_ZBA(ctx);
 
+    /*
+     * The shamt field is only 6 bits for RV64 (with the 7th bit
+     * remaining reserved for RV128).  If the reserved bit is set
+     * on RV64, the encoding is illegal.
+     */
+    if (a->shamt >= TARGET_LONG_BITS) {
+        return false;
+    }
+
     TCGv source1 = tcg_temp_new();
     gen_get_gpr(source1, a->rs1);