Re: [Qemu-devel] [PATCH v2 09/15] target-arm: Add ARMCPU secure property

[Greg Bellows]

On 15 December 2014 at 11:01, Peter Maydell <address@hidden> wrote:

On 11 December 2014 at 23:29, Greg Bellows <address@hidden> 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 <address@hidden>
>
> ---
>
> 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