diff mbox series

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

Message ID 20210903170100.2529121-4-philipp.tomsich@vrull.eu
State New
Headers show
Series target/riscv: Update QEmu for Zb[abcs] 1.0.0 | expand

Commit Message

Philipp Tomsich Sept. 3, 2021, 5 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 <philipp.tomsich@vrull.eu>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
---

Changes in v9:
- Rebased to 8880cc4362.

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 | 10 ++++++++++
 1 file changed, 10 insertions(+)

Comments

Richard Henderson Sept. 3, 2021, 6:45 p.m. UTC | #1 
On 9/3/21 7:00 PM, Philipp Tomsich wrote:
> @@ -652,5 +652,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;
> +    }
> +
>       return gen_shift_imm_fn(ctx, a, EXT_NONE, gen_slli_uw);

As previously stated, drop this patch.
It is done correctly inside gen_shift_imm_fn.


r~
Philipp Tomsich Sept. 3, 2021, 7:07 p.m. UTC | #2 
On Fri, 3 Sept 2021 at 21:45, Richard Henderson <
richard.henderson@linaro.org> wrote:

> On 9/3/21 7:00 PM, Philipp Tomsich wrote:
> > @@ -652,5 +652,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;
> > +    }
> > +
> >       return gen_shift_imm_fn(ctx, a, EXT_NONE, gen_slli_uw);
>
> As previously stated, drop this patch.
> It is done correctly inside gen_shift_imm_fn.


Good catch: I just looked through the changed translate.c and this does
address the slli.uw case.  I won't be able to retest this tonight, though.
I'll send the updated series as soon as possible, though.
diff mbox series

Patch

diff --git a/target/riscv/insn_trans/trans_rvb.c.inc b/target/riscv/insn_trans/trans_rvb.c.inc
index 91da7c5853..77114889de 100644
--- a/target/riscv/insn_trans/trans_rvb.c.inc
+++ b/target/riscv/insn_trans/trans_rvb.c.inc
@@ -652,5 +652,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;
+    }
+
     return gen_shift_imm_fn(ctx, a, EXT_NONE, gen_slli_uw);
 }