Re: [Qemu-devel] [RFC 0/1] riscv: Add full-system emulation support for the RISC-V Instruction Set Architecture (RV64G)

[Richard Henderson <rth@twiddle.net>]

On 02/18/2016 05:02 PM, Sagar Karandikar wrote:
> Some notes/questions:
> - This provides support only for the 64-bit version of the ISA and full system 
>   emulation (no user-mode)
> - This currently applies to the 2.5.0 release version. I will bump the
>   underlying codebase, split this into multiple patches, apply style checks 
>   before submitting real patches

Good.

> - The code in target-riscv/fpu-custom-riscv is an updated/modified version of 
>   softfloat. Is it okay to submodule this until the FPU behavior in RISC-V
>   is stabilized?

No.

I suspect that the version of softfloat from which this is branched is quite
old.  You'll find that many bugs have been fixed, and many guest portability
knobs have been added since then.

You appear to have a good set of unit tests whereby you can determine what
knobs need to be tweaked for this target.

Things that jump out at me right away:

target-riscv/cpu-qom.h should be merged into cpu.h.
The separation was artificial for old targets; new targets shouldn't do that.

> +#define get_field(reg, mask) (((reg) & (target_ulong)(mask)) / ((mask) & ~((mask) << 1)))
> +#define set_field(reg, mask, val) (((reg) & ~(target_ulong)(mask)) | (((target_ulong)(val) * ((mask) & ~((mask) << 1))) & (target_ulong)(mask)))

See extract{32,tl,64} and deposit{32,tl,64}.
Though the field mask is curious...

> +struct TCState {
> +    target_ulong gpr[32];
> +    target_ulong fpr[32];
> +    target_ulong PC;
> +    target_ulong load_reservation;
> +};
> +
> +typedef struct CPURISCVState CPURISCVState;
> +struct CPURISCVState {
> +    TCState active_tc;
> +    uint32_t current_tc;

Don't replicate this bit of "active_tc" silliness from target-mips.

Nor riscv-defs.h.

> +// TODO I think this is related to VMState stuff
> +// commenting it out breaks stuff, and there's an #ifdef CPU_SAVE_VERSION
> +// in include/qemu-common.h
> +#define CPU_SAVE_VERSION 3

VMState stuff is now required.

> +DEF_HELPER_3(mulhsu, tl, env, tl, tl)

Use tcg_gen_muls2_i64, with the low part being assigned to a dummy.

> +DEF_HELPER_5(fmadd_s, tl, env, tl, tl, tl, tl)
> +DEF_HELPER_5(fmadd_d, tl, env, tl, tl, tl, tl)

Use DEF_HELPER_FLAGS_*.  None of these fp functions write to TCG registers.

> +static inline void generate_exception (DisasContext *ctx, int excp)

Don't overdo the inline markers.  Particularly with unlikely exceptional cases.
 In fact, begin by not using any at all and let the compiler decide what needs
to be optimized.

> +    case OPC_RISC_DIV:
> +        {
> +            TCGv spec_source1, spec_source2;
> +            TCGv cond1, cond2;
> +            TCGLabel* handle_zero = gen_new_label();
> +            TCGLabel* handle_overflow = gen_new_label();
> +            TCGLabel* done = gen_new_label();

See how Aurelien changed the mips port to use movcond to avoid branches here.
Although, with this many special cases for return values it might be better
simply to use a helper function instead.

> +inline static void gen_arith_imm(DisasContext *ctx, uint32_t opc, 
> +                      int rd, int rs1, int16_t imm)
...
> +inline static void gen_arith_imm_w(DisasContext *ctx, uint32_t opc, 
> +                      int rd, int rs1, int16_t imm)

Surely these can be combined, such that one switch handles both; a final bit
test of opcode & 0x10 should suffice to handle the final sign-extension for the
W instructions.

> +inline static void gen_arith(DisasContext *ctx, uint32_t opc, 
> +                      int rd, int rs1, int rs2)
...
> +inline static void gen_arith_w(DisasContext *ctx, uint32_t opc, 
> +                      int rd, int rs1, int rs2)

Likewise.

> +    case OPC_RISC_LB:
> +        tcg_gen_qemu_ld8s(t1, t0, ctx->mem_idx);
> +        break;

Update to use tcg_gen_qemu_ld_tl(..., MO_SB) etc.



r~