Re: [Qemu-devel] [PATCH 10/13] target-arm: Add ARMCPU secure property

[Greg Bellows]

On 5 December 2014 at 09:26, Peter Maydell <address@hidden> wrote:

On 3 December 2014 at 20:06, Greg Bellows <address@hidden> 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 <address@hidden>
> ---
>  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