On 5 December 2014 at 09:26, Peter Maydell <peter.mayd...@linaro.org> wrote:
> On 3 December 2014 at 20:06, Greg Bellows <greg.bell...@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.bell...@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
>
