| Message ID | 20230411090211.3039186-1-bmeng@tinylab.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/riscv: Restore the predicate() NULL check behavior | expand |
On 4/11/23 06:02, Bin Meng wrote: > When reading a non-existent CSR QEMU should raise illegal instruction > exception, but currently it just exits due to the g_assert() check. > > This actually reverts commit 0ee342256af9205e7388efdf193a6d8f1ba1a617, > Some comments are also added to indicate that predicate() must be > provided for an implemented CSR. > > Reported-by: Fei Wu <fei2.wu@intel.com> > Signed-off-by: Bin Meng <bmeng@tinylab.org> > --- Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> > > target/riscv/csr.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/target/riscv/csr.c b/target/riscv/csr.c > index d522efc0b6..736ab64275 100644 > --- a/target/riscv/csr.c > +++ b/target/riscv/csr.c > @@ -3797,6 +3797,11 @@ static inline RISCVException riscv_csrrw_check(CPURISCVState *env, > return RISCV_EXCP_ILLEGAL_INST; > } > > + /* ensure CSR is implemented by checking predicate */ > + if (!csr_ops[csrno].predicate) { > + return RISCV_EXCP_ILLEGAL_INST; > + } > + > /* privileged spec version check */ > if (env->priv_ver < csr_min_priv) { > return RISCV_EXCP_ILLEGAL_INST; > @@ -3814,7 +3819,6 @@ static inline RISCVException riscv_csrrw_check(CPURISCVState *env, > * illegal instruction exception should be triggered instead of virtual > * instruction exception. Hence this comes after the read / write check. > */ > - g_assert(csr_ops[csrno].predicate != NULL); > RISCVException ret = csr_ops[csrno].predicate(env, csrno); > if (ret != RISCV_EXCP_NONE) { > return ret; > @@ -3991,7 +3995,10 @@ RISCVException riscv_csrrw_debug(CPURISCVState *env, int csrno, > return ret; > } > > -/* Control and Status Register function table */ > +/* > + * Control and Status Register function table > + * riscv_csr_operations::predicate() must be provided for an implemented CSR > + */ > riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = { > /* User Floating-Point CSRs */ > [CSR_FFLAGS] = { "fflags", fs, read_fflags, write_fflags },
On 2023/4/11 17:02, Bin Meng wrote: > When reading a non-existent CSR QEMU should raise illegal instruction > exception, but currently it just exits due to the g_assert() check. > > This actually reverts commit 0ee342256af9205e7388efdf193a6d8f1ba1a617, > Some comments are also added to indicate that predicate() must be > provided for an implemented CSR. > > Reported-by: Fei Wu <fei2.wu@intel.com> > Signed-off-by: Bin Meng <bmeng@tinylab.org> > --- Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn> Weiwei Li > target/riscv/csr.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/target/riscv/csr.c b/target/riscv/csr.c > index d522efc0b6..736ab64275 100644 > --- a/target/riscv/csr.c > +++ b/target/riscv/csr.c > @@ -3797,6 +3797,11 @@ static inline RISCVException riscv_csrrw_check(CPURISCVState *env, > return RISCV_EXCP_ILLEGAL_INST; > } > > + /* ensure CSR is implemented by checking predicate */ > + if (!csr_ops[csrno].predicate) { > + return RISCV_EXCP_ILLEGAL_INST; > + } > + > /* privileged spec version check */ > if (env->priv_ver < csr_min_priv) { > return RISCV_EXCP_ILLEGAL_INST; > @@ -3814,7 +3819,6 @@ static inline RISCVException riscv_csrrw_check(CPURISCVState *env, > * illegal instruction exception should be triggered instead of virtual > * instruction exception. Hence this comes after the read / write check. > */ > - g_assert(csr_ops[csrno].predicate != NULL); > RISCVException ret = csr_ops[csrno].predicate(env, csrno); > if (ret != RISCV_EXCP_NONE) { > return ret; > @@ -3991,7 +3995,10 @@ RISCVException riscv_csrrw_debug(CPURISCVState *env, int csrno, > return ret; > } > > -/* Control and Status Register function table */ > +/* > + * Control and Status Register function table > + * riscv_csr_operations::predicate() must be provided for an implemented CSR > + */ > riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = { > /* User Floating-Point CSRs */ > [CSR_FFLAGS] = { "fflags", fs, read_fflags, write_fflags },
On 4/11/2023 5:02 PM, Bin Meng wrote: > When reading a non-existent CSR QEMU should raise illegal instruction > exception, but currently it just exits due to the g_assert() check. > I verified that 'csrr t3, 0x4' in user space didn't cause qemu exit but raised illegal instruction after applying this patch. Thanks, Fei. > This actually reverts commit 0ee342256af9205e7388efdf193a6d8f1ba1a617, > Some comments are also added to indicate that predicate() must be > provided for an implemented CSR. > > Reported-by: Fei Wu <fei2.wu@intel.com> > Signed-off-by: Bin Meng <bmeng@tinylab.org> > --- > > target/riscv/csr.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/target/riscv/csr.c b/target/riscv/csr.c > index d522efc0b6..736ab64275 100644 > --- a/target/riscv/csr.c > +++ b/target/riscv/csr.c > @@ -3797,6 +3797,11 @@ static inline RISCVException riscv_csrrw_check(CPURISCVState *env, > return RISCV_EXCP_ILLEGAL_INST; > } > > + /* ensure CSR is implemented by checking predicate */ > + if (!csr_ops[csrno].predicate) { > + return RISCV_EXCP_ILLEGAL_INST; > + } > + > /* privileged spec version check */ > if (env->priv_ver < csr_min_priv) { > return RISCV_EXCP_ILLEGAL_INST; > @@ -3814,7 +3819,6 @@ static inline RISCVException riscv_csrrw_check(CPURISCVState *env, > * illegal instruction exception should be triggered instead of virtual > * instruction exception. Hence this comes after the read / write check. > */ > - g_assert(csr_ops[csrno].predicate != NULL); > RISCVException ret = csr_ops[csrno].predicate(env, csrno); > if (ret != RISCV_EXCP_NONE) { > return ret; > @@ -3991,7 +3995,10 @@ RISCVException riscv_csrrw_debug(CPURISCVState *env, int csrno, > return ret; > } > > -/* Control and Status Register function table */ > +/* > + * Control and Status Register function table > + * riscv_csr_operations::predicate() must be provided for an implemented CSR > + */ > riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = { > /* User Floating-Point CSRs */ > [CSR_FFLAGS] = { "fflags", fs, read_fflags, write_fflags },
On Tue, Apr 11, 2023 at 7:03 PM Bin Meng <bmeng@tinylab.org> wrote: > > When reading a non-existent CSR QEMU should raise illegal instruction > exception, but currently it just exits due to the g_assert() check. > > This actually reverts commit 0ee342256af9205e7388efdf193a6d8f1ba1a617, > Some comments are also added to indicate that predicate() must be > provided for an implemented CSR. > > Reported-by: Fei Wu <fei2.wu@intel.com> > Signed-off-by: Bin Meng <bmeng@tinylab.org> Reviewed-by: Alistair Francis <alistair.francis@wdc.com> Alistair > --- > > target/riscv/csr.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/target/riscv/csr.c b/target/riscv/csr.c > index d522efc0b6..736ab64275 100644 > --- a/target/riscv/csr.c > +++ b/target/riscv/csr.c > @@ -3797,6 +3797,11 @@ static inline RISCVException riscv_csrrw_check(CPURISCVState *env, > return RISCV_EXCP_ILLEGAL_INST; > } > > + /* ensure CSR is implemented by checking predicate */ > + if (!csr_ops[csrno].predicate) { > + return RISCV_EXCP_ILLEGAL_INST; > + } > + > /* privileged spec version check */ > if (env->priv_ver < csr_min_priv) { > return RISCV_EXCP_ILLEGAL_INST; > @@ -3814,7 +3819,6 @@ static inline RISCVException riscv_csrrw_check(CPURISCVState *env, > * illegal instruction exception should be triggered instead of virtual > * instruction exception. Hence this comes after the read / write check. > */ > - g_assert(csr_ops[csrno].predicate != NULL); > RISCVException ret = csr_ops[csrno].predicate(env, csrno); > if (ret != RISCV_EXCP_NONE) { > return ret; > @@ -3991,7 +3995,10 @@ RISCVException riscv_csrrw_debug(CPURISCVState *env, int csrno, > return ret; > } > > -/* Control and Status Register function table */ > +/* > + * Control and Status Register function table > + * riscv_csr_operations::predicate() must be provided for an implemented CSR > + */ > riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = { > /* User Floating-Point CSRs */ > [CSR_FFLAGS] = { "fflags", fs, read_fflags, write_fflags }, > -- > 2.25.1 > >
On 2023/4/11 17:02, Bin Meng wrote: > When reading a non-existent CSR QEMU should raise illegal instruction > exception, but currently it just exits due to the g_assert() check. > > This actually reverts commit 0ee342256af9205e7388efdf193a6d8f1ba1a617, > Some comments are also added to indicate that predicate() must be > provided for an implemented CSR. > > Reported-by: Fei Wu <fei2.wu@intel.com> > Signed-off-by: Bin Meng <bmeng@tinylab.org> Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com> Zhiwei > --- > > target/riscv/csr.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/target/riscv/csr.c b/target/riscv/csr.c > index d522efc0b6..736ab64275 100644 > --- a/target/riscv/csr.c > +++ b/target/riscv/csr.c > @@ -3797,6 +3797,11 @@ static inline RISCVException riscv_csrrw_check(CPURISCVState *env, > return RISCV_EXCP_ILLEGAL_INST; > } > > + /* ensure CSR is implemented by checking predicate */ > + if (!csr_ops[csrno].predicate) { > + return RISCV_EXCP_ILLEGAL_INST; > + } > + > /* privileged spec version check */ > if (env->priv_ver < csr_min_priv) { > return RISCV_EXCP_ILLEGAL_INST; > @@ -3814,7 +3819,6 @@ static inline RISCVException riscv_csrrw_check(CPURISCVState *env, > * illegal instruction exception should be triggered instead of virtual > * instruction exception. Hence this comes after the read / write check. > */ > - g_assert(csr_ops[csrno].predicate != NULL); > RISCVException ret = csr_ops[csrno].predicate(env, csrno); > if (ret != RISCV_EXCP_NONE) { > return ret; > @@ -3991,7 +3995,10 @@ RISCVException riscv_csrrw_debug(CPURISCVState *env, int csrno, > return ret; > } > > -/* Control and Status Register function table */ > +/* > + * Control and Status Register function table > + * riscv_csr_operations::predicate() must be provided for an implemented CSR > + */ > riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = { > /* User Floating-Point CSRs */ > [CSR_FFLAGS] = { "fflags", fs, read_fflags, write_fflags },
On Tue, Apr 11, 2023 at 7:03 PM Bin Meng <bmeng@tinylab.org> wrote: > > When reading a non-existent CSR QEMU should raise illegal instruction > exception, but currently it just exits due to the g_assert() check. > > This actually reverts commit 0ee342256af9205e7388efdf193a6d8f1ba1a617, > Some comments are also added to indicate that predicate() must be > provided for an implemented CSR. Thanks! Do you mind sending a v2 rebased on https://github.com/alistair23/qemu/tree/riscv-to-apply.next Alistair > > Reported-by: Fei Wu <fei2.wu@intel.com> > Signed-off-by: Bin Meng <bmeng@tinylab.org> > --- > > target/riscv/csr.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/target/riscv/csr.c b/target/riscv/csr.c > index d522efc0b6..736ab64275 100644 > --- a/target/riscv/csr.c > +++ b/target/riscv/csr.c > @@ -3797,6 +3797,11 @@ static inline RISCVException riscv_csrrw_check(CPURISCVState *env, > return RISCV_EXCP_ILLEGAL_INST; > } > > + /* ensure CSR is implemented by checking predicate */ > + if (!csr_ops[csrno].predicate) { > + return RISCV_EXCP_ILLEGAL_INST; > + } > + > /* privileged spec version check */ > if (env->priv_ver < csr_min_priv) { > return RISCV_EXCP_ILLEGAL_INST; > @@ -3814,7 +3819,6 @@ static inline RISCVException riscv_csrrw_check(CPURISCVState *env, > * illegal instruction exception should be triggered instead of virtual > * instruction exception. Hence this comes after the read / write check. > */ > - g_assert(csr_ops[csrno].predicate != NULL); > RISCVException ret = csr_ops[csrno].predicate(env, csrno); > if (ret != RISCV_EXCP_NONE) { > return ret; > @@ -3991,7 +3995,10 @@ RISCVException riscv_csrrw_debug(CPURISCVState *env, int csrno, > return ret; > } > > -/* Control and Status Register function table */ > +/* > + * Control and Status Register function table > + * riscv_csr_operations::predicate() must be provided for an implemented CSR > + */ > riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = { > /* User Floating-Point CSRs */ > [CSR_FFLAGS] = { "fflags", fs, read_fflags, write_fflags }, > -- > 2.25.1 > >
On 12/4/23 03:04, Wu, Fei wrote: > On 4/11/2023 5:02 PM, Bin Meng wrote: >> When reading a non-existent CSR QEMU should raise illegal instruction >> exception, but currently it just exits due to the g_assert() check. >> > I verified that 'csrr t3, 0x4' in user space didn't cause qemu exit but > raised illegal instruction after applying this patch. Good candidate to add in tests/tcg/riscv64/ :) >> This actually reverts commit 0ee342256af9205e7388efdf193a6d8f1ba1a617, >> Some comments are also added to indicate that predicate() must be >> provided for an implemented CSR. >> >> Reported-by: Fei Wu <fei2.wu@intel.com> >> Signed-off-by: Bin Meng <bmeng@tinylab.org> >> --- >> >> target/riscv/csr.c | 11 +++++++++-- >> 1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/target/riscv/csr.c b/target/riscv/csr.c index d522efc0b6..736ab64275 100644 --- a/target/riscv/csr.c +++ b/target/riscv/csr.c @@ -3797,6 +3797,11 @@ static inline RISCVException riscv_csrrw_check(CPURISCVState *env, return RISCV_EXCP_ILLEGAL_INST; } + /* ensure CSR is implemented by checking predicate */ + if (!csr_ops[csrno].predicate) { + return RISCV_EXCP_ILLEGAL_INST; + } + /* privileged spec version check */ if (env->priv_ver < csr_min_priv) { return RISCV_EXCP_ILLEGAL_INST; @@ -3814,7 +3819,6 @@ static inline RISCVException riscv_csrrw_check(CPURISCVState *env, * illegal instruction exception should be triggered instead of virtual * instruction exception. Hence this comes after the read / write check. */ - g_assert(csr_ops[csrno].predicate != NULL); RISCVException ret = csr_ops[csrno].predicate(env, csrno); if (ret != RISCV_EXCP_NONE) { return ret; @@ -3991,7 +3995,10 @@ RISCVException riscv_csrrw_debug(CPURISCVState *env, int csrno, return ret; } -/* Control and Status Register function table */ +/* + * Control and Status Register function table + * riscv_csr_operations::predicate() must be provided for an implemented CSR + */ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = { /* User Floating-Point CSRs */ [CSR_FFLAGS] = { "fflags", fs, read_fflags, write_fflags },
When reading a non-existent CSR QEMU should raise illegal instruction exception, but currently it just exits due to the g_assert() check. This actually reverts commit 0ee342256af9205e7388efdf193a6d8f1ba1a617, Some comments are also added to indicate that predicate() must be provided for an implemented CSR. Reported-by: Fei Wu <fei2.wu@intel.com> Signed-off-by: Bin Meng <bmeng@tinylab.org> --- target/riscv/csr.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)