Re: [Qemu-devel] [PATCH] target-mips: Fix incorrect reads and writes to DSPControl register

[Aurelien Jarno]

On Thu, Dec 06, 2012 at 08:30:35PM +0100, Petar Jovanovic wrote:
> From: Petar Jovanovic <address@hidden>
> 
> Upper 4 bits of ccond (bits 31..28 ) of DSPControl register are not used in
> the MIPS32 architecture. They are used in the MIPS64 architecture. For MIPS32
> these bits must be written as zero, and return zero on read.
> 
> The change fixes writes (WRDSP) and reads (RDDSP) to the register. It also 
> fixes
> the tests that use these instructions, and makes them smaller and simpler.
> 
> Signed-off-by: Petar Jovanovic <address@hidden>
> ---
>  target-mips/dsp_helper.c          |    8 ++++++++
>  tests/tcg/mips/mips32-dsp/rddsp.c |   32 ++++++++++++--------------------
>  tests/tcg/mips/mips32-dsp/wrdsp.c |   32 ++++++++++++--------------------
>  3 files changed, 32 insertions(+), 40 deletions(-)
> 
> diff --git a/target-mips/dsp_helper.c b/target-mips/dsp_helper.c
> index 14daf91..acf7ceb 100644
> --- a/target-mips/dsp_helper.c
> +++ b/target-mips/dsp_helper.c
> @@ -3948,7 +3948,11 @@ void helper_wrdsp(target_ulong rs, target_ulong 
> mask_num, CPUMIPSState *env)
>      if (mask[4] == 1) {
>          overwrite &= 0x00FFFFFF;
>          newbits   &= 0x00FFFFFF;
> +#if defined(TARGET_MIPS64)
>          newbits   |= 0xFF000000 & rs;
> +#else
> +        newbits   |= 0x0F000000 & rs;
> +#endif
>      }
>  
>      if (mask[5] == 1) {
> @@ -3999,7 +4003,11 @@ target_ulong helper_rddsp(target_ulong masknum, 
> CPUMIPSState *env)
>      }
>  
>      if (mask[4] == 1) {
> +#if defined(TARGET_MIPS64)
>          temp |= dsp & 0xFF000000;
> +#else
> +        temp |= dsp & 0x0F000000;
> +#endif
>      }
>  
>      if (mask[5] == 1) {
> diff --git a/tests/tcg/mips/mips32-dsp/rddsp.c 
> b/tests/tcg/mips/mips32-dsp/rddsp.c
> index e8948ec..2f30285 100644
> --- a/tests/tcg/mips/mips32-dsp/rddsp.c
> +++ b/tests/tcg/mips/mips32-dsp/rddsp.c
> @@ -6,14 +6,13 @@ int main()
>      int dsp_i, dsp_o;
>      int ccond_i, outflag_i, efi_i, c_i, scount_i, pos_i;
>      int ccond_o, outflag_o, efi_o, c_o, scount_o, pos_o;
> -    int ccond_r, outflag_r, efi_r, c_r, scount_r, pos_r;
>  
> -    ccond_i   = 0x000000BC;/* 4 */
> -    outflag_i = 0x0000001B;/* 3 */
> -    efi_i     = 0x00000001;/* 5 */
> -    c_i       = 0x00000001;/* 2 */
> -    scount_i  = 0x0000000F;/* 1 */
> -    pos_i     = 0x0000000C;/* 0 */
> +    ccond_i   = 0x0000000C;  /* 4 */
> +    outflag_i = 0x0000001B;  /* 3 */
> +    efi_i     = 0x00000001;  /* 5 */
> +    c_i       = 0x00000001;  /* 2 */
> +    scount_i  = 0x0000000F;  /* 1 */
> +    pos_i     = 0x0000000C;  /* 0 */
>  
>      dsp_i = (ccond_i   << 24) | \
>              (outflag_i << 16) | \
> @@ -22,13 +21,6 @@ int main()
>              (scount_i  <<  7) | \
>              pos_i;
>  
> -    ccond_r   = ccond_i;
> -    outflag_r = outflag_i;
> -    efi_r     = efi_i;
> -    c_r       = c_i;
> -    scount_r  = scount_i;
> -    pos_r     = pos_i;
> -
>      __asm
>          ("wrdsp %1, 0x3F\n\t"
>           "rddsp %0, 0x3F\n\t"
> @@ -43,12 +35,12 @@ int main()
>      scount_o  = (dsp_o >>  7) & 0x3F;
>      pos_o     =  dsp_o & 0x1F;
>  
> -    assert(ccond_o   == ccond_r);
> -    assert(outflag_o == outflag_r);
> -    assert(efi_o     == efi_r);
> -    assert(c_o       == c_r);
> -    assert(scount_o  == scount_r);
> -    assert(pos_o     == pos_r);
> +    assert(ccond_o   == ccond_i);
> +    assert(outflag_o == outflag_i);
> +    assert(efi_o     == efi_i);
> +    assert(c_o       == c_i);
> +    assert(scount_o  == scount_i);
> +    assert(pos_o     == pos_i);
>  
>      return 0;
>  }
> diff --git a/tests/tcg/mips/mips32-dsp/wrdsp.c 
> b/tests/tcg/mips/mips32-dsp/wrdsp.c
> index e8948ec..dc54943 100644
> --- a/tests/tcg/mips/mips32-dsp/wrdsp.c
> +++ b/tests/tcg/mips/mips32-dsp/wrdsp.c
> @@ -6,14 +6,13 @@ int main()
>      int dsp_i, dsp_o;
>      int ccond_i, outflag_i, efi_i, c_i, scount_i, pos_i;
>      int ccond_o, outflag_o, efi_o, c_o, scount_o, pos_o;
> -    int ccond_r, outflag_r, efi_r, c_r, scount_r, pos_r;
>  
> -    ccond_i   = 0x000000BC;/* 4 */
> -    outflag_i = 0x0000001B;/* 3 */
> -    efi_i     = 0x00000001;/* 5 */
> -    c_i       = 0x00000001;/* 2 */
> -    scount_i  = 0x0000000F;/* 1 */
> -    pos_i     = 0x0000000C;/* 0 */
> +    ccond_i   = 0x000000BC;  /* 4 */
> +    outflag_i = 0x0000001B;  /* 3 */
> +    efi_i     = 0x00000001;  /* 5 */
> +    c_i       = 0x00000001;  /* 2 */
> +    scount_i  = 0x0000000F;  /* 1 */
> +    pos_i     = 0x0000000C;  /* 0 */
>  
>      dsp_i = (ccond_i   << 24) | \
>              (outflag_i << 16) | \
> @@ -22,13 +21,6 @@ int main()
>              (scount_i  <<  7) | \
>              pos_i;
>  
> -    ccond_r   = ccond_i;
> -    outflag_r = outflag_i;
> -    efi_r     = efi_i;
> -    c_r       = c_i;
> -    scount_r  = scount_i;
> -    pos_r     = pos_i;
> -
>      __asm
>          ("wrdsp %1, 0x3F\n\t"
>           "rddsp %0, 0x3F\n\t"
> @@ -43,12 +35,12 @@ int main()
>      scount_o  = (dsp_o >>  7) & 0x3F;
>      pos_o     =  dsp_o & 0x1F;
>  
> -    assert(ccond_o   == ccond_r);
> -    assert(outflag_o == outflag_r);
> -    assert(efi_o     == efi_r);
> -    assert(c_o       == c_r);
> -    assert(scount_o  == scount_r);
> -    assert(pos_o     == pos_r);
> +    assert(ccond_o   == (ccond_i & 0x0F));
> +    assert(outflag_o == outflag_i);
> +    assert(efi_o     == efi_i);
> +    assert(c_o       == c_i);
> +    assert(scount_o  == scount_i);
> +    assert(pos_o     == pos_i);
>  
>      return 0;
>  }

Thanks, applied.


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