Re: [Qemu-devel] [PATCH 1/4] target-mips: add CMGCRBase register

[Leon Alrae <leon.alrae@imgtec.com>]

On 16/10/15 00:52, Yongbok Kim wrote:
> Physical base address for the memory-mapped Coherency Manager Global
> Configuration Register space.
> The MIPS default location for the GCR_BASE address is 0x1FBF_8.
> This register only exists if Config3 CMGCR is set to one.
> 
> Signed-off-by: Yongbok Kim <yongbok.kim@imgtec.com>
> ---
>  target-mips/cpu.h            |    3 ++-
>  target-mips/translate.c      |   17 +++++++++++++++++
>  target-mips/translate_init.c |    3 ++-
>  3 files changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/target-mips/cpu.h b/target-mips/cpu.h
> index f32a0fd..639ef37 100644
> --- a/target-mips/cpu.h
> +++ b/target-mips/cpu.h
> @@ -389,6 +389,7 @@ struct CPUMIPSState {
>      target_ulong CP0_EPC;
>      int32_t CP0_PRid;
>      int32_t CP0_EBase;
> +    target_ulong CP0_CMGCRBase;
>      int32_t CP0_Config0;
>  #define CP0C0_M    31
>  #define CP0C0_K23  28
> @@ -431,7 +432,7 @@ struct CPUMIPSState {
>      int32_t CP0_Config3;
>  #define CP0C3_M    31
>  #define CP0C3_BPG  30
> -#define CP0C3_CMCGR 29
> +#define CP0C3_CMGCR 29
>  #define CP0C3_MSAP  28
>  #define CP0C3_BP 27
>  #define CP0C3_BI 26
> diff --git a/target-mips/translate.c b/target-mips/translate.c
> index 897839c..c74e8e7 100644
> --- a/target-mips/translate.c
> +++ b/target-mips/translate.c
> @@ -1426,6 +1426,7 @@ typedef struct DisasContext {
>      bool mvh;
>      int CP0_LLAddr_shift;
>      bool ps;
> +    bool cmgcr;
>  } DisasContext;
>  
>  enum {
> @@ -5273,6 +5274,12 @@ static void gen_mfc0(DisasContext *ctx, TCGv arg, int reg, int sel)
>              gen_mfc0_load32(arg, offsetof(CPUMIPSState, CP0_EBase));
>              rn = "EBase";
>              break;
> +        case 3:
> +            check_insn(ctx, ISA_MIPS32R2);
> +            CP0_CHECK(ctx->cmgcr);
> +            gen_mfc0_load32(arg, offsetof(CPUMIPSState, CP0_CMGCRBase));

gen_mfc0_load32 assumes 32-bit CP0 register whereas this one is
target_ulong. The tcg_gen_ld_tl + tcg_gen_ext32s_tl pair should be used
here.

> @@ -19567,6 +19580,7 @@ void gen_intermediate_code(CPUMIPSState *env, struct TranslationBlock *tb)
>      ctx.ulri = (env->CP0_Config3 >> CP0C3_ULRI) & 1;
>      ctx.ps = ((env->active_fpu.fcr0 >> FCR0_PS) & 1) ||
>               (env->insn_flags & (INSN_LOONGSON2E | INSN_LOONGSON2F));
> +    ctx.cmgcr = env->CP0_Config3 & (1 << CP0C3_CMGCR);

Wouldn't it be better to follow the style which is used for other ctx
fields? I.e. (x >> y) & 1

Regards,
Leon