On Tue, Jul 30, 2019 at 1:49 AM Christophe de Dinechin <dinec...@redhat.com> wrote: > > > Alistair Francis writes: > > > Let's creaate a function that tests if floating point support is > > Typo: create
Fixed > > > enabled. We can then protect all floating point operations based on if > > they are enabled. > > > > This patch so far doesn't change anything, it's just preparing for the > > Hypervisor support for floating point operations. > > > > Signed-off-by: Alistair Francis <alistair.fran...@wdc.com> > > --- > > target/riscv/cpu.h | 6 +++++- > > target/riscv/cpu_helper.c | 10 ++++++++++ > > target/riscv/csr.c | 19 ++++++++++--------- > > 3 files changed, 25 insertions(+), 10 deletions(-) > > > > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h > > index 0adb307f32..2dc9b17678 100644 > > --- a/target/riscv/cpu.h > > +++ b/target/riscv/cpu.h > > @@ -255,6 +255,7 @@ void riscv_cpu_do_interrupt(CPUState *cpu); > > int riscv_cpu_gdb_read_register(CPUState *cpu, uint8_t *buf, int reg); > > int riscv_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg); > > bool riscv_cpu_exec_interrupt(CPUState *cs, int interrupt_request); > > +bool riscv_cpu_fp_enabled(CPURISCVState *env); > > int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch); > > hwaddr riscv_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr); > > void riscv_cpu_do_unaligned_access(CPUState *cs, vaddr addr, > > @@ -298,7 +299,10 @@ 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) | (env->mstatus & MSTATUS_FS); > > + *flags = cpu_mmu_index(env, 0); > > + if (riscv_cpu_fp_enabled(env)) { > > + *flags |= env->mstatus & MSTATUS_FS; > > + } > > #endif > > } > > > > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c > > index f027be7f16..225e407cff 100644 > > --- a/target/riscv/cpu_helper.c > > +++ b/target/riscv/cpu_helper.c > > @@ -71,6 +71,16 @@ bool riscv_cpu_exec_interrupt(CPUState *cs, int > > interrupt_request) > > > > #if !defined(CONFIG_USER_ONLY) > > > > +/* Return true is floating point support is currently enabled */ > > +bool riscv_cpu_fp_enabled(CPURISCVState *env) > > +{ > > + if (env->mstatus & MSTATUS_FS) { > > + return true; > > + } > > + > > + return false; > > Will there be more conditions that lead to the "true" case? > If not, please consider making it a one-liner for readability, e.g. > > return env->mstatus & MSTATUS_FS; > > (just a personal preference, feel free to ignore) There are going to be more conditionals when adding the Hypervisor extension. > > > +} > > + > > int riscv_cpu_claim_interrupts(RISCVCPU *cpu, uint32_t interrupts) > > { > > CPURISCVState *env = &cpu->env; > > diff --git a/target/riscv/csr.c b/target/riscv/csr.c > > index af3b762c8b..7b73b73cf7 100644 > > --- a/target/riscv/csr.c > > +++ b/target/riscv/csr.c > > @@ -46,7 +46,7 @@ void riscv_set_csr_ops(int csrno, riscv_csr_operations > > *ops) > > static int fs(CPURISCVState *env, int csrno) > > { > > #if !defined(CONFIG_USER_ONLY) > > - if (!env->debugger && !(env->mstatus & MSTATUS_FS)) { > > + if (!env->debugger && !riscv_cpu_fp_enabled(env)) { > > This was existing behavior, but I'm curious why all these > tests are disabled when env->debugger is set. This was introduced > in 753e3fe20, but I see no rationale in the commit message. > I find it odd, maybe even suspicious, that activating the debugger > would change the behavior :-) This is to allow GDB to access the registers. > > > return -1; > > } > > #endif > > @@ -108,7 +108,7 @@ static int pmp(CPURISCVState *env, int csrno) > > static int read_fflags(CPURISCVState *env, int csrno, target_ulong *val) > > { > > #if !defined(CONFIG_USER_ONLY) > > - if (!env->debugger && !(env->mstatus & MSTATUS_FS)) { > > + if (!env->debugger && !riscv_cpu_fp_enabled(env)) { > > return -1; > > } > > #endif > > @@ -119,7 +119,7 @@ static int read_fflags(CPURISCVState *env, int csrno, > > target_ulong *val) > > static int write_fflags(CPURISCVState *env, int csrno, target_ulong val) > > { > > #if !defined(CONFIG_USER_ONLY) > > - if (!env->debugger && !(env->mstatus & MSTATUS_FS)) { > > + if (!env->debugger && !riscv_cpu_fp_enabled(env)) { > > return -1; > > } > > env->mstatus |= MSTATUS_FS; > > @@ -131,7 +131,7 @@ static int write_fflags(CPURISCVState *env, int csrno, > > target_ulong val) > > static int read_frm(CPURISCVState *env, int csrno, target_ulong *val) > > { > > #if !defined(CONFIG_USER_ONLY) > > - if (!env->debugger && !(env->mstatus & MSTATUS_FS)) { > > + if (!env->debugger && !riscv_cpu_fp_enabled(env)) { > > return -1; > > } > > #endif > > @@ -142,7 +142,7 @@ static int read_frm(CPURISCVState *env, int csrno, > > target_ulong *val) > > static int write_frm(CPURISCVState *env, int csrno, target_ulong val) > > { > > #if !defined(CONFIG_USER_ONLY) > > - if (!env->debugger && !(env->mstatus & MSTATUS_FS)) { > > + if (!env->debugger && !riscv_cpu_fp_enabled(env)) { > > return -1; > > } > > env->mstatus |= MSTATUS_FS; > > @@ -154,7 +154,7 @@ static int write_frm(CPURISCVState *env, int csrno, > > target_ulong val) > > static int read_fcsr(CPURISCVState *env, int csrno, target_ulong *val) > > { > > #if !defined(CONFIG_USER_ONLY) > > - if (!env->debugger && !(env->mstatus & MSTATUS_FS)) { > > + if (!env->debugger && !riscv_cpu_fp_enabled(env)) { > > return -1; > > } > > #endif > > @@ -166,7 +166,7 @@ static int read_fcsr(CPURISCVState *env, int csrno, > > target_ulong *val) > > static int write_fcsr(CPURISCVState *env, int csrno, target_ulong val) > > { > > #if !defined(CONFIG_USER_ONLY) > > - if (!env->debugger && !(env->mstatus & MSTATUS_FS)) { > > + if (!env->debugger && !riscv_cpu_fp_enabled(env)) { > > return -1; > > } > > env->mstatus |= MSTATUS_FS; > > @@ -307,6 +307,7 @@ static int write_mstatus(CPURISCVState *env, int csrno, > > target_ulong val) > > { > > target_ulong mstatus = env->mstatus; > > target_ulong mask = 0; > > + int dirty; > > > > /* flush tlb on mstatus fields that affect VM */ > > if (env->priv_ver <= PRIV_VERSION_1_09_1) { > > @@ -340,8 +341,8 @@ static int write_mstatus(CPURISCVState *env, int csrno, > > target_ulong val) > > > > mstatus = (mstatus & ~mask) | (val & mask); > > > > - int dirty = ((mstatus & MSTATUS_FS) == MSTATUS_FS) | > > - ((mstatus & MSTATUS_XS) == MSTATUS_XS); > > + dirty = riscv_cpu_fp_enabled(env) | > > + ((mstatus & MSTATUS_XS) == MSTATUS_XS); > > mstatus = set_field(mstatus, MSTATUS_SD, dirty); > > env->mstatus = mstatus; > > Reviewed-by: Christophe de Dinechin <dinec...@redhat.com> Thanks! Alistair > > -- > Cheers, > Christophe de Dinechin (IRC c3d)