| Message ID | 20180629001538.11415-6-richard.henderson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | target/arm SVE updates | expand |
Hi Richard, On 06/28/2018 09:15 PM, Richard Henderson wrote: > This register was added to aa32 state by ARMv8.2. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > target/arm/cpu.h | 1 + > target/arm/cpu.c | 4 ++++ > target/arm/cpu64.c | 2 ++ > target/arm/helper.c | 5 ++--- > 4 files changed, 9 insertions(+), 3 deletions(-) > > diff --git a/target/arm/cpu.h b/target/arm/cpu.h > index 6a8441c2dd..1505ac936e 100644 > --- a/target/arm/cpu.h > +++ b/target/arm/cpu.h > @@ -813,6 +813,7 @@ struct ARMCPU { > uint32_t id_isar3; > uint32_t id_isar4; > uint32_t id_isar5; > + uint32_t id_isar6; > uint64_t id_aa64pfr0; > uint64_t id_aa64pfr1; > uint64_t id_aa64dfr0; > diff --git a/target/arm/cpu.c b/target/arm/cpu.c > index 878cc6c7e8..de1a07a9f1 100644 > --- a/target/arm/cpu.c > +++ b/target/arm/cpu.c > @@ -1262,6 +1262,7 @@ static void cortex_m3_initfn(Object *obj) > cpu->id_isar3 = 0x01111110; > cpu->id_isar4 = 0x01310102; > cpu->id_isar5 = 0x00000000; > + cpu->id_isar6 = 0x00000000; > } > > static void cortex_m4_initfn(Object *obj) > @@ -1288,6 +1289,7 @@ static void cortex_m4_initfn(Object *obj) > cpu->id_isar3 = 0x01111110; > cpu->id_isar4 = 0x01310102; > cpu->id_isar5 = 0x00000000; > + cpu->id_isar6 = 0x00000000; > } > > static void cortex_m33_initfn(Object *obj) > @@ -1316,6 +1318,7 @@ static void cortex_m33_initfn(Object *obj) > cpu->id_isar3 = 0x01111131; > cpu->id_isar4 = 0x01310132; > cpu->id_isar5 = 0x00000000; > + cpu->id_isar6 = 0x00000000; > cpu->clidr = 0x00000000; > cpu->ctr = 0x8000c000; > } > @@ -1366,6 +1369,7 @@ static void cortex_r5_initfn(Object *obj) > cpu->id_isar3 = 0x01112131; > cpu->id_isar4 = 0x0010142; > cpu->id_isar5 = 0x0; > + cpu->id_isar6 = 0x0; > cpu->mp_is_up = true; > cpu->pmsav7_dregion = 16; > define_arm_cp_regs(cpu, cortexr5_cp_reginfo); > diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c > index 8040493d5c..d0581d59d8 100644 > --- a/target/arm/cpu64.c > +++ b/target/arm/cpu64.c > @@ -139,6 +139,7 @@ static void aarch64_a57_initfn(Object *obj) > cpu->id_isar3 = 0x01112131; > cpu->id_isar4 = 0x00011142; > cpu->id_isar5 = 0x00011121; > + cpu->id_isar6 = 0; > cpu->id_aa64pfr0 = 0x00002222; > cpu->id_aa64dfr0 = 0x10305106; > cpu->pmceid0 = 0x00000000; > @@ -199,6 +200,7 @@ static void aarch64_a53_initfn(Object *obj) > cpu->id_isar3 = 0x01112131; > cpu->id_isar4 = 0x00011142; > cpu->id_isar5 = 0x00011121; > + cpu->id_isar6 = 0; > cpu->id_aa64pfr0 = 0x00002222; > cpu->id_aa64dfr0 = 0x10305106; > cpu->id_aa64isar0 = 0x00011120; > diff --git a/target/arm/helper.c b/target/arm/helper.c > index a855da045b..e62f02d4e5 100644 > --- a/target/arm/helper.c > +++ b/target/arm/helper.c > @@ -4851,11 +4851,10 @@ void register_cp_regs_for_features(ARMCPU *cpu) > .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 2, .opc2 = 6, > .access = PL1_R, .type = ARM_CP_CONST, > .resetvalue = cpu->id_mmfr4 }, > - /* 7 is as yet unallocated and must RAZ */ > - { .name = "ID_ISAR7_RESERVED", .state = ARM_CP_STATE_BOTH, You added this in if (arm_feature(env, ARM_FEATURE_V6)) { Shouldn't this stay same and a new entry added later in if (arm_feature(env, ARM_FEATURE_V8)) { ? > + { .name = "ID_ISAR6", .state = ARM_CP_STATE_BOTH, If added under ARM_FEATURE_V8, shouldn't this be ARM_CP_STATE_AA32? > .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 2, .opc2 = 7, > .access = PL1_R, .type = ARM_CP_CONST, > - .resetvalue = 0 }, > + .resetvalue = cpu->id_isar6 }, > REGINFO_SENTINEL > }; > define_arm_cp_regs(cpu, v6_idregs); >
On 06/28/2018 09:57 PM, Philippe Mathieu-Daudé wrote: > Hi Richard, > > On 06/28/2018 09:15 PM, Richard Henderson wrote: >> This register was added to aa32 state by ARMv8.2. >> >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> >> --- >> target/arm/cpu.h | 1 + >> target/arm/cpu.c | 4 ++++ >> target/arm/cpu64.c | 2 ++ >> target/arm/helper.c | 5 ++--- >> 4 files changed, 9 insertions(+), 3 deletions(-) >> >> diff --git a/target/arm/cpu.h b/target/arm/cpu.h >> index 6a8441c2dd..1505ac936e 100644 >> --- a/target/arm/cpu.h >> +++ b/target/arm/cpu.h >> @@ -813,6 +813,7 @@ struct ARMCPU { >> uint32_t id_isar3; >> uint32_t id_isar4; >> uint32_t id_isar5; >> + uint32_t id_isar6; >> uint64_t id_aa64pfr0; >> uint64_t id_aa64pfr1; >> uint64_t id_aa64dfr0; >> diff --git a/target/arm/cpu.c b/target/arm/cpu.c >> index 878cc6c7e8..de1a07a9f1 100644 >> --- a/target/arm/cpu.c >> +++ b/target/arm/cpu.c >> @@ -1262,6 +1262,7 @@ static void cortex_m3_initfn(Object *obj) >> cpu->id_isar3 = 0x01111110; >> cpu->id_isar4 = 0x01310102; >> cpu->id_isar5 = 0x00000000; >> + cpu->id_isar6 = 0x00000000; >> } >> >> static void cortex_m4_initfn(Object *obj) >> @@ -1288,6 +1289,7 @@ static void cortex_m4_initfn(Object *obj) >> cpu->id_isar3 = 0x01111110; >> cpu->id_isar4 = 0x01310102; >> cpu->id_isar5 = 0x00000000; >> + cpu->id_isar6 = 0x00000000; >> } >> >> static void cortex_m33_initfn(Object *obj) >> @@ -1316,6 +1318,7 @@ static void cortex_m33_initfn(Object *obj) >> cpu->id_isar3 = 0x01111131; >> cpu->id_isar4 = 0x01310132; >> cpu->id_isar5 = 0x00000000; >> + cpu->id_isar6 = 0x00000000; >> cpu->clidr = 0x00000000; >> cpu->ctr = 0x8000c000; >> } >> @@ -1366,6 +1369,7 @@ static void cortex_r5_initfn(Object *obj) >> cpu->id_isar3 = 0x01112131; >> cpu->id_isar4 = 0x0010142; >> cpu->id_isar5 = 0x0; >> + cpu->id_isar6 = 0x0; >> cpu->mp_is_up = true; >> cpu->pmsav7_dregion = 16; >> define_arm_cp_regs(cpu, cortexr5_cp_reginfo); >> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c >> index 8040493d5c..d0581d59d8 100644 >> --- a/target/arm/cpu64.c >> +++ b/target/arm/cpu64.c >> @@ -139,6 +139,7 @@ static void aarch64_a57_initfn(Object *obj) >> cpu->id_isar3 = 0x01112131; >> cpu->id_isar4 = 0x00011142; >> cpu->id_isar5 = 0x00011121; >> + cpu->id_isar6 = 0; >> cpu->id_aa64pfr0 = 0x00002222; >> cpu->id_aa64dfr0 = 0x10305106; >> cpu->pmceid0 = 0x00000000; >> @@ -199,6 +200,7 @@ static void aarch64_a53_initfn(Object *obj) >> cpu->id_isar3 = 0x01112131; >> cpu->id_isar4 = 0x00011142; >> cpu->id_isar5 = 0x00011121; >> + cpu->id_isar6 = 0; >> cpu->id_aa64pfr0 = 0x00002222; >> cpu->id_aa64dfr0 = 0x10305106; >> cpu->id_aa64isar0 = 0x00011120; >> diff --git a/target/arm/helper.c b/target/arm/helper.c >> index a855da045b..e62f02d4e5 100644 >> --- a/target/arm/helper.c >> +++ b/target/arm/helper.c >> @@ -4851,11 +4851,10 @@ void register_cp_regs_for_features(ARMCPU *cpu) >> .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 2, .opc2 = 6, >> .access = PL1_R, .type = ARM_CP_CONST, >> .resetvalue = cpu->id_mmfr4 }, >> - /* 7 is as yet unallocated and must RAZ */ >> - { .name = "ID_ISAR7_RESERVED", .state = ARM_CP_STATE_BOTH, > > You added this in > > if (arm_feature(env, ARM_FEATURE_V6)) { > > Shouldn't this stay same and a new entry added later in > > if (arm_feature(env, ARM_FEATURE_V8)) { > > ? I now applied this on top of your "target/arm SVE patches" v6 but still see this under the ARM_FEATURE_V6 feature check. > >> + { .name = "ID_ISAR6", .state = ARM_CP_STATE_BOTH, > > If added under ARM_FEATURE_V8, shouldn't this be ARM_CP_STATE_AA32? > >> .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 2, .opc2 = 7, >> .access = PL1_R, .type = ARM_CP_CONST, >> - .resetvalue = 0 }, >> + .resetvalue = cpu->id_isar6 }, >> REGINFO_SENTINEL >> }; >> define_arm_cp_regs(cpu, v6_idregs); >>
On 06/28/2018 05:57 PM, Philippe Mathieu-Daudé wrote: >> @@ -4851,11 +4851,10 @@ void register_cp_regs_for_features(ARMCPU *cpu) >> .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 2, .opc2 = 6, >> .access = PL1_R, .type = ARM_CP_CONST, >> .resetvalue = cpu->id_mmfr4 }, >> - /* 7 is as yet unallocated and must RAZ */ >> - { .name = "ID_ISAR7_RESERVED", .state = ARM_CP_STATE_BOTH, > > You added this in > > if (arm_feature(env, ARM_FEATURE_V6)) { > > Shouldn't this stay same and a new entry added later in > > if (arm_feature(env, ARM_FEATURE_V8)) { > > ? > >> + { .name = "ID_ISAR6", .state = ARM_CP_STATE_BOTH, > > If added under ARM_FEATURE_V8, shouldn't this be ARM_CP_STATE_AA32? You'll notice that I'm replacing an entry that was previously marked "reserved". I'm not going to second-guess that. r~
On 29 June 2018 at 01:15, Richard Henderson <richard.henderson@linaro.org> wrote: > This register was added to aa32 state by ARMv8.2. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > target/arm/cpu.h | 1 + > target/arm/cpu.c | 4 ++++ > target/arm/cpu64.c | 2 ++ > target/arm/helper.c | 5 ++--- > 4 files changed, 9 insertions(+), 3 deletions(-) > > diff --git a/target/arm/cpu.h b/target/arm/cpu.h > index 6a8441c2dd..1505ac936e 100644 > --- a/target/arm/cpu.h > +++ b/target/arm/cpu.h > @@ -813,6 +813,7 @@ struct ARMCPU { > uint32_t id_isar3; > uint32_t id_isar4; > uint32_t id_isar5; > + uint32_t id_isar6; > uint64_t id_aa64pfr0; > uint64_t id_aa64pfr1; > uint64_t id_aa64dfr0; > diff --git a/target/arm/cpu.c b/target/arm/cpu.c > index 878cc6c7e8..de1a07a9f1 100644 > --- a/target/arm/cpu.c > +++ b/target/arm/cpu.c > @@ -1262,6 +1262,7 @@ static void cortex_m3_initfn(Object *obj) > cpu->id_isar3 = 0x01111110; > cpu->id_isar4 = 0x01310102; > cpu->id_isar5 = 0x00000000; > + cpu->id_isar6 = 0x00000000; > } > > static void cortex_m4_initfn(Object *obj) > @@ -1288,6 +1289,7 @@ static void cortex_m4_initfn(Object *obj) > cpu->id_isar3 = 0x01111110; > cpu->id_isar4 = 0x01310102; > cpu->id_isar5 = 0x00000000; > + cpu->id_isar6 = 0x00000000; > } > > static void cortex_m33_initfn(Object *obj) > @@ -1316,6 +1318,7 @@ static void cortex_m33_initfn(Object *obj) > cpu->id_isar3 = 0x01111131; > cpu->id_isar4 = 0x01310132; > cpu->id_isar5 = 0x00000000; > + cpu->id_isar6 = 0x00000000; > cpu->clidr = 0x00000000; > cpu->ctr = 0x8000c000; > } > @@ -1366,6 +1369,7 @@ static void cortex_r5_initfn(Object *obj) > cpu->id_isar3 = 0x01112131; > cpu->id_isar4 = 0x0010142; > cpu->id_isar5 = 0x0; > + cpu->id_isar6 = 0x0; > cpu->mp_is_up = true; > cpu->pmsav7_dregion = 16; > define_arm_cp_regs(cpu, cortexr5_cp_reginfo); > diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c > index 8040493d5c..d0581d59d8 100644 > --- a/target/arm/cpu64.c > +++ b/target/arm/cpu64.c > @@ -139,6 +139,7 @@ static void aarch64_a57_initfn(Object *obj) > cpu->id_isar3 = 0x01112131; > cpu->id_isar4 = 0x00011142; > cpu->id_isar5 = 0x00011121; > + cpu->id_isar6 = 0; > cpu->id_aa64pfr0 = 0x00002222; > cpu->id_aa64dfr0 = 0x10305106; > cpu->pmceid0 = 0x00000000; > @@ -199,6 +200,7 @@ static void aarch64_a53_initfn(Object *obj) > cpu->id_isar3 = 0x01112131; > cpu->id_isar4 = 0x00011142; > cpu->id_isar5 = 0x00011121; > + cpu->id_isar6 = 0; > cpu->id_aa64pfr0 = 0x00002222; > cpu->id_aa64dfr0 = 0x10305106; > cpu->id_aa64isar0 = 0x00011120; The ARMCPU struct fields should all be initially cleared to zero, so you don't really need to explicitly zero-initialize isar6 all over the place like this. (Compare isar5, which is I think only set in CPUs that are new enough that their TRMs mentioned it.) In particular it's somewhat out of place to zero it in the m3/m4/m33 initfns, since there is no such register in the v8M Arm ARM. On the other hand it doesn't hurt. > diff --git a/target/arm/helper.c b/target/arm/helper.c > index a855da045b..e62f02d4e5 100644 > --- a/target/arm/helper.c > +++ b/target/arm/helper.c > @@ -4851,11 +4851,10 @@ void register_cp_regs_for_features(ARMCPU *cpu) > .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 2, .opc2 = 6, > .access = PL1_R, .type = ARM_CP_CONST, > .resetvalue = cpu->id_mmfr4 }, > - /* 7 is as yet unallocated and must RAZ */ > - { .name = "ID_ISAR7_RESERVED", .state = ARM_CP_STATE_BOTH, > + { .name = "ID_ISAR6", .state = ARM_CP_STATE_BOTH, > .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 2, .opc2 = 7, > .access = PL1_R, .type = ARM_CP_CONST, > - .resetvalue = 0 }, > + .resetvalue = cpu->id_isar6 }, > REGINFO_SENTINEL > }; > define_arm_cp_regs(cpu, v6_idregs); Reviewed-by: Peter Maydell <peter.maydell@linaro.org> thanks -- PMM
On 06/29/2018 01:40 AM, Peter Maydell wrote: >> cpu->id_isar5 = 0x00000000; >> + cpu->id_isar6 = 0x00000000; ... >> cpu->id_isar5 = 0x00000000; >> + cpu->id_isar6 = 0x00000000; ... >> cpu->id_isar5 = 0x00000000; >> + cpu->id_isar6 = 0x00000000; ... >> cpu->id_isar5 = 0x0; >> + cpu->id_isar6 = 0x0; ... > The ARMCPU struct fields should all be initially cleared to > zero, so you don't really need to explicitly zero-initialize > isar6 all over the place like this. (Compare isar5, which is > I think only set in CPUs that are new enough that their TRMs > mentioned it.) Actually, the fact that isar5 is explicitly set to 0 in many places is the reason that I initialize isar6. r~
diff --git a/target/arm/cpu.h b/target/arm/cpu.h index 6a8441c2dd..1505ac936e 100644 --- a/target/arm/cpu.h +++ b/target/arm/cpu.h @@ -813,6 +813,7 @@ struct ARMCPU { uint32_t id_isar3; uint32_t id_isar4; uint32_t id_isar5; + uint32_t id_isar6; uint64_t id_aa64pfr0; uint64_t id_aa64pfr1; uint64_t id_aa64dfr0; diff --git a/target/arm/cpu.c b/target/arm/cpu.c index 878cc6c7e8..de1a07a9f1 100644 --- a/target/arm/cpu.c +++ b/target/arm/cpu.c @@ -1262,6 +1262,7 @@ static void cortex_m3_initfn(Object *obj) cpu->id_isar3 = 0x01111110; cpu->id_isar4 = 0x01310102; cpu->id_isar5 = 0x00000000; + cpu->id_isar6 = 0x00000000; } static void cortex_m4_initfn(Object *obj) @@ -1288,6 +1289,7 @@ static void cortex_m4_initfn(Object *obj) cpu->id_isar3 = 0x01111110; cpu->id_isar4 = 0x01310102; cpu->id_isar5 = 0x00000000; + cpu->id_isar6 = 0x00000000; } static void cortex_m33_initfn(Object *obj) @@ -1316,6 +1318,7 @@ static void cortex_m33_initfn(Object *obj) cpu->id_isar3 = 0x01111131; cpu->id_isar4 = 0x01310132; cpu->id_isar5 = 0x00000000; + cpu->id_isar6 = 0x00000000; cpu->clidr = 0x00000000; cpu->ctr = 0x8000c000; } @@ -1366,6 +1369,7 @@ static void cortex_r5_initfn(Object *obj) cpu->id_isar3 = 0x01112131; cpu->id_isar4 = 0x0010142; cpu->id_isar5 = 0x0; + cpu->id_isar6 = 0x0; cpu->mp_is_up = true; cpu->pmsav7_dregion = 16; define_arm_cp_regs(cpu, cortexr5_cp_reginfo); diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c index 8040493d5c..d0581d59d8 100644 --- a/target/arm/cpu64.c +++ b/target/arm/cpu64.c @@ -139,6 +139,7 @@ static void aarch64_a57_initfn(Object *obj) cpu->id_isar3 = 0x01112131; cpu->id_isar4 = 0x00011142; cpu->id_isar5 = 0x00011121; + cpu->id_isar6 = 0; cpu->id_aa64pfr0 = 0x00002222; cpu->id_aa64dfr0 = 0x10305106; cpu->pmceid0 = 0x00000000; @@ -199,6 +200,7 @@ static void aarch64_a53_initfn(Object *obj) cpu->id_isar3 = 0x01112131; cpu->id_isar4 = 0x00011142; cpu->id_isar5 = 0x00011121; + cpu->id_isar6 = 0; cpu->id_aa64pfr0 = 0x00002222; cpu->id_aa64dfr0 = 0x10305106; cpu->id_aa64isar0 = 0x00011120; diff --git a/target/arm/helper.c b/target/arm/helper.c index a855da045b..e62f02d4e5 100644 --- a/target/arm/helper.c +++ b/target/arm/helper.c @@ -4851,11 +4851,10 @@ void register_cp_regs_for_features(ARMCPU *cpu) .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 2, .opc2 = 6, .access = PL1_R, .type = ARM_CP_CONST, .resetvalue = cpu->id_mmfr4 }, - /* 7 is as yet unallocated and must RAZ */ - { .name = "ID_ISAR7_RESERVED", .state = ARM_CP_STATE_BOTH, + { .name = "ID_ISAR6", .state = ARM_CP_STATE_BOTH, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 2, .opc2 = 7, .access = PL1_R, .type = ARM_CP_CONST, - .resetvalue = 0 }, + .resetvalue = cpu->id_isar6 }, REGINFO_SENTINEL }; define_arm_cp_regs(cpu, v6_idregs);
This register was added to aa32 state by ARMv8.2. Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- target/arm/cpu.h | 1 + target/arm/cpu.c | 4 ++++ target/arm/cpu64.c | 2 ++ target/arm/helper.c | 5 ++--- 4 files changed, 9 insertions(+), 3 deletions(-) -- 2.17.1