| Message ID | 1301004428-12976-1-git-send-email-dbaryshkov@gmail.com |
|---|---|
| State | New |
| Headers | show |
On 24 March 2011 22:07, Dmitry Eremin-Solenikov <dbaryshkov@gmail.com> wrote: > Currently target-arm/ assumes at least ARMv5 core. Add support for > handling also ARMv4/ARMv4T. This changes the following instructions: Mostly looks good; comments below. > @@ -161,6 +179,8 @@ static void cpu_reset_model_id(CPUARMState *env, uint32_t id) > break; > case ARM_CPUID_TI915T: > case ARM_CPUID_TI925T: > + set_feature(env, ARM_FEATURE_V4T); > + set_feature(env, ARM_FEATURE_V5); > set_feature(env, ARM_FEATURE_OMAPCP); > env->cp15.c0_cpuid = ARM_CPUID_TI925T; /* Depends on wiring. */ > env->cp15.c0_cachetype = 0x5109149; As far as I can tell from google these are based on the ARM9TDMI which means they're ARMv4T and so shouldn't have the V5 feature set. (You can legitimately feel disgruntled that whoever added these didn't do the v4T stuff properly :-)) > @@ -6129,6 +6131,7 @@ static void disas_arm_insn(CPUState * env, DisasContext *s) > } > } > /* Otherwise PLD; v5TE+ */ > + ARCH(5); > return; > } > if (((insn & 0x0f70f000) == 0x0450f000) || Rather than adding ARCH() lines here and in some of the following hunks it would be simpler to change the if (cond == 0xf){ /* Unconditional instructions. */ to: if (cond == 0xf) { /* In ARMv3 and v4 the NV condition is UNPREDICTABLE; we * choose to UNDEF. In ARMv5 and above the space is used * for miscellaneous unconditional instructions. */ ARCH(5); Some bits that are missing from this patch: You need to guard the Thumb BKPT and BLX decodes with ARCH(5) as they're not in v4T. The CPSR Q bit needs to RAZ/WI on v4 and v4T. For v4 you need to make sure that the core can't get into thumb mode at all. So feature guards in gen_bx_imm() and gen_bx(), make sure PSR masks prevent the T bit getting set, and check helper.c for anything that sets env->thumb from somewhere else... -- PMM
On 3/25/11, Peter Maydell <peter.maydell@linaro.org> wrote: > On 24 March 2011 22:07, Dmitry Eremin-Solenikov <dbaryshkov@gmail.com> > wrote: >> Currently target-arm/ assumes at least ARMv5 core. Add support for >> handling also ARMv4/ARMv4T. This changes the following instructions: > > Mostly looks good; comments below. > >> @@ -161,6 +179,8 @@ static void cpu_reset_model_id(CPUARMState *env, >> uint32_t id) >> break; >> case ARM_CPUID_TI915T: >> case ARM_CPUID_TI925T: >> + set_feature(env, ARM_FEATURE_V4T); >> + set_feature(env, ARM_FEATURE_V5); >> set_feature(env, ARM_FEATURE_OMAPCP); >> env->cp15.c0_cpuid = ARM_CPUID_TI925T; /* Depends on wiring. */ >> env->cp15.c0_cachetype = 0x5109149; > > As far as I can tell from google these are based on the ARM9TDMI > which means they're ARMv4T and so shouldn't have the V5 feature set. > (You can legitimately feel disgruntled that whoever added these didn't > do the v4T stuff properly :-)) Oops. According to cpuid they indeed are V4T. > >> @@ -6129,6 +6131,7 @@ static void disas_arm_insn(CPUState * env, >> DisasContext *s) >> } >> } >> /* Otherwise PLD; v5TE+ */ >> + ARCH(5); >> return; >> } >> if (((insn & 0x0f70f000) == 0x0450f000) || > > Rather than adding ARCH() lines here and in some of the following > hunks it would be simpler to change the > > if (cond == 0xf){ > /* Unconditional instructions. */ > > to: > > if (cond == 0xf) { > /* In ARMv3 and v4 the NV condition is UNPREDICTABLE; we > * choose to UNDEF. In ARMv5 and above the space is used > * for miscellaneous unconditional instructions. > */ > ARCH(5); Ack. I just wanted to be insn-by-insn clear, rather than disabling full blocks. > Some bits that are missing from this patch: > > You need to guard the Thumb BKPT and BLX decodes > with ARCH(5) as they're not in v4T. ... and fix the V4T PUSH containing PC. > The CPSR Q bit needs to RAZ/WI on v4 and v4T. Can we assume (maybe temporarily) that all v5 are also v5TE? It seems it's currently done so, and I don't want to be too intrusive. I'll need to dig more into this...
On 26 March 2011 17:23, Dmitry Eremin-Solenikov <dbaryshkov@gmail.com> wrote: > Can we assume (maybe temporarily) that all v5 are also v5TE? > It seems it's currently done so, and I don't want to be too intrusive. All the cores we currently model that are v5 are v5TE, I think. The current (v7) ARM ARM says the valid v5 variants are v5T, v5TE and v5TEJ (with plain "ARMv5" only being in an "obsolete variants" list), so I think we should distinguish v5T and v5TE (the only difference being that a handful of instructions are v5TE only, so that isn't a very intrusive change, it's just saying ARCH(5TE) in a few of the places where your patch has ARCH(5)). So I think we should have ENABLE_ARCH_5T and ENABLE_ARCH_5TE macros so we can use ARCH(5T) and ARCH(5TE), and not bother with a plain ARCH(5) since it's "obsolete"... (Mostly what I'd like is for us to use the right value of 'foo' where we add ARCH(foo) checks, just so we can trust them in future and don't have to go back and recheck them. I don't mind if they all turn out to be checking the same actual feature flag.) -- PMM
On 3/26/11, Peter Maydell <peter.maydell@linaro.org> wrote: > On 26 March 2011 17:23, Dmitry Eremin-Solenikov <dbaryshkov@gmail.com> > wrote: >> Can we assume (maybe temporarily) that all v5 are also v5TE? >> It seems it's currently done so, and I don't want to be too intrusive. > > All the cores we currently model that are v5 are v5TE, I think. > The current (v7) ARM ARM says the valid v5 variants are > v5T, v5TE and v5TEJ (with plain "ARMv5" only being in an > "obsolete variants" list), so I think we should distinguish v5T Isn't there also the v5TExP? For which I know no chips in the wild. > and v5TE (the only difference being that a handful of instructions > are v5TE only, so that isn't a very intrusive change, it's just > saying ARCH(5TE) in a few of the places where your patch has ARCH(5)). > > So I think we should have ENABLE_ARCH_5T and ENABLE_ARCH_5TE macros > so we can use ARCH(5T) and ARCH(5TE), and not bother with a plain > ARCH(5) since it's "obsolete"... > > (Mostly what I'd like is for us to use the right value of 'foo' > where we add ARCH(foo) checks, just so we can trust them in future > and don't have to go back and recheck them. I don't mind if they > all turn out to be checking the same actual feature flag.) OK. I can then try to check all ARCH(5), substituting them if necessary with ARCH(5TE) or (5T), but for now this will just end with check for ARM_FEATURE_V5. Did I get your idea correct? But this (most probably) will be more or less with low priority patch idea for me.
I've just gone through this distinguishing v5 sublevels. I've also gone back and looked up an older ARM ARM for any v5 vs v5T differences, and it looks like the only difference really is whether Thumb mode works: the ARM instruction set is exactly the same including the existence of BX/BLX. So I'm going to go back on what I suggested earlier, and say that I think leaving it as ARCH(5) is better than ARCH(5T). Sorry for the flip-flopping here. I've marked up all the ARCH() uses in this patch, even the bits which are correct as they stand, just for clarity. The rough summary is that five lines need to change to 5TE. On "v5TExP" -- yes, that's another one in the v7 ARM ARM's list of "obsolete" variants. On 24 March 2011 22:07, Dmitry Eremin-Solenikov <dbaryshkov@gmail.com> wrote: > @@ -6129,6 +6131,7 @@ static void disas_arm_insn(CPUState * env, DisasContext *s) > } > } > /* Otherwise PLD; v5TE+ */ > + ARCH(5); 5TE. > return; > } > if (((insn & 0x0f70f000) == 0x0450f000) || > @@ -6255,6 +6258,7 @@ static void disas_arm_insn(CPUState * env, DisasContext *s) > /* branch link and change to thumb (blx <offset>) */ > int32_t offset; > > + ARCH(5); 5, so delete as covered by the top level ARCH(5) for any unconditional insn. > val = (uint32_t)s->pc; > tmp = tcg_temp_new_i32(); > tcg_gen_movi_i32(tmp, val); > @@ -6268,6 +6272,7 @@ static void disas_arm_insn(CPUState * env, DisasContext *s) > gen_bx_im(s, val); > return; > } else if ((insn & 0x0e000f00) == 0x0c000100) { > + ARCH(5); Can remove, IWMMXT implies 5 anyway. > if (arm_feature(env, ARM_FEATURE_IWMMXT)) { > /* iWMMXt register transfer. */ > if (env->cp15.c15_cpar & (1 << 1)) > @@ -6276,8 +6281,10 @@ static void disas_arm_insn(CPUState * env, DisasContext *s) > } > } else if ((insn & 0x0fe00000) == 0x0c400000) { > /* Coprocessor double register transfer. */ > + ARCH(5); 5TE. > } else if ((insn & 0x0f000010) == 0x0e000010) { > /* Additional coprocessor register transfer. */ > + ARCH(5); 5 (so deletable). > } else if ((insn & 0x0ff10020) == 0x01000000) { > uint32_t mask; > uint32_t val; > @@ -6376,10 +6383,12 @@ static void disas_arm_insn(CPUState * env, DisasContext *s) > case 0x1: > if (op1 == 1) { > /* branch/exchange thumb (bx). */ > + ARCH(4T); 4T. > tmp = load_reg(s, rm); > gen_bx(s, tmp); > } else if (op1 == 3) { > /* clz */ > + ARCH(5); 5. > rd = (insn >> 12) & 0xf; > tmp = load_reg(s, rm); > gen_helper_clz(tmp, tmp); > @@ -6402,6 +6411,7 @@ static void disas_arm_insn(CPUState * env, DisasContext *s) > if (op1 != 1) > goto illegal_op; > > + ARCH(5); 5. (the v5 ARM ARM says BLX works on a non-T v5, it just means you go into a state where everything undefs). > /* branch link/exchange thumb (blx) */ > tmp = load_reg(s, rm); > tmp2 = tcg_temp_new_i32(); > @@ -6410,6 +6420,7 @@ static void disas_arm_insn(CPUState * env, DisasContext *s) > gen_bx(s, tmp); > break; > case 0x5: /* saturating add/subtract */ > + ARCH(5); 5TE. > rd = (insn >> 12) & 0xf; > rn = (insn >> 16) & 0xf; > tmp = load_reg(s, rm); > @@ -6431,12 +6442,14 @@ static void disas_arm_insn(CPUState * env, DisasContext *s) > goto illegal_op; > } > /* bkpt */ > + ARCH(5); 5. > gen_exception_insn(s, 4, EXCP_BKPT); > break; > case 0x8: /* signed multiply */ > case 0xa: > case 0xc: > case 0xe: > + ARCH(5); 5TE. > rs = (insn >> 8) & 0xf; > rn = (insn >> 12) & 0xf; > rd = (insn >> 16) & 0xf; > @@ -6832,6 +6845,7 @@ static void disas_arm_insn(CPUState * env, DisasContext *s) > } > load = 1; > } else if (sh & 2) { > + ARCH(5); 5TE. > /* doubleword */ > if (sh & 1) { > /* store */ -- PMM
Hello, On 3/27/11, Peter Maydell <peter.maydell@linaro.org> wrote: > I've just gone through this distinguishing v5 sublevels. > I've also gone back and looked up an older ARM ARM for any v5 vs > v5T differences, and it looks like the only difference really is > whether Thumb mode works: the ARM instruction set is exactly the > same including the existence of BX/BLX. Will submit the updated patchet in a few minutes. BTW: do you know any real core which used ARMv5/ARMv5T and not ARMv5TE (I've failed to find such one. Maybe I should check older revisions of datasheets: at least 946r0 was ARMv5TExP and r1p1 is ARM9E-S-based and so full ARMv5TE).
diff --git a/target-arm/cpu.h b/target-arm/cpu.h index 1ae7982..e247a7a 100644 --- a/target-arm/cpu.h +++ b/target-arm/cpu.h @@ -360,7 +360,9 @@ enum arm_features { ARM_FEATURE_M, /* Microcontroller profile. */ ARM_FEATURE_OMAPCP, /* OMAP specific CP15 ops handling. */ ARM_FEATURE_THUMB2EE, - ARM_FEATURE_V7MP /* v7 Multiprocessing Extensions */ + ARM_FEATURE_V7MP, /* v7 Multiprocessing Extensions */ + ARM_FEATURE_V4T, + ARM_FEATURE_V5, }; static inline int arm_feature(CPUARMState *env, int feature) diff --git a/target-arm/helper.c b/target-arm/helper.c index 78f3d39..49ff5cf 100644 --- a/target-arm/helper.c +++ b/target-arm/helper.c @@ -48,17 +48,23 @@ static void cpu_reset_model_id(CPUARMState *env, uint32_t id) env->cp15.c0_cpuid = id; switch (id) { case ARM_CPUID_ARM926: + set_feature(env, ARM_FEATURE_V4T); + set_feature(env, ARM_FEATURE_V5); set_feature(env, ARM_FEATURE_VFP); env->vfp.xregs[ARM_VFP_FPSID] = 0x41011090; env->cp15.c0_cachetype = 0x1dd20d2; env->cp15.c1_sys = 0x00090078; break; case ARM_CPUID_ARM946: + set_feature(env, ARM_FEATURE_V4T); + set_feature(env, ARM_FEATURE_V5); set_feature(env, ARM_FEATURE_MPU); env->cp15.c0_cachetype = 0x0f004006; env->cp15.c1_sys = 0x00000078; break; case ARM_CPUID_ARM1026: + set_feature(env, ARM_FEATURE_V4T); + set_feature(env, ARM_FEATURE_V5); set_feature(env, ARM_FEATURE_VFP); set_feature(env, ARM_FEATURE_AUXCR); env->vfp.xregs[ARM_VFP_FPSID] = 0x410110a0; @@ -67,6 +73,8 @@ static void cpu_reset_model_id(CPUARMState *env, uint32_t id) break; case ARM_CPUID_ARM1136_R2: case ARM_CPUID_ARM1136: + set_feature(env, ARM_FEATURE_V4T); + set_feature(env, ARM_FEATURE_V5); set_feature(env, ARM_FEATURE_V6); set_feature(env, ARM_FEATURE_VFP); set_feature(env, ARM_FEATURE_AUXCR); @@ -79,6 +87,8 @@ static void cpu_reset_model_id(CPUARMState *env, uint32_t id) env->cp15.c1_sys = 0x00050078; break; case ARM_CPUID_ARM11MPCORE: + set_feature(env, ARM_FEATURE_V4T); + set_feature(env, ARM_FEATURE_V5); set_feature(env, ARM_FEATURE_V6); set_feature(env, ARM_FEATURE_V6K); set_feature(env, ARM_FEATURE_VFP); @@ -91,6 +101,8 @@ static void cpu_reset_model_id(CPUARMState *env, uint32_t id) env->cp15.c0_cachetype = 0x1dd20d2; break; case ARM_CPUID_CORTEXA8: + set_feature(env, ARM_FEATURE_V4T); + set_feature(env, ARM_FEATURE_V5); set_feature(env, ARM_FEATURE_V6); set_feature(env, ARM_FEATURE_V6K); set_feature(env, ARM_FEATURE_V7); @@ -113,6 +125,8 @@ static void cpu_reset_model_id(CPUARMState *env, uint32_t id) env->cp15.c1_sys = 0x00c50078; break; case ARM_CPUID_CORTEXA9: + set_feature(env, ARM_FEATURE_V4T); + set_feature(env, ARM_FEATURE_V5); set_feature(env, ARM_FEATURE_V6); set_feature(env, ARM_FEATURE_V6K); set_feature(env, ARM_FEATURE_V7); @@ -140,6 +154,8 @@ static void cpu_reset_model_id(CPUARMState *env, uint32_t id) env->cp15.c1_sys = 0x00c50078; break; case ARM_CPUID_CORTEXM3: + set_feature(env, ARM_FEATURE_V4T); + set_feature(env, ARM_FEATURE_V5); set_feature(env, ARM_FEATURE_V6); set_feature(env, ARM_FEATURE_THUMB2); set_feature(env, ARM_FEATURE_V7); @@ -147,6 +163,8 @@ static void cpu_reset_model_id(CPUARMState *env, uint32_t id) set_feature(env, ARM_FEATURE_DIV); break; case ARM_CPUID_ANY: /* For userspace emulation. */ + set_feature(env, ARM_FEATURE_V4T); + set_feature(env, ARM_FEATURE_V5); set_feature(env, ARM_FEATURE_V6); set_feature(env, ARM_FEATURE_V6K); set_feature(env, ARM_FEATURE_V7); @@ -161,6 +179,8 @@ static void cpu_reset_model_id(CPUARMState *env, uint32_t id) break; case ARM_CPUID_TI915T: case ARM_CPUID_TI925T: + set_feature(env, ARM_FEATURE_V4T); + set_feature(env, ARM_FEATURE_V5); set_feature(env, ARM_FEATURE_OMAPCP); env->cp15.c0_cpuid = ARM_CPUID_TI925T; /* Depends on wiring. */ env->cp15.c0_cachetype = 0x5109149; @@ -173,6 +193,8 @@ static void cpu_reset_model_id(CPUARMState *env, uint32_t id) case ARM_CPUID_PXA260: case ARM_CPUID_PXA261: case ARM_CPUID_PXA262: + set_feature(env, ARM_FEATURE_V4T); + set_feature(env, ARM_FEATURE_V5); set_feature(env, ARM_FEATURE_XSCALE); /* JTAG_ID is ((id << 28) | 0x09265013) */ env->cp15.c0_cachetype = 0xd172172; @@ -184,6 +206,8 @@ static void cpu_reset_model_id(CPUARMState *env, uint32_t id) case ARM_CPUID_PXA270_B1: case ARM_CPUID_PXA270_C0: case ARM_CPUID_PXA270_C5: + set_feature(env, ARM_FEATURE_V4T); + set_feature(env, ARM_FEATURE_V5); set_feature(env, ARM_FEATURE_XSCALE); /* JTAG_ID is ((id << 28) | 0x09265013) */ set_feature(env, ARM_FEATURE_IWMMXT); diff --git a/target-arm/translate.c b/target-arm/translate.c index f69912f..c9df87e 100644 --- a/target-arm/translate.c +++ b/target-arm/translate.c @@ -34,6 +34,8 @@ #define GEN_HELPER 1 #include "helpers.h" +#define ENABLE_ARCH_4T arm_feature(env, ARM_FEATURE_V4T) +#define ENABLE_ARCH_5 arm_feature(env, ARM_FEATURE_V5) #define ENABLE_ARCH_5J 0 #define ENABLE_ARCH_6 arm_feature(env, ARM_FEATURE_V6) #define ENABLE_ARCH_6K arm_feature(env, ARM_FEATURE_V6K) @@ -6129,6 +6131,7 @@ static void disas_arm_insn(CPUState * env, DisasContext *s) } } /* Otherwise PLD; v5TE+ */ + ARCH(5); return; } if (((insn & 0x0f70f000) == 0x0450f000) || @@ -6255,6 +6258,7 @@ static void disas_arm_insn(CPUState * env, DisasContext *s) /* branch link and change to thumb (blx <offset>) */ int32_t offset; + ARCH(5); val = (uint32_t)s->pc; tmp = tcg_temp_new_i32(); tcg_gen_movi_i32(tmp, val); @@ -6268,6 +6272,7 @@ static void disas_arm_insn(CPUState * env, DisasContext *s) gen_bx_im(s, val); return; } else if ((insn & 0x0e000f00) == 0x0c000100) { + ARCH(5); if (arm_feature(env, ARM_FEATURE_IWMMXT)) { /* iWMMXt register transfer. */ if (env->cp15.c15_cpar & (1 << 1)) @@ -6276,8 +6281,10 @@ static void disas_arm_insn(CPUState * env, DisasContext *s) } } else if ((insn & 0x0fe00000) == 0x0c400000) { /* Coprocessor double register transfer. */ + ARCH(5); } else if ((insn & 0x0f000010) == 0x0e000010) { /* Additional coprocessor register transfer. */ + ARCH(5); } else if ((insn & 0x0ff10020) == 0x01000000) { uint32_t mask; uint32_t val; @@ -6376,10 +6383,12 @@ static void disas_arm_insn(CPUState * env, DisasContext *s) case 0x1: if (op1 == 1) { /* branch/exchange thumb (bx). */ + ARCH(4T); tmp = load_reg(s, rm); gen_bx(s, tmp); } else if (op1 == 3) { /* clz */ + ARCH(5); rd = (insn >> 12) & 0xf; tmp = load_reg(s, rm); gen_helper_clz(tmp, tmp); @@ -6402,6 +6411,7 @@ static void disas_arm_insn(CPUState * env, DisasContext *s) if (op1 != 1) goto illegal_op; + ARCH(5); /* branch link/exchange thumb (blx) */ tmp = load_reg(s, rm); tmp2 = tcg_temp_new_i32(); @@ -6410,6 +6420,7 @@ static void disas_arm_insn(CPUState * env, DisasContext *s) gen_bx(s, tmp); break; case 0x5: /* saturating add/subtract */ + ARCH(5); rd = (insn >> 12) & 0xf; rn = (insn >> 16) & 0xf; tmp = load_reg(s, rm); @@ -6431,12 +6442,14 @@ static void disas_arm_insn(CPUState * env, DisasContext *s) goto illegal_op; } /* bkpt */ + ARCH(5); gen_exception_insn(s, 4, EXCP_BKPT); break; case 0x8: /* signed multiply */ case 0xa: case 0xc: case 0xe: + ARCH(5); rs = (insn >> 8) & 0xf; rn = (insn >> 12) & 0xf; rd = (insn >> 16) & 0xf; @@ -6832,6 +6845,7 @@ static void disas_arm_insn(CPUState * env, DisasContext *s) } load = 1; } else if (sh & 2) { + ARCH(5); /* doubleword */ if (sh & 1) { /* store */ @@ -7172,10 +7186,11 @@ static void disas_arm_insn(CPUState * env, DisasContext *s) } if (insn & (1 << 20)) { /* Complete the load. */ - if (rd == 15) + if (rd == 15 && ENABLE_ARCH_4T) { gen_bx(s, tmp); - else + } else { store_reg(s, rd, tmp); + } } break; case 0x08: @@ -7229,7 +7244,11 @@ static void disas_arm_insn(CPUState * env, DisasContext *s) /* load */ tmp = gen_ld32(addr, IS_USER(s)); if (i == 15) { - gen_bx(s, tmp); + if (ENABLE_ARCH_4T) { + gen_bx(s, tmp); + } else { + store_reg(s, i, tmp); + } } else if (user) { tmp2 = tcg_const_i32(i); gen_helper_set_user_reg(tmp2, tmp);
Currently target-arm/ assumes at least ARMv5 core. Add support for handling also ARMv4/ARMv4T. This changes the following instructions: BX(v4T and later) BKPT, BLX, CDP2, CLZ, LDC2, LDRD, MCRR, MCRR2, MRRC, MCRR, MRC2, MRRC, MRRC2, PLD QADD, QDADD, QDSUB, QSUB, STRD, SMLAxy, SMLALxy, SMLAWxy, SMULxy, SMULWxy, STC2 (v5 and later) All instructions that are "v5TE and later" are also bound to just v5, as that's how it was before. This patch doesn _not_ include disabling of cp15 access and base-updated data abort model (that will be required to emulate chips based on a ARM7TDMI), because: * no ARM7TDMI chips are currently emulated (or planned) * those features aren't strictly necessary for my purposes (SA-1 core emulation). Patch is heavily based on patch by Filip Navara <filip.navara@gmail.com> which in turn is based on work by Ulrich Hecht <uli@suse.de> and Vincent Sanders <vince@kyllikki.org>. Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com> --- target-arm/cpu.h | 4 +++- target-arm/helper.c | 24 ++++++++++++++++++++++++ target-arm/translate.c | 25 ++++++++++++++++++++++--- 3 files changed, 49 insertions(+), 4 deletions(-)