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

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

[-- Attachment #1: Type: text/plain, Size: 4935 bytes --]

On Tue, Oct 27, 2015 at 05:12:34PM +0000, 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      |   18 ++++++++++++++++++
>  target-mips/translate_init.c |    3 ++-
>  3 files changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/target-mips/cpu.h b/target-mips/cpu.h
> index c68681d..fdec7b7 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 4cb77de..2f219fa 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,13 @@ 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);
> +            tcg_gen_ld_tl(arg, cpu_env, offsetof(CPUMIPSState, CP0_CMGCRBase));
> +            tcg_gen_ext32s_tl(arg, arg);
> +            rn = "CMGCRBase";
> +            break;
>          default:
>              goto cp0_unimplemented;
>         }
> @@ -6527,6 +6535,12 @@ static void gen_dmfc0(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);
> +            tcg_gen_ld_tl(arg, cpu_env, offsetof(CPUMIPSState, CP0_CMGCRBase));
> +            rn = "CMGCRBase";
> +            break;
>          default:
>              goto cp0_unimplemented;
>          }
> @@ -19568,6 +19582,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 >> CP0C3_CMGCR) & 1;
>      restore_cpu_state(env, &ctx);
>  #ifdef CONFIG_USER_ONLY
>          ctx.mem_idx = MIPS_HFLAG_UM;
> @@ -19956,6 +19971,9 @@ void cpu_state_reset(CPUMIPSState *env)
>      } else {
>          env->CP0_EBase |= 0x80000000;
>      }
> +    if (env->CP0_Config3 & (1 << CP0C3_CMGCR)) {
> +        env->CP0_CMGCRBase = 0x1fbf8000 >> 4;

This is technically hardware configurable, but I'm guessing we can worry
about emulation of platforms that set it to something different later.

> +    }
>      env->CP0_Status = (1 << CP0St_BEV) | (1 << CP0St_ERL);
>      /* vectored interrupts not implemented, timer on int 7,
>         no performance counters. */
> diff --git a/target-mips/translate_init.c b/target-mips/translate_init.c
> index 1b45884..a6b8986 100644
> --- a/target-mips/translate_init.c
> +++ b/target-mips/translate_init.c
> @@ -660,7 +660,8 @@ static const mips_def_t mips_defs[] =
>                         (2 << CP0C1_DS) | (4 << CP0C1_DL) | (3 << CP0C1_DA) |
>                         (0 << CP0C1_PC) | (1 << CP0C1_WR) | (1 << CP0C1_EP),
>          .CP0_Config2 = MIPS_CONFIG2,
> -        .CP0_Config3 = MIPS_CONFIG3 | (1U << CP0C3_M) | (1 << CP0C3_MSAP) |
> +        .CP0_Config3 = MIPS_CONFIG3 | (1U << CP0C3_M) |
> +                       (1 << CP0C3_CMGCR) | (1 << CP0C3_MSAP) |

You're enabling the bit to tell the guest that the CMGCRs are present,
but they're not yet, only the cop0 registers. Perhaps this hunk of the
patch should come after those memory mapped registers are implemented,
so as not to break bisection.

Otherwise:
Reviewed-by: James Hogan <james.hogan@imgtec.com>

Cheers
James

>                         (1 << CP0C3_BP) | (1 << CP0C3_BI) | (1 << CP0C3_ULRI) |
>                         (1 << CP0C3_RXI) | (1 << CP0C3_LPA),
>          .CP0_Config4 = MIPS_CONFIG4 | (1U << CP0C4_M) | (3 << CP0C4_IE) |
> -- 
> 1.7.1
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]