Re: [Qemu-ppc] Patch: fix to gen_mcrxr() in target-ppc/translate.c

[Tom Musta]

On 6/10/2014 8:51 AM, Sorav Bansal wrote:
> Hi,
> 
> I saw a minor bug in the gen_mcrxr() function in
> target-ppc/translate.c. Here is the patch for your consideration. I
> have verified the patch by checking the generated code using a
> SAT-solver based verification tool.
> 
> thanks,
> Sorav
> 
> From 31f39e258cbb289c2e0a3c3adde87cde7ae02a15 Mon Sep 17 00:00:00 2001
> From: Sorav Bansal <address@hidden>
> Date: Tue, 10 Jun 2014 19:01:12 +0530
> Subject: [PATCH] Fix to the translation of mcrxr instruction
> 
> ---
>  target-ppc/translate.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/target-ppc/translate.c b/target-ppc/translate.c
> index f089014..b513998 100644
> --- a/target-ppc/translate.c
> +++ b/target-ppc/translate.c
> @@ -4147,8 +4147,8 @@ static void gen_mcrxr(DisasContext *ctx)
>      tcg_gen_trunc_tl_i32(t0, cpu_so);
>      tcg_gen_trunc_tl_i32(t1, cpu_ov);
>      tcg_gen_trunc_tl_i32(dst, cpu_ca);
> -    tcg_gen_shri_i32(t0, t0, 2);
> -    tcg_gen_shri_i32(t1, t1, 1);
> +    tcg_gen_shli_i32(dst, dst, 2);
> +    tcg_gen_shli_i32(t1, t1, 1);
>      tcg_gen_or_i32(dst, dst, t0);
>      tcg_gen_or_i32(dst, dst, t1);
>      tcg_temp_free_i32(t0);
> 

I don't believe this is correct.

- XER[SO] needs to be shifted into the most significant bit of the 4-bit CRF 
(shift by 3 instead of 2)
- likewise for XER[OV] and XER[CA]
- technically, you are not copy XER[35] into the CRF nor are you clearing it.  
However, this bit is not
  defined by the ISA and most implementations do not actually implement the 
latch ... so this is probably
  OK

Also, based on feedback on my work from Richard, I believe a deposit operation 
would be (potentially) more
efficient than a shift-left / or combination.

Lastly, you should also cc the qemu-devel mailing list.