diff mbox series

[v4,3/7] target/riscv: Create function to test if FP is enabled

Message ID 5cc26abb98a9534720f09674b4b9caafb8f2cf0a.1566573576.git.alistair.francis@wdc.com (mailing list archive)
State New, archived
Headers show
Series RISC-V: Hypervisor prep work part 2 | expand

Commit Message

Alistair Francis Aug. 23, 2019, 3:21 p.m. UTC 
Let's create a function that tests if floating point support is
enabled. We can then protect all floating point operations based on if
they are enabled.

This patch so far doesn't change anything, it's just preparing for the
Hypervisor support for floating point operations.

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Christophe de Dinechin <dinechin@redhat.com>
Reviewed-by: Chih-Min Chao <chihmin.chao@sifive.com>
Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
---
 target/riscv/cpu.h        |  6 +++++-
 target/riscv/cpu_helper.c | 10 ++++++++++
 target/riscv/csr.c        | 20 +++++++++++---------
 3 files changed, 26 insertions(+), 10 deletions(-)

Comments

Aurelien Jarno Jan. 5, 2020, 4:36 p.m. UTC | #1 
Hi,

On 2019-08-23 08:21, Alistair Francis wrote:
> Let's create a function that tests if floating point support is
> enabled. We can then protect all floating point operations based on if
> they are enabled.
> 
> This patch so far doesn't change anything, it's just preparing for the
> Hypervisor support for floating point operations.
> 
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Reviewed-by: Christophe de Dinechin <dinechin@redhat.com>
> Reviewed-by: Chih-Min Chao <chihmin.chao@sifive.com>
> Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
> ---
>  target/riscv/cpu.h        |  6 +++++-
>  target/riscv/cpu_helper.c | 10 ++++++++++
>  target/riscv/csr.c        | 20 +++++++++++---------
>  3 files changed, 26 insertions(+), 10 deletions(-)
> 

[ snip ]

> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index e0d4586760..2789215b5e 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c

[ snip ]

