* Re: [PATCH] target/riscv: Restore the predicate() NULL check behavior
2023-04-11 9:02 [PATCH] target/riscv: Restore the predicate() NULL check behavior Bin Meng
@ 2023-04-11 23:29 ` Daniel Henrique Barboza
2023-04-12 1:00 ` liweiwei
` (4 subsequent siblings)
5 siblings, 0 replies; 8+ messages in thread
From: Daniel Henrique Barboza @ 2023-04-11 23:29 UTC (permalink / raw)
To: Bin Meng, Alistair Francis, Liu Zhiwei, Weiwei Li, Palmer Dabbelt
Cc: qemu-devel, qemu-riscv, Fei Wu
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 },
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] target/riscv: Restore the predicate() NULL check behavior
2023-04-11 9:02 [PATCH] target/riscv: Restore the predicate() NULL check behavior Bin Meng
2023-04-11 23:29 ` Daniel Henrique Barboza
@ 2023-04-12 1:00 ` liweiwei
2023-04-12 1:04 ` Wu, Fei
` (3 subsequent siblings)
5 siblings, 0 replies; 8+ messages in thread
From: liweiwei @ 2023-04-12 1:00 UTC (permalink / raw)
To: Bin Meng, Alistair Francis, Daniel Henrique Barboza, Liu Zhiwei,
Weiwei Li, Palmer Dabbelt
Cc: qemu-devel, qemu-riscv, Fei Wu
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 },
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] target/riscv: Restore the predicate() NULL check behavior
2023-04-11 9:02 [PATCH] target/riscv: Restore the predicate() NULL check behavior Bin Meng
2023-04-11 23:29 ` Daniel Henrique Barboza
2023-04-12 1:00 ` liweiwei
@ 2023-04-12 1:04 ` Wu, Fei
2023-04-12 10:08 ` Philippe Mathieu-Daudé
2023-04-12 1:15 ` Alistair Francis
` (2 subsequent siblings)
5 siblings, 1 reply; 8+ messages in thread
From: Wu, Fei @ 2023-04-12 1:04 UTC (permalink / raw)
To: Bin Meng, Alistair Francis, Daniel Henrique Barboza, Liu Zhiwei,
Weiwei Li, Palmer Dabbelt
Cc: qemu-devel, qemu-riscv
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 },
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] target/riscv: Restore the predicate() NULL check behavior
2023-04-12 1:04 ` Wu, Fei
@ 2023-04-12 10:08 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 8+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-04-12 10:08 UTC (permalink / raw)
To: Wu, Fei, Bin Meng, Alistair Francis, Daniel Henrique Barboza,
Liu Zhiwei, Weiwei Li, Palmer Dabbelt
Cc: qemu-devel, qemu-riscv
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(-)
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] target/riscv: Restore the predicate() NULL check behavior
2023-04-11 9:02 [PATCH] target/riscv: Restore the predicate() NULL check behavior Bin Meng
` (2 preceding siblings ...)
2023-04-12 1:04 ` Wu, Fei
@ 2023-04-12 1:15 ` Alistair Francis
2023-04-12 1:32 ` LIU Zhiwei
2023-04-12 1:46 ` Alistair Francis
5 siblings, 0 replies; 8+ messages in thread
From: Alistair Francis @ 2023-04-12 1:15 UTC (permalink / raw)
To: Bin Meng
Cc: Alistair Francis, Daniel Henrique Barboza, Liu Zhiwei, Weiwei Li,
Palmer Dabbelt, qemu-devel, qemu-riscv, Fei Wu
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
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] target/riscv: Restore the predicate() NULL check behavior
2023-04-11 9:02 [PATCH] target/riscv: Restore the predicate() NULL check behavior Bin Meng
` (3 preceding siblings ...)
2023-04-12 1:15 ` Alistair Francis
@ 2023-04-12 1:32 ` LIU Zhiwei
2023-04-12 1:46 ` Alistair Francis
5 siblings, 0 replies; 8+ messages in thread
From: LIU Zhiwei @ 2023-04-12 1:32 UTC (permalink / raw)
To: Bin Meng, Alistair Francis, Daniel Henrique Barboza, Weiwei Li,
Palmer Dabbelt
Cc: qemu-devel, qemu-riscv, Fei Wu
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 },
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] target/riscv: Restore the predicate() NULL check behavior
2023-04-11 9:02 [PATCH] target/riscv: Restore the predicate() NULL check behavior Bin Meng
` (4 preceding siblings ...)
2023-04-12 1:32 ` LIU Zhiwei
@ 2023-04-12 1:46 ` Alistair Francis
5 siblings, 0 replies; 8+ messages in thread
From: Alistair Francis @ 2023-04-12 1:46 UTC (permalink / raw)
To: Bin Meng
Cc: Alistair Francis, Daniel Henrique Barboza, Liu Zhiwei, Weiwei Li,
Palmer Dabbelt, qemu-devel, qemu-riscv, Fei Wu
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
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread