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

[Michael Clark]

On Fri, Mar 30, 2018 at 12:11 AM, Alex Bennée <address@hidden>
wrote:

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


It was an incorrect inference from the available data at the time given the
problem showed up on SMP but not uniprocessor.

After inspecting the logic we concluded we were just lucky in the
uniprocessor case because our test case only have once process using the
CPU, whereas with SMP the process was rescheduled to another core which
made the corruption visible on SMP.

We removed the MTTCG condition from the subsequent version of the patch,
updated the patch description and comment and removed references to MTTCG.


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