Re: [Qemu-devel] [PATCH v4] target-microblaze: lwx/swx: first implementation

[Stefan Weil]

Hi Peter,

this patch (which was applied to QEMU master) breaks debug builds
(configure --enable-debug). see my comment below.

Regards,

Stefan W.


Am 01.06.2012 05:23, schrieb Peter A. G. Crosthwaite:
> Signed-off-by: Peter A. G. Crosthwaite<address@hidden>
> ---
> changed from v3:
> simplified tcg local variable usage aqcross branch
> changed from v2:
> fixed tcg local variable usage across branch
> reworked carry logic (with new write_carryi() function)
> made LOG_DIS show lwx swx properly
> changed from v1:
> implemented reservation address checking
> created new cpu state variable specifically for reservation address/flag state
>
>   target-microblaze/cpu.c       |    1 +
>   target-microblaze/cpu.h       |    4 ++
>   target-microblaze/helper.c    |    2 +
>   target-microblaze/translate.c |   62 +++++++++++++++++++++++++++++++++++++---
>   4 files changed, 64 insertions(+), 5 deletions(-)

...

> diff --git a/target-microblaze/translate.c b/target-microblaze/translate.c
> index a362938..f0ebd59 100644
> --- a/target-microblaze/translate.c
> +++ b/target-microblaze/translate.c
> @@ -162,6 +162,14 @@ static void write_carry(DisasContext *dc, TCGv v)
>       tcg_temp_free(t0);
>   }
>
> +static void write_carryi(DisasContext *dc, int carry)
> +{
> +    TCGv t0 = tcg_temp_new();
> +    tcg_gen_movi_tl(t0, carry ? 1 : 0);
> +    write_carry(dc, t0);
> +    tcg_temp_free(t0);
> +}
> +
>   /* True if ALU operand b is a small immediate that may deserve
>      faster treatment.  */
>   static inline int dec_alu_op_b_is_small_imm(DisasContext *dc)
> @@ -948,12 +956,13 @@ static inline void dec_byteswap(DisasContext *dc, TCGv 
> dst, TCGv src, int size)
>   static void dec_load(DisasContext *dc)
>   {
>       TCGv t, *addr;
> -    unsigned int size, rev = 0;
> +    unsigned int size, rev = 0, ex = 0;
>
>       size = 1<<  (dc->opcode&  3);
>
>       if (!dc->type_b) {
>           rev = (dc->ir>>  9)&  1;
> +        ex = (dc->ir>>  10)&  1;
>       }
>
>       if (size>  4&&  (dc->tb_flags&  MSR_EE_FLAG)
> @@ -963,7 +972,8 @@ static void dec_load(DisasContext *dc)
>           return;
>       }
>
> -    LOG_DIS("l%d%s%s\n", size, dc->type_b ? "i" : "", rev ? "r" : "");
> +    LOG_DIS("l%d%s%s%s\n", size, dc->type_b ? "i" : "", rev ? "r" : "",
> +                                                        ex ? "x" : "");
>
>       t_sync_flags(dc);
>       addr = compute_ldst_addr(dc,&t);
> @@ -1019,6 +1029,17 @@ static void dec_load(DisasContext *dc)
>           }
>       }
>
> +    /* lwx does not throw unaligned access errors, so force alignment */
> +    if (ex) {
> +        /* Force addr into the temp.  */
> +        if (addr !=&t) {
> +            t = tcg_temp_new();
> +            tcg_gen_mov_tl(t, *addr);
> +            addr =&t;
> +        }
> +        tcg_gen_andi_tl(t, t, ~3);
> +    }
> +
>       /* If we get a fault on a dslot, the jmpstate better be in sync.  */
>       sync_jmpstate(dc);
>
> @@ -1057,6 +1078,12 @@ static void dec_load(DisasContext *dc)
>           }
>       }
>
> +    if (ex) { /* lwx */
> +        /* no support for for AXI exclusive so always clear C */
> +        write_carryi(dc, 0);
> +        tcg_gen_st_tl(*addr, cpu_env, offsetof(CPUMBState, res_addr));
> +    }
> +
>       if (addr ==&t)
>           tcg_temp_free(t);
>   }
> @@ -1078,12 +1105,14 @@ static void gen_store(DisasContext *dc, TCGv addr, TCGv 
> val,
>
>   static void dec_store(DisasContext *dc)
>   {
> -    TCGv t, *addr;
> -    unsigned int size, rev = 0;
> +    TCGv t, *addr, swx_addr, r_check = 0;

With --enable-debug, TCGv is a struct (not an int),
therefore r_check = 0 does not work.

swx_addr and r_check are used in the same context.
Can their declarations be moved to the block where
they are first used?

If that is not possible, either none or both of these
two variables should be initialised.


> +    int swx_skip = 0;
> +    unsigned int size, rev = 0, ex = 0;
>
>       size = 1<<  (dc->opcode&  3);
>       if (!dc->type_b) {
>           rev = (dc->ir>>  9)&  1;
> +        ex = (dc->ir>>  10)&  1;
>       }
>
>       if (size>  4&&  (dc->tb_flags&  MSR_EE_FLAG)
> @@ -1093,12 +1122,30 @@ static void dec_store(DisasContext *dc)
>           return;
>       }
>
> -    LOG_DIS("s%d%s%s\n", size, dc->type_b ? "i" : "", rev ? "r" : "");
> +    LOG_DIS("s%d%s%s%s\n", size, dc->type_b ? "i" : "", rev ? "r" : "",
> +                                                        ex ? "x" : "");
>       t_sync_flags(dc);
>       /* If we get a fault on a dslot, the jmpstate better be in sync.  */
>       sync_jmpstate(dc);
>       addr = compute_ldst_addr(dc,&t);
>
> +    if (ex) { /* swx */
> +        r_check = tcg_temp_new();
> +        swx_addr = tcg_temp_local_new();


TCGv r_check = ...;
TCGv swx_addr = ...;

> +
> +        /* Force addr into the swx_addr. */
> +        tcg_gen_mov_tl(swx_addr, *addr);
> +        addr =&swx_addr;
> +        /* swx does not throw unaligned access errors, so force alignment */
> +        tcg_gen_andi_tl(swx_addr, swx_addr, ~3);
> +
> +        tcg_gen_ld_tl(r_check, cpu_env, offsetof(CPUMBState, res_addr));
> +        write_carryi(dc, 1);
> +        swx_skip = gen_new_label();
> +        tcg_gen_brcond_tl(TCG_COND_NE, r_check, swx_addr, swx_skip);
> +        write_carryi(dc, 0);

Add this code from block at end of function?

        gen_set_label(swx_skip);
        tcg_temp_free(swx_addr);
        tcg_temp_free(r_check);

> +    }
> +
>       if (rev&&  size != 4) {
>           /* Endian reverse the address. t is addr.  */
>           switch (size) {
> @@ -1174,6 +1221,11 @@ static void dec_store(DisasContext *dc)
>           gen_helper_memalign(*addr, tcg_const_tl(dc->rd),
>                               tcg_const_tl(1), tcg_const_tl(size - 1));
>       }

Move the following conditional block up?

> +    if (ex) {
> +        gen_set_label(swx_skip);
> +        tcg_temp_free(r_check);
> +        tcg_temp_free(swx_addr);
> +    }