[Top][All Lists]

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

Re: [Qemu-devel] [PATCH 4/9] target/riscv: Introduce cpu_riscv_get_fcsr

From: Michael Clark
Subject: Re: [Qemu-devel] [PATCH 4/9] target/riscv: Introduce cpu_riscv_get_fcsr
Date: Fri, 18 May 2018 14:46:09 +1200

On Fri, May 11, 2018 at 3:52 PM, Richard Henderson <
address@hidden> wrote:

> Cc: Michael Clark <address@hidden>
> Cc: Palmer Dabbelt <address@hidden>
> Cc: Sagar Karandikar <address@hidden>
> Cc: Bastian Koppelmann <address@hidden>
> Signed-off-by: Richard Henderson <address@hidden>

I'm not against this change but it conflicts with changes in the riscv
repo. I should post my patch queue to the list...

We have made a somewhat medium sized change and have unraveled two
monolithic switch statements out of csr_read_helper switch and
csr_write_helper into clearly decomposed functions for modifying control
and status registers, along with an interface to allow CPUs to hook custom
control and status registers. This was done to support atomic
read/modify/write CSRs which was not possible to achieve with the current
helpers which separately called via the csr_read_helper followed by
csr_write_helper. Given the only way to modify CSRs was via the switch
statements, we needed to move them out to provide a mechanism for CSRs that
wish to be truly atomic. e.g. 'mip'. The CSR functions are defined in The
RISC-V Instruction Set Manual Volume I: User-Level ISA Document Version 2.2
as "atomic" instructions:

- CSRRW (Atomic Read/Write CSR)
- CSRRS (Atomic Read and Set Bits in CSR)
- CSRRC (Atomic Read and Clear Bits in CSR)

We have thus changed QEMU to allow truly atomic CSR implementations. The
new implementation replaces the compiler doing compare/branch vs jump table
switch codegen for a sparse CSR address space with a single array of
function pointers. i.e. load, indirect jump. Along with this change we have
also renamed functions in target/riscv to use riscv_ prefix and added a
public interface to hook custom CSRs. The CSR changes will allow out of
tree code to hook custom CSRs without needing to change target/riscv code.

- riscv_cpu_ won over cpu_riscv_ given the number of functions conforming
with the former riscv_ prefix and the desire for consistency in target/riscv

In the riscv tree we now have riscv_csr_read(env, CSR_FCSR)
and riscv_csr_write(env, CSR_FCSR, fcsr) as the method to read and write
the composite. There is also a user in linux-user/riscv/signal.c that
should probably use the new interface. We could change
linux-user/riscv/signal.c to use your new interface however your interface
only provides a read method and no write method, so the write interface
remains in the (current) big CSR switch statement, leaving an inconsitency
between the encapsulation of read and write. We currently have the new fcsr
read and write encapsulated in static functions read_fcsr and write_fcsr in
a new csr module (which should perhaps be called csr_helper.c).


- https://github.com/riscv/riscv-qemu/commits/qemu-2.13-for-upstream

> ---
>  target/riscv/cpu.h        | 1 +
>  target/riscv/fpu_helper.c | 6 ++++++
>  target/riscv/op_helper.c  | 3 +--
>  3 files changed, 8 insertions(+), 2 deletions(-)
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 34abc383e3..f2bc243b95 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -265,6 +265,7 @@ void QEMU_NORETURN do_raise_exception_err(CPURISCVState
> *env,
>                                            uint32_t exception, uintptr_t
> pc);
>  target_ulong cpu_riscv_get_fflags(CPURISCVState *env);
> +target_ulong cpu_riscv_get_fcsr(CPURISCVState *env);
>  void cpu_riscv_set_fflags(CPURISCVState *env, target_ulong);
>  #define TB_FLAGS_MMU_MASK  3
> diff --git a/target/riscv/fpu_helper.c b/target/riscv/fpu_helper.c
> index abbadead5c..41c7352115 100644
> --- a/target/riscv/fpu_helper.c
> +++ b/target/riscv/fpu_helper.c
> @@ -37,6 +37,12 @@ target_ulong cpu_riscv_get_fflags(CPURISCVState *env)
>      return hard;
>  }
> +target_ulong cpu_riscv_get_fcsr(CPURISCVState *env)
> +{
> +    return (cpu_riscv_get_fflags(env) << FSR_AEXC_SHIFT)
> +         | (env->frm << FSR_RD_SHIFT);
> +}
> +
>  void cpu_riscv_set_fflags(CPURISCVState *env, target_ulong hard)
>  {
>      int soft = 0;
> diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
> index 3abf52453c..fd2d8c0a9d 100644
> --- a/target/riscv/op_helper.c
> +++ b/target/riscv/op_helper.c
> @@ -423,8 +423,7 @@ target_ulong csr_read_helper(CPURISCVState *env,
> target_ulong csrno)
>          return env->frm;
>      case CSR_FCSR:
>          validate_mstatus_fs(env, GETPC());
> -        return (cpu_riscv_get_fflags(env) << FSR_AEXC_SHIFT)
> -                | (env->frm << FSR_RD_SHIFT);
> +        return cpu_riscv_get_fcsr(env);
>      /* rdtime/rdtimeh is trapped and emulated by bbl in system mode */
>      case CSR_TIME:
> --
> 2.17.0

reply via email to

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