Re: [Qemu-devel] [PATCH v4 3/7] target/riscv: Create function to test if FP is enabled

[Aurelien Jarno <aurelien@aurel32.net>]

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                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                 http://www.aurel32.net