qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Michael Clark
Subject: Re: [Qemu-devel] [PATCH v1 1/1] RISC-V: Workaround for critical mstatus.FS MTTCG bug
Date: Tue, 27 Mar 2018 17:15:41 -0700

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


reply via email to

[Prev in Thread] Current Thread [Next in Thread]