* [PATCH] target/riscv/csr.c: fix H extension TVM trap
@ 2023-03-08 12:34 chenyi2000
2023-03-08 19:44 ` Daniel Henrique Barboza
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: chenyi2000 @ 2023-03-08 12:34 UTC (permalink / raw)
To: qemu-devel
Cc: Yi Chen, Palmer Dabbelt, Alistair Francis, Bin Meng, Weiwei Li,
Daniel Henrique Barboza, Liu Zhiwei, open list:RISC-V TCG CPUs
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] target/riscv/csr.c: fix H extension TVM trap
2023-03-08 12:34 [PATCH] target/riscv/csr.c: fix H extension TVM trap chenyi2000
@ 2023-03-08 19:44 ` Daniel Henrique Barboza
2023-03-09 14:42 ` CHEN Yi
2023-03-09 7:48 ` liweiwei
2023-03-10 2:12 ` LIU Zhiwei
2 siblings, 1 reply; 10+ messages in thread
From: Daniel Henrique Barboza @ 2023-03-08 19:44 UTC (permalink / raw)
To: chenyi2000, qemu-devel
Cc: Palmer Dabbelt, Alistair Francis, Bin Meng, Weiwei Li, Liu Zhiwei,
open list:RISC-V TCG CPUs
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;
> }
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] target/riscv/csr.c: fix H extension TVM trap
2023-03-08 12:34 [PATCH] target/riscv/csr.c: fix H extension TVM trap chenyi2000
2023-03-08 19:44 ` Daniel Henrique Barboza
@ 2023-03-09 7:48 ` liweiwei
2023-03-09 15:02 ` CHEN Yi
2023-03-10 2:12 ` LIU Zhiwei
2 siblings, 1 reply; 10+ messages in thread
From: liweiwei @ 2023-03-09 7:48 UTC (permalink / raw)
To: chenyi2000, qemu-devel
Cc: Palmer Dabbelt, Alistair Francis, Bin Meng,
Daniel Henrique Barboza, Liu Zhiwei, open list:RISC-V TCG CPUs
[-- Attachment #1: Type: text/plain, Size: 2490 bytes --]
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;
> }
>
[-- Attachment #2: Type: text/html, Size: 3536 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Re: [PATCH] target/riscv/csr.c: fix H extension TVM trap
2023-03-08 19:44 ` Daniel Henrique Barboza
@ 2023-03-09 14:42 ` CHEN Yi
0 siblings, 0 replies; 10+ messages in thread
From: CHEN Yi @ 2023-03-09 14:42 UTC (permalink / raw)
To: Daniel Henrique Barboza, qemu-devel
Cc: Palmer Dabbelt, Alistair Francis, Bin Meng, Weiwei Li, Liu Zhiwei,
open list:RISC-V TCG CPUs
> -----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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Re: [PATCH] target/riscv/csr.c: fix H extension TVM trap
2023-03-09 7:48 ` liweiwei
@ 2023-03-09 15:02 ` CHEN Yi
2023-03-09 15:27 ` liweiwei
0 siblings, 1 reply; 10+ messages in thread
From: CHEN Yi @ 2023-03-09 15:02 UTC (permalink / raw)
To: liweiwei, qemu-devel
Cc: Palmer Dabbelt, Alistair Francis, Bin Meng,
Daniel Henrique Barboza, Liu Zhiwei, open list:RISC-V TCG CPUs
[-- Attachment #1: Type: text/plain, Size: 3267 bytes --]
-----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
[-- Attachment #2: Type: text/html, Size: 5222 bytes --]
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] target/riscv/csr.c: fix H extension TVM trap
2023-03-09 15:02 ` CHEN Yi
@ 2023-03-09 15:27 ` liweiwei
0 siblings, 0 replies; 10+ messages in thread
From: liweiwei @ 2023-03-09 15:27 UTC (permalink / raw)
To: CHEN Yi, qemu-devel
Cc: liweiwei, Palmer Dabbelt, Alistair Francis, Bin Meng,
Daniel Henrique Barboza, Liu Zhiwei, open list:RISC-V TCG CPUs
[-- Attachment #1: Type: text/plain, Size: 4047 bytes --]
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
>
[-- Attachment #2: Type: text/html, Size: 7420 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] target/riscv/csr.c: fix H extension TVM trap
2023-03-08 12:34 [PATCH] target/riscv/csr.c: fix H extension TVM trap chenyi2000
2023-03-08 19:44 ` Daniel Henrique Barboza
2023-03-09 7:48 ` liweiwei
@ 2023-03-10 2:12 ` LIU Zhiwei
2023-03-10 9:08 ` CHEN Yi
2 siblings, 1 reply; 10+ messages in thread
From: LIU Zhiwei @ 2023-03-10 2:12 UTC (permalink / raw)
To: chenyi2000, qemu-devel
Cc: Palmer Dabbelt, Alistair Francis, Bin Meng, Weiwei Li,
Daniel Henrique Barboza, open list:RISC-V TCG CPUs
[-- Attachment #1: Type: text/plain, Size: 2587 bytes --]
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;
> }
>
[-- Attachment #2: Type: text/html, Size: 3412 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Re: [PATCH] target/riscv/csr.c: fix H extension TVM trap
2023-03-10 2:12 ` LIU Zhiwei
@ 2023-03-10 9:08 ` CHEN Yi
2023-03-10 9:18 ` LIU Zhiwei
0 siblings, 1 reply; 10+ messages in thread
From: CHEN Yi @ 2023-03-10 9:08 UTC (permalink / raw)
To: LIU Zhiwei, qemu-devel
Cc: Palmer Dabbelt, Alistair Francis, Bin Meng, Weiwei Li,
Daniel Henrique Barboza, open list:RISC-V TCG CPUs
[-- Attachment #1: Type: text/plain, Size: 3339 bytes --]
-----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;
}
[-- Attachment #2: Type: text/html, Size: 4766 bytes --]
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] target/riscv/csr.c: fix H extension TVM trap
2023-03-10 9:08 ` CHEN Yi
@ 2023-03-10 9:18 ` LIU Zhiwei
2023-03-10 10:33 ` CHEN Yi
0 siblings, 1 reply; 10+ messages in thread
From: LIU Zhiwei @ 2023-03-10 9:18 UTC (permalink / raw)
To: CHEN Yi, qemu-devel
Cc: Palmer Dabbelt, Alistair Francis, Bin Meng, Weiwei Li,
Daniel Henrique Barboza, open list:RISC-V TCG CPUs
[-- Attachment #1: Type: text/plain, Size: 4023 bytes --]
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;
>> }
>
[-- Attachment #2: Type: text/html, Size: 6677 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Re: [PATCH] target/riscv/csr.c: fix H extension TVM trap
2023-03-10 9:18 ` LIU Zhiwei
@ 2023-03-10 10:33 ` CHEN Yi
0 siblings, 0 replies; 10+ messages in thread
From: CHEN Yi @ 2023-03-10 10:33 UTC (permalink / raw)
To: LIU Zhiwei, qemu-devel
Cc: Palmer Dabbelt, Alistair Francis, Bin Meng, Weiwei Li,
Daniel Henrique Barboza, open list:RISC-V TCG CPUs
[-- Attachment #1: Type: text/plain, Size: 4139 bytes --]
-----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;
}
[-- Attachment #2: Type: text/html, Size: 7478 bytes --]
^ permalink raw reply related [flat|nested] 10+ messages in thread
end of thread, other threads:[~2023-03-10 10:35 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-03-08 12:34 [PATCH] target/riscv/csr.c: fix H extension TVM trap chenyi2000
2023-03-08 19:44 ` Daniel Henrique Barboza
2023-03-09 14:42 ` CHEN Yi
2023-03-09 7:48 ` liweiwei
2023-03-09 15:02 ` CHEN Yi
2023-03-09 15:27 ` liweiwei
2023-03-10 2:12 ` LIU Zhiwei
2023-03-10 9:08 ` CHEN Yi
2023-03-10 9:18 ` LIU Zhiwei
2023-03-10 10:33 ` CHEN Yi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).