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

[Alex Bennée]

Michael Clark <address@hidden> writes:

> On Tue, Mar 27, 2018 at 3:17 PM, Philippe Mathieu-Daudé <address@hidden>
> wrote:
>
>> 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, */
>>
>
> On looking at this again, I think we may need to remove the
> qemu_tcg_mttcg_enabled conditional and always return dirty if the state is
> initial or clean, but not off.
>
> While testing on uniprocessor worked okay, it's likely because we were
> lucky and there was no task migration or multiple FPU tasks working. This
> would mean we would revert to Richard W.M. Jones initial patch.
>
>> +        if (qemu_tcg_mttcg_enabled()) {
>> > +            /* FP is always dirty or off */
>> > +            if (mstatus & MSTATUS_FS) {
>> > +                mstatus |= MSTATUS_FS;
>> > +            }
>> > +        }


I'm confused. If mstatus is a per-vCPU variable why does the enabling or
not of MTTCG matter here?

>> > +
>> > +        int dirty = ((mstatus & MSTATUS_FS) == MSTATUS_FS) |
>> > +                    ((mstatus & MSTATUS_XS) == MSTATUS_XS);
>> >          mstatus = set_field(mstatus, MSTATUS_SD, dirty);
>> >          env->mstatus = mstatus;
>> >          break;
>> >
>>
>
> The text from the specification that allows us to always return dirty if
> set to initial or clean, is below i.e. Dirty implies state has
> "potentially" been modified, so that gives us wriggle room.
>
> "
> When an extension's status is set to Off , any instruction that attempts to
> read or write the corresponding
> state will cause an exception. When the status is Initial, the
> corresponding state should
> have an initial constant value. When the status is Clean, the corresponding
> state is potentially
> di fferent from the initial value, but matches the last value stored on a
> context swap. When the
> status is Dirty, the corresponding state has potentially been modif ed
> since the last context save.
> "
>
> I think the problem is Linux is setting the state to clean after saving
> fpu register state [1], but we have no code in the QEMU FPU operations to
> set the state to dirty, if is clean or initial, only code to cause an
> exception if the floating point extension state is set to off e.g.
>
> static void gen_fp_store(DisasContext *ctx, uint32_t opc, int rs1,
>         int rs2, target_long imm)
> {
>     TCGv t0;
>
>     if (!(ctx->flags & TB_FLAGS_FP_ENABLE)) {
>         gen_exception_illegal(ctx);
>         return;
>     }
>
>     t0 = tcg_temp_new();
>     gen_get_gpr(t0, rs1);
>     tcg_gen_addi_tl(t0, t0, imm);
>
>     switch (opc) {
>     case OPC_RISC_FSW:
>         tcg_gen_qemu_st_i64(cpu_fpr[rs2], t0, ctx->mem_idx, MO_TEUL);
>         break;
>     case OPC_RISC_FSD:
>         tcg_gen_qemu_st_i64(cpu_fpr[rs2], t0, ctx->mem_idx, MO_TEQ);
>         break;
>     default:
>         gen_exception_illegal(ctx);
>         break;
>     }
>
>     tcg_temp_free(t0);
> }
>
> [1]
> https://github.com/torvalds/linux/blob/master/arch/riscv/include/asm/switch_to.h


--
Alex Bennée