From: Christophe de Dinechin <dinechin@redhat.com>
To: qemu-devel@nongnu.org
Cc: qemu-riscv@nongnu.org, alistair23@gmail.com, palmer@sifive.com,
alistair.francis@wdc.com
Subject: Re: [Qemu-riscv] [Qemu-devel] [PATCH-4.2 v1 4/6] target/riscv: Create function to test if FP is enabled
Date: Tue, 30 Jul 2019 10:49:26 +0200 [thread overview]
Message-ID: <m1zhkvoqjt.fsf@redhat.com> (raw)
In-Reply-To: <edea77ff4aa6900d01ab7146d5b52c2dae4a856e.1564080680.git.alistair.francis@wdc.com>
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)
WARNING: multiple messages have this Message-ID (diff)
From: Christophe de Dinechin <dinechin@redhat.com>
To: qemu-devel@nongnu.org
Cc: alistair23@gmail.com, alistair.francis@wdc.com,
palmer@sifive.com, qemu-riscv@nongnu.org
Subject: Re: [Qemu-devel] [PATCH-4.2 v1 4/6] target/riscv: Create function to test if FP is enabled
Date: Tue, 30 Jul 2019 10:49:26 +0200 [thread overview]
Message-ID: <m1zhkvoqjt.fsf@redhat.com> (raw)
In-Reply-To: <edea77ff4aa6900d01ab7146d5b52c2dae4a856e.1564080680.git.alistair.francis@wdc.com>
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)
next prev parent reply other threads:[~2019-07-30 9:37 UTC|newest]
Thread overview: 52+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-07-25 18:51 [Qemu-riscv] [PATCH-4.2 v1 0/6] RISC-V: Hypervisor prep work part 2 Alistair Francis
2019-07-25 18:51 ` [Qemu-devel] " Alistair Francis
2019-07-25 18:52 ` [Qemu-riscv] [PATCH-4.2 v1 1/6] target/riscv: Don't set write permissions on dirty PTEs Alistair Francis
2019-07-25 18:52 ` [Qemu-devel] " Alistair Francis
2019-07-29 14:33 ` [Qemu-riscv] " Philippe Mathieu-Daudé
2019-07-29 14:33 ` Philippe Mathieu-Daudé
2019-07-25 18:52 ` [Qemu-riscv] [PATCH-4.2 v1 2/6] target/riscv: Remove strict perm checking for CSR R/W Alistair Francis
2019-07-25 18:52 ` [Qemu-devel] " Alistair Francis
2019-07-25 21:47 ` [Qemu-riscv] " Jonathan Behrens
2019-07-25 21:47 ` [Qemu-devel] " Jonathan Behrens
2019-07-26 20:24 ` Alistair Francis
2019-07-26 20:24 ` [Qemu-devel] " Alistair Francis
2019-07-26 21:00 ` Jonathan Behrens
2019-07-26 21:00 ` [Qemu-devel] " Jonathan Behrens
2019-07-26 22:28 ` Alistair Francis
2019-07-26 22:28 ` [Qemu-devel] " Alistair Francis
2019-07-25 18:52 ` [Qemu-riscv] [PATCH-4.2 v1 3/6] riscv: plic: Remove unused interrupt functions Alistair Francis
2019-07-25 18:52 ` [Qemu-devel] " Alistair Francis
2019-07-26 15:22 ` [Qemu-riscv] " Jonathan Behrens
2019-07-26 15:22 ` [Qemu-devel] " Jonathan Behrens
2019-07-29 14:28 ` [Qemu-riscv] [Qemu-devel] " Philippe Mathieu-Daudé
2019-07-29 14:28 ` Philippe Mathieu-Daudé
2019-07-29 17:32 ` [Qemu-riscv] " Chih-Min Chao
2019-07-29 17:32 ` Chih-Min Chao
2019-07-25 18:52 ` [Qemu-riscv] [PATCH-4.2 v1 4/6] target/riscv: Create function to test if FP is enabled Alistair Francis
2019-07-25 18:52 ` [Qemu-devel] " Alistair Francis
2019-07-29 14:31 ` [Qemu-riscv] " Philippe Mathieu-Daudé
2019-07-29 14:31 ` Philippe Mathieu-Daudé
2019-07-29 16:56 ` [Qemu-riscv] " Chih-Min Chao
2019-07-29 16:56 ` [Qemu-devel] " Chih-Min Chao
2019-07-30 18:32 ` Alistair Francis
2019-07-30 18:32 ` [Qemu-devel] " Alistair Francis
2019-07-30 8:49 ` Christophe de Dinechin [this message]
2019-07-30 8:49 ` [Qemu-devel] " Christophe de Dinechin
2019-07-30 18:33 ` [Qemu-riscv] " Alistair Francis
2019-07-30 18:33 ` Alistair Francis
2019-07-25 18:52 ` [Qemu-riscv] [PATCH-4.2 v1 5/6] target/riscv: Update the Hypervisor CSRs to v0.4 Alistair Francis
2019-07-25 18:52 ` [Qemu-devel] " Alistair Francis
2019-07-26 17:40 ` [Qemu-riscv] " Chih-Min Chao
2019-07-26 17:40 ` Chih-Min Chao
2019-07-26 20:20 ` [Qemu-riscv] " Alistair Francis
2019-07-26 20:20 ` Alistair Francis
2019-07-25 18:52 ` [Qemu-riscv] [PATCH-4.2 v1 6/6] target/riscv: Fix Floating Point register names Alistair Francis
2019-07-25 18:52 ` [Qemu-devel] " Alistair Francis
2019-07-29 15:19 ` [Qemu-riscv] " Chih-Min Chao
2019-07-29 15:19 ` Chih-Min Chao
2019-07-30 18:37 ` [Qemu-riscv] " Alistair Francis
2019-07-30 18:37 ` Alistair Francis
2019-07-31 8:10 ` [Qemu-riscv] " Chih-Min Chao
2019-07-31 8:10 ` Chih-Min Chao
2019-08-05 17:49 ` [Qemu-riscv] " Alistair Francis
2019-08-05 17:49 ` Alistair Francis
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=m1zhkvoqjt.fsf@redhat.com \
--to=dinechin@redhat.com \
--cc=alistair.francis@wdc.com \
--cc=alistair23@gmail.com \
--cc=palmer@sifive.com \
--cc=qemu-devel@nongnu.org \
--cc=qemu-riscv@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.