| Message ID | edea77ff4aa6900d01ab7146d5b52c2dae4a856e.1564080680.git.alistair.francis@wdc.com |
|---|---|
| State | New |
| Headers | show |
| Series | RISC-V: Hypervisor prep work part 2 | expand |
On 7/25/19 8:52 PM, Alistair Francis wrote: > Let's creaate a function that tests if floating point support is "create" > 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.francis@wdc.com> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.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; > +} > + > 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)) { > 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; > >
On Fri, Jul 26, 2019 at 2:55 AM Alistair Francis <alistair.francis@wdc.com> wrote: > Let's creaate a function that tests if floating point support is > 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.francis@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; > +} > + > 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)) { > 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); > FS are 2bits original: only 3 is true new 1, 2, 3 all make dirty true Since only 3 means dirty, keeps this part unchanged should be reasonable. chihmin mstatus = set_field(mstatus, MSTATUS_SD, dirty); > env->mstatus = mstatus; > > -- > 2.22.0 > > >
Alistair Francis writes: > Let's creaate a function that tests if floating point support is Typo: create > 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.francis@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) > +} > + > 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 :-) > 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 <dinechin@redhat.com> -- Cheers, Christophe de Dinechin (IRC c3d)
On Mon, Jul 29, 2019 at 9:56 AM Chih-Min Chao <chihmin.chao@sifive.com> wrote: > > > > On Fri, Jul 26, 2019 at 2:55 AM Alistair Francis <alistair.francis@wdc.com> wrote: >> >> Let's creaate a function that tests if floating point support is >> 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.francis@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; >> +} >> + >> 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)) { >> 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); > > > FS are 2bits > original: > only 3 is true > new > 1, 2, 3 all make dirty true > > Since only 3 means dirty, keeps this part unchanged should be reasonable. Good point, I have updated this. Alistair > > chihmin > >> mstatus = set_field(mstatus, MSTATUS_SD, dirty); >> env->mstatus = mstatus; >> >> -- >> 2.22.0 >> >>
On Tue, Jul 30, 2019 at 1:49 AM Christophe de Dinechin <dinechin@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.francis@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 <dinechin@redhat.com> Thanks! Alistair > > -- > Cheers, > Christophe de Dinechin (IRC c3d)
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; +} + 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)) { 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;
Let's creaate a function that tests if floating point support is 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.francis@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(-)