Re: [Qemu-devel] [PATCH v1 10/10] target-arm: Add VMPIDR_EL2

[Peter Maydell]

On 3 September 2015 at 21:14, Edgar E. Iglesias
<address@hidden> wrote:
> From: "Edgar E. Iglesias" <address@hidden>
>
> Signed-off-by: Edgar E. Iglesias <address@hidden>
> ---
>  target-arm/cpu.h    |  1 +
>  target-arm/helper.c | 20 ++++++++++++++++++--
>  2 files changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index cdecfdf..1929a2f 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -385,6 +385,7 @@ typedef struct CPUARMState {
>          uint64_t c15_ccnt;
>          uint64_t pmccfiltr_el0; /* Performance Monitor Filter Register */
>          uint64_t vpidr_el2; /* Virtualization Processor ID Register */
> +        uint64_t vmpidr_el2; /* Virtualization Multiprocessor ID Register */
>      } cp15;
>
>      struct {
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 3701207..e335f8f 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -2447,6 +2447,12 @@ static uint64_t mpidr_read(CPUARMState *env, const 
> ARMCPRegInfo *ri)
>  {
>      ARMCPU *cpu = ARM_CPU(arm_env_get_cpu(env));
>      uint64_t mpidr = cpu->mp_affinity;
> +    unsigned int cur_el = arm_current_el(env);
> +    bool secure = arm_is_secure(env);
> +
> +    if (arm_feature(env, ARM_FEATURE_EL2) && !secure && cur_el == 2) {
> +        mpidr = env->cp15.vmpidr_el2;
> +    }

Shouldn't we be returning the VMPIDR if we're in NS-EL1, not NS-EL2?

>      if (arm_feature(env, ARM_FEATURE_V7MP)) {
>          mpidr |= (1U << 31);
> @@ -4124,6 +4130,11 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>                .access = PL2_RW, .accessfn = access_el3_aa32ns_aa64any,
>                .resetvalue = cpu->midr,
>                .fieldoffset = offsetof(CPUARMState, cp15.vpidr_el2) },
> +            { .name = "VMPIDR_EL2", .state = ARM_CP_STATE_BOTH,
> +              .opc0 = 3, .opc1 = 4, .crn = 0, .crm = 0, .opc2 = 5,
> +              .access = PL2_RW, .accessfn = access_el3_aa32ns_aa64any,
> +              .resetvalue = cpu->mp_affinity,

This resetvalue is missing the M and U bits which are ORed in when we
read the MPIDR.

> +              .fieldoffset = offsetof(CPUARMState, cp15.vmpidr_el2) },
>              REGINFO_SENTINEL
>          };
>          define_arm_cp_regs(cpu, vpidr_regs);
> @@ -4142,8 +4153,8 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>           * register the no_el2 reginfos.
>           */
>          if (arm_feature(env, ARM_FEATURE_EL3)) {
> -            /* When EL3 exists but not EL2, VPIDR takes the value
> -             * of MIDR_EL1.
> +            /* When EL3 exists but not EL2, VPIDR and VMPIDR take the value
> +             * of MIDR_EL1 and MPIDR_EL1.
>               */
>              ARMCPRegInfo vpidr_regs[] = {
>                  { .name = "VPIDR_EL2", .state = ARM_CP_STATE_BOTH,
> @@ -4152,6 +4163,11 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>                    .access = PL2_RW, .accessfn = access_el3_aa32ns_aa64any,
>                    .type = ARM_CP_CONST,
>                    .fieldoffset = offsetof(CPUARMState, cp15.vpidr_el2) },
> +                { .name = "VMPIDR_EL2", .state = ARM_CP_STATE_AA64,
> +                  .opc0 = 3, .opc1 = 4, .crn = 0, .crm = 0, .opc2 = 5,
> +                  .access = PL2_RW, .accessfn = access_el3_aa32ns_aa64any,
> +                  .type = ARM_CP_CONST | ARM_CP_NO_RAW,
> +                  .readfn = mpidr_read },

CP_CONST and a readfn doesn't make much sense.

>                  REGINFO_SENTINEL
>              };
>              define_arm_cp_regs(cpu, vpidr_regs);
> --
> 1.9.1
>

thanks
-- PMM