[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3] target/riscv: raise an exception when CSRRS/CSRRC writes
|
From: |
Alistair Francis |
|
Subject: |
Re: [PATCH v3] target/riscv: raise an exception when CSRRS/CSRRC writes a read-only CSR |
|
Date: |
Tue, 4 Jun 2024 13:41:30 +1000 |
On Mon, Jun 3, 2024 at 4:00 PM Yuming Yu-Ming Chang(張育銘)
<yumin686@andestech.com> wrote:
>
> Hi Alistair,
>
> I think we need the following patch to fix this issue:
I have dropped the original patch from my tree. Please fix the issue
and send a new patch with the fix incorporated.
Alistair
>
> From 6175c9aee103e40b5a5da587f659563de93b3d85 Mon Sep 17 00:00:00 2001
> From: Alvin Chang <alvinga@andestech.com>
> Date: Thu, 18 Apr 2024 14:52:36 +0800
> Subject: [PATCH] target/riscv: Fix GDB can not read the read-only CSR
>
> From commit 563581cb60, use riscv_csrrw() to read a read-only CSR will
> lead to exception. Fix it by calling riscv_csrr() when GDB wants to read
> a read-only CSR.
>
> Signed-off-by: Alvin Chang <alvinga@andestech.com>
> ---
> target/riscv/csr.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 7aab267916..96accc1549 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -4625,7 +4625,11 @@ RISCVException riscv_csrrw_debug(CPURISCVState *env,
> int csrno,
> #if !defined(CONFIG_USER_ONLY)
> env->debugger = true;
> #endif
> - ret = riscv_csrrw(env, csrno, ret_value, new_value, write_mask);
> + if (!write_mask) {
> + ret = riscv_csrr(env, csrno, ret_value);
> + } else {
> + ret = riscv_csrrw(env, csrno, ret_value, new_value, write_mask);
> + }
> #if !defined(CONFIG_USER_ONLY)
> env->debugger = false;
> #endif
> --
> 2.34.1
>
> Best regards,
> Yuming
>
> -----Original Message-----
> From: Alistair Francis <alistair23@gmail.com>
> Sent: Monday, June 3, 2024 1:39 PM
> To: Yuming Yu-Ming Chang(張育銘) <yumin686@andestech.com>
> Cc: qemu-riscv@nongnu.org; qemu-devel@nongnu.org; palmer@dabbelt.com;
> alistair.francis@wdc.com; bin.meng@windriver.com; liwei1518@gmail.com;
> dbarboza@ventanamicro.com; zhiwei_liu@linux.alibaba.com
> Subject: Re: [PATCH v3] target/riscv: raise an exception when CSRRS/CSRRC
> writes a read-only CSR
>
> [EXTERNAL MAIL]
>
> On Wed, Apr 3, 2024 at 5:10 PM Yu-Ming Chang via <qemu-devel@nongnu.org>
> wrote:
> >
> > Both CSRRS and CSRRC always read the addressed CSR and cause any read side
> > effects regardless of rs1 and rd fields. Note that if rs1 specifies a
> > register
> > holding a zero value other than x0, the instruction will still attempt to
> > write
> > the unmodified value back to the CSR and will cause any attendant side
> > effects.
> >
> > So if CSRRS or CSRRC tries to write a read-only CSR with rs1 which specifies
> > a register holding a zero value, an illegal instruction exception should be
> > raised.
> >
> > Signed-off-by: Yu-Ming Chang <yumin686@andestech.com>
>
> This fails the GitLab CI tests
>
> https://gitlab.com/qemu-project/qemu/-/jobs/6953349448
>
> ERROR:../tests/plugin/insn.c:58:vcpu_init: assertion failed: (count > 0)
> timeout: the monitored command dumped core
> Aborted
> make[1]: *** [Makefile:178: run-plugin-catch-syscalls-with-libinsn.so] Error
> 134
> make: *** [/builds/qemu-project/qemu/tests/Makefile.include:56:
> run-tcg-tests-riscv64-linux-user] Error 2
>
> #0 riscv_gdb_get_csr (cs=<optimized out>, buf=0x5555558e7f50, n=3072)
> at ../src/target/riscv/gdbstub.c:183
> #1 0x00007ffff7fb7841 in vcpu_init (id=<optimized out>,
> vcpu_index=<optimized out>) at ../src/tests/plugin/insn.c:57
> #2 0x000055555569ef1a in plugin_vcpu_cb__simple (cpu=0x5555558fb820,
> ev=<optimized out>) at ../src/plugins/core.c:111
>
>
> After
>
> 182 result = riscv_csrrw_debug(env, n, &val, 0, 0);
>
> result == 2.
>
> I haven't had much luck reproducing this locally, so I don't have a
> great idea of why it isn't working. I suspect you need to ignore the
> checks for debug accesses
>
> Alistair
>
> > ---
> > Hi maintainers,
> > Do I need to make any further improvements to this patch?
> >
> > Best regards,
> > Yuming
> >
> > target/riscv/cpu.h | 4 ++++
> > target/riscv/csr.c | 51 ++++++++++++++++++++++++++++++++++++----
> > target/riscv/op_helper.c | 6 ++---
> > 3 files changed, 53 insertions(+), 8 deletions(-)
> >
> > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > index 3b1a02b944..99006bdb45 100644
> > --- a/target/riscv/cpu.h
> > +++ b/target/riscv/cpu.h
> > @@ -710,6 +710,8 @@ void cpu_get_tb_cpu_state(CPURISCVState *env, vaddr *pc,
> > void riscv_cpu_update_mask(CPURISCVState *env);
> > bool riscv_cpu_is_32bit(RISCVCPU *cpu);
> >
> > +RISCVException riscv_csrr(CPURISCVState *env, int csrno,
> > + target_ulong *ret_value);
> > RISCVException riscv_csrrw(CPURISCVState *env, int csrno,
> > target_ulong *ret_value,
> > target_ulong new_value, target_ulong
> > write_mask);
> > @@ -742,6 +744,8 @@ typedef RISCVException (*riscv_csr_op_fn)(CPURISCVState
> > *env, int csrno,
> > target_ulong new_value,
> > target_ulong write_mask);
> >
> > +RISCVException riscv_csrr_i128(CPURISCVState *env, int csrno,
> > + Int128 *ret_value);
> > RISCVException riscv_csrrw_i128(CPURISCVState *env, int csrno,
> > Int128 *ret_value,
> > Int128 new_value, Int128 write_mask);
> > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > index 726096444f..35662e1777 100644
> > --- a/target/riscv/csr.c
> > +++ b/target/riscv/csr.c
> > @@ -4312,7 +4312,7 @@ static RISCVException rmw_seed(CPURISCVState *env,
> > int csrno,
> >
> > static inline RISCVException riscv_csrrw_check(CPURISCVState *env,
> > int csrno,
> > - bool write_mask)
> > + bool write)
> > {
> > /* check privileges and return RISCV_EXCP_ILLEGAL_INST if check fails
> > */
> > bool read_only = get_field(csrno, 0xC00) == 3;
> > @@ -4334,7 +4334,7 @@ static inline RISCVException
> > riscv_csrrw_check(CPURISCVState *env,
> > }
> >
> > /* read / write check */
> > - if (write_mask && read_only) {
> > + if (write && read_only) {
> > return RISCV_EXCP_ILLEGAL_INST;
> > }
> >
> > @@ -4421,11 +4421,22 @@ static RISCVException
> > riscv_csrrw_do64(CPURISCVState *env, int csrno,
> > return RISCV_EXCP_NONE;
> > }
> >
> > +RISCVException riscv_csrr(CPURISCVState *env, int csrno,
> > + target_ulong *ret_value)
> > +{
> > + RISCVException ret = riscv_csrrw_check(env, csrno, false);
> > + if (ret != RISCV_EXCP_NONE) {
> > + return ret;
> > + }
> > +
> > + return riscv_csrrw_do64(env, csrno, ret_value, 0, 0);
> > +}
> > +
> > RISCVException riscv_csrrw(CPURISCVState *env, int csrno,
> > target_ulong *ret_value,
> > target_ulong new_value, target_ulong write_mask)
> > {
> > - RISCVException ret = riscv_csrrw_check(env, csrno, write_mask);
> > + RISCVException ret = riscv_csrrw_check(env, csrno, true);
> > if (ret != RISCV_EXCP_NONE) {
> > return ret;
> > }
> > @@ -4473,13 +4484,45 @@ static RISCVException
> > riscv_csrrw_do128(CPURISCVState *env, int csrno,
> > return RISCV_EXCP_NONE;
> > }
> >
> > +RISCVException riscv_csrr_i128(CPURISCVState *env, int csrno,
> > + Int128 *ret_value)
> > +{
> > + RISCVException ret;
> > +
> > + ret = riscv_csrrw_check(env, csrno, false);
> > + if (ret != RISCV_EXCP_NONE) {
> > + return ret;
> > + }
> > +
> > + if (csr_ops[csrno].read128) {
> > + return riscv_csrrw_do128(env, csrno, ret_value,
> > + int128_zero(), int128_zero());
> > + }
> > +
> > + /*
> > + * Fall back to 64-bit version for now, if the 128-bit alternative
> > isn't
> > + * at all defined.
> > + * Note, some CSRs don't need to extend to MXLEN (64 upper bits non
> > + * significant), for those, this fallback is correctly handling the
> > + * accesses
> > + */
> > + target_ulong old_value;
> > + ret = riscv_csrrw_do64(env, csrno, &old_value,
> > + (target_ulong)0,
> > + (target_ulong)0);
> > + if (ret == RISCV_EXCP_NONE && ret_value) {
> > + *ret_value = int128_make64(old_value);
> > + }
> > + return ret;
> > +}
> > +
> > RISCVException riscv_csrrw_i128(CPURISCVState *env, int csrno,
> > Int128 *ret_value,
> > Int128 new_value, Int128 write_mask)
> > {
> > RISCVException ret;
> >
> > - ret = riscv_csrrw_check(env, csrno, int128_nz(write_mask));
> > + ret = riscv_csrrw_check(env, csrno, true);
> > if (ret != RISCV_EXCP_NONE) {
> > return ret;
> > }
> > diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
> > index f414aaebdb..b95d47e9ac 100644
> > --- a/target/riscv/op_helper.c
> > +++ b/target/riscv/op_helper.c
> > @@ -51,7 +51,7 @@ target_ulong helper_csrr(CPURISCVState *env, int csr)
> > }
> >
> > target_ulong val = 0;
> > - RISCVException ret = riscv_csrrw(env, csr, &val, 0, 0);
> > + RISCVException ret = riscv_csrr(env, csr, &val);
> >
> > if (ret != RISCV_EXCP_NONE) {
> > riscv_raise_exception(env, ret, GETPC());
> > @@ -84,9 +84,7 @@ target_ulong helper_csrrw(CPURISCVState *env, int csr,
> > target_ulong helper_csrr_i128(CPURISCVState *env, int csr)
> > {
> > Int128 rv = int128_zero();
> > - RISCVException ret = riscv_csrrw_i128(env, csr, &rv,
> > - int128_zero(),
> > - int128_zero());
> > + RISCVException ret = riscv_csrr_i128(env, csr, &rv);
> >
> > if (ret != RISCV_EXCP_NONE) {
> > riscv_raise_exception(env, ret, GETPC());
> > --
> > 2.34.1
> >
> >
> CONFIDENTIALITY NOTICE:
>
> This e-mail (and its attachments) may contain confidential and legally
> privileged information or information protected from disclosure. If you are
> not the intended recipient, you are hereby notified that any disclosure,
> copying, distribution, or use of the information contained herein is strictly
> prohibited. In this case, please immediately notify the sender by return
> e-mail, delete the message (and any accompanying documents) and destroy all
> printed hard copies. Thank you for your cooperation.
>
> Copyright ANDES TECHNOLOGY CORPORATION - All Rights Reserved.