target/riscv/csr.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-)
From: Yi Chen <chenyi2000@zju.edu.cn>
Trap accesses to hgatp if MSTATUS_TVM is enabled.
Don't trap accesses to vsatp even if MSTATUS_TVM is enabled.
Signed-off-by: Yi Chen <chenyi2000@zju.edu.cn>
---
target/riscv/csr.c | 18 ++++++++++++++----
1 file changed, 14 insertions(+), 4 deletions(-)
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index ab56663..09bc780 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -2655,7 +2655,7 @@ static RISCVException read_satp(CPURISCVState *env, int csrno,
return RISCV_EXCP_NONE;
}
- if (env->priv == PRV_S && get_field(env->mstatus, MSTATUS_TVM)) {
+ if (env->priv == PRV_S && !riscv_cpu_virt_enabled(env) && get_field(env->mstatus, MSTATUS_TVM)) {
return RISCV_EXCP_ILLEGAL_INST;
} else {
*val = env->satp;
@@ -2683,7 +2683,7 @@ static RISCVException write_satp(CPURISCVState *env, int csrno,
}
if (vm && mask) {
- if (env->priv == PRV_S && get_field(env->mstatus, MSTATUS_TVM)) {
+ if (env->priv == PRV_S && !riscv_cpu_virt_enabled(env) && get_field(env->mstatus, MSTATUS_TVM)) {
return RISCV_EXCP_ILLEGAL_INST;
} else {
/*
@@ -3047,14 +3047,24 @@ static RISCVException read_hgeip(CPURISCVState *env, int csrno,
static RISCVException read_hgatp(CPURISCVState *env, int csrno,
target_ulong *val)
{
- *val = env->hgatp;
+ if (env->priv == PRV_S && get_field(env->mstatus, MSTATUS_TVM)) {
+ return RISCV_EXCP_ILLEGAL_INST;
+ } else {
+ *val = env->hgatp;
+ }
+
return RISCV_EXCP_NONE;
}
static RISCVException write_hgatp(CPURISCVState *env, int csrno,
target_ulong val)
{
- env->hgatp = val;
+ if (env->priv == PRV_S && get_field(env->mstatus, MSTATUS_TVM)) {
+ return RISCV_EXCP_ILLEGAL_INST;
+ } else {
+ env->hgatp = val;
+ }
+
return RISCV_EXCP_NONE;
}
--
2.39.2
On 2023/3/8 20:34, chenyi2000@zju.edu.cn wrote: > From: Yi Chen<chenyi2000@zju.edu.cn> > > Trap accesses to hgatp if MSTATUS_TVM is enabled. > Don't trap accesses to vsatp even if MSTATUS_TVM is enabled. By the way, do you know why mstatus_tvm and hstatus_tvm are needed? The specification said, The TVM mechanism improves virtualization efficiency by permitting guest operating systems to execute in S-mode, rather than classically virtualizing them in U-mode. This approach obviates the need to trap accesses to most S-mode CSRs. I don't know how the tvm field obviates the need to trap accesses to most S-mode CSRs. Thanks, Zhiwei > > Signed-off-by: Yi Chen<chenyi2000@zju.edu.cn> > --- > target/riscv/csr.c | 18 ++++++++++++++---- > 1 file changed, 14 insertions(+), 4 deletions(-) > > diff --git a/target/riscv/csr.c b/target/riscv/csr.c > index ab56663..09bc780 100644 > --- a/target/riscv/csr.c > +++ b/target/riscv/csr.c > @@ -2655,7 +2655,7 @@ static RISCVException read_satp(CPURISCVState *env, int csrno, > return RISCV_EXCP_NONE; > } > > - if (env->priv == PRV_S && get_field(env->mstatus, MSTATUS_TVM)) { > + if (env->priv == PRV_S && !riscv_cpu_virt_enabled(env) && get_field(env->mstatus, MSTATUS_TVM)) { > return RISCV_EXCP_ILLEGAL_INST; > } else { > *val = env->satp; > @@ -2683,7 +2683,7 @@ static RISCVException write_satp(CPURISCVState *env, int csrno, > } > > if (vm && mask) { > - if (env->priv == PRV_S && get_field(env->mstatus, MSTATUS_TVM)) { > + if (env->priv == PRV_S && !riscv_cpu_virt_enabled(env) && get_field(env->mstatus, MSTATUS_TVM)) { > return RISCV_EXCP_ILLEGAL_INST; > } else { > /* > @@ -3047,14 +3047,24 @@ static RISCVException read_hgeip(CPURISCVState *env, int csrno, > static RISCVException read_hgatp(CPURISCVState *env, int csrno, > target_ulong *val) > { > - *val = env->hgatp; > + if (env->priv == PRV_S && get_field(env->mstatus, MSTATUS_TVM)) { > + return RISCV_EXCP_ILLEGAL_INST; > + } else { > + *val = env->hgatp; > + } > + > return RISCV_EXCP_NONE; > } > > static RISCVException write_hgatp(CPURISCVState *env, int csrno, > target_ulong val) > { > - env->hgatp = val; > + if (env->priv == PRV_S && get_field(env->mstatus, MSTATUS_TVM)) { > + return RISCV_EXCP_ILLEGAL_INST; > + } else { > + env->hgatp = val; > + } > + > return RISCV_EXCP_NONE; > } >
-----Original Messages----- From:"LIU Zhiwei" <zhiwei_liu@linux.alibaba.com> Sent Time:2023-03-10 10:12:10 (Friday) To: chenyi2000@zju.edu.cn, qemu-devel@nongnu.org Cc: "Palmer Dabbelt" <palmer@dabbelt.com>, "Alistair Francis" <alistair.francis@wdc.com>, "Bin Meng" <bin.meng@windriver.com>, "Weiwei Li" <liweiwei@iscas.ac.cn>, "Daniel Henrique Barboza" <dbarboza@ventanamicro.com>, "open list:RISC-V TCG CPUs" <qemu-riscv@nongnu.org> Subject: Re: [PATCH] target/riscv/csr.c: fix H extension TVM trap On 2023/3/8 20:34, chenyi2000@zju.edu.cn wrote: From: Yi Chen <chenyi2000@zju.edu.cn> Trap accesses to hgatp if MSTATUS_TVM is enabled. Don't trap accesses to vsatp even if MSTATUS_TVM is enabled. By the way, do you know why mstatus_tvm and hstatus_tvm are needed? The specification said, The TVM mechanism improves virtualization efficiency by permitting guest operating systems to execute in S-mode, rather than classically virtualizing them in U-mode. This approach obviates the need to trap accesses to most S-mode CSRs. I don't know how the tvm field obviates the need to trap accesses to most S-mode CSRs. Thanks, Zhiwei When VMs are in U-mode, their accesses to S-mode CSR registers must be emulated. Otherwise, the behavior of the host OS will be affected. But I guess since TVM helps insert another stage of address translation below that controlled by the OS, it enables VMs to run in S-mode, which means that VMs can directly use most S-mode CSR registers instead of emulated ones. Best, Yi Signed-off-by: Yi Chen <chenyi2000@zju.edu.cn> --- target/riscv/csr.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/target/riscv/csr.c b/target/riscv/csr.c index ab56663..09bc780 100644 --- a/target/riscv/csr.c +++ b/target/riscv/csr.c @@ -2655,7 +2655,7 @@ static RISCVException read_satp(CPURISCVState *env, int csrno, return RISCV_EXCP_NONE; } - if (env->priv == PRV_S && get_field(env->mstatus, MSTATUS_TVM)) { + if (env->priv == PRV_S && !riscv_cpu_virt_enabled(env) && get_field(env->mstatus, MSTATUS_TVM)) { return RISCV_EXCP_ILLEGAL_INST; } else { *val = env->satp; @@ -2683,7 +2683,7 @@ static RISCVException write_satp(CPURISCVState *env, int csrno, } if (vm && mask) { - if (env->priv == PRV_S && get_field(env->mstatus, MSTATUS_TVM)) { + if (env->priv == PRV_S && !riscv_cpu_virt_enabled(env) && get_field(env->mstatus, MSTATUS_TVM)) { return RISCV_EXCP_ILLEGAL_INST; } else { /* @@ -3047,14 +3047,24 @@ static RISCVException read_hgeip(CPURISCVState *env, int csrno, static RISCVException read_hgatp(CPURISCVState *env, int csrno, target_ulong *val) { - *val = env->hgatp; + if (env->priv == PRV_S && get_field(env->mstatus, MSTATUS_TVM)) { + return RISCV_EXCP_ILLEGAL_INST; + } else { + *val = env->hgatp; + } + return RISCV_EXCP_NONE; } static RISCVException write_hgatp(CPURISCVState *env, int csrno, target_ulong val) { - env->hgatp = val; + if (env->priv == PRV_S && get_field(env->mstatus, MSTATUS_TVM)) { + return RISCV_EXCP_ILLEGAL_INST; + } else { + env->hgatp = val; + } + return RISCV_EXCP_NONE; }
On 2023/3/10 17:08, CHEN Yi wrote: > > -----Original Messages----- > *From:*"LIU Zhiwei" <zhiwei_liu@linux.alibaba.com> > *Sent Time:*2023-03-10 10:12:10 (Friday) > *To:* chenyi2000@zju.edu.cn, qemu-devel@nongnu.org > *Cc:* "Palmer Dabbelt" <palmer@dabbelt.com>, "Alistair Francis" > <alistair.francis@wdc.com>, "Bin Meng" <bin.meng@windriver.com>, > "Weiwei Li" <liweiwei@iscas.ac.cn>, "Daniel Henrique Barboza" > <dbarboza@ventanamicro.com>, "open list:RISC-V TCG CPUs" > <qemu-riscv@nongnu.org> > *Subject:* Re: [PATCH] target/riscv/csr.c: fix H extension TVM trap > > > On 2023/3/8 20:34, chenyi2000@zju.edu.cn wrote: >> From: Yi Chen<chenyi2000@zju.edu.cn> Trap accesses to hgatp if MSTATUS_TVM is enabled. >> Don't trap accesses to vsatp even if MSTATUS_TVM is enabled. > > By the way, do you know why mstatus_tvm and hstatus_tvm are needed? > > The specification said, > > The TVM mechanism improves virtualization efficiency by permitting guest operating systems to > execute in S-mode, rather than classically virtualizing them in U-mode. This approach obviates > the need to trap accesses to most S-mode CSRs. > > I don't know how the tvm field obviates the need to trap accesses > to most S-mode CSRs. > > Thanks, > Zhiwei > > When VMs are in U-mode, their accesses to S-mode CSR registers must be > emulated. Otherwise, the behavior of the host OS will be affected. But > I guess since TVM helps insert another stage of address translation > below that controlled by the OS, it enables VMs to run in S-mode, > which means that VMs can directly use most S-mode CSR registers > instead of emulated ones. > If the guest running in S-mode, the other smode CSR accesses may break the host OS. Zhiwei > > Best, > > Yi > > > >> Signed-off-by: Yi Chen<chenyi2000@zju.edu.cn> --- >> target/riscv/csr.c | 18 ++++++++++++++---- >> 1 file changed, 14 insertions(+), 4 deletions(-) >> >> diff --git a/target/riscv/csr.c b/target/riscv/csr.c >> index ab56663..09bc780 100644 >> --- a/target/riscv/csr.c >> +++ b/target/riscv/csr.c >> @@ -2655,7 +2655,7 @@ static RISCVException read_satp(CPURISCVState *env, int csrno, >> return RISCV_EXCP_NONE; >> } >> >> - if (env->priv == PRV_S && get_field(env->mstatus, MSTATUS_TVM)) { >> + if (env->priv == PRV_S && !riscv_cpu_virt_enabled(env) && get_field(env->mstatus, MSTATUS_TVM)) { >> return RISCV_EXCP_ILLEGAL_INST; >> } else { >> *val = env->satp; >> @@ -2683,7 +2683,7 @@ static RISCVException write_satp(CPURISCVState *env, int csrno, >> } >> >> if (vm && mask) { >> - if (env->priv == PRV_S && get_field(env->mstatus, MSTATUS_TVM)) { >> + if (env->priv == PRV_S && !riscv_cpu_virt_enabled(env) && get_field(env->mstatus, MSTATUS_TVM)) { >> return RISCV_EXCP_ILLEGAL_INST; >> } else { >> /* >> @@ -3047,14 +3047,24 @@ static RISCVException read_hgeip(CPURISCVState *env, int csrno, >> static RISCVException read_hgatp(CPURISCVState *env, int csrno, >> target_ulong *val) >> { >> - *val = env->hgatp; >> + if (env->priv == PRV_S && get_field(env->mstatus, MSTATUS_TVM)) { >> + return RISCV_EXCP_ILLEGAL_INST; >> + } else { >> + *val = env->hgatp; >> + } >> + >> return RISCV_EXCP_NONE; >> } >> >> static RISCVException write_hgatp(CPURISCVState *env, int csrno, >> target_ulong val) >> { >> - env->hgatp = val; >> + if (env->priv == PRV_S && get_field(env->mstatus, MSTATUS_TVM)) { >> + return RISCV_EXCP_ILLEGAL_INST; >> + } else { >> + env->hgatp = val; >> + } >> + >> return RISCV_EXCP_NONE; >> } >
-----Original Messages----- From:"LIU Zhiwei" <zhiwei_liu@linux.alibaba.com> Sent Time:2023-03-10 17:18:56 (Friday) To: "CHEN Yi" <chenyi2000@zju.edu.cn>, qemu-devel@nongnu.org Cc: "Palmer Dabbelt" <palmer@dabbelt.com>, "Alistair Francis" <alistair.francis@wdc.com>, "Bin Meng" <bin.meng@windriver.com>, "Weiwei Li" <liweiwei@iscas.ac.cn>, "Daniel Henrique Barboza" <dbarboza@ventanamicro.com>, "open list:RISC-V TCG CPUs" <qemu-riscv@nongnu.org> Subject: Re: [PATCH] target/riscv/csr.c: fix H extension TVM trap On 2023/3/10 17:08, CHEN Yi wrote: -----Original Messages----- From:"LIU Zhiwei" <zhiwei_liu@linux.alibaba.com> Sent Time:2023-03-10 10:12:10 (Friday) To:chenyi2000@zju.edu.cn, qemu-devel@nongnu.org Cc: "Palmer Dabbelt" <palmer@dabbelt.com>, "Alistair Francis" <alistair.francis@wdc.com>, "Bin Meng" <bin.meng@windriver.com>, "Weiwei Li" <liweiwei@iscas.ac.cn>, "Daniel Henrique Barboza" <dbarboza@ventanamicro.com>, "open list:RISC-V TCG CPUs" <qemu-riscv@nongnu.org> Subject: Re: [PATCH] target/riscv/csr.c: fix H extension TVM trap On 2023/3/8 20:34, chenyi2000@zju.edu.cn wrote: From: Yi Chen <chenyi2000@zju.edu.cn> Trap accesses to hgatp if MSTATUS_TVM is enabled. Don't trap accesses to vsatp even if MSTATUS_TVM is enabled. By the way, do you know why mstatus_tvm and hstatus_tvm are needed? The specification said, The TVM mechanism improves virtualization efficiency by permitting guest operating systems to execute in S-mode, rather than classically virtualizing them in U-mode. This approach obviates the need to trap accesses to most S-mode CSRs. I don't know how the tvm field obviates the need to trap accesses to most S-mode CSRs. Thanks, Zhiwei When VMs are in U-mode, their accesses to S-mode CSR registers must be emulated. Otherwise, the behavior of the host OS will be affected. But I guess since TVM helps insert another stage of address translation below that controlled by the OS, it enables VMs to run in S-mode, which means that VMs can directly use most S-mode CSR registers instead of emulated ones. If the guest running in S-mode, the other smode CSR accesses may break the host OS. Zhiwei I guess hypervisors can be (partially) put in M-mode. Do you have any example where access to a specific CSR has to be trapped? Best, Yi Best, Yi Signed-off-by: Yi Chen <chenyi2000@zju.edu.cn> --- target/riscv/csr.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/target/riscv/csr.c b/target/riscv/csr.c index ab56663..09bc780 100644 --- a/target/riscv/csr.c +++ b/target/riscv/csr.c @@ -2655,7 +2655,7 @@ static RISCVException read_satp(CPURISCVState *env, int csrno, return RISCV_EXCP_NONE; } - if (env->priv == PRV_S && get_field(env->mstatus, MSTATUS_TVM)) { + if (env->priv == PRV_S && !riscv_cpu_virt_enabled(env) && get_field(env->mstatus, MSTATUS_TVM)) { return RISCV_EXCP_ILLEGAL_INST; } else { *val = env->satp; @@ -2683,7 +2683,7 @@ static RISCVException write_satp(CPURISCVState *env, int csrno, } if (vm && mask) { - if (env->priv == PRV_S && get_field(env->mstatus, MSTATUS_TVM)) { + if (env->priv == PRV_S && !riscv_cpu_virt_enabled(env) && get_field(env->mstatus, MSTATUS_TVM)) { return RISCV_EXCP_ILLEGAL_INST; } else { /* @@ -3047,14 +3047,24 @@ static RISCVException read_hgeip(CPURISCVState *env, int csrno, static RISCVException read_hgatp(CPURISCVState *env, int csrno, target_ulong *val) { - *val = env->hgatp; + if (env->priv == PRV_S && get_field(env->mstatus, MSTATUS_TVM)) { + return RISCV_EXCP_ILLEGAL_INST; + } else { + *val = env->hgatp; + } + return RISCV_EXCP_NONE; } static RISCVException write_hgatp(CPURISCVState *env, int csrno, target_ulong val) { - env->hgatp = val; + if (env->priv == PRV_S && get_field(env->mstatus, MSTATUS_TVM)) { + return RISCV_EXCP_ILLEGAL_INST; + } else { + env->hgatp = val; + } + return RISCV_EXCP_NONE; }
On 2023/3/8 20:34, chenyi2000@zju.edu.cn wrote: > From: Yi Chen<chenyi2000@zju.edu.cn> > > Trap accesses to hgatp if MSTATUS_TVM is enabled. > Don't trap accesses to vsatp even if MSTATUS_TVM is enabled. > > Signed-off-by: Yi Chen<chenyi2000@zju.edu.cn> > --- > target/riscv/csr.c | 18 ++++++++++++++---- > 1 file changed, 14 insertions(+), 4 deletions(-) > > diff --git a/target/riscv/csr.c b/target/riscv/csr.c > index ab56663..09bc780 100644 > --- a/target/riscv/csr.c > +++ b/target/riscv/csr.c > @@ -2655,7 +2655,7 @@ static RISCVException read_satp(CPURISCVState *env, int csrno, > return RISCV_EXCP_NONE; > } > > - if (env->priv == PRV_S && get_field(env->mstatus, MSTATUS_TVM)) { > + if (env->priv == PRV_S && !riscv_cpu_virt_enabled(env) && get_field(env->mstatus, MSTATUS_TVM)) { > return RISCV_EXCP_ILLEGAL_INST; This line seems too long (> 80). And hstatus.VTVM should also be taken into consideration. Similar to following write_satp. > } else { > *val = env->satp; > @@ -2683,7 +2683,7 @@ static RISCVException write_satp(CPURISCVState *env, int csrno, > } > > if (vm && mask) { > - if (env->priv == PRV_S && get_field(env->mstatus, MSTATUS_TVM)) { > + if (env->priv == PRV_S && !riscv_cpu_virt_enabled(env) && get_field(env->mstatus, MSTATUS_TVM)) { > return RISCV_EXCP_ILLEGAL_INST; > } else { > /* > @@ -3047,14 +3047,24 @@ static RISCVException read_hgeip(CPURISCVState *env, int csrno, > static RISCVException read_hgatp(CPURISCVState *env, int csrno, > target_ulong *val) > { > - *val = env->hgatp; > + if (env->priv == PRV_S && get_field(env->mstatus, MSTATUS_TVM)) { > + return RISCV_EXCP_ILLEGAL_INST; This check will do before privilege check in riscv_csrrw_check. So it will make VS mode access trigger ILLEGAL_INST exception, However, it should be VIRTUAL_INST exception in this case. Regards, Weiwei Li > + } else { > + *val = env->hgatp; > + } > + > return RISCV_EXCP_NONE; > } > > static RISCVException write_hgatp(CPURISCVState *env, int csrno, > target_ulong val) > { > - env->hgatp = val; > + if (env->priv == PRV_S && get_field(env->mstatus, MSTATUS_TVM)) { > + return RISCV_EXCP_ILLEGAL_INST; > + } else { > + env->hgatp = val; > + } > + > return RISCV_EXCP_NONE; > } >
-----Original Messages----- From:liweiwei <liweiwei@iscas.ac.cn> Sent Time:2023-03-09 15:48:17 (Thursday) To: chenyi2000@zju.edu.cn, qemu-devel@nongnu.org Cc: "Palmer Dabbelt" <palmer@dabbelt.com>, "Alistair Francis" <alistair.francis@wdc.com>, "Bin Meng" <bin.meng@windriver.com>, "Daniel Henrique Barboza" <dbarboza@ventanamicro.com>, "Liu Zhiwei" <zhiwei_liu@linux.alibaba.com>, "open list:RISC-V TCG CPUs" <qemu-riscv@nongnu.org> Subject: Re: [PATCH] target/riscv/csr.c: fix H extension TVM trap On 2023/3/8 20:34, chenyi2000@zju.edu.cn wrote: From: Yi Chen <chenyi2000@zju.edu.cn> Trap accesses to hgatp if MSTATUS_TVM is enabled. Don't trap accesses to vsatp even if MSTATUS_TVM is enabled. Signed-off-by: Yi Chen <chenyi2000@zju.edu.cn> --- target/riscv/csr.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/target/riscv/csr.c b/target/riscv/csr.c index ab56663..09bc780 100644 --- a/target/riscv/csr.c +++ b/target/riscv/csr.c @@ -2655,7 +2655,7 @@ static RISCVException read_satp(CPURISCVState *env, int csrno, return RISCV_EXCP_NONE; } - if (env->priv == PRV_S && get_field(env->mstatus, MSTATUS_TVM)) { + if (env->priv == PRV_S && !riscv_cpu_virt_enabled(env) && get_field(env->mstatus, MSTATUS_TVM)) { return RISCV_EXCP_ILLEGAL_INST; This line seems too long (> 80). And hstatus.VTVM should also be taken into consideration. Similar to following write_satp. } else { *val = env->satp; @@ -2683,7 +2683,7 @@ static RISCVException write_satp(CPURISCVState *env, int csrno, } if (vm && mask) { - if (env->priv == PRV_S && get_field(env->mstatus, MSTATUS_TVM)) { + if (env->priv == PRV_S && !riscv_cpu_virt_enabled(env) && get_field(env->mstatus, MSTATUS_TVM)) { return RISCV_EXCP_ILLEGAL_INST; } else { /* Thanks a lot. In the next version, I will fix the code style issue and consider hstatus.VTVM. @@ -3047,14 +3047,24 @@ static RISCVException read_hgeip(CPURISCVState *env, int csrno, static RISCVException read_hgatp(CPURISCVState *env, int csrno, target_ulong *val) { - *val = env->hgatp; + if (env->priv == PRV_S && get_field(env->mstatus, MSTATUS_TVM)) { + return RISCV_EXCP_ILLEGAL_INST; This check will do before privilege check in riscv_csrrw_check. So it will make VS mode access trigger ILLEGAL_INST exception, However, it should be VIRTUAL_INST exception in this case. Regards, Weiwei Li In riscv_csrrw(), riscv_csrrw_check() is called before riscv_csrrw_do64(). So I think VIRTUAL_INST will be triggered. Could you please explain why this check will do before the privilege check in riscv_csrrw_check? I'm new to Qemu source code and am sorry I can't understand that. + } else { + *val = env->hgatp; + } + return RISCV_EXCP_NONE; } static RISCVException write_hgatp(CPURISCVState *env, int csrno, target_ulong val) { - env->hgatp = val; + if (env->priv == PRV_S && get_field(env->mstatus, MSTATUS_TVM)) { + return RISCV_EXCP_ILLEGAL_INST; + } else { + env->hgatp = val; + } + return RISCV_EXCP_NONE; } Thanks, Yi
On 2023/3/9 23:02, CHEN Yi wrote: > > > > -----Original Messages----- > *From:*liweiwei <liweiwei@iscas.ac.cn> > *Sent Time:*2023-03-09 15:48:17 (Thursday) > *To:* chenyi2000@zju.edu.cn, qemu-devel@nongnu.org > *Cc:* "Palmer Dabbelt" <palmer@dabbelt.com>, "Alistair Francis" > <alistair.francis@wdc.com>, "Bin Meng" <bin.meng@windriver.com>, > "Daniel Henrique Barboza" <dbarboza@ventanamicro.com>, "Liu > Zhiwei" <zhiwei_liu@linux.alibaba.com>, "open list:RISC-V TCG > CPUs" <qemu-riscv@nongnu.org> > *Subject:* Re: [PATCH] target/riscv/csr.c: fix H extension TVM trap > > > On 2023/3/8 20:34, chenyi2000@zju.edu.cn wrote: >> From: Yi Chen<chenyi2000@zju.edu.cn> Trap accesses to hgatp if MSTATUS_TVM is enabled. >> Don't trap accesses to vsatp even if MSTATUS_TVM is enabled. >> >> Signed-off-by: Yi Chen<chenyi2000@zju.edu.cn> --- >> target/riscv/csr.c | 18 ++++++++++++++---- >> 1 file changed, 14 insertions(+), 4 deletions(-) >> >> diff --git a/target/riscv/csr.c b/target/riscv/csr.c >> index ab56663..09bc780 100644 >> --- a/target/riscv/csr.c >> +++ b/target/riscv/csr.c >> @@ -2655,7 +2655,7 @@ static RISCVException read_satp(CPURISCVState *env, int csrno, >> return RISCV_EXCP_NONE; >> } >> >> - if (env->priv == PRV_S && get_field(env->mstatus, MSTATUS_TVM)) { >> + if (env->priv == PRV_S && !riscv_cpu_virt_enabled(env) && get_field(env->mstatus, MSTATUS_TVM)) { >> return RISCV_EXCP_ILLEGAL_INST; > > This line seems too long (> 80). > > And hstatus.VTVM should also be taken into consideration. > > Similar to following write_satp. > >> } else { >> *val = env->satp; >> @@ -2683,7 +2683,7 @@ static RISCVException write_satp(CPURISCVState *env, int csrno, >> } >> >> if (vm && mask) { >> - if (env->priv == PRV_S && get_field(env->mstatus, MSTATUS_TVM)) { >> + if (env->priv == PRV_S && !riscv_cpu_virt_enabled(env) && get_field(env->mstatus, MSTATUS_TVM)) { >> return RISCV_EXCP_ILLEGAL_INST; >> } else { >> /* > > > Thanks a lot. In the next version, I will fix the code style issue and > consider hstatus.VTVM. > > >> @@ -3047,14 +3047,24 @@ static RISCVException read_hgeip(CPURISCVState *env, int csrno, >> static RISCVException read_hgatp(CPURISCVState *env, int csrno, >> target_ulong *val) >> { >> - *val = env->hgatp; >> + if (env->priv == PRV_S && get_field(env->mstatus, MSTATUS_TVM)) { >> + return RISCV_EXCP_ILLEGAL_INST; > > This check will do before privilege check in riscv_csrrw_check. So > it will make VS mode access trigger > > ILLEGAL_INST exception, However, it should be VIRTUAL_INST > exception in this case. > > Regards, > > Weiwei Li > > > > In riscv_csrrw(), riscv_csrrw_check() is called before > riscv_csrrw_do64(). So I think VIRTUAL_INST will be triggered. Could > you please explain why this check will do before the privilege check > in riscv_csrrw_check? I'm new to Qemu source code and am sorry I can't > understand that. > > Yeah, You are right. Sorry that I mistook this check for check in the predicate. By the way, I think this check is better to be done in the predicate. Regards, Weiwei Li >> + } else { >> + *val = env->hgatp; >> + } >> + >> return RISCV_EXCP_NONE; >> } >> >> static RISCVException write_hgatp(CPURISCVState *env, int csrno, >> target_ulong val) >> { >> - env->hgatp = val; >> + if (env->priv == PRV_S && get_field(env->mstatus, MSTATUS_TVM)) { >> + return RISCV_EXCP_ILLEGAL_INST; >> + } else { >> + env->hgatp = val; >> + } >> + >> return RISCV_EXCP_NONE; >> } >> > > > Thanks, > > Yi >
On 3/8/23 09:34, chenyi2000@zju.edu.cn wrote: > From: Yi Chen <chenyi2000@zju.edu.cn> > > Trap accesses to hgatp if MSTATUS_TVM is enabled. > Don't trap accesses to vsatp even if MSTATUS_TVM is enabled. > > Signed-off-by: Yi Chen <chenyi2000@zju.edu.cn> > --- > target/riscv/csr.c | 18 ++++++++++++++---- > 1 file changed, 14 insertions(+), 4 deletions(-) > > diff --git a/target/riscv/csr.c b/target/riscv/csr.c > index ab56663..09bc780 100644 > --- a/target/riscv/csr.c > +++ b/target/riscv/csr.c > @@ -2655,7 +2655,7 @@ static RISCVException read_satp(CPURISCVState *env, int csrno, > return RISCV_EXCP_NONE; > } > > - if (env->priv == PRV_S && get_field(env->mstatus, MSTATUS_TVM)) { > + if (env->priv == PRV_S && !riscv_cpu_virt_enabled(env) && get_field(env->mstatus, MSTATUS_TVM)) { The commit message mentions 'vsatp' but this patch is changing satp callbacks. Any reason to not change read_vsatp() and write_vsatp() instead? > return RISCV_EXCP_ILLEGAL_INST; > } else { > *val = env->satp; > @@ -2683,7 +2683,7 @@ static RISCVException write_satp(CPURISCVState *env, int csrno, > } > > if (vm && mask) { > - if (env->priv == PRV_S && get_field(env->mstatus, MSTATUS_TVM)) { > + if (env->priv == PRV_S && !riscv_cpu_virt_enabled(env) && get_field(env->mstatus, MSTATUS_TVM)) { > return RISCV_EXCP_ILLEGAL_INST; > } else { > /* > @@ -3047,14 +3047,24 @@ static RISCVException read_hgeip(CPURISCVState *env, int csrno, > static RISCVException read_hgatp(CPURISCVState *env, int csrno, > target_ulong *val) > { > - *val = env->hgatp; > + if (env->priv == PRV_S && get_field(env->mstatus, MSTATUS_TVM)) { > + return RISCV_EXCP_ILLEGAL_INST; The end of the first paragraph of ISA 8.2.10 goes as follows: ==== When mstatus.TVM=1, attempts to read or write hgatp while executing in HS-mode will raise an illegal instruction exception. ==== I believe you need to check for HS-mode, not just PRV_S. riscv_csrrw_check() in target/riscv/csr.c checks for HS-mode as follows: if (riscv_has_ext(env, RVH) && env->priv == PRV_S && !riscv_cpu_virt_enabled(env)) { Same goes for write_hgatp() below. > + } else { > + *val = env->hgatp; > + } > + You can discard the 'else' since you're doing a return in the if: if (...) { return RISCV_EXCP_ILLEGAL_INST; } *val = env->hgatp; > return RISCV_EXCP_NONE; > } > > static RISCVException write_hgatp(CPURISCVState *env, int csrno, > target_ulong val) > { > - env->hgatp = val; > + if (env->priv == PRV_S && get_field(env->mstatus, MSTATUS_TVM)) { > + return RISCV_EXCP_ILLEGAL_INST; > + } else { > + env->hgatp = val; > + } No need for else here either: if (...) { return RISCV_EXCP_ILLEGAL_INST; } env->hgatp = val; Thanks, Daniel > + > return RISCV_EXCP_NONE; > } >
> -----Original Messages----- > From: "Daniel Henrique Barboza" <dbarboza@ventanamicro.com> > Sent Time: 2023-03-09 03:44:03 (Thursday) > To: chenyi2000@zju.edu.cn, qemu-devel@nongnu.org > Cc: "Palmer Dabbelt" <palmer@dabbelt.com>, "Alistair Francis" <alistair.francis@wdc.com>, "Bin Meng" <bin.meng@windriver.com>, "Weiwei Li" <liweiwei@iscas.ac.cn>, "Liu Zhiwei" <zhiwei_liu@linux.alibaba.com>, "open list:RISC-V TCG CPUs" <qemu-riscv@nongnu.org> > Subject: Re: [PATCH] target/riscv/csr.c: fix H extension TVM trap > > > > On 3/8/23 09:34, chenyi2000@zju.edu.cn wrote: > > From: Yi Chen <chenyi2000@zju.edu.cn> > > > > Trap accesses to hgatp if MSTATUS_TVM is enabled. > > Don't trap accesses to vsatp even if MSTATUS_TVM is enabled. > > > > Signed-off-by: Yi Chen <chenyi2000@zju.edu.cn> > > --- > > target/riscv/csr.c | 18 ++++++++++++++---- > > 1 file changed, 14 insertions(+), 4 deletions(-) > > > > diff --git a/target/riscv/csr.c b/target/riscv/csr.c > > index ab56663..09bc780 100644 > > --- a/target/riscv/csr.c > > +++ b/target/riscv/csr.c > > @@ -2655,7 +2655,7 @@ static RISCVException read_satp(CPURISCVState *env, int csrno, > > return RISCV_EXCP_NONE; > > } > > > > - if (env->priv == PRV_S && get_field(env->mstatus, MSTATUS_TVM)) { > > + if (env->priv == PRV_S && !riscv_cpu_virt_enabled(env) && get_field(env->mstatus, MSTATUS_TVM)) { > > The commit message mentions 'vsatp' but this patch is changing satp callbacks. > > Any reason to not change read_vsatp() and write_vsatp() instead? read_vsatp() and write_vsatp() have correctly implemented the behavior of MSTATUS.TVM. Meanwhile, if an HS-mode hart tries to access 'satp', what it actually accesses is 'vsatp' according to the ISA. In Qemu's implementation, the 'satp' callbacks are called at first, and riscv_cpu_swap_hypervisor_regs() will be called afterward. So we also need to modify read_satp() and write_satp(). > > > return RISCV_EXCP_ILLEGAL_INST; > > } else { > > *val = env->satp; > > @@ -2683,7 +2683,7 @@ static RISCVException write_satp(CPURISCVState *env, int csrno, > > } > > > > if (vm && mask) { > > - if (env->priv == PRV_S && get_field(env->mstatus, MSTATUS_TVM)) { > > + if (env->priv == PRV_S && !riscv_cpu_virt_enabled(env) && get_field(env->mstatus, MSTATUS_TVM)) { > > return RISCV_EXCP_ILLEGAL_INST; > > } else { > > /* > > @@ -3047,14 +3047,24 @@ static RISCVException read_hgeip(CPURISCVState *env, int csrno, > > static RISCVException read_hgatp(CPURISCVState *env, int csrno, > > target_ulong *val) > > { > > - *val = env->hgatp; > > + if (env->priv == PRV_S && get_field(env->mstatus, MSTATUS_TVM)) { > > + return RISCV_EXCP_ILLEGAL_INST; > > The end of the first paragraph of ISA 8.2.10 goes as follows: > > ==== > When mstatus.TVM=1, attempts to read or write hgatp while executing > in HS-mode will raise an illegal instruction exception. > ==== > > I believe you need to check for HS-mode, not just PRV_S. riscv_csrrw_check() in > target/riscv/csr.c checks for HS-mode as follows: > > if (riscv_has_ext(env, RVH) && env->priv == PRV_S && > !riscv_cpu_virt_enabled(env)) { > > Same goes for write_hgatp() below. > > > + } else { > > + *val = env->hgatp; > > + } > > + > I think VS-mode can't access HS-mode CSR registers, which has been ensured in riscv_csrrw_check(). You can see other callbacks of HS-mode CSR registers (e.g., read_hgeip()) assume that it's M-mode or HS-mode, too. > You can discard the 'else' since you're doing a return in the if: > > if (...) { > return RISCV_EXCP_ILLEGAL_INST; > } > > *val = env->hgatp; > > > > return RISCV_EXCP_NONE; > > } > > > > static RISCVException write_hgatp(CPURISCVState *env, int csrno, > > target_ulong val) > > { > > - env->hgatp = val; > > + if (env->priv == PRV_S && get_field(env->mstatus, MSTATUS_TVM)) { > > + return RISCV_EXCP_ILLEGAL_INST; > > + } else { > > + env->hgatp = val; > > + } > > No need for else here either: > > if (...) { > return RISCV_EXCP_ILLEGAL_INST; > } > > env->hgatp = val; > > I see. I will fix that in the next version of this patch. > > Thanks, > > > Daniel > > > + > > return RISCV_EXCP_NONE; > > } > > Thanks! Yi
© 2016 - 2024 Red Hat, Inc.