| Message ID | 1579069053-22190-1-git-send-email-shihpo.hung@sifive.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v3,1/3] target/riscv: Fix tb->flags FS status | expand |
On Wed, Jan 15, 2020 at 4:18 PM <shihpo.hung@sifive.com> wrote: > > It was found that running libquantum on riscv-linux qemu produced an > incorrect result. After investigation, FP registers are not saved > during context switch due to incorrect mstatus.FS. > > In current implementation tb->flags merges all non-disabled state to > dirty. This means the code in mark_fs_dirty in translate.c that > handles initial and clean states is unreachable. > > This patch fixes it and is successfully tested with: > libquantum > > Thanks to Richard for pointing out the actual bug. > > v3: remove the redundant condition > v2: root cause FS problem > > Suggested-by: Richard Henderson <richard.henderson@linaro.org> > Signed-off-by: ShihPo Hung <shihpo.hung@sifive.com> > Reviewed-by: Richard Henderson <richard.henderson@linaro.org> > --- > target/riscv/cpu.h | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) > > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h > index e59343e..de0a8d8 100644 > --- a/target/riscv/cpu.h > +++ b/target/riscv/cpu.h > @@ -293,10 +293,7 @@ static inline void cpu_get_tb_cpu_state(CPURISCVState *env, target_ulong *pc, > #ifdef CONFIG_USER_ONLY > *flags = TB_FLAGS_MSTATUS_FS; > #else > - *flags = cpu_mmu_index(env, 0); > - if (riscv_cpu_fp_enabled(env)) { > - *flags |= TB_FLAGS_MSTATUS_FS; > - } > + *flags = cpu_mmu_index(env, 0) | (env->mstatus & MSTATUS_FS); I don't think this is right, you should use the riscv_cpu_fp_enabled() function. Right now it's the same as env->mstatus & MSTATUS_FS but when the Hypervisor extension goes in riscv_cpu_fp_enabled() will be more complex. Alistair > #endif > } > > -- > 2.7.4 > >
On Wed, Jan 15, 2020 at 2:29 PM Alistair Francis <alistair23@gmail.com> wrote: > > - *flags = cpu_mmu_index(env, 0); > > - if (riscv_cpu_fp_enabled(env)) { > > - *flags |= TB_FLAGS_MSTATUS_FS; > > - } > > + *flags = cpu_mmu_index(env, 0) | (env->mstatus & MSTATUS_FS); > > I don't think this is right, you should use the riscv_cpu_fp_enabled() > function. > > Right now it's the same as env->mstatus & MSTATUS_FS but when the > Hypervisor extension goes in riscv_cpu_fp_enabled() will be more > complex. > > Alistair > > I agree using riscv_cpu_fp_enabled() to hide the complexity when checking FP, but here I only duplicate the FP status (disabled/initial/clean/dirty) to tb->flags no matter FP is enabled or not. Is it still necessary to check this before duplicating it? I think it is not as long as TB_FLAGS_MSTATUS_FS is equivalent to MSTATUS_FS. But I don't know what changes hypervisor extension brings, please correct me if I am wrong. ShihPo
On 1/14/20 8:28 PM, Alistair Francis wrote: > On Wed, Jan 15, 2020 at 4:18 PM <shihpo.hung@sifive.com> wrote: >> >> It was found that running libquantum on riscv-linux qemu produced an >> incorrect result. After investigation, FP registers are not saved >> during context switch due to incorrect mstatus.FS. >> >> In current implementation tb->flags merges all non-disabled state to >> dirty. This means the code in mark_fs_dirty in translate.c that >> handles initial and clean states is unreachable. >> >> This patch fixes it and is successfully tested with: >> libquantum >> >> Thanks to Richard for pointing out the actual bug. >> >> v3: remove the redundant condition >> v2: root cause FS problem >> >> Suggested-by: Richard Henderson <richard.henderson@linaro.org> >> Signed-off-by: ShihPo Hung <shihpo.hung@sifive.com> >> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> >> --- >> target/riscv/cpu.h | 5 +---- >> 1 file changed, 1 insertion(+), 4 deletions(-) >> >> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h >> index e59343e..de0a8d8 100644 >> --- a/target/riscv/cpu.h >> +++ b/target/riscv/cpu.h >> @@ -293,10 +293,7 @@ static inline void cpu_get_tb_cpu_state(CPURISCVState *env, target_ulong *pc, >> #ifdef CONFIG_USER_ONLY >> *flags = TB_FLAGS_MSTATUS_FS; >> #else >> - *flags = cpu_mmu_index(env, 0); >> - if (riscv_cpu_fp_enabled(env)) { >> - *flags |= TB_FLAGS_MSTATUS_FS; >> - } >> + *flags = cpu_mmu_index(env, 0) | (env->mstatus & MSTATUS_FS); > > I don't think this is right, you should use the riscv_cpu_fp_enabled() function. > > Right now it's the same as env->mstatus & MSTATUS_FS but when the > Hypervisor extension goes in riscv_cpu_fp_enabled() will be more > complex. Hmm. Are you sure something like flags |= riscv_cpu_effective_mstatus(env) & MSTATUS_FS; wouldn't be more appropriate for the hypervisor extension? I guess I should have another browse through your hv patchset, but I worry now about bare uses of env->mstatus, if they no longer mean what they appear to mean. r~
On Thu, Jan 16, 2020 at 7:46 AM Richard Henderson <richard.henderson@linaro.org> wrote: > > On 1/14/20 8:28 PM, Alistair Francis wrote: > > On Wed, Jan 15, 2020 at 4:18 PM <shihpo.hung@sifive.com> wrote: > >> > >> It was found that running libquantum on riscv-linux qemu produced an > >> incorrect result. After investigation, FP registers are not saved > >> during context switch due to incorrect mstatus.FS. > >> > >> In current implementation tb->flags merges all non-disabled state to > >> dirty. This means the code in mark_fs_dirty in translate.c that > >> handles initial and clean states is unreachable. > >> > >> This patch fixes it and is successfully tested with: > >> libquantum > >> > >> Thanks to Richard for pointing out the actual bug. > >> > >> v3: remove the redundant condition > >> v2: root cause FS problem > >> > >> Suggested-by: Richard Henderson <richard.henderson@linaro.org> > >> Signed-off-by: ShihPo Hung <shihpo.hung@sifive.com> > >> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> > >> --- > >> target/riscv/cpu.h | 5 +---- > >> 1 file changed, 1 insertion(+), 4 deletions(-) > >> > >> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h > >> index e59343e..de0a8d8 100644 > >> --- a/target/riscv/cpu.h > >> +++ b/target/riscv/cpu.h > >> @@ -293,10 +293,7 @@ static inline void cpu_get_tb_cpu_state(CPURISCVState *env, target_ulong *pc, > >> #ifdef CONFIG_USER_ONLY > >> *flags = TB_FLAGS_MSTATUS_FS; > >> #else > >> - *flags = cpu_mmu_index(env, 0); > >> - if (riscv_cpu_fp_enabled(env)) { > >> - *flags |= TB_FLAGS_MSTATUS_FS; > >> - } > >> + *flags = cpu_mmu_index(env, 0) | (env->mstatus & MSTATUS_FS); > > > > I don't think this is right, you should use the riscv_cpu_fp_enabled() function. > > > > Right now it's the same as env->mstatus & MSTATUS_FS but when the > > Hypervisor extension goes in riscv_cpu_fp_enabled() will be more > > complex. > > Hmm. Are you sure something like > > flags |= riscv_cpu_effective_mstatus(env) & MSTATUS_FS; > > wouldn't be more appropriate for the hypervisor extension? I was more thinking: if (riscv_cpu_fp_enabled(env)) { *flags |= env->mstatus & MSTATUS_FS; } as floating point can be disabled from multiple places when we have the H extension. > > I guess I should have another browse through your hv patchset, but I worry now > about bare uses of env->mstatus, if they no longer mean what they appear to mean. That was why this was all refacted in the first place as we now need to check against env->vsstatus as well (depending on virt status). Alistair > > > r~
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h index e59343e..de0a8d8 100644 --- a/target/riscv/cpu.h +++ b/target/riscv/cpu.h @@ -293,10 +293,7 @@ static inline void cpu_get_tb_cpu_state(CPURISCVState *env, target_ulong *pc, #ifdef CONFIG_USER_ONLY *flags = TB_FLAGS_MSTATUS_FS; #else - *flags = cpu_mmu_index(env, 0); - if (riscv_cpu_fp_enabled(env)) { - *flags |= TB_FLAGS_MSTATUS_FS; - } + *flags = cpu_mmu_index(env, 0) | (env->mstatus & MSTATUS_FS); #endif }