Re: [Qemu-devel] [PATCH 2/2] target-mips: fix EXTPDP and setting up pos field in the DSPControl reg

[Aurelien Jarno]

On Mon, May 13, 2013 at 03:20:27PM +0200, Petar Jovanovic wrote:
> From: Petar Jovanovic <address@hidden>
> 
> This change makes sure that modifications of pos field in the DSPControl
> register do not trash other bits in the register. This bug can be triggered
> with the additional test case in mips32-dsp/extpdp.c in this commit.
> 
> In addition to this, this change corrects incorrect calculation of the mask
> for EXTPDP.
> 
> Signed-off-by: Petar Jovanovic <address@hidden>
> ---
>  target-mips/dsp_helper.c           |    9 ++++-----
>  tests/tcg/mips/mips32-dsp/extpdp.c |   18 ++++++++++++++++++
>  2 files changed, 22 insertions(+), 5 deletions(-)
> 
> diff --git a/target-mips/dsp_helper.c b/target-mips/dsp_helper.c
> index a55f866..27283ed 100644
> --- a/target-mips/dsp_helper.c
> +++ b/target-mips/dsp_helper.c
> @@ -90,10 +90,10 @@ static inline void set_DSPControl_pos(uint32_t pos, 
> CPUMIPSState *env)
>      dspc = env->active_tc.DSPControl;
>  #ifndef TARGET_MIPS64
>      dspc = dspc & 0xFFFFFFC0;
> -    dspc |= pos;
> +    dspc |= (pos & 0x3F);
>  #else
>      dspc = dspc & 0xFFFFFF80;
> -    dspc |= pos;
> +    dspc |= (pos & 0x7F);
>  #endif
>      env->active_tc.DSPControl = dspc;
>  }
> @@ -3439,10 +3439,9 @@ target_ulong helper_extpdp(target_ulong ac, 
> target_ulong size,
>      if (sub >= -1) {
>          acc  = ((uint64_t)env->active_tc.HI[ac] << 32) |
>                 ((uint64_t)env->active_tc.LO[ac] & MIPSDSP_LLO);
> -        temp = (acc >> (start_pos - size)) &
> -               (((uint32_t)0x01 << (size + 1)) - 1);
> +        temp = (acc >> (start_pos - size)) & (~0U >> (31 - size));
>  
> -        set_DSPControl_pos(start_pos - (size + 1), env);
> +        set_DSPControl_pos(sub & 0x3F, env);

I am not sure it is correct there for the 64-bit mode.

The value that should be written should be DSPcontrolpos6..0 - (size
+ 1), therefore keeeping only last 6 bits doesn't seems correct to me.
Given the change above, I think it should simply be 
set_DSPControl_pos(sub, env);

>          set_DSPControl_efi(0, env);
>      } else {
>          set_DSPControl_efi(1, env);
> diff --git a/tests/tcg/mips/mips32-dsp/extpdp.c 
> b/tests/tcg/mips/mips32-dsp/extpdp.c
> index 15ba082..79ee16e 100644
> --- a/tests/tcg/mips/mips32-dsp/extpdp.c
> +++ b/tests/tcg/mips/mips32-dsp/extpdp.c
> @@ -42,5 +42,23 @@ int main()
>      efi = (dsp >> 14) & 0x01;
>      assert(efi == 1);
>  
> +
> +    ach = 0;
> +    acl = 0;
> +    dsp = 0;
> +    result = 0;
> +
> +    __asm
> +        ("wrdsp %1\n\t"
> +         "mthi %2, $ac1\n\t"
> +         "mtlo %3, $ac1\n\t"
> +         "extpdp %0, $ac1, 0x00\n\t"
> +         "rddsp %1\n\t"
> +         : "=r"(rt), "+r"(dsp)
> +         : "r"(ach), "r"(acl)
> +        );
> +    assert(dsp == 0x3F);
> +    assert(result == rt);
> +
>      return 0;
>  }
> -- 
> 1.7.9.5
> 
> 

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
address@hidden                 http://www.aurel32.net