[PATCH] target/riscv/csr.c: fix H extension TVM trap

[PATCH] target/riscv/csr.c: fix H extension TVM trap

[chenyi2000]

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

Re: [PATCH] target/riscv/csr.c: fix H extension TVM trap

[Daniel Henrique Barboza]

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;
>   }
>   

Re: Re: [PATCH] target/riscv/csr.c: fix H extension TVM trap

[CHEN Yi]

> -----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

Re: [PATCH] target/riscv/csr.c: fix H extension TVM trap

[liweiwei]

[-- 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 --]

Re: Re: [PATCH] target/riscv/csr.c: fix H extension TVM trap

[CHEN Yi]

[-- 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 --]

Re: [PATCH] target/riscv/csr.c: fix H extension TVM trap

[liweiwei]

[-- 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 --]

Re: [PATCH] target/riscv/csr.c: fix H extension TVM trap

[LIU Zhiwei]

[-- 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 --]

Re: Re: [PATCH] target/riscv/csr.c: fix H extension TVM trap

[CHEN Yi]

[-- 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 --]

Re: [PATCH] target/riscv/csr.c: fix H extension TVM trap

[LIU Zhiwei]

[-- 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 --]

Re: Re: [PATCH] target/riscv/csr.c: fix H extension TVM trap

[CHEN Yi]

[-- 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 --]