> @@ -307,6 +307,7 @@ static int write_mstatus(CPURISCVState *env, int csrno, target_ulong val)
>  {
>      target_ulong mstatus = env->mstatus;
>      target_ulong mask = 0;
> +    int dirty;
>  
>      /* flush tlb on mstatus fields that affect VM */
>      if (env->priv_ver <= PRIV_VERSION_1_09_1) {
> @@ -340,8 +341,9 @@ static int write_mstatus(CPURISCVState *env, int csrno, target_ulong val)
>  
>      mstatus = (mstatus & ~mask) | (val & mask);
>  
> -    int dirty = ((mstatus & MSTATUS_FS) == MSTATUS_FS) |
> -                ((mstatus & MSTATUS_XS) == MSTATUS_XS);
> +    dirty = (riscv_cpu_fp_enabled(env) &&
> +             ((mstatus & MSTATUS_FS) == MSTATUS_FS)) |
> +            ((mstatus & MSTATUS_XS) == MSTATUS_XS);
>      mstatus = set_field(mstatus, MSTATUS_SD, dirty);
>      env->mstatus = mstatus;

This patch, and more precisely the above two hunks broke
qemu-system-riscv64. More precisely, when running a Debian sid system
inside QEMU, sshd hangs during key exchange.

Reverting this commit "fixes" the issue up to the following commit which
breaks things again:

| commit bdce1a5c6d512257f83b6b6831bee2c975643bbd
| Author: Alistair Francis <alistair.francis@wdc.com>
| Date:   Fri Aug 23 08:21:25 2019 -0700
| 
|     target/riscv: Use TB_FLAGS_MSTATUS_FS for floating point
| 
|     Use the TB_FLAGS_MSTATUS_FS macro when enabling floating point in the tb
|     flags.
| 
|     Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
|     Reviewed-by: Palmer Dabbelt <palmer@sifive.com>
|     Signed-off-by: Palmer Dabbelt <palmer@sifive.com>

I wonder if the issue is related to the fact that MSTATUS_FS and thus
TB_FLAGS_MSTATUS_FS actually consist in 2 bits and are not a simple
flag.

Overall I am able to get QEMU v4.2 working again by applying the
following patch:

diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index e59343e13c..f0ff57e27a 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -295,7 +295,7 @@ static inline void cpu_get_tb_cpu_state(CPURISCVState *env, target_ulong *pc,
 #else
     *flags = cpu_mmu_index(env, 0);
     if (riscv_cpu_fp_enabled(env)) {
-        *flags |= TB_FLAGS_MSTATUS_FS;
+        *flags |= env->mstatus & MSTATUS_FS;
     }
 #endif
 }
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index da02f9f0b1..1754c6c575 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -307,7 +307,6 @@ static int write_mstatus(CPURISCVState *env, int csrno, target_ulong val)
 {
     target_ulong mstatus = env->mstatus;
     target_ulong mask = 0;
-    int dirty;
 
     /* flush tlb on mstatus fields that affect VM */
     if (env->priv_ver <= PRIV_VERSION_1_09_1) {
@@ -341,9 +340,8 @@ static int write_mstatus(CPURISCVState *env, int csrno, target_ulong val)
 
     mstatus = (mstatus & ~mask) | (val & mask);
 
-    dirty = (riscv_cpu_fp_enabled(env) &&
-             ((mstatus & MSTATUS_FS) == MSTATUS_FS)) |
-            ((mstatus & MSTATUS_XS) == MSTATUS_XS);
+    int dirty = ((mstatus & MSTATUS_FS) == MSTATUS_FS) |
+                ((mstatus & MSTATUS_XS) == MSTATUS_XS);
     mstatus = set_field(mstatus, MSTATUS_SD, dirty);
     env->mstatus = mstatus;
Aurelien Jarno Jan. 5, 2020, 4:59 p.m. UTC | #2 
On 2020-01-05 17:36, Aurelien Jarno wrote:
> > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > index e0d4586760..2789215b5e 100644
> > --- a/target/riscv/csr.c
> > +++ b/target/riscv/csr.c
> 
> [ snip ]
> 
> > @@ -307,6 +307,7 @@ static int write_mstatus(CPURISCVState *env, int csrno, target_ulong val)
> >  {
> >      target_ulong mstatus = env->mstatus;
> >      target_ulong mask = 0;
> > +    int dirty;
> >  
> >      /* flush tlb on mstatus fields that affect VM */
> >      if (env->priv_ver <= PRIV_VERSION_1_09_1) {
> > @@ -340,8 +341,9 @@ static int write_mstatus(CPURISCVState *env, int csrno, target_ulong val)
> >  
> >      mstatus = (mstatus & ~mask) | (val & mask);
> >  
> > -    int dirty = ((mstatus & MSTATUS_FS) == MSTATUS_FS) |
> > -                ((mstatus & MSTATUS_XS) == MSTATUS_XS);
> > +    dirty = (riscv_cpu_fp_enabled(env) &&
> > +             ((mstatus & MSTATUS_FS) == MSTATUS_FS)) |
> > +            ((mstatus & MSTATUS_XS) == MSTATUS_XS);
> >      mstatus = set_field(mstatus, MSTATUS_SD, dirty);
> >      env->mstatus = mstatus;
> 
> This patch, and more precisely the above two hunks broke
> qemu-system-riscv64. More precisely, when running a Debian sid system
> inside QEMU, sshd hangs during key exchange.

The problem is that at this stage, mstatus != env->status. Prior to that
patch, dirty was computed exclusively on the new mstatus status, after
the update by val. With this patch, riscv_cpu_fp_enabled() refers to the
old value of mstatus. Therefore when FS is changed from "Off" (FS = 00)
to "Dirty" (FS == 11), the SD bit is not set.
Alistair Francis Jan. 20, 2020, 12:31 a.m. UTC | #3 
On Mon, Jan 6, 2020 at 2:59 AM Aurelien Jarno <aurelien@aurel32.net> wrote:
>
> On 2020-01-05 17:36, Aurelien Jarno wrote:
> > > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > > index e0d4586760..2789215b5e 100644
> > > --- a/target/riscv/csr.c
> > > +++ b/target/riscv/csr.c
> >
> > [ snip ]
> >
> > > @@ -307,6 +307,7 @@ static int write_mstatus(CPURISCVState *env, int csrno, target_ulong val)
> > >  {
> > >      target_ulong mstatus = env->mstatus;
> > >      target_ulong mask = 0;
> > > +    int dirty;
> > >
> > >      /* flush tlb on mstatus fields that affect VM */
> > >      if (env->priv_ver <= PRIV_VERSION_1_09_1) {
> > > @@ -340,8 +341,9 @@ static int write_mstatus(CPURISCVState *env, int csrno, target_ulong val)
> > >
> > >      mstatus = (mstatus & ~mask) | (val & mask);
> > >
> > > -    int dirty = ((mstatus & MSTATUS_FS) == MSTATUS_FS) |
> > > -                ((mstatus & MSTATUS_XS) == MSTATUS_XS);
> > > +    dirty = (riscv_cpu_fp_enabled(env) &&
> > > +             ((mstatus & MSTATUS_FS) == MSTATUS_FS)) |
> > > +            ((mstatus & MSTATUS_XS) == MSTATUS_XS);
> > >      mstatus = set_field(mstatus, MSTATUS_SD, dirty);
> > >      env->mstatus = mstatus;
> >
> > This patch, and more precisely the above two hunks broke
> > qemu-system-riscv64. More precisely, when running a Debian sid system
> > inside QEMU, sshd hangs during key exchange.
>
> The problem is that at this stage, mstatus != env->status. Prior to that
> patch, dirty was computed exclusively on the new mstatus status, after
> the update by val. With this patch, riscv_cpu_fp_enabled() refers to the
> old value of mstatus. Therefore when FS is changed from "Off" (FS = 00)
> to "Dirty" (FS == 11), the SD bit is not set.

Thanks for reporting this!

Can you try this branch (it should be a PR to mainline QEMU soon) and
let me know if that fixes the issue?

https://github.com/palmer-dabbelt/qemu/commits/for-master

Alistair

>
> --
> Aurelien Jarno                          GPG: 4096R/1DDD8C9B
> aurelien@aurel32.net                 http://www.aurel32.net
Aurelien Jarno Jan. 21, 2020, 8:37 p.m. UTC | #4 
Hi,

On 2020-01-20 10:31, Alistair Francis wrote:
> On Mon, Jan 6, 2020 at 2:59 AM Aurelien Jarno <aurelien@aurel32.net> wrote:
> >
> > On 2020-01-05 17:36, Aurelien Jarno wrote:
> > > > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > > > index e0d4586760..2789215b5e 100644
> > > > --- a/target/riscv/csr.c
> > > > +++ b/target/riscv/csr.c
> > >
> > > [ snip ]
> > >
> > > > @@ -307,6 +307,7 @@ static int write_mstatus(CPURISCVState *env, int csrno, target_ulong val)
> > > >  {
> > > >      target_ulong mstatus = env->mstatus;
> > > >      target_ulong mask = 0;
> > > > +    int dirty;
> > > >
> > > >      /* flush tlb on mstatus fields that affect VM */
> > > >      if (env->priv_ver <= PRIV_VERSION_1_09_1) {
> > > > @@ -340,8 +341,9 @@ static int write_mstatus(CPURISCVState *env, int csrno, target_ulong val)
> > > >
> > > >      mstatus = (mstatus & ~mask) | (val & mask);
> > > >
> > > > -    int dirty = ((mstatus & MSTATUS_FS) == MSTATUS_FS) |
> > > > -                ((mstatus & MSTATUS_XS) == MSTATUS_XS);
> > > > +    dirty = (riscv_cpu_fp_enabled(env) &&
> > > > +             ((mstatus & MSTATUS_FS) == MSTATUS_FS)) |
> > > > +            ((mstatus & MSTATUS_XS) == MSTATUS_XS);
> > > >      mstatus = set_field(mstatus, MSTATUS_SD, dirty);
> > > >      env->mstatus = mstatus;
> > >
> > > This patch, and more precisely the above two hunks broke
> > > qemu-system-riscv64. More precisely, when running a Debian sid system
> > > inside QEMU, sshd hangs during key exchange.
> >
> > The problem is that at this stage, mstatus != env->status. Prior to that
> > patch, dirty was computed exclusively on the new mstatus status, after
> > the update by val. With this patch, riscv_cpu_fp_enabled() refers to the
> > old value of mstatus. Therefore when FS is changed from "Off" (FS = 00)
> > to "Dirty" (FS == 11), the SD bit is not set.
> 
> Thanks for reporting this!
> 
> Can you try this branch (it should be a PR to mainline QEMU soon) and
> let me know if that fixes the issue?
> 
> https://github.com/palmer-dabbelt/qemu/commits/for-master

Thanks for the patchset. I confirm this fixes the issue.

Aurelien
Alistair Francis Jan. 21, 2020, 10:20 p.m. UTC | #5 
On Wed, Jan 22, 2020 at 6:37 AM Aurelien Jarno <aurelien@aurel32.net> wrote:
>
> Hi,
>
> On 2020-01-20 10:31, Alistair Francis wrote:
> > On Mon, Jan 6, 2020 at 2:59 AM Aurelien Jarno <aurelien@aurel32.net> wrote:
> > >
> > > On 2020-01-05 17:36, Aurelien Jarno wrote:
> > > > > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > > > > index e0d4586760..2789215b5e 100644
> > > > > --- a/target/riscv/csr.c
> > > > > +++ b/target/riscv/csr.c
> > > >
> > > > [ snip ]
> > > >
> > > > > @@ -307,6 +307,7 @@ static int write_mstatus(CPURISCVState *env, int csrno, target_ulong val)
> > > > >  {
> > > > >      target_ulong mstatus = env->mstatus;
> > > > >      target_ulong mask = 0;
> > > > > +    int dirty;
> > > > >
> > > > >      /* flush tlb on mstatus fields that affect VM */
> > > > >      if (env->priv_ver <= PRIV_VERSION_1_09_1) {
> > > > > @@ -340,8 +341,9 @@ static int write_mstatus(CPURISCVState *env, int csrno, target_ulong val)
> > > > >
> > > > >      mstatus = (mstatus & ~mask) | (val & mask);
> > > > >
> > > > > -    int dirty = ((mstatus & MSTATUS_FS) == MSTATUS_FS) |
> > > > > -                ((mstatus & MSTATUS_XS) == MSTATUS_XS);
> > > > > +    dirty = (riscv_cpu_fp_enabled(env) &&
> > > > > +             ((mstatus & MSTATUS_FS) == MSTATUS_FS)) |
> > > > > +            ((mstatus & MSTATUS_XS) == MSTATUS_XS);
> > > > >      mstatus = set_field(mstatus, MSTATUS_SD, dirty);
> > > > >      env->mstatus = mstatus;
> > > >
> > > > This patch, and more precisely the above two hunks broke
> > > > qemu-system-riscv64. More precisely, when running a Debian sid system
> > > > inside QEMU, sshd hangs during key exchange.
> > >
> > > The problem is that at this stage, mstatus != env->status. Prior to that
> > > patch, dirty was computed exclusively on the new mstatus status, after
> > > the update by val. With this patch, riscv_cpu_fp_enabled() refers to the
> > > old value of mstatus. Therefore when FS is changed from "Off" (FS = 00)
> > > to "Dirty" (FS == 11), the SD bit is not set.
> >
> > Thanks for reporting this!
> >
> > Can you try this branch (it should be a PR to mainline QEMU soon) and
> > let me know if that fixes the issue?
> >
> > https://github.com/palmer-dabbelt/qemu/commits/for-master
>
> Thanks for the patchset. I confirm this fixes the issue.

Great! Sorry we were so slow in replying to you, I was traveling.
Hopefully this is pushed to master soon.

Alistair

>
> Aurelien
>
> --
> Aurelien Jarno                          GPG: 4096R/1DDD8C9B
> aurelien@aurel32.net                 http://www.aurel32.net
diff mbox series

Patch

diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 240b31e2eb..eb7b5b0af3 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -255,6 +255,7 @@  void riscv_cpu_do_interrupt(CPUState *cpu);
 int riscv_cpu_gdb_read_register(CPUState *cpu, uint8_t *buf, int reg);
 int riscv_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
 bool riscv_cpu_exec_interrupt(CPUState *cs, int interrupt_request);
+bool riscv_cpu_fp_enabled(CPURISCVState *env);
 int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch);
 hwaddr riscv_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr);
 void  riscv_cpu_do_unaligned_access(CPUState *cs, vaddr addr,
@@ -298,7 +299,10 @@  static inline void cpu_get_tb_cpu_state(CPURISCVState *env, target_ulong *pc,
 #ifdef CONFIG_USER_ONLY
     *flags = TB_FLAGS_MSTATUS_FS;
 #else
-    *flags = cpu_mmu_index(env, 0) | (env->mstatus & MSTATUS_FS);
+    *flags = cpu_mmu_index(env, 0);
+    if (riscv_cpu_fp_enabled(env)) {
+        *flags |= env->mstatus & MSTATUS_FS;
+    }
 #endif
 }
 
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index f027be7f16..225e407cff 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -71,6 +71,16 @@  bool riscv_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
 
 #if !defined(CONFIG_USER_ONLY)
 
+/* Return true is floating point support is currently enabled */
+bool riscv_cpu_fp_enabled(CPURISCVState *env)
+{
+    if (env->mstatus & MSTATUS_FS) {
+        return true;
+    }
+
+    return false;
+}
+
 int riscv_cpu_claim_interrupts(RISCVCPU *cpu, uint32_t interrupts)
 {
     CPURISCVState *env = &cpu->env;
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index e0d4586760..2789215b5e 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -46,7 +46,7 @@  void riscv_set_csr_ops(int csrno, riscv_csr_operations *ops)
 static int fs(CPURISCVState *env, int csrno)
 {
 #if !defined(CONFIG_USER_ONLY)
-    if (!env->debugger && !(env->mstatus & MSTATUS_FS)) {
+    if (!env->debugger && !riscv_cpu_fp_enabled(env)) {
         return -1;
     }
 #endif
@@ -108,7 +108,7 @@  static int pmp(CPURISCVState *env, int csrno)
 static int read_fflags(CPURISCVState *env, int csrno, target_ulong *val)
 {
 #if !defined(CONFIG_USER_ONLY)
-    if (!env->debugger && !(env->mstatus & MSTATUS_FS)) {
+    if (!env->debugger && !riscv_cpu_fp_enabled(env)) {
         return -1;
     }
 #endif
@@ -119,7 +119,7 @@  static int read_fflags(CPURISCVState *env, int csrno, target_ulong *val)
 static int write_fflags(CPURISCVState *env, int csrno, target_ulong val)
 {
 #if !defined(CONFIG_USER_ONLY)
-    if (!env->debugger && !(env->mstatus & MSTATUS_FS)) {
+    if (!env->debugger && !riscv_cpu_fp_enabled(env)) {
         return -1;
     }
     env->mstatus |= MSTATUS_FS;
@@ -131,7 +131,7 @@  static int write_fflags(CPURISCVState *env, int csrno, target_ulong val)
 static int read_frm(CPURISCVState *env, int csrno, target_ulong *val)
 {
 #if !defined(CONFIG_USER_ONLY)
-    if (!env->debugger && !(env->mstatus & MSTATUS_FS)) {
+    if (!env->debugger && !riscv_cpu_fp_enabled(env)) {
         return -1;
     }
 #endif
@@ -142,7 +142,7 @@  static int read_frm(CPURISCVState *env, int csrno, target_ulong *val)
 static int write_frm(CPURISCVState *env, int csrno, target_ulong val)
 {
 #if !defined(CONFIG_USER_ONLY)
-    if (!env->debugger && !(env->mstatus & MSTATUS_FS)) {
+    if (!env->debugger && !riscv_cpu_fp_enabled(env)) {
         return -1;
     }
     env->mstatus |= MSTATUS_FS;
@@ -154,7 +154,7 @@  static int write_frm(CPURISCVState *env, int csrno, target_ulong val)
 static int read_fcsr(CPURISCVState *env, int csrno, target_ulong *val)
 {
 #if !defined(CONFIG_USER_ONLY)
-    if (!env->debugger && !(env->mstatus & MSTATUS_FS)) {
+    if (!env->debugger && !riscv_cpu_fp_enabled(env)) {
         return -1;
     }
 #endif
@@ -166,7 +166,7 @@  static int read_fcsr(CPURISCVState *env, int csrno, target_ulong *val)
 static int write_fcsr(CPURISCVState *env, int csrno, target_ulong val)
 {
 #if !defined(CONFIG_USER_ONLY)
-    if (!env->debugger && !(env->mstatus & MSTATUS_FS)) {
+    if (!env->debugger && !riscv_cpu_fp_enabled(env)) {
         return -1;
     }
     env->mstatus |= MSTATUS_FS;
@@ -307,6 +307,7 @@  static int write_mstatus(CPURISCVState *env, int csrno, target_ulong val)
 {
     target_ulong mstatus = env->mstatus;
     target_ulong mask = 0;
+    int dirty;
 
     /* flush tlb on mstatus fields that affect VM */
     if (env->priv_ver <= PRIV_VERSION_1_09_1) {
@@ -340,8 +341,9 @@  static int write_mstatus(CPURISCVState *env, int csrno, target_ulong val)
 
     mstatus = (mstatus & ~mask) | (val & mask);
 
-    int dirty = ((mstatus & MSTATUS_FS) == MSTATUS_FS) |
-                ((mstatus & MSTATUS_XS) == MSTATUS_XS);
+    dirty = (riscv_cpu_fp_enabled(env) &&
+             ((mstatus & MSTATUS_FS) == MSTATUS_FS)) |
+            ((mstatus & MSTATUS_XS) == MSTATUS_XS);
     mstatus = set_field(mstatus, MSTATUS_SD, dirty);
     env->mstatus = mstatus;