Message ID | 20210223065935.20208-1-frank.chang@sifive.com |
---|---|
State | New |
Headers | show |
Series | target/riscv: fix vs() to return proper error code | expand |
On 2/22/21 10:59 PM, frank.chang@sifive.com wrote: > From: Frank Chang <frank.chang@sifive.com> > > vs() should return -RISCV_EXCP_ILLEGAL_INST instead of -1 if rvv feature > is not enabled. > > If -1 is returned, exception will be raised and cs->exception_index will > be set to the negative return value. The exception will then be treated > as an instruction access fault instead of illegal instruction fault. It does seem an unfortunate interface; -1 seems so tempting, but does not by itself mean anything. I wonder if we should dispense with the whole "negative number" thing and simply return an exception value. Then for bonus points put all of the RISCV_EXCP_* values in an enumeration, and return that type from these functions so that it's perfectly clear what the interface really is. That said, > @@ -54,7 +54,7 @@ static int vs(CPURISCVState *env, int csrno) > if (env->misa & RVV) { > return 0; > } > - return -1; > + return -RISCV_EXCP_ILLEGAL_INST; this fixes the immediate bug, so Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
On Tue, Feb 23, 2021 at 1:46 PM Richard Henderson <richard.henderson@linaro.org> wrote: > > On 2/22/21 10:59 PM, frank.chang@sifive.com wrote: > > From: Frank Chang <frank.chang@sifive.com> > > > > vs() should return -RISCV_EXCP_ILLEGAL_INST instead of -1 if rvv feature > > is not enabled. > > > > If -1 is returned, exception will be raised and cs->exception_index will > > be set to the negative return value. The exception will then be treated > > as an instruction access fault instead of illegal instruction fault. > > It does seem an unfortunate interface; -1 seems so tempting, but does not by > itself mean anything. > > I wonder if we should dispense with the whole "negative number" thing and > simply return an exception value. Then for bonus points put all of the > RISCV_EXCP_* values in an enumeration, and return that type from these > functions so that it's perfectly clear what the interface really is. Good idea! Reviewed-by: Alistair Francis <alistair.francis@wdc.com> Alistair > > That said, > > > @@ -54,7 +54,7 @@ static int vs(CPURISCVState *env, int csrno) > > if (env->misa & RVV) { > > return 0; > > } > > - return -1; > > + return -RISCV_EXCP_ILLEGAL_INST; > > this fixes the immediate bug, so > Reviewed-by: Richard Henderson <richard.henderson@linaro.org> > > > r~ >
diff --git a/target/riscv/csr.c b/target/riscv/csr.c index fd2e6363f39..d2ae73e4a08 100644 --- a/target/riscv/csr.c +++ b/target/riscv/csr.c @@ -54,7 +54,7 @@ static int vs(CPURISCVState *env, int csrno) if (env->misa & RVV) { return 0; } - return -1; + return -RISCV_EXCP_ILLEGAL_INST; } static int ctr(CPURISCVState *env, int csrno)