Re: [Qemu-devel] [PATCH v1 1/1] RISC-V: Workaround for critical mstatus.FS MTTCG bug

[Philippe Mathieu-Daudé]

Cc'ing Alex and Richard.

On 03/27/2018 04:54 PM, Michael Clark wrote:
> This change is a workaround for a bug where mstatus.FS
> is not correctly reporting dirty when MTTCG and SMP are
> enabled which results in the floating point register file
> not being saved during context switches. This a critical
> bug for RISC-V in QEMU as it results in floating point
> register file corruption when running SMP Linux in the
> RISC-V 'virt' machine.
> 
> This workaround will return dirty if mstatus.FS is
> switched from off to initial or clean. We have checked
> the specification and it is legal for an implementation
> to return either off, or dirty, if set to initial or clean.
> 
> This workaround will result in unnecessary floating point
> save restore. When mstatus.FS is off, floating point
> instruction trap to indicate the process is using the FPU.
> The OS can then save floating-point state of the previous
> process using the FPU and set mstatus.FS to initial or
> clean. With this workaround, mstatus.FS will always return
> dirty if set to a non-zero value, indicating floating point
> save restore is necessary, versus misreporting mstatus.FS
> resulting in floating point register file corruption.
> 
> Cc: Palmer Dabbelt <address@hidden>
> Cc: Sagar Karandikar <address@hidden>
> Cc: Bastian Koppelmann <address@hidden>
> Cc: Peter Maydell <address@hidden>
> Tested-by: Richard W.M. Jones <address@hidden>
> Signed-off-by: Michael Clark <address@hidden>
> ---
>  target/riscv/op_helper.c | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
> index e34715d..7281b98 100644
> --- a/target/riscv/op_helper.c
> +++ b/target/riscv/op_helper.c
> @@ -144,8 +144,23 @@ void csr_write_helper(CPURISCVState *env, target_ulong 
> val_to_write,
>          }
>  
>          mstatus = (mstatus & ~mask) | (val_to_write & mask);
> -        int dirty = (mstatus & MSTATUS_FS) == MSTATUS_FS;
> -        dirty |= (mstatus & MSTATUS_XS) == MSTATUS_XS;
> +
> +        /* Note: this is a workaround for an issue where mstatus.FS
> +           does not report dirty when SMP and MTTCG is enabled. This
> +           workaround is technically compliant with the RISC-V Privileged
> +           specification as it is legal to return only off, or dirty,
> +           however this may cause unnecessary saves of floating point state.
> +           Without this workaround, floating point state is not saved and
> +           restored correctly when SMP and MTTCG is enabled, */
> +        if (qemu_tcg_mttcg_enabled()) {
> +            /* FP is always dirty or off */
> +            if (mstatus & MSTATUS_FS) {
> +                mstatus |= MSTATUS_FS;
> +            }
> +        }
> +
> +        int dirty = ((mstatus & MSTATUS_FS) == MSTATUS_FS) |
> +                    ((mstatus & MSTATUS_XS) == MSTATUS_XS);
>          mstatus = set_field(mstatus, MSTATUS_SD, dirty);
>          env->mstatus = mstatus;
>          break;
>