| Message ID | 1417637167-20640-11-git-send-email-greg.bellows@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 3 December 2014 at 20:06, Greg Bellows <greg.bellows@linaro.org> wrote: > Added a "secure" state property to the ARMCPU descriptor. This property > indicates whether the ARMCPU is enabled for secure state or not. By default it > is disabled at this time. Shouldn't this feature be "has_el3" ? It's the configurable version of the ARM_FEATURE_EL3. > Signed-off-by: Greg Bellows <greg.bellows@linaro.org> > --- > target-arm/cpu-qom.h | 2 ++ > target-arm/cpu.c | 24 ++++++++++++++++++++++++ > 2 files changed, 26 insertions(+) > > diff --git a/target-arm/cpu-qom.h b/target-arm/cpu-qom.h > index dcfda7d..8dab91b 100644 > --- a/target-arm/cpu-qom.h > +++ b/target-arm/cpu-qom.h > @@ -100,6 +100,8 @@ typedef struct ARMCPU { > bool start_powered_off; > /* CPU currently in PSCI powered-off state */ > bool powered_off; > + /* CPU secure state enabled */ > + bool secure; > > /* PSCI conduit used to invoke PSCI methods > * 0 - disabled, 1 - smc, 2 - hvc > diff --git a/target-arm/cpu.c b/target-arm/cpu.c > index 01afed2..0e660f9 100644 > --- a/target-arm/cpu.c > +++ b/target-arm/cpu.c > @@ -388,6 +388,9 @@ static Property arm_cpu_reset_hivecs_property = > static Property arm_cpu_rvbar_property = > DEFINE_PROP_UINT64("rvbar", ARMCPU, rvbar, 0); > > +static Property arm_cpu_secure_property = > + DEFINE_PROP_BOOL("secure", ARMCPU, secure, false); > + > static void arm_cpu_post_init(Object *obj) > { > ARMCPU *cpu = ARM_CPU(obj); > @@ -407,6 +410,14 @@ static void arm_cpu_post_init(Object *obj) > qdev_property_add_static(DEVICE(obj), &arm_cpu_rvbar_property, > &error_abort); > } > + > + if (arm_feature(&cpu->env, ARM_FEATURE_EL3)) { > + /* Add the secure state CPU property only if EL3 is allowed. This will > + * prevent "secure" from existing on non EL3 enabled machines. "on CPUs which cannot support EL3". > + */ > + qdev_property_add_static(DEVICE(obj), &arm_cpu_secure_property, > + &error_abort); > + } > } > > static void arm_cpu_finalizefn(Object *obj) > @@ -476,6 +487,19 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp) > cpu->reset_sctlr |= (1 << 13); > } > > + if (arm_feature(env, ARM_FEATURE_V6) && !cpu->secure) { > + /* The security extension and ID_PFR1 only apply to ARMv6 and up. IF > + * this is the case and secure state has not been enabled then we > + * disable the security extension feature. > + */ > + unset_feature(env, ARM_FEATURE_EL3); You don't need to conditionalize this on V6 being set; just do if (!cpu->secure) { unset_feature(...); } to reflect the property setting into the feature bits. The property won't exist in the first place on pre-v6 cores. > + > + /* Disable the security extension feature bits in the processor feature > + * register as well. This is id_pfr1[7:4]. > + */ > + cpu->id_pfr1 &= ~0xf0; This is a bit of a tricky area, because we don't in general mess with our ID register values to match the features we do or don't happen to implement, so this would be an odd special case. Can we get away without touching PFR1 here? > + } > + > register_cp_regs_for_features(cpu); > arm_cpu_register_gdb_regs_for_features(cpu); thanks -- PMM
On 5 December 2014 at 09:26, Peter Maydell <peter.maydell@linaro.org> wrote: > On 3 December 2014 at 20:06, Greg Bellows <greg.bellows@linaro.org> wrote: > > Added a "secure" state property to the ARMCPU descriptor. This property > > indicates whether the ARMCPU is enabled for secure state or not. By > default it > > is disabled at this time. > > Shouldn't this feature be "has_el3" ? It's the configurable > version of the ARM_FEATURE_EL3. > Sure, that makes sense, I can rename it. > > > Signed-off-by: Greg Bellows <greg.bellows@linaro.org> > > --- > > target-arm/cpu-qom.h | 2 ++ > > target-arm/cpu.c | 24 ++++++++++++++++++++++++ > > 2 files changed, 26 insertions(+) > > > > diff --git a/target-arm/cpu-qom.h b/target-arm/cpu-qom.h > > index dcfda7d..8dab91b 100644 > > --- a/target-arm/cpu-qom.h > > +++ b/target-arm/cpu-qom.h > > @@ -100,6 +100,8 @@ typedef struct ARMCPU { > > bool start_powered_off; > > /* CPU currently in PSCI powered-off state */ > > bool powered_off; > > + /* CPU secure state enabled */ > > + bool secure; > > > > /* PSCI conduit used to invoke PSCI methods > > * 0 - disabled, 1 - smc, 2 - hvc > > diff --git a/target-arm/cpu.c b/target-arm/cpu.c > > index 01afed2..0e660f9 100644 > > --- a/target-arm/cpu.c > > +++ b/target-arm/cpu.c > > @@ -388,6 +388,9 @@ static Property arm_cpu_reset_hivecs_property = > > static Property arm_cpu_rvbar_property = > > DEFINE_PROP_UINT64("rvbar", ARMCPU, rvbar, 0); > > > > +static Property arm_cpu_secure_property = > > + DEFINE_PROP_BOOL("secure", ARMCPU, secure, false); > > + > > static void arm_cpu_post_init(Object *obj) > > { > > ARMCPU *cpu = ARM_CPU(obj); > > @@ -407,6 +410,14 @@ static void arm_cpu_post_init(Object *obj) > > qdev_property_add_static(DEVICE(obj), &arm_cpu_rvbar_property, > > &error_abort); > > } > > + > > + if (arm_feature(&cpu->env, ARM_FEATURE_EL3)) { > > + /* Add the secure state CPU property only if EL3 is allowed. > This will > > + * prevent "secure" from existing on non EL3 enabled machines. > > "on CPUs which cannot support EL3". > > That's better, fixed in next version. > > + */ > > + qdev_property_add_static(DEVICE(obj), &arm_cpu_secure_property, > > + &error_abort); > > + } > > } > > > > static void arm_cpu_finalizefn(Object *obj) > > @@ -476,6 +487,19 @@ static void arm_cpu_realizefn(DeviceState *dev, > Error **errp) > > cpu->reset_sctlr |= (1 << 13); > > } > > > > + if (arm_feature(env, ARM_FEATURE_V6) && !cpu->secure) { > > + /* The security extension and ID_PFR1 only apply to ARMv6 and > up. IF > > + * this is the case and secure state has not been enabled then > we > > + * disable the security extension feature. > > + */ > > + unset_feature(env, ARM_FEATURE_EL3); > > You don't need to conditionalize this on V6 being set; > just do > if (!cpu->secure) { > unset_feature(...); > } > > to reflect the property setting into the feature bits. The > property won't exist in the first place on pre-v6 cores. > > Originally, I had just what you suggested, but I didn't want updates to id_pfr1 on pre v6 as I was not sure they had the equivalent bits. I removed the check as it is likely benign. > > + > > + /* Disable the security extension feature bits in the processor > feature > > + * register as well. This is id_pfr1[7:4]. > > + */ > > + cpu->id_pfr1 &= ~0xf0; > > This is a bit of a tricky area, because we don't in general > mess with our ID register values to match the features we do or > don't happen to implement, so this would be an odd special case. > Can we get away without touching PFR1 here? > It depends if you want SW to be able to correctly use the register. For instance, my TZ test checks the ID_PFR1 to see if it should be performing security extension operations. I'm not sure of another way for this check to be performed. > > > + } > > + > > register_cp_regs_for_features(cpu); > > arm_cpu_register_gdb_regs_for_features(cpu); > > thanks > -- PMM >
diff --git a/target-arm/cpu-qom.h b/target-arm/cpu-qom.h index dcfda7d..8dab91b 100644 --- a/target-arm/cpu-qom.h +++ b/target-arm/cpu-qom.h @@ -100,6 +100,8 @@ typedef struct ARMCPU { bool start_powered_off; /* CPU currently in PSCI powered-off state */ bool powered_off; + /* CPU secure state enabled */ + bool secure; /* PSCI conduit used to invoke PSCI methods * 0 - disabled, 1 - smc, 2 - hvc diff --git a/target-arm/cpu.c b/target-arm/cpu.c index 01afed2..0e660f9 100644 --- a/target-arm/cpu.c +++ b/target-arm/cpu.c @@ -388,6 +388,9 @@ static Property arm_cpu_reset_hivecs_property = static Property arm_cpu_rvbar_property = DEFINE_PROP_UINT64("rvbar", ARMCPU, rvbar, 0); +static Property arm_cpu_secure_property = + DEFINE_PROP_BOOL("secure", ARMCPU, secure, false); + static void arm_cpu_post_init(Object *obj) { ARMCPU *cpu = ARM_CPU(obj); @@ -407,6 +410,14 @@ static void arm_cpu_post_init(Object *obj) qdev_property_add_static(DEVICE(obj), &arm_cpu_rvbar_property, &error_abort); } + + if (arm_feature(&cpu->env, ARM_FEATURE_EL3)) { + /* Add the secure state CPU property only if EL3 is allowed. This will + * prevent "secure" from existing on non EL3 enabled machines. + */ + qdev_property_add_static(DEVICE(obj), &arm_cpu_secure_property, + &error_abort); + } } static void arm_cpu_finalizefn(Object *obj) @@ -476,6 +487,19 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp) cpu->reset_sctlr |= (1 << 13); } + if (arm_feature(env, ARM_FEATURE_V6) && !cpu->secure) { + /* The security extension and ID_PFR1 only apply to ARMv6 and up. IF + * this is the case and secure state has not been enabled then we + * disable the security extension feature. + */ + unset_feature(env, ARM_FEATURE_EL3); + + /* Disable the security extension feature bits in the processor feature + * register as well. This is id_pfr1[7:4]. + */ + cpu->id_pfr1 &= ~0xf0; + } + register_cp_regs_for_features(cpu); arm_cpu_register_gdb_regs_for_features(cpu);
Added a "secure" state property to the ARMCPU descriptor. This property indicates whether the ARMCPU is enabled for secure state or not. By default it is disabled at this time. Signed-off-by: Greg Bellows <greg.bellows@linaro.org> --- target-arm/cpu-qom.h | 2 ++ target-arm/cpu.c | 24 ++++++++++++++++++++++++ 2 files changed, 26 insertions(+)