| Message ID | 1418340569-30519-10-git-send-email-greg.bellows@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 11 December 2014 at 23:29, Greg Bellows <greg.bellows@linaro.org> wrote: > Added a "has_el3" state property to the ARMCPU descriptor. This property > indicates whether the ARMCPU has security extensions enabled (EL3) or not. > By default it is disabled at this time. > > Signed-off-by: Greg Bellows <greg.bellows@linaro.org> > > --- > > v1 -> v2 > - Added set of has_el3 to true when EL3 is enabled > --- > 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..ed5a644 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 has security extension */ > + bool has_el3; > > /* 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..758e8f8 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_has_el3_property = > + DEFINE_PROP_BOOL("has_el3", ARMCPU, has_el3, false); I think the default value here should be "true": we want CPUs to default to having all their features turned on unless the board specifically disables it. (This won't affect behaviour til we get to patch 15 because before then ARM_FEATURE_EL3 isn't set and we never add the property here.) > + > static void arm_cpu_post_init(Object *obj) > { > ARMCPU *cpu = ARM_CPU(obj); > @@ -407,6 +410,15 @@ 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 has_el3 state CPU property only if EL3 is allowed. This will > + * prevent "has_el3" from existing on CPUs which cannot support EL3. > + */ > + qdev_property_add_static(DEVICE(obj), &arm_cpu_has_el3_property, > + &error_abort); > + cpu->has_el3 = true; > + } > } > > static void arm_cpu_finalizefn(Object *obj) > @@ -476,6 +488,18 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp) > cpu->reset_sctlr |= (1 << 13); > } > > + if (!cpu->has_el3) { > + /* If the hsa_el3 CPU property is disabled then we need to disable the > + * feature. Typo: "has_el3". > + */ > + 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); thanks -- PMM
On 15 December 2014 at 11:01, Peter Maydell <peter.maydell@linaro.org> wrote: > > On 11 December 2014 at 23:29, Greg Bellows <greg.bellows@linaro.org> > wrote: > > Added a "has_el3" state property to the ARMCPU descriptor. This property > > indicates whether the ARMCPU has security extensions enabled (EL3) or > not. > > By default it is disabled at this time. > > > > Signed-off-by: Greg Bellows <greg.bellows@linaro.org> > > > > --- > > > > v1 -> v2 > > - Added set of has_el3 to true when EL3 is enabled > > --- > > 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..ed5a644 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 has security extension */ > > + bool has_el3; > > > > /* 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..758e8f8 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_has_el3_property = > > + DEFINE_PROP_BOOL("has_el3", ARMCPU, has_el3, false); > > I think the default value here should be "true": we want > CPUs to default to having all their features turned on > unless the board specifically disables it. > > (This won't affect behaviour til we get to patch 15 because > before then ARM_FEATURE_EL3 isn't set and we never add > the property here.) > I went with false in the initialization because it matches how features are initialized (disabled unless otherwise enabled). In the case of EL3, it is disabled on CPUs by default unless specifically enabled. This is also the case of the has_el3 property, disabled by default unless we see that the EL3 feature is enabled. As-is, the tunable is set to true once we discover that the EL3 feature is set and from there it is up to the board code to unset has_el3 and in turn the EL3 feature. > > > + > > static void arm_cpu_post_init(Object *obj) > > { > > ARMCPU *cpu = ARM_CPU(obj); > > @@ -407,6 +410,15 @@ 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 has_el3 state CPU property only if EL3 is allowed. > This will > > + * prevent "has_el3" from existing on CPUs which cannot support > EL3. > > + */ > > + qdev_property_add_static(DEVICE(obj), &arm_cpu_has_el3_property, > > + &error_abort); > > + cpu->has_el3 = true; > > + } > > } > > > > static void arm_cpu_finalizefn(Object *obj) > > @@ -476,6 +488,18 @@ static void arm_cpu_realizefn(DeviceState *dev, > Error **errp) > > cpu->reset_sctlr |= (1 << 13); > > } > > > > + if (!cpu->has_el3) { > > + /* If the hsa_el3 CPU property is disabled then we need to > disable the > > + * feature. > > Typo: "has_el3". > Fixed. > > > + */ > > + 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); > > thanks > -- PMM >
On 15 December 2014 at 17:18, Greg Bellows <greg.bellows@linaro.org> wrote: > > > On 15 December 2014 at 11:01, Peter Maydell <peter.maydell@linaro.org> > wrote: >> >> On 11 December 2014 at 23:29, Greg Bellows <greg.bellows@linaro.org> >> wrote: >> > +static Property arm_cpu_has_el3_property = >> > + DEFINE_PROP_BOOL("has_el3", ARMCPU, has_el3, false); >> >> I think the default value here should be "true": we want >> CPUs to default to having all their features turned on >> unless the board specifically disables it. >> >> (This won't affect behaviour til we get to patch 15 because >> before then ARM_FEATURE_EL3 isn't set and we never add >> the property here.) > > > I went with false in the initialization because it matches how features are > initialized (disabled unless otherwise enabled). In the case of EL3, it is > disabled on CPUs by default unless specifically enabled. This is also the > case of the has_el3 property, disabled by default unless we see that the EL3 > feature is enabled. This is correct... > As-is, the tunable is set to true once we discover that > the EL3 feature is set ...but this isn't. When you call qdev_property_add_static() it will call object_property_set_bool() to set the flag to the default value as specified in the Property struct, so we'll end up with has_el3 set to false by default on CPUs which can support EL3, which is not what we want. In fact you're manually setting cpu->has_el3 = true in the arm_cpu_post_init(). You should just set the property struct's default value correctly in the first place... thanks -- PMM
On 15 December 2014 at 11:21, Peter Maydell <peter.maydell@linaro.org> wrote: > > On 15 December 2014 at 17:18, Greg Bellows <greg.bellows@linaro.org> > wrote: > > > > > > On 15 December 2014 at 11:01, Peter Maydell <peter.maydell@linaro.org> > > wrote: > >> > >> On 11 December 2014 at 23:29, Greg Bellows <greg.bellows@linaro.org> > >> wrote: > >> > +static Property arm_cpu_has_el3_property = > >> > + DEFINE_PROP_BOOL("has_el3", ARMCPU, has_el3, false); > >> > >> I think the default value here should be "true": we want > >> CPUs to default to having all their features turned on > >> unless the board specifically disables it. > >> > >> (This won't affect behaviour til we get to patch 15 because > >> before then ARM_FEATURE_EL3 isn't set and we never add > >> the property here.) > > > > > > I went with false in the initialization because it matches how features > are > > initialized (disabled unless otherwise enabled). In the case of EL3, it > is > > disabled on CPUs by default unless specifically enabled. This is also > the > > case of the has_el3 property, disabled by default unless we see that the > EL3 > > feature is enabled. > > This is correct... > > > As-is, the tunable is set to true once we discover that > > the EL3 feature is set > > ...but this isn't. When you call qdev_property_add_static() > it will call object_property_set_bool() to set the flag to > the default value as specified in the Property struct, so > we'll end up with has_el3 set to false by default on CPUs > which can support EL3, which is not what we want. > > In fact you're manually setting cpu->has_el3 = true in > the arm_cpu_post_init(). You should just set the property > struct's default value correctly in the first place... > > Ah... I see what you are saying. If I init it to true I can skip manual setting of it in the step that follows. I'll fix it in the next version. > thanks > -- PMM >
diff --git a/target-arm/cpu-qom.h b/target-arm/cpu-qom.h index dcfda7d..ed5a644 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 has security extension */ + bool has_el3; /* 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..758e8f8 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_has_el3_property = + DEFINE_PROP_BOOL("has_el3", ARMCPU, has_el3, false); + static void arm_cpu_post_init(Object *obj) { ARMCPU *cpu = ARM_CPU(obj); @@ -407,6 +410,15 @@ 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 has_el3 state CPU property only if EL3 is allowed. This will + * prevent "has_el3" from existing on CPUs which cannot support EL3. + */ + qdev_property_add_static(DEVICE(obj), &arm_cpu_has_el3_property, + &error_abort); + cpu->has_el3 = true; + } } static void arm_cpu_finalizefn(Object *obj) @@ -476,6 +488,18 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp) cpu->reset_sctlr |= (1 << 13); } + if (!cpu->has_el3) { + /* If the hsa_el3 CPU property is disabled then we need to disable the + * 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 "has_el3" state property to the ARMCPU descriptor. This property indicates whether the ARMCPU has security extensions enabled (EL3) or not. By default it is disabled at this time. Signed-off-by: Greg Bellows <greg.bellows@linaro.org> --- v1 -> v2 - Added set of has_el3 to true when EL3 is enabled --- target-arm/cpu-qom.h | 2 ++ target-arm/cpu.c | 24 ++++++++++++++++++++++++ 2 files changed, 26 insertions(+)