Re: [Qemu-devel] [PATCH 07/20] target-mips: add msa_reset(), global msa register

[James Hogan <james.hogan@imgtec.com>]

Hi,

On 14/07/14 10:55, Yongbok Kim wrote:
> +static const char * const msaregnames[] = {
> +    "w0.d0",  "w0.d1",  "w1.d0",  "w1.d1",
> +    "w2.d0",  "w2.d1",  "w3.d0",  "w3.d1",
> +    "w4.d0",  "w4.d1",  "w4.d0",  "w4.d1",

I think those last 2 should be w5.d0 and w5.d1

> +static inline int check_msa_access(CPUMIPSState *env, DisasContext *ctx,
> +                                    int wt, int ws, int wd)
> +{
> +    if (unlikely((ctx->hflags & MIPS_HFLAG_FPU) &&
> +                 !(ctx->hflags & MIPS_HFLAG_F64))) {
> +        generate_exception(ctx, EXCP_RI);
> +        return 0;
> +    }
> +
> +    if (unlikely(!(ctx->hflags & MIPS_HFLAG_MSA))) {
> +        if (ctx->insn_flags & ASE_MSA) {
> +            generate_exception(ctx, EXCP_MSADIS);
> +            return 0;
> +        } else {
> +            generate_exception(ctx, EXCP_RI);
> +            return 0;
> +        }
> +    }
> +
> +    if (env->active_msa.msair & MSAIR_WRP_BIT) {
> +        int curr_request  = 0;
> +        if (wd != -1) {
> +            curr_request |= (1 << wd);
> +        }
> +        if (wt != -1) {
> +            curr_request |= (1 << wt);
> +        }
> +        if (ws != -1) {
> +            curr_request |= (1 << ws);
> +        }
> +        env->active_msa.msarequest = curr_request
> +                & (~env->active_msa.msaaccess | env->active_msa.msasave);
> +        if (unlikely(env->active_msa.msarequest != 0)) {

Are you sure it's safe to access env here during code generation? How do
you guarantee the values at translation time match the values at run time?

> +            generate_exception(ctx, EXCP_MSADIS);
> +            return 0;
> +        }
> +    }
> +    return 1;
> +}

newline between functions?

> diff --git a/target-mips/translate_init.c b/target-mips/translate_init.c
> index 29dc2ef..9e0f67b 100644
> --- a/target-mips/translate_init.c
> +++ b/target-mips/translate_init.c
> @@ -688,3 +688,48 @@ static void mvp_init (CPUMIPSState *env, const mips_def_t *def)
>                               (0x0 << CP0MVPC1_PCX) | (0x0 << CP0MVPC1_PCP2) |
>                               (0x1 << CP0MVPC1_PCP1);
>  }
> +
> +static void msa_reset(CPUMIPSState *env)
> +{
> +#ifdef CONFIG_USER_ONLY
> +    /* MSA access enabled */
> +    env->CP0_Config5 |= 1 << CP0C5_MSAEn;
> +
> +    /* DSP and CP1 enabled, 64-bit FPRs */
> +    env->CP0_Status |= (1 << CP0St_MX);
> +    env->hflags |= MIPS_HFLAG_DSP;

why do you enable DSP?

> +    env->CP0_Status |= (1 << CP0St_CU1) | (1 << CP0St_FR);
> +    env->hflags |= MIPS_HFLAG_F64 | MIPS_HFLAG_COP1X;

shouldn't that depend on the program being loaded, and whether it's
built for fp32 or fp64?

> +#endif
> +
> +    /* Vector register partitioning not implemented */
> +    env->active_msa.msair = 0;
> +    env->active_msa.msaaccess  = 0xffffffff;

the reset state is 0 according to the manual. Maybe this should depend
on CONFIG_USER_ONLY.

Cheers
James