[Qemu-devel] [PATCH target-arm v1 0/9] ARM Cortex R5 Support

[Qemu-devel] [PATCH target-arm v1 0/9] ARM Cortex R5 Support

[Peter Crosthwaite]

Hi Peter and all,

This patch series adds ARM Cortex R5 processor support. The PMSAv7 MPU
is implemented. Two R5s are added to the Xilinx ZynqMP SoC.

Regards,
Peter


Peter Crosthwaite (9):
  target-arm: Prepare support for Cortex-R5
  arm: helper: Factor out CP regs common to [pv]msa
  target-arm/helper.c: define MPUIR register
  target-arm: Add registers for PMSAv7
  arm: helper: rename get_phys_addr_mpu
  target-arm: Implement PMSAv7 MPU
  arm: r5: Implement dummy ATCM, BTCM and D-cache invalidate
  arm: xlnx-zynqmp: Preface CPU variables with "A"
  arm: xlnx-zynqmp: Add 2xCortexR5 CPUs

 hw/arm/xlnx-ep108.c          |   2 +-
 hw/arm/xlnx-zynqmp.c         |  50 +++++++---
 include/hw/arm/xlnx-zynqmp.h |   6 +-
 target-arm/cpu.c             |  39 ++++++++
 target-arm/cpu.h             |   9 ++
 target-arm/helper.c          | 226 +++++++++++++++++++++++++++++++++++++++----
 6 files changed, 299 insertions(+), 33 deletions(-)

-- 
2.4.2.3.g2ffcb72

[Qemu-devel] [PATCH target-arm v1 1/9] target-arm: Prepare support for Cortex-R5

[Peter Crosthwaite]

Introduce a CPU model for the Cortex R5 processor. ARMv7 with MPU,
and both thumb and ARM div instructions.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---
 target-arm/cpu.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/target-arm/cpu.c b/target-arm/cpu.c
index 4a888ab..4872d9c 100644
--- a/target-arm/cpu.c
+++ b/target-arm/cpu.c
@@ -794,6 +794,32 @@ static void arm_v7m_class_init(ObjectClass *oc, void *data)
     cc->cpu_exec_interrupt = arm_v7m_cpu_exec_interrupt;
 }
 
+static void cortex_r5_initfn(Object *obj)
+{
+    ARMCPU *cpu = ARM_CPU(obj);
+
+    set_feature(&cpu->env, ARM_FEATURE_V7);
+    set_feature(&cpu->env, ARM_FEATURE_THUMB_DIV);
+    set_feature(&cpu->env, ARM_FEATURE_ARM_DIV);
+    set_feature(&cpu->env, ARM_FEATURE_V7MP);
+    set_feature(&cpu->env, ARM_FEATURE_MPU);
+    cpu->midr = 0x411fc153; /* r1p3 */
+    cpu->id_pfr0 = 0x0131;
+    cpu->id_pfr1 = 0x001;
+    cpu->id_dfr0 = 0x010400;
+    cpu->id_afr0 = 0x0;
+    cpu->id_mmfr0 = 0x0210030;
+    cpu->id_mmfr1 = 0x00000000;
+    cpu->id_mmfr2 = 0x01200000;
+    cpu->id_mmfr3 = 0x0211;
+    cpu->id_isar0 = 0x2101111;
+    cpu->id_isar1 = 0x13112111;
+    cpu->id_isar2 = 0x21232141;
+    cpu->id_isar3 = 0x01112131;
+    cpu->id_isar4 = 0x0010142;
+    cpu->id_isar5 = 0x0;
+}
+
 static const ARMCPRegInfo cortexa8_cp_reginfo[] = {
     { .name = "L2LOCKDOWN", .cp = 15, .crn = 9, .crm = 0, .opc1 = 1, .opc2 = 0,
       .access = PL1_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
@@ -1185,6 +1211,7 @@ static const ARMCPUInfo arm_cpus[] = {
     { .name = "arm11mpcore", .initfn = arm11mpcore_initfn },
     { .name = "cortex-m3",   .initfn = cortex_m3_initfn,
                              .class_init = arm_v7m_class_init },
+    { .name = "cortex-r5",   .initfn = cortex_r5_initfn },
     { .name = "cortex-a8",   .initfn = cortex_a8_initfn },
     { .name = "cortex-a9",   .initfn = cortex_a9_initfn },
     { .name = "cortex-a15",  .initfn = cortex_a15_initfn },
-- 
2.4.2.3.g2ffcb72

Re: [Qemu-devel] [PATCH target-arm v1 1/9] target-arm: Prepare support for Cortex-R5

[Peter Maydell]

On 1 June 2015 at 19:04, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> Introduce a CPU model for the Cortex R5 processor. ARMv7 with MPU,
> and both thumb and ARM div instructions.
>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

This patch needs to go last, not first -- implement all
the support the R5 needs, and only then let somebody
create one.

-- PMM

> ---
>  target-arm/cpu.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
>
> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> index 4a888ab..4872d9c 100644
> --- a/target-arm/cpu.c
> +++ b/target-arm/cpu.c
> @@ -794,6 +794,32 @@ static void arm_v7m_class_init(ObjectClass *oc, void *data)
>      cc->cpu_exec_interrupt = arm_v7m_cpu_exec_interrupt;
>  }
>
> +static void cortex_r5_initfn(Object *obj)
> +{
> +    ARMCPU *cpu = ARM_CPU(obj);
> +
> +    set_feature(&cpu->env, ARM_FEATURE_V7);
> +    set_feature(&cpu->env, ARM_FEATURE_THUMB_DIV);

Should we have a feature bit for "this is an R profile
core" ? For instance THUMB_DIV is mandatory for v7R
and so we could infer it in cpu realize.

> +    set_feature(&cpu->env, ARM_FEATURE_ARM_DIV);
> +    set_feature(&cpu->env, ARM_FEATURE_V7MP);

This will turn on a bunch of TLB invalidate insns we don't want
(v7mp_cp_reginfo assumes VMSA).

It also won't give the right value for MPIDR; see the comment
in mpidr_read():
        /* Cores which are uniprocessor (non-coherent)
         * but still implement the MP extensions set
         * bit 30. (For instance, A9UP.) However we do
         * not currently model any of those cores.
         */

The R5 (from r1p0 and up) is such a core...

> +    set_feature(&cpu->env, ARM_FEATURE_MPU);
> +    cpu->midr = 0x411fc153; /* r1p3 */
> +    cpu->id_pfr0 = 0x0131;
> +    cpu->id_pfr1 = 0x001;
> +    cpu->id_dfr0 = 0x010400;
> +    cpu->id_afr0 = 0x0;
> +    cpu->id_mmfr0 = 0x0210030;
> +    cpu->id_mmfr1 = 0x00000000;
> +    cpu->id_mmfr2 = 0x01200000;
> +    cpu->id_mmfr3 = 0x0211;
> +    cpu->id_isar0 = 0x2101111;
> +    cpu->id_isar1 = 0x13112111;
> +    cpu->id_isar2 = 0x21232141;
> +    cpu->id_isar3 = 0x01112131;
> +    cpu->id_isar4 = 0x0010142;
> +    cpu->id_isar5 = 0x0;
> +}
> +
>  static const ARMCPRegInfo cortexa8_cp_reginfo[] = {
>      { .name = "L2LOCKDOWN", .cp = 15, .crn = 9, .crm = 0, .opc1 = 1, .opc2 = 0,
>        .access = PL1_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
> @@ -1185,6 +1211,7 @@ static const ARMCPUInfo arm_cpus[] = {
>      { .name = "arm11mpcore", .initfn = arm11mpcore_initfn },
>      { .name = "cortex-m3",   .initfn = cortex_m3_initfn,
>                               .class_init = arm_v7m_class_init },
> +    { .name = "cortex-r5",   .initfn = cortex_r5_initfn },
>      { .name = "cortex-a8",   .initfn = cortex_a8_initfn },
>      { .name = "cortex-a9",   .initfn = cortex_a9_initfn },
>      { .name = "cortex-a15",  .initfn = cortex_a15_initfn },
> --
> 2.4.2.3.g2ffcb72
>

thanks
-- PMM

Re: [Qemu-devel] [PATCH target-arm v1 1/9] target-arm: Prepare support for Cortex-R5

[Peter Crosthwaite]

On Mon, Jun 1, 2015 at 11:44 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 1 June 2015 at 19:04, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
>> Introduce a CPU model for the Cortex R5 processor. ARMv7 with MPU,
>> and both thumb and ARM div instructions.
>>
>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>
> This patch needs to go last, not first -- implement all
> the support the R5 needs, and only then let somebody
> create one.
>

OK

> -- PMM
>
>> ---
>>  target-arm/cpu.c | 27 +++++++++++++++++++++++++++
>>  1 file changed, 27 insertions(+)
>>
>> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
>> index 4a888ab..4872d9c 100644
>> --- a/target-arm/cpu.c
>> +++ b/target-arm/cpu.c
>> @@ -794,6 +794,32 @@ static void arm_v7m_class_init(ObjectClass *oc, void *data)
>>      cc->cpu_exec_interrupt = arm_v7m_cpu_exec_interrupt;
>>  }
>>
>> +static void cortex_r5_initfn(Object *obj)
>> +{
>> +    ARMCPU *cpu = ARM_CPU(obj);
>> +
>> +    set_feature(&cpu->env, ARM_FEATURE_V7);
>> +    set_feature(&cpu->env, ARM_FEATURE_THUMB_DIV);
>
> Should we have a feature bit for "this is an R profile
> core" ? For instance THUMB_DIV is mandatory for v7R
> and so we could infer it in cpu realize.
>

So is this best done as rename of ARM_FEATURE_MPU? We can still set R
profile for ARMv5  MPU capable procs in much the same way we do with
"EL2" and "EL3" for preV8.

Regards,
Peter

>> +    set_feature(&cpu->env, ARM_FEATURE_ARM_DIV);
>> +    set_feature(&cpu->env, ARM_FEATURE_V7MP);
>
> This will turn on a bunch of TLB invalidate insns we don't want
> (v7mp_cp_reginfo assumes VMSA).
>
> It also won't give the right value for MPIDR; see the comment
> in mpidr_read():
>         /* Cores which are uniprocessor (non-coherent)
>          * but still implement the MP extensions set
>          * bit 30. (For instance, A9UP.) However we do
>          * not currently model any of those cores.
>          */
>
> The R5 (from r1p0 and up) is such a core...
>
>> +    set_feature(&cpu->env, ARM_FEATURE_MPU);
>> +    cpu->midr = 0x411fc153; /* r1p3 */
>> +    cpu->id_pfr0 = 0x0131;
>> +    cpu->id_pfr1 = 0x001;
>> +    cpu->id_dfr0 = 0x010400;
>> +    cpu->id_afr0 = 0x0;
>> +    cpu->id_mmfr0 = 0x0210030;
>> +    cpu->id_mmfr1 = 0x00000000;
>> +    cpu->id_mmfr2 = 0x01200000;
>> +    cpu->id_mmfr3 = 0x0211;
>> +    cpu->id_isar0 = 0x2101111;
>> +    cpu->id_isar1 = 0x13112111;
>> +    cpu->id_isar2 = 0x21232141;
>> +    cpu->id_isar3 = 0x01112131;
>> +    cpu->id_isar4 = 0x0010142;
>> +    cpu->id_isar5 = 0x0;
>> +}
>> +
>>  static const ARMCPRegInfo cortexa8_cp_reginfo[] = {
>>      { .name = "L2LOCKDOWN", .cp = 15, .crn = 9, .crm = 0, .opc1 = 1, .opc2 = 0,
>>        .access = PL1_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
>> @@ -1185,6 +1211,7 @@ static const ARMCPUInfo arm_cpus[] = {
>>      { .name = "arm11mpcore", .initfn = arm11mpcore_initfn },
>>      { .name = "cortex-m3",   .initfn = cortex_m3_initfn,
>>                               .class_init = arm_v7m_class_init },
>> +    { .name = "cortex-r5",   .initfn = cortex_r5_initfn },
>>      { .name = "cortex-a8",   .initfn = cortex_a8_initfn },
>>      { .name = "cortex-a9",   .initfn = cortex_a9_initfn },
>>      { .name = "cortex-a15",  .initfn = cortex_a15_initfn },
>> --
>> 2.4.2.3.g2ffcb72
>>
>
> thanks
> -- PMM
>

Re: [Qemu-devel] [PATCH target-arm v1 1/9] target-arm: Prepare support for Cortex-R5

[Peter Maydell]

On 2 June 2015 at 10:25, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> On Mon, Jun 1, 2015 at 11:44 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> +static void cortex_r5_initfn(Object *obj)
>>> +{
>>> +    ARMCPU *cpu = ARM_CPU(obj);
>>> +
>>> +    set_feature(&cpu->env, ARM_FEATURE_V7);
>>> +    set_feature(&cpu->env, ARM_FEATURE_THUMB_DIV);
>>
>> Should we have a feature bit for "this is an R profile
>> core" ? For instance THUMB_DIV is mandatory for v7R
>> and so we could infer it in cpu realize.

> So is this best done as rename of ARM_FEATURE_MPU? We can still set R
> profile for ARMv5  MPU capable procs in much the same way we do with
> "EL2" and "EL3" for preV8.

So I originally suggested this because I read the bit in the
R5 TRM that says that the MPU is optional. But looking more
closely, I think that we would handle that by defining
FEATURE_MPU but 0 regions.

Bear in mind that FEATURE_MPU applies to M profile cores as
well. So if we find ourselves saying "MPU but not M profile"
a lot that might justify an R feature bit... But for now
we don't need to add it I guess.

-- PMM

Re: [Qemu-devel] [PATCH target-arm v1 1/9] target-arm: Prepare support for Cortex-R5

[Peter Crosthwaite]

On Mon, Jun 1, 2015 at 11:44 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 1 June 2015 at 19:04, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
>> Introduce a CPU model for the Cortex R5 processor. ARMv7 with MPU,
>> and both thumb and ARM div instructions.
>>
>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>
> This patch needs to go last, not first -- implement all
> the support the R5 needs, and only then let somebody
> create one.
>
> -- PMM
>
>> ---
>>  target-arm/cpu.c | 27 +++++++++++++++++++++++++++
>>  1 file changed, 27 insertions(+)
>>
>> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
>> index 4a888ab..4872d9c 100644
>> --- a/target-arm/cpu.c
>> +++ b/target-arm/cpu.c
>> @@ -794,6 +794,32 @@ static void arm_v7m_class_init(ObjectClass *oc, void *data)
>>      cc->cpu_exec_interrupt = arm_v7m_cpu_exec_interrupt;
>>  }
>>
>> +static void cortex_r5_initfn(Object *obj)
>> +{
>> +    ARMCPU *cpu = ARM_CPU(obj);
>> +
>> +    set_feature(&cpu->env, ARM_FEATURE_V7);
>> +    set_feature(&cpu->env, ARM_FEATURE_THUMB_DIV);
>
> Should we have a feature bit for "this is an R profile
> core" ? For instance THUMB_DIV is mandatory for v7R
> and so we could infer it in cpu realize.
>
>> +    set_feature(&cpu->env, ARM_FEATURE_ARM_DIV);
>> +    set_feature(&cpu->env, ARM_FEATURE_V7MP);
>
> This will turn on a bunch of TLB invalidate insns we don't want
> (v7mp_cp_reginfo assumes VMSA).
>

Knocked those out with a !ARM_FEATURE_MPU

> It also won't give the right value for MPIDR; see the comment
> in mpidr_read():
>         /* Cores which are uniprocessor (non-coherent)
>          * but still implement the MP extensions set
>          * bit 30. (For instance, A9UP.) However we do
>          * not currently model any of those cores.
>          */
>

I have added the MPIDR U bit as a new configuration that
cortex_foo_initfn can set.

Regards,
Peter

> The R5 (from r1p0 and up) is such a core...
>
>> +    set_feature(&cpu->env, ARM_FEATURE_MPU);
>> +    cpu->midr = 0x411fc153; /* r1p3 */
>> +    cpu->id_pfr0 = 0x0131;
>> +    cpu->id_pfr1 = 0x001;
>> +    cpu->id_dfr0 = 0x010400;
>> +    cpu->id_afr0 = 0x0;
>> +    cpu->id_mmfr0 = 0x0210030;
>> +    cpu->id_mmfr1 = 0x00000000;
>> +    cpu->id_mmfr2 = 0x01200000;
>> +    cpu->id_mmfr3 = 0x0211;
>> +    cpu->id_isar0 = 0x2101111;
>> +    cpu->id_isar1 = 0x13112111;
>> +    cpu->id_isar2 = 0x21232141;
>> +    cpu->id_isar3 = 0x01112131;
>> +    cpu->id_isar4 = 0x0010142;
>> +    cpu->id_isar5 = 0x0;
>> +}
>> +
>>  static const ARMCPRegInfo cortexa8_cp_reginfo[] = {
>>      { .name = "L2LOCKDOWN", .cp = 15, .crn = 9, .crm = 0, .opc1 = 1, .opc2 = 0,
>>        .access = PL1_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
>> @@ -1185,6 +1211,7 @@ static const ARMCPUInfo arm_cpus[] = {
>>      { .name = "arm11mpcore", .initfn = arm11mpcore_initfn },
>>      { .name = "cortex-m3",   .initfn = cortex_m3_initfn,
>>                               .class_init = arm_v7m_class_init },
>> +    { .name = "cortex-r5",   .initfn = cortex_r5_initfn },
>>      { .name = "cortex-a8",   .initfn = cortex_a8_initfn },
>>      { .name = "cortex-a9",   .initfn = cortex_a9_initfn },
>>      { .name = "cortex-a15",  .initfn = cortex_a15_initfn },
>> --
>> 2.4.2.3.g2ffcb72
>>
>
> thanks
> -- PMM
>

[Qemu-devel] [PATCH target-arm v1 2/9] arm: helper: Factor out CP regs common to [pv]msa

[Peter Crosthwaite]

V6+ PMSA and VMSA share some common registers that are currently
in the VMSA definition block. Split them out into a new def that can
be shared to PMSA.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---
 target-arm/helper.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 1cc4993..78b6406 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -1846,7 +1846,7 @@ static void vmsa_ttbr_write(CPUARMState *env, const ARMCPRegInfo *ri,
     raw_write(env, ri, value);
 }
 
-static const ARMCPRegInfo vmsa_cp_reginfo[] = {
+static const ARMCPRegInfo vmsa_pmsa_cp_reginfo[] = {
     { .name = "DFSR", .cp = 15, .crn = 5, .crm = 0, .opc1 = 0, .opc2 = 0,
       .access = PL1_RW, .type = ARM_CP_ALIAS,
       .bank_fieldoffsets = { offsetoflow32(CPUARMState, cp15.dfsr_s),
@@ -1856,6 +1856,14 @@ static const ARMCPRegInfo vmsa_cp_reginfo[] = {
       .access = PL1_RW, .resetvalue = 0,
       .bank_fieldoffsets = { offsetoflow32(CPUARMState, cp15.ifsr_s),
                              offsetoflow32(CPUARMState, cp15.ifsr_ns) } },
+    { .name = "DFAR", .cp = 15, .opc1 = 0, .crn = 6, .crm = 0, .opc2 = 0,
+      .access = PL1_RW, .resetvalue = 0,
+      .bank_fieldoffsets = { offsetof(CPUARMState, cp15.dfar_s),
+                             offsetof(CPUARMState, cp15.dfar_ns) } },
+    REGINFO_SENTINEL
+};
+
+static const ARMCPRegInfo vmsa_cp_reginfo[] = {
     { .name = "ESR_EL1", .state = ARM_CP_STATE_AA64,
       .opc0 = 3, .crn = 5, .crm = 2, .opc1 = 0, .opc2 = 0,
       .access = PL1_RW,
@@ -1884,10 +1892,6 @@ static const ARMCPRegInfo vmsa_cp_reginfo[] = {
       .opc0 = 3, .crn = 6, .crm = 0, .opc1 = 0, .opc2 = 0,
       .access = PL1_RW, .fieldoffset = offsetof(CPUARMState, cp15.far_el[1]),
       .resetvalue = 0, },
-    { .name = "DFAR", .cp = 15, .opc1 = 0, .crn = 6, .crm = 0, .opc2 = 0,
-      .access = PL1_RW, .resetvalue = 0,
-      .bank_fieldoffsets = { offsetof(CPUARMState, cp15.dfar_s),
-                             offsetof(CPUARMState, cp15.dfar_ns) } },
     REGINFO_SENTINEL
 };
 
@@ -3279,6 +3283,7 @@ void register_cp_regs_for_features(ARMCPU *cpu)
         assert(!arm_feature(env, ARM_FEATURE_V6));
         define_arm_cp_regs(cpu, pmsav5_cp_reginfo);
     } else {
+        define_arm_cp_regs(cpu, vmsa_pmsa_cp_reginfo);
         define_arm_cp_regs(cpu, vmsa_cp_reginfo);
     }
     if (arm_feature(env, ARM_FEATURE_THUMB2EE)) {
-- 
2.4.2.3.g2ffcb72

Re: [Qemu-devel] [PATCH target-arm v1 2/9] arm: helper: Factor out CP regs common to [pv]msa

[Peter Maydell]

On 1 June 2015 at 19:04, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> V6+ PMSA and VMSA share some common registers that are currently
> in the VMSA definition block. Split them out into a new def that can
> be shared to PMSA.
>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
>  target-arm/helper.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 1cc4993..78b6406 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -1846,7 +1846,7 @@ static void vmsa_ttbr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>      raw_write(env, ri, value);
>  }
>
> -static const ARMCPRegInfo vmsa_cp_reginfo[] = {
> +static const ARMCPRegInfo vmsa_pmsa_cp_reginfo[] = {
>      { .name = "DFSR", .cp = 15, .crn = 5, .crm = 0, .opc1 = 0, .opc2 = 0,
>        .access = PL1_RW, .type = ARM_CP_ALIAS,
>        .bank_fieldoffsets = { offsetoflow32(CPUARMState, cp15.dfsr_s),
> @@ -1856,6 +1856,14 @@ static const ARMCPRegInfo vmsa_cp_reginfo[] = {
>        .access = PL1_RW, .resetvalue = 0,
>        .bank_fieldoffsets = { offsetoflow32(CPUARMState, cp15.ifsr_s),
>                               offsetoflow32(CPUARMState, cp15.ifsr_ns) } },
> +    { .name = "DFAR", .cp = 15, .opc1 = 0, .crn = 6, .crm = 0, .opc2 = 0,
> +      .access = PL1_RW, .resetvalue = 0,
> +      .bank_fieldoffsets = { offsetof(CPUARMState, cp15.dfar_s),
> +                             offsetof(CPUARMState, cp15.dfar_ns) } },

Can you move the FAR_EL1 reginfo as well, please? They should stay
together because they're the 32 and 64 bit versions of the same
register.

(Aside: probably there's a missing ALIAS mark.)

Otherwise looks OK.

-- PMM

Re: [Qemu-devel] [PATCH target-arm v1 2/9] arm: helper: Factor out CP regs common to [pv]msa

[Peter Crosthwaite]

On Mon, Jun 1, 2015 at 11:48 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 1 June 2015 at 19:04, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
>> V6+ PMSA and VMSA share some common registers that are currently
>> in the VMSA definition block. Split them out into a new def that can
>> be shared to PMSA.
>>
>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>> ---
>>  target-arm/helper.c | 15 ++++++++++-----
>>  1 file changed, 10 insertions(+), 5 deletions(-)
>>
>> diff --git a/target-arm/helper.c b/target-arm/helper.c
>> index 1cc4993..78b6406 100644
>> --- a/target-arm/helper.c
>> +++ b/target-arm/helper.c
>> @@ -1846,7 +1846,7 @@ static void vmsa_ttbr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>>      raw_write(env, ri, value);
>>  }
>>
>> -static const ARMCPRegInfo vmsa_cp_reginfo[] = {
>> +static const ARMCPRegInfo vmsa_pmsa_cp_reginfo[] = {
>>      { .name = "DFSR", .cp = 15, .crn = 5, .crm = 0, .opc1 = 0, .opc2 = 0,
>>        .access = PL1_RW, .type = ARM_CP_ALIAS,
>>        .bank_fieldoffsets = { offsetoflow32(CPUARMState, cp15.dfsr_s),
>> @@ -1856,6 +1856,14 @@ static const ARMCPRegInfo vmsa_cp_reginfo[] = {
>>        .access = PL1_RW, .resetvalue = 0,
>>        .bank_fieldoffsets = { offsetoflow32(CPUARMState, cp15.ifsr_s),
>>                               offsetoflow32(CPUARMState, cp15.ifsr_ns) } },
>> +    { .name = "DFAR", .cp = 15, .opc1 = 0, .crn = 6, .crm = 0, .opc2 = 0,
>> +      .access = PL1_RW, .resetvalue = 0,
>> +      .bank_fieldoffsets = { offsetof(CPUARMState, cp15.dfar_s),
>> +                             offsetof(CPUARMState, cp15.dfar_ns) } },
>
> Can you move the FAR_EL1 reginfo as well, please? They should stay
> together because they're the 32 and 64 bit versions of the same
> register.
>

Done.

> (Aside: probably there's a missing ALIAS mark.)
>

I'm not sure of the full impact of this change. Who should be the
aliaser and aliasee? I have left it out for the moment.

Regards,
Peter

> Otherwise looks OK.
>
> -- PMM
>

Re: [Qemu-devel] [PATCH target-arm v1 2/9] arm: helper: Factor out CP regs common to [pv]msa

[Peter Maydell]

On 4 June 2015 at 18:54, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> On Mon, Jun 1, 2015 at 11:48 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>>
>>> -static const ARMCPRegInfo vmsa_cp_reginfo[] = {
>>> +static const ARMCPRegInfo vmsa_pmsa_cp_reginfo[] = {
>>>      { .name = "DFSR", .cp = 15, .crn = 5, .crm = 0, .opc1 = 0, .opc2 = 0,
>>>        .access = PL1_RW, .type = ARM_CP_ALIAS,
>>>        .bank_fieldoffsets = { offsetoflow32(CPUARMState, cp15.dfsr_s),
>>> @@ -1856,6 +1856,14 @@ static const ARMCPRegInfo vmsa_cp_reginfo[] = {
>>>        .access = PL1_RW, .resetvalue = 0,
>>>        .bank_fieldoffsets = { offsetoflow32(CPUARMState, cp15.ifsr_s),
>>>                               offsetoflow32(CPUARMState, cp15.ifsr_ns) } },
>>> +    { .name = "DFAR", .cp = 15, .opc1 = 0, .crn = 6, .crm = 0, .opc2 = 0,
>>> +      .access = PL1_RW, .resetvalue = 0,
>>> +      .bank_fieldoffsets = { offsetof(CPUARMState, cp15.dfar_s),
>>> +                             offsetof(CPUARMState, cp15.dfar_ns) } },
>>
>> Can you move the FAR_EL1 reginfo as well, please? They should stay
>> together because they're the 32 and 64 bit versions of the same
>> register.
>>
>
> Done.
>
>> (Aside: probably there's a missing ALIAS mark.)
>>
>
> I'm not sure of the full impact of this change. Who should be the
> aliaser and aliasee? I have left it out for the moment.

The rule is that coprocessor state should be transmitted once, which
means that if we have two reginfo structs referring to (parts of)
the same underlying data, we mark one as ALIAS so it's not transferred.
The approach we've taken is that the 64-bit version is the "master"
and the 32-bit version is the alias. (Even a 32-bit CPU will have
the 64-bit regdefs, they just aren't guest visible so they end up
just acting as the means for migrating the cpreg state.)

Looking more closely, though, the FARs have a complicated setup where
the secure banked versions are aliased to FAR_EL2, which might not
exist in some configs. That's a bit weird and is probably why they're
not marked ALIAS. I think transferring the state twice should be pretty
harmless, so leave it as it is for now. I may get back to looking at
whether this is the best thing (or more likely may not...)

thanks
-- PMM

[Qemu-devel] [PATCH target-arm v1 3/9] target-arm/helper.c: define MPUIR register

[Peter Crosthwaite]

Just hardcoded to 16way unified MPU.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---
 target-arm/cpu.h    | 2 ++
 target-arm/helper.c | 4 ++++
 2 files changed, 6 insertions(+)

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 21b5b8e..09cc16d 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -115,6 +115,8 @@ typedef struct ARMGenericTimer {
 #define GTIMER_VIRT 1
 #define NUM_GTIMERS 2
 
+#define PMSAV7_MPU_NUM_REGIONS 16
+
 typedef struct {
     uint64_t raw_tcr;
     uint32_t mask;
diff --git a/target-arm/helper.c b/target-arm/helper.c
index 78b6406..cb21bbf 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -3387,6 +3387,10 @@ void register_cp_regs_for_features(ARMCPU *cpu)
             { .name = "TLBTR",
               .cp = 15, .crn = 0, .crm = 0, .opc1 = 0, .opc2 = 3,
               .access = PL1_R, .type = ARM_CP_CONST, .resetvalue = 0 },
+            { .name = "MPUIR",
+              .cp = 15, .crn = 0, .crm = 0, .opc1 = 0, .opc2 = 4,
+              .access = PL1_R, .type = ARM_CP_CONST,
+              .resetvalue = PMSAV7_MPU_NUM_REGIONS << 8 },
             REGINFO_SENTINEL
         };
         ARMCPRegInfo crn0_wi_reginfo = {
-- 
2.4.2.3.g2ffcb72

Re: [Qemu-devel] [PATCH target-arm v1 3/9] target-arm/helper.c: define MPUIR register

[Peter Maydell]

On 1 June 2015 at 19:04, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> Just hardcoded to 16way unified MPU.
>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
>  target-arm/cpu.h    | 2 ++
>  target-arm/helper.c | 4 ++++
>  2 files changed, 6 insertions(+)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 21b5b8e..09cc16d 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -115,6 +115,8 @@ typedef struct ARMGenericTimer {
>  #define GTIMER_VIRT 1
>  #define NUM_GTIMERS 2
>
> +#define PMSAV7_MPU_NUM_REGIONS 16
> +
>  typedef struct {
>      uint64_t raw_tcr;
>      uint32_t mask;
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 78b6406..cb21bbf 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -3387,6 +3387,10 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>              { .name = "TLBTR",
>                .cp = 15, .crn = 0, .crm = 0, .opc1 = 0, .opc2 = 3,
>                .access = PL1_R, .type = ARM_CP_CONST, .resetvalue = 0 },
> +            { .name = "MPUIR",
> +              .cp = 15, .crn = 0, .crm = 0, .opc1 = 0, .opc2 = 4,
> +              .access = PL1_R, .type = ARM_CP_CONST,
> +              .resetvalue = PMSAV7_MPU_NUM_REGIONS << 8 },
>              REGINFO_SENTINEL
>          };
>          ARMCPRegInfo crn0_wi_reginfo = {

Isn't this going to define the register for VMSA as well?

-- PMM

Re: [Qemu-devel] [PATCH target-arm v1 3/9] target-arm/helper.c: define MPUIR register

[Peter Crosthwaite]

On Mon, Jun 1, 2015 at 11:50 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 1 June 2015 at 19:04, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
>> Just hardcoded to 16way unified MPU.
>>
>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>> ---
>>  target-arm/cpu.h    | 2 ++
>>  target-arm/helper.c | 4 ++++
>>  2 files changed, 6 insertions(+)
>>
>> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
>> index 21b5b8e..09cc16d 100644
>> --- a/target-arm/cpu.h
>> +++ b/target-arm/cpu.h
>> @@ -115,6 +115,8 @@ typedef struct ARMGenericTimer {
>>  #define GTIMER_VIRT 1
>>  #define NUM_GTIMERS 2
>>
>> +#define PMSAV7_MPU_NUM_REGIONS 16
>> +
>>  typedef struct {
>>      uint64_t raw_tcr;
>>      uint32_t mask;
>> diff --git a/target-arm/helper.c b/target-arm/helper.c
>> index 78b6406..cb21bbf 100644
>> --- a/target-arm/helper.c
>> +++ b/target-arm/helper.c
>> @@ -3387,6 +3387,10 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>>              { .name = "TLBTR",
>>                .cp = 15, .crn = 0, .crm = 0, .opc1 = 0, .opc2 = 3,
>>                .access = PL1_R, .type = ARM_CP_CONST, .resetvalue = 0 },
>> +            { .name = "MPUIR",
>> +              .cp = 15, .crn = 0, .crm = 0, .opc1 = 0, .opc2 = 4,
>> +              .access = PL1_R, .type = ARM_CP_CONST,
>> +              .resetvalue = PMSAV7_MPU_NUM_REGIONS << 8 },
>>              REGINFO_SENTINEL
>>          };
>>          ARMCPRegInfo crn0_wi_reginfo = {
>
> Isn't this going to define the register for VMSA as well?
>

Yes. So I was going for symmetry with TLBTR which is VMSA only but
defined for PMSA. Should we put MPUIR in the PMSA register defs?

Regards,
Peter

> -- PMM
>

Re: [Qemu-devel] [PATCH target-arm v1 3/9] target-arm/helper.c: define MPUIR register

[Peter Maydell]

On 2 June 2015 at 10:29, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> On Mon, Jun 1, 2015 at 11:50 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 1 June 2015 at 19:04, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
>>> Just hardcoded to 16way unified MPU.
>>>
>>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>>> ---
>>>  target-arm/cpu.h    | 2 ++
>>>  target-arm/helper.c | 4 ++++
>>>  2 files changed, 6 insertions(+)
>>>
>>> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
>>> index 21b5b8e..09cc16d 100644
>>> --- a/target-arm/cpu.h
>>> +++ b/target-arm/cpu.h
>>> @@ -115,6 +115,8 @@ typedef struct ARMGenericTimer {
>>>  #define GTIMER_VIRT 1
>>>  #define NUM_GTIMERS 2
>>>
>>> +#define PMSAV7_MPU_NUM_REGIONS 16
>>> +
>>>  typedef struct {
>>>      uint64_t raw_tcr;
>>>      uint32_t mask;
>>> diff --git a/target-arm/helper.c b/target-arm/helper.c
>>> index 78b6406..cb21bbf 100644
>>> --- a/target-arm/helper.c
>>> +++ b/target-arm/helper.c
>>> @@ -3387,6 +3387,10 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>>>              { .name = "TLBTR",
>>>                .cp = 15, .crn = 0, .crm = 0, .opc1 = 0, .opc2 = 3,
>>>                .access = PL1_R, .type = ARM_CP_CONST, .resetvalue = 0 },
>>> +            { .name = "MPUIR",
>>> +              .cp = 15, .crn = 0, .crm = 0, .opc1 = 0, .opc2 = 4,
>>> +              .access = PL1_R, .type = ARM_CP_CONST,
>>> +              .resetvalue = PMSAV7_MPU_NUM_REGIONS << 8 },
>>>              REGINFO_SENTINEL
>>>          };
>>>          ARMCPRegInfo crn0_wi_reginfo = {
>>
>> Isn't this going to define the register for VMSA as well?
>>
>
> Yes. So I was going for symmetry with TLBTR which is VMSA only but
> defined for PMSA. Should we put MPUIR in the PMSA register defs?

If we have VMSA-only registers which get defined for PMSA this
is basically just a bug resulting from the fact that thus far
nobody's cared very much about PMSA cores.

In fact on the only core we have with MPU and cp15 (the 946)
the TLBTR (0, c0, c0, 3) should be an alias of the main ID
register, so having it be the TLBTR on that core is definitely
wrong. Ideally you should fish that out of the common definitions
so we only define it on VMSA cores.

-- PMM

Re: [Qemu-devel] [PATCH target-arm v1 3/9] target-arm/helper.c: define MPUIR register

[Peter Crosthwaite]

On Tue, Jun 2, 2015 at 2:51 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 2 June 2015 at 10:29, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
>> On Mon, Jun 1, 2015 at 11:50 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> On 1 June 2015 at 19:04, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
>>>> Just hardcoded to 16way unified MPU.
>>>>
>>>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>>>> ---
>>>>  target-arm/cpu.h    | 2 ++
>>>>  target-arm/helper.c | 4 ++++
>>>>  2 files changed, 6 insertions(+)
>>>>
>>>> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
>>>> index 21b5b8e..09cc16d 100644
>>>> --- a/target-arm/cpu.h
>>>> +++ b/target-arm/cpu.h
>>>> @@ -115,6 +115,8 @@ typedef struct ARMGenericTimer {
>>>>  #define GTIMER_VIRT 1
>>>>  #define NUM_GTIMERS 2
>>>>
>>>> +#define PMSAV7_MPU_NUM_REGIONS 16
>>>> +
>>>>  typedef struct {
>>>>      uint64_t raw_tcr;
>>>>      uint32_t mask;
>>>> diff --git a/target-arm/helper.c b/target-arm/helper.c
>>>> index 78b6406..cb21bbf 100644
>>>> --- a/target-arm/helper.c
>>>> +++ b/target-arm/helper.c
>>>> @@ -3387,6 +3387,10 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>>>>              { .name = "TLBTR",
>>>>                .cp = 15, .crn = 0, .crm = 0, .opc1 = 0, .opc2 = 3,
>>>>                .access = PL1_R, .type = ARM_CP_CONST, .resetvalue = 0 },
>>>> +            { .name = "MPUIR",
>>>> +              .cp = 15, .crn = 0, .crm = 0, .opc1 = 0, .opc2 = 4,
>>>> +              .access = PL1_R, .type = ARM_CP_CONST,
>>>> +              .resetvalue = PMSAV7_MPU_NUM_REGIONS << 8 },
>>>>              REGINFO_SENTINEL
>>>>          };
>>>>          ARMCPRegInfo crn0_wi_reginfo = {
>>>
>>> Isn't this going to define the register for VMSA as well?
>>>
>>
>> Yes. So I was going for symmetry with TLBTR which is VMSA only but
>> defined for PMSA. Should we put MPUIR in the PMSA register defs?
>
> If we have VMSA-only registers which get defined for PMSA this
> is basically just a bug resulting from the fact that thus far
> nobody's cared very much about PMSA cores.
>
> In fact on the only core we have with MPU and cp15 (the 946)
> the TLBTR (0, c0, c0, 3) should be an alias of the main ID
> register, so having it be the TLBTR on that core is definitely
> wrong. Ideally you should fish that out of the common definitions
> so we only define it on VMSA cores.
>

Fished. This will be a new patch in V2.

Regards,
Peter

> -- PMM
>

Re: [Qemu-devel] [PATCH target-arm v1 3/9] target-arm/helper.c: define MPUIR register

[Peter Crosthwaite]

On Tue, Jun 2, 2015 at 2:51 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 2 June 2015 at 10:29, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
>> On Mon, Jun 1, 2015 at 11:50 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> On 1 June 2015 at 19:04, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
>>>> Just hardcoded to 16way unified MPU.
>>>>
>>>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>>>> ---
>>>>  target-arm/cpu.h    | 2 ++
>>>>  target-arm/helper.c | 4 ++++
>>>>  2 files changed, 6 insertions(+)
>>>>
>>>> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
>>>> index 21b5b8e..09cc16d 100644
>>>> --- a/target-arm/cpu.h
>>>> +++ b/target-arm/cpu.h
>>>> @@ -115,6 +115,8 @@ typedef struct ARMGenericTimer {
>>>>  #define GTIMER_VIRT 1
>>>>  #define NUM_GTIMERS 2
>>>>
>>>> +#define PMSAV7_MPU_NUM_REGIONS 16
>>>> +
>>>>  typedef struct {
>>>>      uint64_t raw_tcr;
>>>>      uint32_t mask;
>>>> diff --git a/target-arm/helper.c b/target-arm/helper.c
>>>> index 78b6406..cb21bbf 100644
>>>> --- a/target-arm/helper.c
>>>> +++ b/target-arm/helper.c
>>>> @@ -3387,6 +3387,10 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>>>>              { .name = "TLBTR",
>>>>                .cp = 15, .crn = 0, .crm = 0, .opc1 = 0, .opc2 = 3,
>>>>                .access = PL1_R, .type = ARM_CP_CONST, .resetvalue = 0 },
>>>> +            { .name = "MPUIR",
>>>> +              .cp = 15, .crn = 0, .crm = 0, .opc1 = 0, .opc2 = 4,
>>>> +              .access = PL1_R, .type = ARM_CP_CONST,
>>>> +              .resetvalue = PMSAV7_MPU_NUM_REGIONS << 8 },
>>>>              REGINFO_SENTINEL
>>>>          };
>>>>          ARMCPRegInfo crn0_wi_reginfo = {
>>>
>>> Isn't this going to define the register for VMSA as well?
>>>
>>
>> Yes. So I was going for symmetry with TLBTR which is VMSA only but
>> defined for PMSA. Should we put MPUIR in the PMSA register defs?
>
> If we have VMSA-only registers which get defined for PMSA this
> is basically just a bug resulting from the fact that thus far
> nobody's cared very much about PMSA cores.
>
> In fact on the only core we have with MPU and cp15 (the 946)
> the TLBTR (0, c0, c0, 3) should be an alias of the main ID
> register, so having it be the TLBTR on that core is definitely
> wrong. Ideally you should fish that out of the common definitions
> so we only define it on VMSA cores.
>

So I got ARM ARM v6 doc (I couldn't seem to get one specific to v5)
and the MPUIR seems to be an invention starting in V6.

0b011 TLB type register
0b100 MPU type register (PMSAv6)

(section B3.3 of ARM DDI 0100I)

So the TLBTR should be an always WRT to versions but the MPUIR needs
ARM_FEATURE_V6.

Regards,
Peter

> -- PMM
>

[Qemu-devel] [PATCH target-arm v1 4/9] target-arm: Add registers for PMSAv7

[Peter Crosthwaite]

define the arm CP registers for PMSAv7 and their accessor functions.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---
 target-arm/cpu.h    |  6 ++++++
 target-arm/helper.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 54 insertions(+)

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 09cc16d..9cb2e49 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -286,6 +286,12 @@ typedef struct CPUARMState {
             };
             uint64_t par_el[4];
         };
+
+        uint32_t c6_rgnr;
+        uint32_t c6_drbar[PMSAV7_MPU_NUM_REGIONS];
+        uint32_t c6_drsr[PMSAV7_MPU_NUM_REGIONS];
+        uint32_t c6_dracr[PMSAV7_MPU_NUM_REGIONS];
+
         uint32_t c9_insn; /* Cache lockdown registers.  */
         uint32_t c9_data;
         uint64_t c9_pmcr; /* performance monitor control register */
diff --git a/target-arm/helper.c b/target-arm/helper.c
index cb21bbf..f11efea 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -1709,6 +1709,54 @@ static uint64_t pmsav5_insn_ap_read(CPUARMState *env, const ARMCPRegInfo *ri)
     return simple_mpu_ap_bits(env->cp15.pmsav5_insn_ap);
 }
 
+static uint64_t pmsav7_read(CPUARMState *env, const ARMCPRegInfo *ri)
+{
+    uint32_t *u32p = (void *)env + ri->fieldoffset;
+
+    u32p += env->cp15.c6_rgnr;
+    return *u32p;
+}
+
+static void pmsav7_write(CPUARMState *env, const ARMCPRegInfo *ri,
+                         uint64_t value)
+{
+    ARMCPU *cpu = arm_env_get_cpu(env);
+    uint32_t *u32p = (void *)env + ri->fieldoffset;
+
+    u32p += env->cp15.c6_rgnr;
+    tlb_flush(CPU(cpu), 1); /* Mappings may have changed - purge! */
+    *u32p = value;
+}
+
+static void pmsav7_reset(CPUARMState *env, const ARMCPRegInfo *ri)
+{
+    int i;
+    uint32_t *u32p = (void *)env + ri->fieldoffset;
+
+    for (i = 0; i < 16; ++i) {
+        u32p[i] = 0;
+    }
+}
+
+static const ARMCPRegInfo pmsav7_cp_reginfo[] = {
+    { .name = "DRBAR", .cp = 15, .crn = 6, .opc1 = 0, .crm = 1, .opc2 = 0,
+      .access = PL1_RW,
+      .fieldoffset = offsetof(CPUARMState, cp15.c6_drbar[0]),
+      .readfn = pmsav7_read, .writefn = pmsav7_write, .resetfn = pmsav7_reset },
+    { .name = "DRSR", .cp = 15, .crn = 6, .opc1 = 0, .crm = 1, .opc2 = 2,
+      .access = PL1_RW,
+      .fieldoffset = offsetof(CPUARMState, cp15.c6_drsr[0]),
+      .readfn = pmsav7_read, .writefn = pmsav7_write, .resetfn = pmsav7_reset },
+    { .name = "DRACR", .cp = 15, .crn = 6, .opc1 = 0, .crm = 1, .opc2 = 4,
+      .access = PL1_RW,
+      .fieldoffset = offsetof(CPUARMState, cp15.c6_dracr[0]),
+      .readfn = pmsav7_read, .writefn = pmsav7_write, .resetfn = pmsav7_reset },
+    { .name = "RGNR", .cp = 15, .crn = 6, .opc1 = 0, .crm = 2, .opc2 = 0,
+      .access = PL1_RW,
+      .fieldoffset = offsetof(CPUARMState, cp15.c6_rgnr), .resetvalue = 0, },
+    REGINFO_SENTINEL
+};
+
 static const ARMCPRegInfo pmsav5_cp_reginfo[] = {
     { .name = "DATA_AP", .cp = 15, .crn = 5, .crm = 0, .opc1 = 0, .opc2 = 0,
       .access = PL1_RW, .type = ARM_CP_ALIAS,
-- 
2.4.2.3.g2ffcb72

Re: [Qemu-devel] [PATCH target-arm v1 4/9] target-arm: Add registers for PMSAv7

[Peter Maydell]

On 1 June 2015 at 19:04, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> define the arm CP registers for PMSAv7 and their accessor functions.
>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
>  target-arm/cpu.h    |  6 ++++++
>  target-arm/helper.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 54 insertions(+)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 09cc16d..9cb2e49 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -286,6 +286,12 @@ typedef struct CPUARMState {
>              };
>              uint64_t par_el[4];
>          };
> +
> +        uint32_t c6_rgnr;
> +        uint32_t c6_drbar[PMSAV7_MPU_NUM_REGIONS];
> +        uint32_t c6_drsr[PMSAV7_MPU_NUM_REGIONS];
> +        uint32_t c6_dracr[PMSAV7_MPU_NUM_REGIONS];
> +
>          uint32_t c9_insn; /* Cache lockdown registers.  */
>          uint32_t c9_data;
>          uint64_t c9_pmcr; /* performance monitor control register */
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index cb21bbf..f11efea 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -1709,6 +1709,54 @@ static uint64_t pmsav5_insn_ap_read(CPUARMState *env, const ARMCPRegInfo *ri)
>      return simple_mpu_ap_bits(env->cp15.pmsav5_insn_ap);
>  }
>
> +static uint64_t pmsav7_read(CPUARMState *env, const ARMCPRegInfo *ri)
> +{
> +    uint32_t *u32p = (void *)env + ri->fieldoffset;

This is raw_ptr(env, ri);

> +
> +    u32p += env->cp15.c6_rgnr;
> +    return *u32p;
> +}
> +
> +static void pmsav7_write(CPUARMState *env, const ARMCPRegInfo *ri,
> +                         uint64_t value)
> +{
> +    ARMCPU *cpu = arm_env_get_cpu(env);
> +    uint32_t *u32p = (void *)env + ri->fieldoffset;
> +
> +    u32p += env->cp15.c6_rgnr;
> +    tlb_flush(CPU(cpu), 1); /* Mappings may have changed - purge! */
> +    *u32p = value;

Since you're not boundary-checking c6_rgnr either when the guest
tries to set it (or on inbound migration) or at point of use,
this is a handy way for the guest to do controlled writes to
anywhere in QEMU's address space...

> +}
> +
> +static void pmsav7_reset(CPUARMState *env, const ARMCPRegInfo *ri)
> +{
> +    int i;
> +    uint32_t *u32p = (void *)env + ri->fieldoffset;
> +
> +    for (i = 0; i < 16; ++i) {

You have a #define for number of regions...
In any case you could just memset() the whole thing rather
than looping.

> +        u32p[i] = 0;
> +    }
> +}
> +
> +static const ARMCPRegInfo pmsav7_cp_reginfo[] = {
> +    { .name = "DRBAR", .cp = 15, .crn = 6, .opc1 = 0, .crm = 1, .opc2 = 0,
> +      .access = PL1_RW,
> +      .fieldoffset = offsetof(CPUARMState, cp15.c6_drbar[0]),
> +      .readfn = pmsav7_read, .writefn = pmsav7_write, .resetfn = pmsav7_reset },
> +    { .name = "DRSR", .cp = 15, .crn = 6, .opc1 = 0, .crm = 1, .opc2 = 2,
> +      .access = PL1_RW,
> +      .fieldoffset = offsetof(CPUARMState, cp15.c6_drsr[0]),
> +      .readfn = pmsav7_read, .writefn = pmsav7_write, .resetfn = pmsav7_reset },
> +    { .name = "DRACR", .cp = 15, .crn = 6, .opc1 = 0, .crm = 1, .opc2 = 4,
> +      .access = PL1_RW,
> +      .fieldoffset = offsetof(CPUARMState, cp15.c6_dracr[0]),
> +      .readfn = pmsav7_read, .writefn = pmsav7_write, .resetfn = pmsav7_reset },
> +    { .name = "RGNR", .cp = 15, .crn = 6, .opc1 = 0, .crm = 2, .opc2 = 0,
> +      .access = PL1_RW,
> +      .fieldoffset = offsetof(CPUARMState, cp15.c6_rgnr), .resetvalue = 0, },
> +    REGINFO_SENTINEL
> +};

This won't handle migration/state save/load properly.

-- PMM

Re: [Qemu-devel] [PATCH target-arm v1 4/9] target-arm: Add registers for PMSAv7

[Peter Crosthwaite]

On Mon, Jun 1, 2015 at 11:56 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 1 June 2015 at 19:04, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
>> define the arm CP registers for PMSAv7 and their accessor functions.
>>
>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>> ---
>>  target-arm/cpu.h    |  6 ++++++
>>  target-arm/helper.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 54 insertions(+)
>>
>> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
>> index 09cc16d..9cb2e49 100644
>> --- a/target-arm/cpu.h
>> +++ b/target-arm/cpu.h
>> @@ -286,6 +286,12 @@ typedef struct CPUARMState {
>>              };
>>              uint64_t par_el[4];
>>          };
>> +
>> +        uint32_t c6_rgnr;
>> +        uint32_t c6_drbar[PMSAV7_MPU_NUM_REGIONS];
>> +        uint32_t c6_drsr[PMSAV7_MPU_NUM_REGIONS];
>> +        uint32_t c6_dracr[PMSAV7_MPU_NUM_REGIONS];
>> +
>>          uint32_t c9_insn; /* Cache lockdown registers.  */
>>          uint32_t c9_data;
>>          uint64_t c9_pmcr; /* performance monitor control register */
>> diff --git a/target-arm/helper.c b/target-arm/helper.c
>> index cb21bbf..f11efea 100644
>> --- a/target-arm/helper.c
>> +++ b/target-arm/helper.c
>> @@ -1709,6 +1709,54 @@ static uint64_t pmsav5_insn_ap_read(CPUARMState *env, const ARMCPRegInfo *ri)
>>      return simple_mpu_ap_bits(env->cp15.pmsav5_insn_ap);
>>  }
>>
>> +static uint64_t pmsav7_read(CPUARMState *env, const ARMCPRegInfo *ri)
>> +{
>> +    uint32_t *u32p = (void *)env + ri->fieldoffset;
>
> This is raw_ptr(env, ri);
>

Fixed.

>> +
>> +    u32p += env->cp15.c6_rgnr;
>> +    return *u32p;
>> +}
>> +
>> +static void pmsav7_write(CPUARMState *env, const ARMCPRegInfo *ri,
>> +                         uint64_t value)
>> +{
>> +    ARMCPU *cpu = arm_env_get_cpu(env);
>> +    uint32_t *u32p = (void *)env + ri->fieldoffset;
>> +
>> +    u32p += env->cp15.c6_rgnr;
>> +    tlb_flush(CPU(cpu), 1); /* Mappings may have changed - purge! */
>> +    *u32p = value;
>
> Since you're not boundary-checking c6_rgnr either when the guest
> tries to set it (or on inbound migration) or at point of use.
> this is a handy way for the guest to do controlled writes to
> anywhere in QEMU's address space...
>

Boundary check added at time of set.

>> +}
>> +
>> +static void pmsav7_reset(CPUARMState *env, const ARMCPRegInfo *ri)
>> +{
>> +    int i;
>> +    uint32_t *u32p = (void *)env + ri->fieldoffset;
>> +
>> +    for (i = 0; i < 16; ++i) {
>
> You have a #define for number of regions...
> In any case you could just memset() the whole thing rather
> than looping.
>

Memset added. Hard constant removed.

>> +        u32p[i] = 0;
>> +    }
>> +}
>> +
>> +static const ARMCPRegInfo pmsav7_cp_reginfo[] = {
>> +    { .name = "DRBAR", .cp = 15, .crn = 6, .opc1 = 0, .crm = 1, .opc2 = 0,
>> +      .access = PL1_RW,
>> +      .fieldoffset = offsetof(CPUARMState, cp15.c6_drbar[0]),
>> +      .readfn = pmsav7_read, .writefn = pmsav7_write, .resetfn = pmsav7_reset },
>> +    { .name = "DRSR", .cp = 15, .crn = 6, .opc1 = 0, .crm = 1, .opc2 = 2,
>> +      .access = PL1_RW,
>> +      .fieldoffset = offsetof(CPUARMState, cp15.c6_drsr[0]),
>> +      .readfn = pmsav7_read, .writefn = pmsav7_write, .resetfn = pmsav7_reset },
>> +    { .name = "DRACR", .cp = 15, .crn = 6, .opc1 = 0, .crm = 1, .opc2 = 4,
>> +      .access = PL1_RW,
>> +      .fieldoffset = offsetof(CPUARMState, cp15.c6_dracr[0]),
>> +      .readfn = pmsav7_read, .writefn = pmsav7_write, .resetfn = pmsav7_reset },
>> +    { .name = "RGNR", .cp = 15, .crn = 6, .opc1 = 0, .crm = 2, .opc2 = 0,
>> +      .access = PL1_RW,
>> +      .fieldoffset = offsetof(CPUARMState, cp15.c6_rgnr), .resetvalue = 0, },
>> +    REGINFO_SENTINEL
>> +};
>
> This won't handle migration/state save/load properly.
>

So this was non trivial in the end. I have marked the arrays as
ARM_CP_NO_RAW as they simply don't have state in their own right. I
have then added a subsection in machine.c that VMSTATEs the arrays. I
could have used VMSTATE_UINT32_ARRAY but decided to refactor the
series to have the MPU number or regions available as variable in
ARMCPU. This means we can get the VMSTATE right straight away with a
VMSTATE_VARRAY for these arrays.

The tricky part is the RGNR incoming boundary validation. I have added
this to the new subsection but it is disjunct from the actual VMSD for
RGNR itself which just uses the automatic CP reginfo VMSD.

Regards,
Peter


> -- PMM
>

[Qemu-devel] [PATCH target-arm v1 5/9] arm: helper: rename get_phys_addr_mpu

[Peter Crosthwaite]

This get_phys_addr is really for pmsav5. Rename it accordingly.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---
 target-arm/helper.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index f11efea..63859a4 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -5720,9 +5720,9 @@ do_fault:
     return (1 << 9) | (fault_type << 2) | level;
 }
 
-static int get_phys_addr_mpu(CPUARMState *env, uint32_t address,
-                             int access_type, ARMMMUIdx mmu_idx,
-                             hwaddr *phys_ptr, int *prot)
+static int get_phys_addr_pmsav5(CPUARMState *env, uint32_t address,
+                                int access_type, ARMMMUIdx mmu_idx,
+                                hwaddr *phys_ptr, int *prot)
 {
     int n;
     uint32_t mask;
@@ -5857,8 +5857,8 @@ static inline int get_phys_addr(CPUARMState *env, target_ulong address,
 
     if (arm_feature(env, ARM_FEATURE_MPU)) {
         *page_size = TARGET_PAGE_SIZE;
-        return get_phys_addr_mpu(env, address, access_type, mmu_idx, phys_ptr,
-                                 prot);
+        return get_phys_addr_pmsav5(env, address, access_type, mmu_idx,
+                                    phys_ptr, prot);
     }
 
     if (regime_using_lpae_format(env, mmu_idx)) {
-- 
2.4.2.3.g2ffcb72

Re: [Qemu-devel] [PATCH target-arm v1 5/9] arm: helper: rename get_phys_addr_mpu

[Peter Maydell]

On 1 June 2015 at 19:04, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> This get_phys_addr is really for pmsav5. Rename it accordingly.
>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
>  target-arm/helper.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index f11efea..63859a4 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -5720,9 +5720,9 @@ do_fault:
>      return (1 << 9) | (fault_type << 2) | level;
>  }
>
> -static int get_phys_addr_mpu(CPUARMState *env, uint32_t address,
> -                             int access_type, ARMMMUIdx mmu_idx,
> -                             hwaddr *phys_ptr, int *prot)
> +static int get_phys_addr_pmsav5(CPUARMState *env, uint32_t address,
> +                                int access_type, ARMMMUIdx mmu_idx,
> +                                hwaddr *phys_ptr, int *prot)
>  {
>      int n;
>      uint32_t mask;
> @@ -5857,8 +5857,8 @@ static inline int get_phys_addr(CPUARMState *env, target_ulong address,
>
>      if (arm_feature(env, ARM_FEATURE_MPU)) {
>          *page_size = TARGET_PAGE_SIZE;
> -        return get_phys_addr_mpu(env, address, access_type, mmu_idx, phys_ptr,
> -                                 prot);
> +        return get_phys_addr_pmsav5(env, address, access_type, mmu_idx,
> +                                    phys_ptr, prot);
>      }
>
>      if (regime_using_lpae_format(env, mmu_idx)) {
> --
> 2.4.2.3.g2ffcb72
>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[Qemu-devel] [PATCH target-arm v1 6/9] target-arm: Implement PMSAv7 MPU

[Peter Crosthwaite]

Unified MPU only. Uses ARM architecture major revision to switch
between PMSAv5 and v7 when ARM_FEATURE_MPU is set. PMSA v6 remains
unsupported and is asserted against.

The return code from get_phys_addr has to patched to handle the case
of a 0 FSR with error. Use -1 to indicate this condition.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---
 target-arm/cpu.h    |   1 +
 target-arm/helper.c | 153 ++++++++++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 144 insertions(+), 10 deletions(-)

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 9cb2e49..73e2619 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -559,6 +559,7 @@ void pmccntr_sync(CPUARMState *env);
 #define SCTLR_DT      (1U << 16) /* up to ??, RAO in v6 and v7 */
 #define SCTLR_nTWI    (1U << 16) /* v8 onward */
 #define SCTLR_HA      (1U << 17)
+#define SCTLR_BR      (1U << 17) /* PMSA only */
 #define SCTLR_IT      (1U << 18) /* up to ??, RAO in v6 and v7 */
 #define SCTLR_nTWE    (1U << 18) /* v8 onward */
 #define SCTLR_WXN     (1U << 19)
diff --git a/target-arm/helper.c b/target-arm/helper.c
index 63859a4..09210d3 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -3323,13 +3323,13 @@ void register_cp_regs_for_features(ARMCPU *cpu)
         define_one_arm_cp_reg(cpu, &rvbar);
     }
     if (arm_feature(env, ARM_FEATURE_MPU)) {
-        /* These are the MPU registers prior to PMSAv6. Any new
-         * PMSA core later than the ARM946 will require that we
-         * implement the PMSAv6 or PMSAv7 registers, which are
-         * completely different.
-         */
-        assert(!arm_feature(env, ARM_FEATURE_V6));
-        define_arm_cp_regs(cpu, pmsav5_cp_reginfo);
+        if (arm_feature(env, ARM_FEATURE_V6)) {
+            assert(arm_feature(env, ARM_FEATURE_V7));
+            define_arm_cp_regs(cpu, vmsa_pmsa_cp_reginfo);
+            define_arm_cp_regs(cpu, pmsav7_cp_reginfo);
+        } else {
+            define_arm_cp_regs(cpu, pmsav5_cp_reginfo);
+        }
     } else {
         define_arm_cp_regs(cpu, vmsa_pmsa_cp_reginfo);
         define_arm_cp_regs(cpu, vmsa_cp_reginfo);
@@ -5720,6 +5720,130 @@ do_fault:
     return (1 << 9) | (fault_type << 2) | level;
 }
 
+static inline void get_phys_addr_pmsav7_default(CPUARMState *env,
+                                                ARMMMUIdx mmu_idx,
+                                                int32_t address, int *prot)
+{
+    *prot = PAGE_READ | PAGE_WRITE;
+    switch (address) {
+    case 0xF0000000 ... 0xFFFFFFFF:
+        if (regime_sctlr(env, mmu_idx) & SCTLR_V) { /* hivecs execing is ok */
+            *prot |= PAGE_EXEC;
+        }
+        break;
+    case 0x00000000 ... 0x7FFFFFFF:
+        *prot |= PAGE_EXEC;
+        break;
+    }
+
+}
+
+static int get_phys_addr_pmsav7(CPUARMState *env, uint32_t address,
+                                int access_type, ARMMMUIdx mmu_idx,
+                                hwaddr *phys_ptr, int *prot)
+{
+    int n;
+    bool is_user = regime_is_user(env, mmu_idx);
+
+    *phys_ptr = address;
+    *prot = 0;
+
+    if (regime_translation_disabled(env, mmu_idx)) { /* MPU disabled */
+        get_phys_addr_pmsav7_default(env, mmu_idx, address, prot);
+    } else { /* MPU enabled */
+        for (n = 15; n >= 0; n--) { /* region search */
+            uint32_t base = env->cp15.c6_drbar[n];
+            uint32_t rsize = extract32(env->cp15.c6_drsr[n], 1, 5) + 1;
+            int snd;
+
+            if (!(env->cp15.c6_drsr[n] & 0x1)) {
+                continue;
+            }
+            if (rsize < 2) {
+                qemu_log_mask(LOG_GUEST_ERROR, "DRSR.Rsize field can not be 0");
+            }
+
+            if (address < base || address > base - 1 + (1ull << rsize)) {
+                continue;
+            }
+
+            if (rsize < 8) { /* no subregions for regions < 256 bytes */
+                break;
+            }
+
+            rsize -= 3; /* sub region size (power of 2) */
+            snd = (address - base) >> rsize & 0x7;
+            if (env->cp15.c6_drsr[n] & 1 << (snd + 8)) {
+                continue;
+            }
+            break;
+        }
+
+        if (n == -1) { /* no hits */
+            if (is_user || !(regime_sctlr(env, mmu_idx) & SCTLR_BR)) {
+                /* background fault */
+                return -1;
+            } else {
+                get_phys_addr_pmsav7_default(env, mmu_idx, address, prot);
+            }
+        } else { /* a MPU hit! */
+            uint32_t ap = extract32(env->cp15.c6_dracr[n], 8, 3);
+
+            if (is_user) { /* User mode AP bit decoding */
+                switch (ap) {
+                case 0:
+                case 1:
+                case 5:
+                    break; /* no access */
+                case 3:
+                    *prot |= PAGE_WRITE;
+                    /* fall through */
+                case 2:
+                case 6:
+                    *prot |= PAGE_READ | PAGE_EXEC;
+                    break;
+                default:
+                    qemu_log_mask(LOG_GUEST_ERROR, "Bad value for AP bits in "
+                                  "DRACR");
+                }
+            } else { /* Priv. mode AP bits decoding */
+                switch (ap) {
+                case 0:
+                    break; /* no access */
+                case 1:
+                case 2:
+                case 3:
+                    *prot |= PAGE_WRITE;
+                    /* fall through */
+                case 5:
+                case 6:
+                    *prot |= PAGE_READ | PAGE_EXEC;
+                    break;
+                default:
+                    qemu_log_mask(LOG_GUEST_ERROR, "Bad value for AP bits in "
+                                  "DRACR");
+                }
+            }
+
+            /* execute never */
+            *prot &= env->cp15.c6_dracr[n] & 1 << 12 ? ~PAGE_EXEC : ~0;
+        }
+    }
+
+    switch (access_type) {
+    case 0:
+        return *prot & PAGE_READ ? 0 : 0x00D;
+    case 1:
+        return *prot & PAGE_WRITE ? 0 : 0x00D;
+    case 2:
+        return *prot & PAGE_EXEC ? 0 : 0x00D;
+    default:
+        abort(); /* should be unreachable */
+        return 0;
+    }
+
+}
+
 static int get_phys_addr_pmsav5(CPUARMState *env, uint32_t address,
                                 int access_type, ARMMMUIdx mmu_idx,
                                 hwaddr *phys_ptr, int *prot)
@@ -5795,13 +5919,14 @@ static int get_phys_addr_pmsav5(CPUARMState *env, uint32_t address,
  * MPU state on MPU based systems.
  *
  * Returns 0 if the translation was successful. Otherwise, phys_ptr, attrs,
- * prot and page_size may not be filled in, and the return value provides
+ * prot and page_size may or may-not be filled in, and the return value provides
  * information on why the translation aborted, in the format of a
  * DFSR/IFSR fault register, with the following caveats:
  *  * we honour the short vs long DFSR format differences.
  *  * the WnR bit is never set (the caller must do this).
  *  * for MPU based systems we don't bother to return a full FSR format
  *    value.
+ *  * -1 return value indicates a 0 FSR.
  *
  * @env: CPUARMState
  * @address: virtual address to get physical address for
@@ -5857,8 +5982,14 @@ static inline int get_phys_addr(CPUARMState *env, target_ulong address,
 
     if (arm_feature(env, ARM_FEATURE_MPU)) {
         *page_size = TARGET_PAGE_SIZE;
-        return get_phys_addr_pmsav5(env, address, access_type, mmu_idx,
-                                    phys_ptr, prot);
+        if (arm_feature(env, ARM_FEATURE_V6)) {
+            assert(arm_feature(env, ARM_FEATURE_V7));
+            return get_phys_addr_pmsav7(env, address, access_type, mmu_idx,
+                                        phys_ptr, prot);
+        } else {
+            return get_phys_addr_pmsav5(env, address, access_type, mmu_idx,
+                                        phys_ptr, prot);
+        }
     }
 
     if (regime_using_lpae_format(env, mmu_idx)) {
@@ -5897,6 +6028,8 @@ int arm_tlb_fill(CPUState *cs, vaddr address,
         tlb_set_page_with_attrs(cs, address, phys_addr, attrs,
                                 prot, mmu_idx, page_size);
         return 0;
+    } else if (ret == -1) {
+        ret = 0;
     }
 
     return ret;
-- 
2.4.2.3.g2ffcb72

Re: [Qemu-devel] [PATCH target-arm v1 6/9] target-arm: Implement PMSAv7 MPU

[Peter Maydell]

On 1 June 2015 at 19:04, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> Unified MPU only. Uses ARM architecture major revision to switch
> between PMSAv5 and v7 when ARM_FEATURE_MPU is set. PMSA v6 remains
> unsupported and is asserted against.
>
> The return code from get_phys_addr has to patched to handle the case
> of a 0 FSR with error. Use -1 to indicate this condition.
>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
>  target-arm/cpu.h    |   1 +
>  target-arm/helper.c | 153 ++++++++++++++++++++++++++++++++++++++++++++++++----
>  2 files changed, 144 insertions(+), 10 deletions(-)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 9cb2e49..73e2619 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -559,6 +559,7 @@ void pmccntr_sync(CPUARMState *env);
>  #define SCTLR_DT      (1U << 16) /* up to ??, RAO in v6 and v7 */
>  #define SCTLR_nTWI    (1U << 16) /* v8 onward */
>  #define SCTLR_HA      (1U << 17)
> +#define SCTLR_BR      (1U << 17) /* PMSA only */
>  #define SCTLR_IT      (1U << 18) /* up to ??, RAO in v6 and v7 */
>  #define SCTLR_nTWE    (1U << 18) /* v8 onward */
>  #define SCTLR_WXN     (1U << 19)
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 63859a4..09210d3 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -3323,13 +3323,13 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>          define_one_arm_cp_reg(cpu, &rvbar);
>      }
>      if (arm_feature(env, ARM_FEATURE_MPU)) {
> -        /* These are the MPU registers prior to PMSAv6. Any new
> -         * PMSA core later than the ARM946 will require that we
> -         * implement the PMSAv6 or PMSAv7 registers, which are
> -         * completely different.
> -         */
> -        assert(!arm_feature(env, ARM_FEATURE_V6));
> -        define_arm_cp_regs(cpu, pmsav5_cp_reginfo);
> +        if (arm_feature(env, ARM_FEATURE_V6)) {
> +            assert(arm_feature(env, ARM_FEATURE_V7));

Could use a comment /* PMSAv6 is not implemented */

> +            define_arm_cp_regs(cpu, vmsa_pmsa_cp_reginfo);

It's a bit unfortunate that we've managed to end up with
a cpreg array for "present in both VMSA and PMSA" but the
code path doesn't then allow for calling the define_ function
just once. Oh well.

> +            define_arm_cp_regs(cpu, pmsav7_cp_reginfo);
> +        } else {
> +            define_arm_cp_regs(cpu, pmsav5_cp_reginfo);
> +        }
>      } else {
>          define_arm_cp_regs(cpu, vmsa_pmsa_cp_reginfo);
>          define_arm_cp_regs(cpu, vmsa_cp_reginfo);
> @@ -5720,6 +5720,130 @@ do_fault:
>      return (1 << 9) | (fault_type << 2) | level;
>  }
>
> +static inline void get_phys_addr_pmsav7_default(CPUARMState *env,
> +                                                ARMMMUIdx mmu_idx,
> +                                                int32_t address, int *prot)
> +{
> +    *prot = PAGE_READ | PAGE_WRITE;
> +    switch (address) {
> +    case 0xF0000000 ... 0xFFFFFFFF:
> +        if (regime_sctlr(env, mmu_idx) & SCTLR_V) { /* hivecs execing is ok */
> +            *prot |= PAGE_EXEC;
> +        }
> +        break;
> +    case 0x00000000 ... 0x7FFFFFFF:
> +        *prot |= PAGE_EXEC;
> +        break;
> +    }
> +
> +}
> +
> +static int get_phys_addr_pmsav7(CPUARMState *env, uint32_t address,
> +                                int access_type, ARMMMUIdx mmu_idx,
> +                                hwaddr *phys_ptr, int *prot)
> +{
> +    int n;
> +    bool is_user = regime_is_user(env, mmu_idx);
> +
> +    *phys_ptr = address;
> +    *prot = 0;
> +
> +    if (regime_translation_disabled(env, mmu_idx)) { /* MPU disabled */
> +        get_phys_addr_pmsav7_default(env, mmu_idx, address, prot);

When will this be reached? get_phys_addr() has already caught the
'MPU disabled' case.

> +    } else { /* MPU enabled */
> +        for (n = 15; n >= 0; n--) { /* region search */
> +            uint32_t base = env->cp15.c6_drbar[n];
> +            uint32_t rsize = extract32(env->cp15.c6_drsr[n], 1, 5) + 1;
> +            int snd;
> +
> +            if (!(env->cp15.c6_drsr[n] & 0x1)) {
> +                continue;
> +            }
> +            if (rsize < 2) {
> +                qemu_log_mask(LOG_GUEST_ERROR, "DRSR.Rsize field can not be 0");

This case is UNPREDICTABLE so I would continue here (ie treat the
region as disabled) rather than doing something potentially odd
with an out of range value later.

> +            }

Our effective minimum region size is 1K because that's the
target pagesize setting. Should we enforce that (minimum region
size is impdef so it would be architecturally ok) or will that
break otherwise ok guests?

Base address not aligned to the region size is also UNPREDICTABLE,
incidentally.

> +
> +            if (address < base || address > base - 1 + (1ull << rsize)) {
> +                continue;
> +            }
> +
> +            if (rsize < 8) { /* no subregions for regions < 256 bytes */
> +                break;
> +            }
> +
> +            rsize -= 3; /* sub region size (power of 2) */
> +            snd = (address - base) >> rsize & 0x7;
> +            if (env->cp15.c6_drsr[n] & 1 << (snd + 8)) {

I think I would find these easier to read with more parens
so I didn't have to look up & vs << precedence.

> +                continue;
> +            }
> +            break;
> +        }
> +
> +        if (n == -1) { /* no hits */
> +            if (is_user || !(regime_sctlr(env, mmu_idx) & SCTLR_BR)) {
> +                /* background fault */
> +                return -1;
> +            } else {
> +                get_phys_addr_pmsav7_default(env, mmu_idx, address, prot);
> +            }
> +        } else { /* a MPU hit! */
> +            uint32_t ap = extract32(env->cp15.c6_dracr[n], 8, 3);
> +
> +            if (is_user) { /* User mode AP bit decoding */
> +                switch (ap) {
> +                case 0:
> +                case 1:
> +                case 5:
> +                    break; /* no access */
> +                case 3:
> +                    *prot |= PAGE_WRITE;
> +                    /* fall through */
> +                case 2:
> +                case 6:
> +                    *prot |= PAGE_READ | PAGE_EXEC;
> +                    break;
> +                default:
> +                    qemu_log_mask(LOG_GUEST_ERROR, "Bad value for AP bits in "
> +                                  "DRACR");

Putting the line break after the ',' would let you avoid
splitting the string.

> +                }
> +            } else { /* Priv. mode AP bits decoding */
> +                switch (ap) {
> +                case 0:
> +                    break; /* no access */
> +                case 1:
> +                case 2:
> +                case 3:
> +                    *prot |= PAGE_WRITE;
> +                    /* fall through */
> +                case 5:
> +                case 6:
> +                    *prot |= PAGE_READ | PAGE_EXEC;
> +                    break;
> +                default:
> +                    qemu_log_mask(LOG_GUEST_ERROR, "Bad value for AP bits in "
> +                                  "DRACR");
> +                }
> +            }
> +
> +            /* execute never */
> +            *prot &= env->cp15.c6_dracr[n] & 1 << 12 ? ~PAGE_EXEC : ~0;

Break this down into something more readable, please.

> +        }
> +    }
> +
> +    switch (access_type) {
> +    case 0:
> +        return *prot & PAGE_READ ? 0 : 0x00D;
> +    case 1:
> +        return *prot & PAGE_WRITE ? 0 : 0x00D;
> +    case 2:
> +        return *prot & PAGE_EXEC ? 0 : 0x00D;

This is
   if (!(*prot & (1 << access_type))) {
       return 0xD; /* Permission fault */
   } else {
       return 0;
   }

isn't it?

> +    default:
> +        abort(); /* should be unreachable */
> +        return 0;

The usual way to write this is g_assert_not_reached(), and the return
isn't needed.

> +    }
> +
> +}
> +
>  static int get_phys_addr_pmsav5(CPUARMState *env, uint32_t address,
>                                  int access_type, ARMMMUIdx mmu_idx,
>                                  hwaddr *phys_ptr, int *prot)
> @@ -5795,13 +5919,14 @@ static int get_phys_addr_pmsav5(CPUARMState *env, uint32_t address,
>   * MPU state on MPU based systems.
>   *
>   * Returns 0 if the translation was successful. Otherwise, phys_ptr, attrs,
> - * prot and page_size may not be filled in, and the return value provides
> + * prot and page_size may or may-not be filled in, and the return value provides

No hyphen.

>   * information on why the translation aborted, in the format of a
>   * DFSR/IFSR fault register, with the following caveats:
>   *  * we honour the short vs long DFSR format differences.
>   *  * the WnR bit is never set (the caller must do this).
>   *  * for MPU based systems we don't bother to return a full FSR format
>   *    value.

Is this bullet point still true? If it is, is that a bug
we now need to fix?

> + *  * -1 return value indicates a 0 FSR.
>   *
>   * @env: CPUARMState
>   * @address: virtual address to get physical address for
> @@ -5857,8 +5982,14 @@ static inline int get_phys_addr(CPUARMState *env, target_ulong address,
>
>      if (arm_feature(env, ARM_FEATURE_MPU)) {
>          *page_size = TARGET_PAGE_SIZE;
> -        return get_phys_addr_pmsav5(env, address, access_type, mmu_idx,
> -                                    phys_ptr, prot);
> +        if (arm_feature(env, ARM_FEATURE_V6)) {
> +            assert(arm_feature(env, ARM_FEATURE_V7));
> +            return get_phys_addr_pmsav7(env, address, access_type, mmu_idx,
> +                                        phys_ptr, prot);

v7M will take this code path now (which is the right thing, it is
closer to v7R than to the 946); did you cross check against
the M profile spec to see if any of this is R-profile specific?

(We don't actually implement the M profile MPU right now, but
it would be nice to avoid leaving beartraps for whoever does.)

> +        } else {
> +            return get_phys_addr_pmsav5(env, address, access_type, mmu_idx,
> +                                        phys_ptr, prot);
> +        }
>      }
>
>      if (regime_using_lpae_format(env, mmu_idx)) {
> @@ -5897,6 +6028,8 @@ int arm_tlb_fill(CPUState *cs, vaddr address,
>          tlb_set_page_with_attrs(cs, address, phys_addr, attrs,
>                                  prot, mmu_idx, page_size);
>          return 0;
> +    } else if (ret == -1) {
> +        ret = 0;
>      }
>
>      return ret;

This isn't going to work, because tlb_fill() which calls this
function making the same "0 means OK, non-0 means FSR value"
assumption.

I think the best thing to do here is to switch
get_phys_addr() and friends to a bool return type for
success/failure and pass in a uint32_t* for FSR value.

thanks
-- PMM

Re: [Qemu-devel] [PATCH target-arm v1 6/9] target-arm: Implement PMSAv7 MPU

[Peter Crosthwaite]

On Tue, Jun 2, 2015 at 4:59 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 1 June 2015 at 19:04, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
>> Unified MPU only. Uses ARM architecture major revision to switch
>> between PMSAv5 and v7 when ARM_FEATURE_MPU is set. PMSA v6 remains
>> unsupported and is asserted against.
>>
>> The return code from get_phys_addr has to patched to handle the case
>> of a 0 FSR with error. Use -1 to indicate this condition.
>>
>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>> ---
>>  target-arm/cpu.h    |   1 +
>>  target-arm/helper.c | 153 ++++++++++++++++++++++++++++++++++++++++++++++++----
>>  2 files changed, 144 insertions(+), 10 deletions(-)
>>
>> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
>> index 9cb2e49..73e2619 100644
>> --- a/target-arm/cpu.h
>> +++ b/target-arm/cpu.h
>> @@ -559,6 +559,7 @@ void pmccntr_sync(CPUARMState *env);
>>  #define SCTLR_DT      (1U << 16) /* up to ??, RAO in v6 and v7 */
>>  #define SCTLR_nTWI    (1U << 16) /* v8 onward */
>>  #define SCTLR_HA      (1U << 17)
>> +#define SCTLR_BR      (1U << 17) /* PMSA only */
>>  #define SCTLR_IT      (1U << 18) /* up to ??, RAO in v6 and v7 */
>>  #define SCTLR_nTWE    (1U << 18) /* v8 onward */
>>  #define SCTLR_WXN     (1U << 19)
>> diff --git a/target-arm/helper.c b/target-arm/helper.c
>> index 63859a4..09210d3 100644
>> --- a/target-arm/helper.c
>> +++ b/target-arm/helper.c
>> @@ -3323,13 +3323,13 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>>          define_one_arm_cp_reg(cpu, &rvbar);
>>      }
>>      if (arm_feature(env, ARM_FEATURE_MPU)) {
>> -        /* These are the MPU registers prior to PMSAv6. Any new
>> -         * PMSA core later than the ARM946 will require that we
>> -         * implement the PMSAv6 or PMSAv7 registers, which are
>> -         * completely different.
>> -         */
>> -        assert(!arm_feature(env, ARM_FEATURE_V6));
>> -        define_arm_cp_regs(cpu, pmsav5_cp_reginfo);
>> +        if (arm_feature(env, ARM_FEATURE_V6)) {
>> +            assert(arm_feature(env, ARM_FEATURE_V7));
>
> Could use a comment /* PMSAv6 is not implemented */
>

Added.

>> +            define_arm_cp_regs(cpu, vmsa_pmsa_cp_reginfo);
>
> It's a bit unfortunate that we've managed to end up with
> a cpreg array for "present in both VMSA and PMSA" but the
> code path doesn't then allow for calling the define_ function
> just once. Oh well.
>

Yeh, so I thought this preferrable to having to dup the arm_feature() iffery.

>> +            define_arm_cp_regs(cpu, pmsav7_cp_reginfo);
>> +        } else {
>> +            define_arm_cp_regs(cpu, pmsav5_cp_reginfo);
>> +        }
>>      } else {
>>          define_arm_cp_regs(cpu, vmsa_pmsa_cp_reginfo);
>>          define_arm_cp_regs(cpu, vmsa_cp_reginfo);
>> @@ -5720,6 +5720,130 @@ do_fault:
>>      return (1 << 9) | (fault_type << 2) | level;
>>  }
>>
>> +static inline void get_phys_addr_pmsav7_default(CPUARMState *env,
>> +                                                ARMMMUIdx mmu_idx,
>> +                                                int32_t address, int *prot)
>> +{
>> +    *prot = PAGE_READ | PAGE_WRITE;
>> +    switch (address) {
>> +    case 0xF0000000 ... 0xFFFFFFFF:
>> +        if (regime_sctlr(env, mmu_idx) & SCTLR_V) { /* hivecs execing is ok */
>> +            *prot |= PAGE_EXEC;
>> +        }
>> +        break;
>> +    case 0x00000000 ... 0x7FFFFFFF:
>> +        *prot |= PAGE_EXEC;
>> +        break;
>> +    }
>> +
>> +}
>> +
>> +static int get_phys_addr_pmsav7(CPUARMState *env, uint32_t address,
>> +                                int access_type, ARMMMUIdx mmu_idx,
>> +                                hwaddr *phys_ptr, int *prot)
>> +{
>> +    int n;
>> +    bool is_user = regime_is_user(env, mmu_idx);
>> +
>> +    *phys_ptr = address;
>> +    *prot = 0;
>> +
>> +    if (regime_translation_disabled(env, mmu_idx)) { /* MPU disabled */
>> +        get_phys_addr_pmsav7_default(env, mmu_idx, address, prot);
>
> When will this be reached? get_phys_addr() has already caught the
> 'MPU disabled' case.
>

Nice catch. It is actually a bug in get_phys_addr. Unlike the existing
get_phys_addr_foo implementations, get_phys_addr_pmsav7 needs to
handle the disabled case, as the behaviour should still default to the
backgrounding rather than full system access. That check needs to be
disabled in our case.

>> +    } else { /* MPU enabled */
>> +        for (n = 15; n >= 0; n--) { /* region search */
>> +            uint32_t base = env->cp15.c6_drbar[n];
>> +            uint32_t rsize = extract32(env->cp15.c6_drsr[n], 1, 5) + 1;
>> +            int snd;
>> +
>> +            if (!(env->cp15.c6_drsr[n] & 0x1)) {
>> +                continue;
>> +            }
>> +            if (rsize < 2) {
>> +                qemu_log_mask(LOG_GUEST_ERROR, "DRSR.Rsize field can not be 0");
>
> This case is UNPREDICTABLE so I would continue here (ie treat the
> region as disabled) rather than doing something potentially odd
> with an out of range value later.
>

Fixed.

>> +            }
>
> Our effective minimum region size is 1K because that's the
> target pagesize setting. Should we enforce that (minimum region
> size is impdef so it would be architecturally ok) or will that
> break otherwise ok guests?
>

So this gets complex due to subregions. The real permissions
granularity in on the subregion level. To be safe we would have to go
lowest common denominator with 8K regions so that subregions are only
1k. I have instead added a check to allow use for a 1k region with
consistent subregion settings but disallow inconsistent SRs. The
solution is generalised for all region sizes and subregion bit
consistency combinations (e.g. 2k region with 4 consistent SR bits is
ok too).

I'm disallowing accesses with an UNIMP (ignoring region and
continuing) in the inconsistent case.

> Base address not aligned to the region size is also UNPREDICTABLE,
> incidentally.
>

Check added. region is ignored (continue) in this case.

>> +
>> +            if (address < base || address > base - 1 + (1ull << rsize)) {
>> +                continue;
>> +            }
>> +
>> +            if (rsize < 8) { /* no subregions for regions < 256 bytes */
>> +                break;
>> +            }
>> +
>> +            rsize -= 3; /* sub region size (power of 2) */
>> +            snd = (address - base) >> rsize & 0x7;
>> +            if (env->cp15.c6_drsr[n] & 1 << (snd + 8)) {
>
> I think I would find these easier to read with more parens
> so I didn't have to look up & vs << precedence.
>

changed to extract32.

>> +                continue;
>> +            }
>> +            break;
>> +        }
>> +
>> +        if (n == -1) { /* no hits */
>> +            if (is_user || !(regime_sctlr(env, mmu_idx) & SCTLR_BR)) {
>> +                /* background fault */
>> +                return -1;
>> +            } else {
>> +                get_phys_addr_pmsav7_default(env, mmu_idx, address, prot);
>> +            }
>> +        } else { /* a MPU hit! */
>> +            uint32_t ap = extract32(env->cp15.c6_dracr[n], 8, 3);
>> +
>> +            if (is_user) { /* User mode AP bit decoding */
>> +                switch (ap) {
>> +                case 0:
>> +                case 1:
>> +                case 5:
>> +                    break; /* no access */
>> +                case 3:
>> +                    *prot |= PAGE_WRITE;
>> +                    /* fall through */
>> +                case 2:
>> +                case 6:
>> +                    *prot |= PAGE_READ | PAGE_EXEC;
>> +                    break;
>> +                default:
>> +                    qemu_log_mask(LOG_GUEST_ERROR, "Bad value for AP bits in "
>> +                                  "DRACR");
>
> Putting the line break after the ',' would let you avoid
> splitting the string.
>

Fixed. This message should probably also include said bad value of ap
so I have added that.

>> +                }
>> +            } else { /* Priv. mode AP bits decoding */
>> +                switch (ap) {
>> +                case 0:
>> +                    break; /* no access */
>> +                case 1:
>> +                case 2:
>> +                case 3:
>> +                    *prot |= PAGE_WRITE;
>> +                    /* fall through */
>> +                case 5:
>> +                case 6:
>> +                    *prot |= PAGE_READ | PAGE_EXEC;
>> +                    break;
>> +                default:
>> +                    qemu_log_mask(LOG_GUEST_ERROR, "Bad value for AP bits in "
>> +                                  "DRACR");

Same.

>> +                }
>> +            }
>> +
>> +            /* execute never */
>> +            *prot &= env->cp15.c6_dracr[n] & 1 << 12 ? ~PAGE_EXEC : ~0;
>
> Break this down into something more readable, please.
>

With the FSR return refactoring this is now an if anyway.

>> +        }
>> +    }
>> +
>> +    switch (access_type) {
>> +    case 0:
>> +        return *prot & PAGE_READ ? 0 : 0x00D;
>> +    case 1:
>> +        return *prot & PAGE_WRITE ? 0 : 0x00D;
>> +    case 2:
>> +        return *prot & PAGE_EXEC ? 0 : 0x00D;
>
> This is
>    if (!(*prot & (1 << access_type))) {
>        return 0xD; /* Permission fault */
>    } else {
>        return 0;
>    }
>
> isn't it?
>

Yes but that assumes that access_type encoding is correlated to
PAGE_FOO masks so I didn't want this to break if one or the other was
re-encoded.

>> +    default:
>> +        abort(); /* should be unreachable */
>> +        return 0;
>
> The usual way to write this is g_assert_not_reached(), and the return
> isn't needed.
>

Fixed.

>> +    }
>> +
>> +}
>> +
>>  static int get_phys_addr_pmsav5(CPUARMState *env, uint32_t address,
>>                                  int access_type, ARMMMUIdx mmu_idx,
>>                                  hwaddr *phys_ptr, int *prot)
>> @@ -5795,13 +5919,14 @@ static int get_phys_addr_pmsav5(CPUARMState *env, uint32_t address,
>>   * MPU state on MPU based systems.
>>   *
>>   * Returns 0 if the translation was successful. Otherwise, phys_ptr, attrs,
>> - * prot and page_size may not be filled in, and the return value provides
>> + * prot and page_size may or may-not be filled in, and the return value provides
>
> No hyphen.
>

Fixed.

>>   * information on why the translation aborted, in the format of a
>>   * DFSR/IFSR fault register, with the following caveats:
>>   *  * we honour the short vs long DFSR format differences.
>>   *  * the WnR bit is never set (the caller must do this).
>>   *  * for MPU based systems we don't bother to return a full FSR format
>>   *    value.
>
> Is this bullet point still true?

Not quite. It should read "PMSAv5 based systems". Fixed

> If it is, is that a bug
> we now need to fix?
>

Don't think so. Should be ok.

>> + *  * -1 return value indicates a 0 FSR.
>>   *
>>   * @env: CPUARMState
>>   * @address: virtual address to get physical address for
>> @@ -5857,8 +5982,14 @@ static inline int get_phys_addr(CPUARMState *env, target_ulong address,
>>
>>      if (arm_feature(env, ARM_FEATURE_MPU)) {
>>          *page_size = TARGET_PAGE_SIZE;
>> -        return get_phys_addr_pmsav5(env, address, access_type, mmu_idx,
>> -                                    phys_ptr, prot);
>> +        if (arm_feature(env, ARM_FEATURE_V6)) {
>> +            assert(arm_feature(env, ARM_FEATURE_V7));
>> +            return get_phys_addr_pmsav7(env, address, access_type, mmu_idx,
>> +                                        phys_ptr, prot);
>
> v7M will take this code path now (which is the right thing, it is
> closer to v7R than to the 946); did you cross check against
> the M profile spec to see if any of this is R-profile specific?
>

Nope. Just add !ARM_FEATURE_M to avoid the beartrap?

> (We don't actually implement the M profile MPU right now, but
> it would be nice to avoid leaving beartraps for whoever does.)
>
>> +        } else {
>> +            return get_phys_addr_pmsav5(env, address, access_type, mmu_idx,
>> +                                        phys_ptr, prot);
>> +        }
>>      }
>>
>>      if (regime_using_lpae_format(env, mmu_idx)) {
>> @@ -5897,6 +6028,8 @@ int arm_tlb_fill(CPUState *cs, vaddr address,
>>          tlb_set_page_with_attrs(cs, address, phys_addr, attrs,
>>                                  prot, mmu_idx, page_size);
>>          return 0;
>> +    } else if (ret == -1) {
>> +        ret = 0;
>>      }
>>
>>      return ret;
>
> This isn't going to work, because tlb_fill() which calls this
> function making the same "0 means OK, non-0 means FSR value"
> assumption.
>
> I think the best thing to do here is to switch
> get_phys_addr() and friends to a bool return type for
> success/failure and pass in a uint32_t* for FSR value.
>

And done (new patch that'll go up front of v2).

Regards,
Peter

> thanks
> -- PMM
>

Re: [Qemu-devel] [PATCH target-arm v1 6/9] target-arm: Implement PMSAv7 MPU

[Peter Maydell]

On 10 June 2015 at 23:17, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> On Tue, Jun 2, 2015 at 4:59 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 1 June 2015 at 19:04, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
>>> +    switch (access_type) {
>>> +    case 0:
>>> +        return *prot & PAGE_READ ? 0 : 0x00D;
>>> +    case 1:
>>> +        return *prot & PAGE_WRITE ? 0 : 0x00D;
>>> +    case 2:
>>> +        return *prot & PAGE_EXEC ? 0 : 0x00D;
>>
>> This is
>>    if (!(*prot & (1 << access_type))) {
>>        return 0xD; /* Permission fault */
>>    } else {
>>        return 0;
>>    }
>>
>> isn't it?
>>
>
> Yes but that assumes that access_type encoding is correlated to
> PAGE_FOO masks so I didn't want this to break if one or the other was
> re-encoded.

We already do this in the lpae code path; I think it's safe.

>>> + *  * -1 return value indicates a 0 FSR.
>>>   *
>>>   * @env: CPUARMState
>>>   * @address: virtual address to get physical address for
>>> @@ -5857,8 +5982,14 @@ static inline int get_phys_addr(CPUARMState *env, target_ulong address,
>>>
>>>      if (arm_feature(env, ARM_FEATURE_MPU)) {
>>>          *page_size = TARGET_PAGE_SIZE;
>>> -        return get_phys_addr_pmsav5(env, address, access_type, mmu_idx,
>>> -                                    phys_ptr, prot);
>>> +        if (arm_feature(env, ARM_FEATURE_V6)) {
>>> +            assert(arm_feature(env, ARM_FEATURE_V7));
>>> +            return get_phys_addr_pmsav7(env, address, access_type, mmu_idx,
>>> +                                        phys_ptr, prot);
>>
>> v7M will take this code path now (which is the right thing, it is
>> closer to v7R than to the 946); did you cross check against
>> the M profile spec to see if any of this is R-profile specific?
>>
>
> Nope. Just add !ARM_FEATURE_M to avoid the beartrap?

Currently we don't set the MPU feature bit for M3; anybody
adding MPU support will need to check the code anyway, so
having the function not be called at all for M profile is
probably more confusing than helpful. I just wondered if
you'd looked at whether the two are really identical or not...

thanks
-- PMM

Re: [Qemu-devel] [PATCH target-arm v1 6/9] target-arm: Implement PMSAv7 MPU

[Peter Crosthwaite]

On Wed, Jun 10, 2015 at 3:21 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 10 June 2015 at 23:17, Peter Crosthwaite
> <peter.crosthwaite@xilinx.com> wrote:
>> On Tue, Jun 2, 2015 at 4:59 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> On 1 June 2015 at 19:04, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
>>>> +    switch (access_type) {
>>>> +    case 0:
>>>> +        return *prot & PAGE_READ ? 0 : 0x00D;
>>>> +    case 1:
>>>> +        return *prot & PAGE_WRITE ? 0 : 0x00D;
>>>> +    case 2:
>>>> +        return *prot & PAGE_EXEC ? 0 : 0x00D;
>>>
>>> This is
>>>    if (!(*prot & (1 << access_type))) {
>>>        return 0xD; /* Permission fault */
>>>    } else {
>>>        return 0;
>>>    }
>>>
>>> isn't it?
>>>
>>
>> Yes but that assumes that access_type encoding is correlated to
>> PAGE_FOO masks so I didn't want this to break if one or the other was
>> re-encoded.
>
> We already do this in the lpae code path; I think it's safe.
>

Ok, doing it the quick way.

Regards,
Peter

[Qemu-devel] [PATCH target-arm v1 7/9] arm: r5: Implement dummy ATCM, BTCM and D-cache invalidate

[Peter Crosthwaite]

These CPs are defined for R5 but don't have a lot of meaning in QEMU
yet. Raz them so the guest can proceed if they are read. The TCM
registers will return a size of 0, indicating no TCM.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---
 target-arm/cpu.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/target-arm/cpu.c b/target-arm/cpu.c
index 4872d9c..3538802 100644
--- a/target-arm/cpu.c
+++ b/target-arm/cpu.c
@@ -794,6 +794,17 @@ static void arm_v7m_class_init(ObjectClass *oc, void *data)
     cc->cpu_exec_interrupt = arm_v7m_cpu_exec_interrupt;
 }
 
+static const ARMCPRegInfo cortexr5_cp_reginfo[] = {
+    /* Dummy the TCM region regs for the moment */
+    { .name = "ATCM", .cp = 15, .crn = 9, .crm = 1, .opc1 = 0, .opc2 = 0,
+      .access = PL1_RW, .type = ARM_CP_CONST },
+    { .name = "BTCM", .cp = 15, .crn = 9, .crm = 1, .opc1 = 0, .opc2 = 1,
+      .access = PL1_RW, .type = ARM_CP_CONST },
+    { .name = "DCIALLU", .cp = 15, .crn = 15, .crm = 5, .opc1 = 0, .opc2 = 0,
+      .access = PL1_RW, .type = ARM_CP_NOP },
+    REGINFO_SENTINEL
+};
+
 static void cortex_r5_initfn(Object *obj)
 {
     ARMCPU *cpu = ARM_CPU(obj);
@@ -818,6 +829,7 @@ static void cortex_r5_initfn(Object *obj)
     cpu->id_isar3 = 0x01112131;
     cpu->id_isar4 = 0x0010142;
     cpu->id_isar5 = 0x0;
+    define_arm_cp_regs(cpu, cortexr5_cp_reginfo);
 }
 
 static const ARMCPRegInfo cortexa8_cp_reginfo[] = {
-- 
2.4.2.3.g2ffcb72

Re: [Qemu-devel] [PATCH target-arm v1 7/9] arm: r5: Implement dummy ATCM, BTCM and D-cache invalidate

[Peter Maydell]

On 1 June 2015 at 19:04, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> These CPs are defined for R5 but don't have a lot of meaning in QEMU
> yet. Raz them so the guest can proceed if they are read. The TCM
> registers will return a size of 0, indicating no TCM.
>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
>  target-arm/cpu.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> index 4872d9c..3538802 100644
> --- a/target-arm/cpu.c
> +++ b/target-arm/cpu.c
> @@ -794,6 +794,17 @@ static void arm_v7m_class_init(ObjectClass *oc, void *data)
>      cc->cpu_exec_interrupt = arm_v7m_cpu_exec_interrupt;
>  }
>
> +static const ARMCPRegInfo cortexr5_cp_reginfo[] = {
> +    /* Dummy the TCM region regs for the moment */
> +    { .name = "ATCM", .cp = 15, .crn = 9, .crm = 1, .opc1 = 0, .opc2 = 0,
> +      .access = PL1_RW, .type = ARM_CP_CONST },
> +    { .name = "BTCM", .cp = 15, .crn = 9, .crm = 1, .opc1 = 0, .opc2 = 1,
> +      .access = PL1_RW, .type = ARM_CP_CONST },

Preferred ordering is cp, opc1, crn, crm, opc2 (same as
MRC/MCR insns).

> +    { .name = "DCIALLU", .cp = 15, .crn = 15, .crm = 5, .opc1 = 0, .opc2 = 0,
> +      .access = PL1_RW, .type = ARM_CP_NOP },

I can't find this in the R5 TRM?

> +    REGINFO_SENTINEL
> +};
> +
>  static void cortex_r5_initfn(Object *obj)
>  {
>      ARMCPU *cpu = ARM_CPU(obj);
> @@ -818,6 +829,7 @@ static void cortex_r5_initfn(Object *obj)
>      cpu->id_isar3 = 0x01112131;
>      cpu->id_isar4 = 0x0010142;
>      cpu->id_isar5 = 0x0;
> +    define_arm_cp_regs(cpu, cortexr5_cp_reginfo);
>  }
>
>  static const ARMCPRegInfo cortexa8_cp_reginfo[] = {

thanks
-- PMM

[Qemu-devel] [PATCH target-arm v1 8/9] arm: xlnx-zynqmp: Preface CPU variables with "A"

[Peter Crosthwaite]

The CPUs currently supported by zynqmp are the APU (application
processing unit) CPUs. There are other CPUs in Zynqmp so unqualified
"cpus" in ambiguous. Preface the variables with "A" accordingly, to
prepare support adding the RPU (realtime processing unit) processors.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---
 hw/arm/xlnx-ep108.c          |  2 +-
 hw/arm/xlnx-zynqmp.c         | 24 ++++++++++++------------
 include/hw/arm/xlnx-zynqmp.h |  4 ++--
 3 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/hw/arm/xlnx-ep108.c b/hw/arm/xlnx-ep108.c
index b924f5e..1893b9f 100644
--- a/hw/arm/xlnx-ep108.c
+++ b/hw/arm/xlnx-ep108.c
@@ -65,7 +65,7 @@ static void xlnx_ep108_init(MachineState *machine)
     xlnx_ep108_binfo.kernel_cmdline = machine->kernel_cmdline;
     xlnx_ep108_binfo.initrd_filename = machine->initrd_filename;
     xlnx_ep108_binfo.loader_start = 0;
-    arm_load_kernel(&s->soc.cpu[0], &xlnx_ep108_binfo);
+    arm_load_kernel(&s->soc.acpu[0], &xlnx_ep108_binfo);
 }
 
 static QEMUMachine xlnx_ep108_machine = {
diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
index 6b01965..6faa578 100644
--- a/hw/arm/xlnx-zynqmp.c
+++ b/hw/arm/xlnx-zynqmp.c
@@ -64,10 +64,10 @@ static void xlnx_zynqmp_init(Object *obj)
     XlnxZynqMPState *s = XLNX_ZYNQMP(obj);
     int i;
 
-    for (i = 0; i < XLNX_ZYNQMP_NUM_CPUS; i++) {
-        object_initialize(&s->cpu[i], sizeof(s->cpu[i]),
+    for (i = 0; i < XLNX_ZYNQMP_NUM_ACPUS; i++) {
+        object_initialize(&s->acpu[i], sizeof(s->acpu[i]),
                           "cortex-a53-" TYPE_ARM_CPU);
-        object_property_add_child(obj, "cpu[*]", OBJECT(&s->cpu[i]),
+        object_property_add_child(obj, "acpu[*]", OBJECT(&s->acpu[i]),
                                   &error_abort);
     }
 
@@ -95,7 +95,7 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
 
     qdev_prop_set_uint32(DEVICE(&s->gic), "num-irq", GIC_NUM_SPI_INTR + 32);
     qdev_prop_set_uint32(DEVICE(&s->gic), "revision", 2);
-    qdev_prop_set_uint32(DEVICE(&s->gic), "num-cpu", XLNX_ZYNQMP_NUM_CPUS);
+    qdev_prop_set_uint32(DEVICE(&s->gic), "num-cpu", XLNX_ZYNQMP_NUM_ACPUS);
     object_property_set_bool(OBJECT(&s->gic), true, "realized", &err);
     if (err) {
         error_propagate((errp), (err));
@@ -121,38 +121,38 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
         }
     }
 
-    for (i = 0; i < XLNX_ZYNQMP_NUM_CPUS; i++) {
+    for (i = 0; i < XLNX_ZYNQMP_NUM_ACPUS; i++) {
         qemu_irq irq;
 
-        object_property_set_int(OBJECT(&s->cpu[i]), QEMU_PSCI_CONDUIT_SMC,
+        object_property_set_int(OBJECT(&s->acpu[i]), QEMU_PSCI_CONDUIT_SMC,
                                 "psci-conduit", &error_abort);
         if (i > 0) {
             /* Secondary CPUs start in PSCI powered-down state */
-            object_property_set_bool(OBJECT(&s->cpu[i]), true,
+            object_property_set_bool(OBJECT(&s->acpu[i]), true,
                                      "start-powered-off", &error_abort);
         }
 
-        object_property_set_int(OBJECT(&s->cpu[i]), GIC_BASE_ADDR,
+        object_property_set_int(OBJECT(&s->acpu[i]), GIC_BASE_ADDR,
                                 "reset-cbar", &err);
         if (err) {
             error_propagate((errp), (err));
             return;
         }
 
-        object_property_set_bool(OBJECT(&s->cpu[i]), true, "realized", &err);
+        object_property_set_bool(OBJECT(&s->acpu[i]), true, "realized", &err);
         if (err) {
             error_propagate((errp), (err));
             return;
         }
 
         sysbus_connect_irq(SYS_BUS_DEVICE(&s->gic), i,
-                           qdev_get_gpio_in(DEVICE(&s->cpu[i]), ARM_CPU_IRQ));
+                           qdev_get_gpio_in(DEVICE(&s->acpu[i]), ARM_CPU_IRQ));
         irq = qdev_get_gpio_in(DEVICE(&s->gic),
                                arm_gic_ppi_index(i, ARM_PHYS_TIMER_PPI));
-        qdev_connect_gpio_out(DEVICE(&s->cpu[i]), 0, irq);
+        qdev_connect_gpio_out(DEVICE(&s->acpu[i]), 0, irq);
         irq = qdev_get_gpio_in(DEVICE(&s->gic),
                                arm_gic_ppi_index(i, ARM_VIRT_TIMER_PPI));
-        qdev_connect_gpio_out(DEVICE(&s->cpu[i]), 1, irq);
+        qdev_connect_gpio_out(DEVICE(&s->acpu[i]), 1, irq);
     }
 
     for (i = 0; i < GIC_NUM_SPI_INTR; i++) {
diff --git a/include/hw/arm/xlnx-zynqmp.h b/include/hw/arm/xlnx-zynqmp.h
index 79c2b0b..bb67ef6 100644
--- a/include/hw/arm/xlnx-zynqmp.h
+++ b/include/hw/arm/xlnx-zynqmp.h
@@ -27,7 +27,7 @@
 #define XLNX_ZYNQMP(obj) OBJECT_CHECK(XlnxZynqMPState, (obj), \
                                        TYPE_XLNX_ZYNQMP)
 
-#define XLNX_ZYNQMP_NUM_CPUS 4
+#define XLNX_ZYNQMP_NUM_ACPUS 4
 #define XLNX_ZYNQMP_NUM_GEMS 4
 #define XLNX_ZYNQMP_NUM_UARTS 2
 
@@ -47,7 +47,7 @@ typedef struct XlnxZynqMPState {
     DeviceState parent_obj;
 
     /*< public >*/
-    ARMCPU cpu[XLNX_ZYNQMP_NUM_CPUS];
+    ARMCPU acpu[XLNX_ZYNQMP_NUM_ACPUS];
     GICState gic;
     MemoryRegion gic_mr[XLNX_ZYNQMP_GIC_REGIONS][XLNX_ZYNQMP_GIC_ALIASES];
     CadenceGEMState gem[XLNX_ZYNQMP_NUM_GEMS];
-- 
2.4.2.3.g2ffcb72

Re: [Qemu-devel] [PATCH target-arm v1 8/9] arm: xlnx-zynqmp: Preface CPU variables with "A"

[Peter Maydell]

On 1 June 2015 at 19:04, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> The CPUs currently supported by zynqmp are the APU (application
> processing unit) CPUs. There are other CPUs in Zynqmp so unqualified
> "cpus" in ambiguous. Preface the variables with "A" accordingly, to
> prepare support adding the RPU (realtime processing unit) processors.
>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

I'm assuming another Xilinx person will review the Zynq specific
changes.

thanks
-- PMM

Re: [Qemu-devel] [PATCH target-arm v1 8/9] arm: xlnx-zynqmp: Preface CPU variables with "A"

[Alistair Francis]

On Tue, Jun 2, 2015 at 4:04 AM, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> The CPUs currently supported by zynqmp are the APU (application
> processing unit) CPUs. There are other CPUs in Zynqmp so unqualified
> "cpus" in ambiguous. Preface the variables with "A" accordingly, to
> prepare support adding the RPU (realtime processing unit) processors.
>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
>  hw/arm/xlnx-ep108.c          |  2 +-
>  hw/arm/xlnx-zynqmp.c         | 24 ++++++++++++------------
>  include/hw/arm/xlnx-zynqmp.h |  4 ++--
>  3 files changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/hw/arm/xlnx-ep108.c b/hw/arm/xlnx-ep108.c
> index b924f5e..1893b9f 100644
> --- a/hw/arm/xlnx-ep108.c
> +++ b/hw/arm/xlnx-ep108.c
> @@ -65,7 +65,7 @@ static void xlnx_ep108_init(MachineState *machine)
>      xlnx_ep108_binfo.kernel_cmdline = machine->kernel_cmdline;
>      xlnx_ep108_binfo.initrd_filename = machine->initrd_filename;
>      xlnx_ep108_binfo.loader_start = 0;
> -    arm_load_kernel(&s->soc.cpu[0], &xlnx_ep108_binfo);
> +    arm_load_kernel(&s->soc.acpu[0], &xlnx_ep108_binfo);
>  }
>

Hey Peter,

Why is this acpu instead of apu? APU follows the standard ZynqMP naming
conventions, while Application Central Processing Unit (ACPU) doesn't really
make sense.

Thanks,

Alistair

>  static QEMUMachine xlnx_ep108_machine = {
> diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
> index 6b01965..6faa578 100644
> --- a/hw/arm/xlnx-zynqmp.c
> +++ b/hw/arm/xlnx-zynqmp.c
> @@ -64,10 +64,10 @@ static void xlnx_zynqmp_init(Object *obj)
>      XlnxZynqMPState *s = XLNX_ZYNQMP(obj);
>      int i;
>
> -    for (i = 0; i < XLNX_ZYNQMP_NUM_CPUS; i++) {
> -        object_initialize(&s->cpu[i], sizeof(s->cpu[i]),
> +    for (i = 0; i < XLNX_ZYNQMP_NUM_ACPUS; i++) {
> +        object_initialize(&s->acpu[i], sizeof(s->acpu[i]),
>                            "cortex-a53-" TYPE_ARM_CPU);
> -        object_property_add_child(obj, "cpu[*]", OBJECT(&s->cpu[i]),
> +        object_property_add_child(obj, "acpu[*]", OBJECT(&s->acpu[i]),
>                                    &error_abort);
>      }
>
> @@ -95,7 +95,7 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
>
>      qdev_prop_set_uint32(DEVICE(&s->gic), "num-irq", GIC_NUM_SPI_INTR + 32);
>      qdev_prop_set_uint32(DEVICE(&s->gic), "revision", 2);
> -    qdev_prop_set_uint32(DEVICE(&s->gic), "num-cpu", XLNX_ZYNQMP_NUM_CPUS);
> +    qdev_prop_set_uint32(DEVICE(&s->gic), "num-cpu", XLNX_ZYNQMP_NUM_ACPUS);
>      object_property_set_bool(OBJECT(&s->gic), true, "realized", &err);
>      if (err) {
>          error_propagate((errp), (err));
> @@ -121,38 +121,38 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
>          }
>      }
>
> -    for (i = 0; i < XLNX_ZYNQMP_NUM_CPUS; i++) {
> +    for (i = 0; i < XLNX_ZYNQMP_NUM_ACPUS; i++) {
>          qemu_irq irq;
>
> -        object_property_set_int(OBJECT(&s->cpu[i]), QEMU_PSCI_CONDUIT_SMC,
> +        object_property_set_int(OBJECT(&s->acpu[i]), QEMU_PSCI_CONDUIT_SMC,
>                                  "psci-conduit", &error_abort);
>          if (i > 0) {
>              /* Secondary CPUs start in PSCI powered-down state */
> -            object_property_set_bool(OBJECT(&s->cpu[i]), true,
> +            object_property_set_bool(OBJECT(&s->acpu[i]), true,
>                                       "start-powered-off", &error_abort);
>          }
>
> -        object_property_set_int(OBJECT(&s->cpu[i]), GIC_BASE_ADDR,
> +        object_property_set_int(OBJECT(&s->acpu[i]), GIC_BASE_ADDR,
>                                  "reset-cbar", &err);
>          if (err) {
>              error_propagate((errp), (err));
>              return;
>          }
>
> -        object_property_set_bool(OBJECT(&s->cpu[i]), true, "realized", &err);
> +        object_property_set_bool(OBJECT(&s->acpu[i]), true, "realized", &err);
>          if (err) {
>              error_propagate((errp), (err));
>              return;
>          }
>
>          sysbus_connect_irq(SYS_BUS_DEVICE(&s->gic), i,
> -                           qdev_get_gpio_in(DEVICE(&s->cpu[i]), ARM_CPU_IRQ));
> +                           qdev_get_gpio_in(DEVICE(&s->acpu[i]), ARM_CPU_IRQ));
>          irq = qdev_get_gpio_in(DEVICE(&s->gic),
>                                 arm_gic_ppi_index(i, ARM_PHYS_TIMER_PPI));
> -        qdev_connect_gpio_out(DEVICE(&s->cpu[i]), 0, irq);
> +        qdev_connect_gpio_out(DEVICE(&s->acpu[i]), 0, irq);
>          irq = qdev_get_gpio_in(DEVICE(&s->gic),
>                                 arm_gic_ppi_index(i, ARM_VIRT_TIMER_PPI));
> -        qdev_connect_gpio_out(DEVICE(&s->cpu[i]), 1, irq);
> +        qdev_connect_gpio_out(DEVICE(&s->acpu[i]), 1, irq);
>      }
>
>      for (i = 0; i < GIC_NUM_SPI_INTR; i++) {
> diff --git a/include/hw/arm/xlnx-zynqmp.h b/include/hw/arm/xlnx-zynqmp.h
> index 79c2b0b..bb67ef6 100644
> --- a/include/hw/arm/xlnx-zynqmp.h
> +++ b/include/hw/arm/xlnx-zynqmp.h
> @@ -27,7 +27,7 @@
>  #define XLNX_ZYNQMP(obj) OBJECT_CHECK(XlnxZynqMPState, (obj), \
>                                         TYPE_XLNX_ZYNQMP)
>
> -#define XLNX_ZYNQMP_NUM_CPUS 4
> +#define XLNX_ZYNQMP_NUM_ACPUS 4
>  #define XLNX_ZYNQMP_NUM_GEMS 4
>  #define XLNX_ZYNQMP_NUM_UARTS 2
>
> @@ -47,7 +47,7 @@ typedef struct XlnxZynqMPState {
>      DeviceState parent_obj;
>
>      /*< public >*/
> -    ARMCPU cpu[XLNX_ZYNQMP_NUM_CPUS];
> +    ARMCPU acpu[XLNX_ZYNQMP_NUM_ACPUS];
>      GICState gic;
>      MemoryRegion gic_mr[XLNX_ZYNQMP_GIC_REGIONS][XLNX_ZYNQMP_GIC_ALIASES];
>      CadenceGEMState gem[XLNX_ZYNQMP_NUM_GEMS];
> --
> 2.4.2.3.g2ffcb72
>
>

Re: [Qemu-devel] [PATCH target-arm v1 8/9] arm: xlnx-zynqmp: Preface CPU variables with "A"

[Peter Crosthwaite]

On Tue, Jun 2, 2015 at 4:57 PM, Alistair Francis
<alistair.francis@xilinx.com> wrote:
> On Tue, Jun 2, 2015 at 4:04 AM, Peter Crosthwaite
> <peter.crosthwaite@xilinx.com> wrote:
>> The CPUs currently supported by zynqmp are the APU (application
>> processing unit) CPUs. There are other CPUs in Zynqmp so unqualified
>> "cpus" in ambiguous. Preface the variables with "A" accordingly, to
>> prepare support adding the RPU (realtime processing unit) processors.
>>
>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>> ---
>>  hw/arm/xlnx-ep108.c          |  2 +-
>>  hw/arm/xlnx-zynqmp.c         | 24 ++++++++++++------------
>>  include/hw/arm/xlnx-zynqmp.h |  4 ++--
>>  3 files changed, 15 insertions(+), 15 deletions(-)
>>
>> diff --git a/hw/arm/xlnx-ep108.c b/hw/arm/xlnx-ep108.c
>> index b924f5e..1893b9f 100644
>> --- a/hw/arm/xlnx-ep108.c
>> +++ b/hw/arm/xlnx-ep108.c
>> @@ -65,7 +65,7 @@ static void xlnx_ep108_init(MachineState *machine)
>>      xlnx_ep108_binfo.kernel_cmdline = machine->kernel_cmdline;
>>      xlnx_ep108_binfo.initrd_filename = machine->initrd_filename;
>>      xlnx_ep108_binfo.loader_start = 0;
>> -    arm_load_kernel(&s->soc.cpu[0], &xlnx_ep108_binfo);
>> +    arm_load_kernel(&s->soc.acpu[0], &xlnx_ep108_binfo);
>>  }
>>
>
> Hey Peter,
>
> Why is this acpu instead of apu? APU follows the standard ZynqMP naming
> conventions, while Application Central Processing Unit (ACPU) doesn't really
> make sense.
>

So "apu" (or "rpu") doesn't work either, as each "processing unit" can
contain more than just CPUs. E.G. The GIC should actually have this
"apu" preface as well. I was trying to avoid text bloat with the short
form, but I think the correct answer is going to be:

"apu_cpu"

The PU in each does mean the same thing though :|

Regards,
Peter

> Thanks,
>
> Alistair
>
>>  static QEMUMachine xlnx_ep108_machine = {
>> diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
>> index 6b01965..6faa578 100644
>> --- a/hw/arm/xlnx-zynqmp.c
>> +++ b/hw/arm/xlnx-zynqmp.c
>> @@ -64,10 +64,10 @@ static void xlnx_zynqmp_init(Object *obj)
>>      XlnxZynqMPState *s = XLNX_ZYNQMP(obj);
>>      int i;
>>
>> -    for (i = 0; i < XLNX_ZYNQMP_NUM_CPUS; i++) {
>> -        object_initialize(&s->cpu[i], sizeof(s->cpu[i]),
>> +    for (i = 0; i < XLNX_ZYNQMP_NUM_ACPUS; i++) {
>> +        object_initialize(&s->acpu[i], sizeof(s->acpu[i]),
>>                            "cortex-a53-" TYPE_ARM_CPU);
>> -        object_property_add_child(obj, "cpu[*]", OBJECT(&s->cpu[i]),
>> +        object_property_add_child(obj, "acpu[*]", OBJECT(&s->acpu[i]),
>>                                    &error_abort);
>>      }
>>
>> @@ -95,7 +95,7 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
>>
>>      qdev_prop_set_uint32(DEVICE(&s->gic), "num-irq", GIC_NUM_SPI_INTR + 32);
>>      qdev_prop_set_uint32(DEVICE(&s->gic), "revision", 2);
>> -    qdev_prop_set_uint32(DEVICE(&s->gic), "num-cpu", XLNX_ZYNQMP_NUM_CPUS);
>> +    qdev_prop_set_uint32(DEVICE(&s->gic), "num-cpu", XLNX_ZYNQMP_NUM_ACPUS);
>>      object_property_set_bool(OBJECT(&s->gic), true, "realized", &err);
>>      if (err) {
>>          error_propagate((errp), (err));
>> @@ -121,38 +121,38 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
>>          }
>>      }
>>
>> -    for (i = 0; i < XLNX_ZYNQMP_NUM_CPUS; i++) {
>> +    for (i = 0; i < XLNX_ZYNQMP_NUM_ACPUS; i++) {
>>          qemu_irq irq;
>>
>> -        object_property_set_int(OBJECT(&s->cpu[i]), QEMU_PSCI_CONDUIT_SMC,
>> +        object_property_set_int(OBJECT(&s->acpu[i]), QEMU_PSCI_CONDUIT_SMC,
>>                                  "psci-conduit", &error_abort);
>>          if (i > 0) {
>>              /* Secondary CPUs start in PSCI powered-down state */
>> -            object_property_set_bool(OBJECT(&s->cpu[i]), true,
>> +            object_property_set_bool(OBJECT(&s->acpu[i]), true,
>>                                       "start-powered-off", &error_abort);
>>          }
>>
>> -        object_property_set_int(OBJECT(&s->cpu[i]), GIC_BASE_ADDR,
>> +        object_property_set_int(OBJECT(&s->acpu[i]), GIC_BASE_ADDR,
>>                                  "reset-cbar", &err);
>>          if (err) {
>>              error_propagate((errp), (err));
>>              return;
>>          }
>>
>> -        object_property_set_bool(OBJECT(&s->cpu[i]), true, "realized", &err);
>> +        object_property_set_bool(OBJECT(&s->acpu[i]), true, "realized", &err);
>>          if (err) {
>>              error_propagate((errp), (err));
>>              return;
>>          }
>>
>>          sysbus_connect_irq(SYS_BUS_DEVICE(&s->gic), i,
>> -                           qdev_get_gpio_in(DEVICE(&s->cpu[i]), ARM_CPU_IRQ));
>> +                           qdev_get_gpio_in(DEVICE(&s->acpu[i]), ARM_CPU_IRQ));
>>          irq = qdev_get_gpio_in(DEVICE(&s->gic),
>>                                 arm_gic_ppi_index(i, ARM_PHYS_TIMER_PPI));
>> -        qdev_connect_gpio_out(DEVICE(&s->cpu[i]), 0, irq);
>> +        qdev_connect_gpio_out(DEVICE(&s->acpu[i]), 0, irq);
>>          irq = qdev_get_gpio_in(DEVICE(&s->gic),
>>                                 arm_gic_ppi_index(i, ARM_VIRT_TIMER_PPI));
>> -        qdev_connect_gpio_out(DEVICE(&s->cpu[i]), 1, irq);
>> +        qdev_connect_gpio_out(DEVICE(&s->acpu[i]), 1, irq);
>>      }
>>
>>      for (i = 0; i < GIC_NUM_SPI_INTR; i++) {
>> diff --git a/include/hw/arm/xlnx-zynqmp.h b/include/hw/arm/xlnx-zynqmp.h
>> index 79c2b0b..bb67ef6 100644
>> --- a/include/hw/arm/xlnx-zynqmp.h
>> +++ b/include/hw/arm/xlnx-zynqmp.h
>> @@ -27,7 +27,7 @@
>>  #define XLNX_ZYNQMP(obj) OBJECT_CHECK(XlnxZynqMPState, (obj), \
>>                                         TYPE_XLNX_ZYNQMP)
>>
>> -#define XLNX_ZYNQMP_NUM_CPUS 4
>> +#define XLNX_ZYNQMP_NUM_ACPUS 4
>>  #define XLNX_ZYNQMP_NUM_GEMS 4
>>  #define XLNX_ZYNQMP_NUM_UARTS 2
>>
>> @@ -47,7 +47,7 @@ typedef struct XlnxZynqMPState {
>>      DeviceState parent_obj;
>>
>>      /*< public >*/
>> -    ARMCPU cpu[XLNX_ZYNQMP_NUM_CPUS];
>> +    ARMCPU acpu[XLNX_ZYNQMP_NUM_ACPUS];
>>      GICState gic;
>>      MemoryRegion gic_mr[XLNX_ZYNQMP_GIC_REGIONS][XLNX_ZYNQMP_GIC_ALIASES];
>>      CadenceGEMState gem[XLNX_ZYNQMP_NUM_GEMS];
>> --
>> 2.4.2.3.g2ffcb72
>>
>>
>

Re: [Qemu-devel] [PATCH target-arm v1 8/9] arm: xlnx-zynqmp: Preface CPU variables with "A"

[Alistair Francis]

On Thu, Jun 11, 2015 at 9:58 AM, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> On Tue, Jun 2, 2015 at 4:57 PM, Alistair Francis
> <alistair.francis@xilinx.com> wrote:
>> On Tue, Jun 2, 2015 at 4:04 AM, Peter Crosthwaite
>> <peter.crosthwaite@xilinx.com> wrote:
>>> The CPUs currently supported by zynqmp are the APU (application
>>> processing unit) CPUs. There are other CPUs in Zynqmp so unqualified
>>> "cpus" in ambiguous. Preface the variables with "A" accordingly, to
>>> prepare support adding the RPU (realtime processing unit) processors.
>>>
>>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>>> ---
>>>  hw/arm/xlnx-ep108.c          |  2 +-
>>>  hw/arm/xlnx-zynqmp.c         | 24 ++++++++++++------------
>>>  include/hw/arm/xlnx-zynqmp.h |  4 ++--
>>>  3 files changed, 15 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/hw/arm/xlnx-ep108.c b/hw/arm/xlnx-ep108.c
>>> index b924f5e..1893b9f 100644
>>> --- a/hw/arm/xlnx-ep108.c
>>> +++ b/hw/arm/xlnx-ep108.c
>>> @@ -65,7 +65,7 @@ static void xlnx_ep108_init(MachineState *machine)
>>>      xlnx_ep108_binfo.kernel_cmdline = machine->kernel_cmdline;
>>>      xlnx_ep108_binfo.initrd_filename = machine->initrd_filename;
>>>      xlnx_ep108_binfo.loader_start = 0;
>>> -    arm_load_kernel(&s->soc.cpu[0], &xlnx_ep108_binfo);
>>> +    arm_load_kernel(&s->soc.acpu[0], &xlnx_ep108_binfo);
>>>  }
>>>
>>
>> Hey Peter,
>>
>> Why is this acpu instead of apu? APU follows the standard ZynqMP naming
>> conventions, while Application Central Processing Unit (ACPU) doesn't really
>> make sense.
>>
>
> So "apu" (or "rpu") doesn't work either, as each "processing unit" can
> contain more than just CPUs. E.G. The GIC should actually have this
> "apu" preface as well. I was trying to avoid text bloat with the short
> form, but I think the correct answer is going to be:
>
> "apu_cpu"
>
> The PU in each does mean the same thing though :|

Ok, I see what you are saying. I agree with you, I think the best option
is the long names 'apu_*' and 'rpu_*'.

Thanks,

Alistair

>
> Regards,
> Peter
>
>> Thanks,
>>
>> Alistair
>>
>>>  static QEMUMachine xlnx_ep108_machine = {
>>> diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
>>> index 6b01965..6faa578 100644
>>> --- a/hw/arm/xlnx-zynqmp.c
>>> +++ b/hw/arm/xlnx-zynqmp.c
>>> @@ -64,10 +64,10 @@ static void xlnx_zynqmp_init(Object *obj)
>>>      XlnxZynqMPState *s = XLNX_ZYNQMP(obj);
>>>      int i;
>>>
>>> -    for (i = 0; i < XLNX_ZYNQMP_NUM_CPUS; i++) {
>>> -        object_initialize(&s->cpu[i], sizeof(s->cpu[i]),
>>> +    for (i = 0; i < XLNX_ZYNQMP_NUM_ACPUS; i++) {
>>> +        object_initialize(&s->acpu[i], sizeof(s->acpu[i]),
>>>                            "cortex-a53-" TYPE_ARM_CPU);
>>> -        object_property_add_child(obj, "cpu[*]", OBJECT(&s->cpu[i]),
>>> +        object_property_add_child(obj, "acpu[*]", OBJECT(&s->acpu[i]),
>>>                                    &error_abort);
>>>      }
>>>
>>> @@ -95,7 +95,7 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
>>>
>>>      qdev_prop_set_uint32(DEVICE(&s->gic), "num-irq", GIC_NUM_SPI_INTR + 32);
>>>      qdev_prop_set_uint32(DEVICE(&s->gic), "revision", 2);
>>> -    qdev_prop_set_uint32(DEVICE(&s->gic), "num-cpu", XLNX_ZYNQMP_NUM_CPUS);
>>> +    qdev_prop_set_uint32(DEVICE(&s->gic), "num-cpu", XLNX_ZYNQMP_NUM_ACPUS);
>>>      object_property_set_bool(OBJECT(&s->gic), true, "realized", &err);
>>>      if (err) {
>>>          error_propagate((errp), (err));
>>> @@ -121,38 +121,38 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
>>>          }
>>>      }
>>>
>>> -    for (i = 0; i < XLNX_ZYNQMP_NUM_CPUS; i++) {
>>> +    for (i = 0; i < XLNX_ZYNQMP_NUM_ACPUS; i++) {
>>>          qemu_irq irq;
>>>
>>> -        object_property_set_int(OBJECT(&s->cpu[i]), QEMU_PSCI_CONDUIT_SMC,
>>> +        object_property_set_int(OBJECT(&s->acpu[i]), QEMU_PSCI_CONDUIT_SMC,
>>>                                  "psci-conduit", &error_abort);
>>>          if (i > 0) {
>>>              /* Secondary CPUs start in PSCI powered-down state */
>>> -            object_property_set_bool(OBJECT(&s->cpu[i]), true,
>>> +            object_property_set_bool(OBJECT(&s->acpu[i]), true,
>>>                                       "start-powered-off", &error_abort);
>>>          }
>>>
>>> -        object_property_set_int(OBJECT(&s->cpu[i]), GIC_BASE_ADDR,
>>> +        object_property_set_int(OBJECT(&s->acpu[i]), GIC_BASE_ADDR,
>>>                                  "reset-cbar", &err);
>>>          if (err) {
>>>              error_propagate((errp), (err));
>>>              return;
>>>          }
>>>
>>> -        object_property_set_bool(OBJECT(&s->cpu[i]), true, "realized", &err);
>>> +        object_property_set_bool(OBJECT(&s->acpu[i]), true, "realized", &err);
>>>          if (err) {
>>>              error_propagate((errp), (err));
>>>              return;
>>>          }
>>>
>>>          sysbus_connect_irq(SYS_BUS_DEVICE(&s->gic), i,
>>> -                           qdev_get_gpio_in(DEVICE(&s->cpu[i]), ARM_CPU_IRQ));
>>> +                           qdev_get_gpio_in(DEVICE(&s->acpu[i]), ARM_CPU_IRQ));
>>>          irq = qdev_get_gpio_in(DEVICE(&s->gic),
>>>                                 arm_gic_ppi_index(i, ARM_PHYS_TIMER_PPI));
>>> -        qdev_connect_gpio_out(DEVICE(&s->cpu[i]), 0, irq);
>>> +        qdev_connect_gpio_out(DEVICE(&s->acpu[i]), 0, irq);
>>>          irq = qdev_get_gpio_in(DEVICE(&s->gic),
>>>                                 arm_gic_ppi_index(i, ARM_VIRT_TIMER_PPI));
>>> -        qdev_connect_gpio_out(DEVICE(&s->cpu[i]), 1, irq);
>>> +        qdev_connect_gpio_out(DEVICE(&s->acpu[i]), 1, irq);
>>>      }
>>>
>>>      for (i = 0; i < GIC_NUM_SPI_INTR; i++) {
>>> diff --git a/include/hw/arm/xlnx-zynqmp.h b/include/hw/arm/xlnx-zynqmp.h
>>> index 79c2b0b..bb67ef6 100644
>>> --- a/include/hw/arm/xlnx-zynqmp.h
>>> +++ b/include/hw/arm/xlnx-zynqmp.h
>>> @@ -27,7 +27,7 @@
>>>  #define XLNX_ZYNQMP(obj) OBJECT_CHECK(XlnxZynqMPState, (obj), \
>>>                                         TYPE_XLNX_ZYNQMP)
>>>
>>> -#define XLNX_ZYNQMP_NUM_CPUS 4
>>> +#define XLNX_ZYNQMP_NUM_ACPUS 4
>>>  #define XLNX_ZYNQMP_NUM_GEMS 4
>>>  #define XLNX_ZYNQMP_NUM_UARTS 2
>>>
>>> @@ -47,7 +47,7 @@ typedef struct XlnxZynqMPState {
>>>      DeviceState parent_obj;
>>>
>>>      /*< public >*/
>>> -    ARMCPU cpu[XLNX_ZYNQMP_NUM_CPUS];
>>> +    ARMCPU acpu[XLNX_ZYNQMP_NUM_ACPUS];
>>>      GICState gic;
>>>      MemoryRegion gic_mr[XLNX_ZYNQMP_GIC_REGIONS][XLNX_ZYNQMP_GIC_ALIASES];
>>>      CadenceGEMState gem[XLNX_ZYNQMP_NUM_GEMS];
>>> --
>>> 2.4.2.3.g2ffcb72
>>>
>>>
>>
>

[Qemu-devel] [PATCH target-arm v1 9/9] arm: xlnx-zynqmp: Add 2xCortexR5 CPUs

[Peter Crosthwaite]

Add the 2xCortexR5 CPUs to zynqmp board. They are powered off on reset
(this is true of real hardware).

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---
 hw/arm/xlnx-zynqmp.c         | 26 ++++++++++++++++++++++++++
 include/hw/arm/xlnx-zynqmp.h |  2 ++
 2 files changed, 28 insertions(+)

diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
index 6faa578..bf46f7c 100644
--- a/hw/arm/xlnx-zynqmp.c
+++ b/hw/arm/xlnx-zynqmp.c
@@ -71,6 +71,13 @@ static void xlnx_zynqmp_init(Object *obj)
                                   &error_abort);
     }
 
+    for (i = 0; i < XLNX_ZYNQMP_NUM_RCPUS; i++) {
+        object_initialize(&s->rcpu[i], sizeof(s->rcpu[i]),
+                          "cortex-r5-" TYPE_ARM_CPU);
+        object_property_add_child(obj, "rcpu[*]", OBJECT(&s->rcpu[i]),
+                                  &error_abort);
+    }
+
     object_initialize(&s->gic, sizeof(s->gic), TYPE_ARM_GIC);
     qdev_set_parent_bus(DEVICE(&s->gic), sysbus_get_default());
 
@@ -155,6 +162,25 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
         qdev_connect_gpio_out(DEVICE(&s->acpu[i]), 1, irq);
     }
 
+    for (i = 0; i < XLNX_ZYNQMP_NUM_RCPUS; i++) {
+        /* RCPUs and held in reset on startup, by the reset controller */
+        object_property_set_bool(OBJECT(&s->rcpu[i]), true,
+                                 "start-powered-off", &error_abort);
+
+        object_property_set_bool(OBJECT(&s->rcpu[i]), true, "reset-hivecs",
+                                 &err);
+        if (err != NULL) {
+            error_propagate(errp, err);
+            return;
+        }
+
+        object_property_set_bool(OBJECT(&s->rcpu[i]), true, "realized", &err);
+        if (err) {
+            error_propagate((errp), (err));
+            return;
+        }
+    }
+
     for (i = 0; i < GIC_NUM_SPI_INTR; i++) {
         gic_spi[i] = qdev_get_gpio_in(DEVICE(&s->gic), i);
     }
diff --git a/include/hw/arm/xlnx-zynqmp.h b/include/hw/arm/xlnx-zynqmp.h
index bb67ef6..1272be3 100644
--- a/include/hw/arm/xlnx-zynqmp.h
+++ b/include/hw/arm/xlnx-zynqmp.h
@@ -28,6 +28,7 @@
                                        TYPE_XLNX_ZYNQMP)
 
 #define XLNX_ZYNQMP_NUM_ACPUS 4
+#define XLNX_ZYNQMP_NUM_RCPUS 2
 #define XLNX_ZYNQMP_NUM_GEMS 4
 #define XLNX_ZYNQMP_NUM_UARTS 2
 
@@ -48,6 +49,7 @@ typedef struct XlnxZynqMPState {
 
     /*< public >*/
     ARMCPU acpu[XLNX_ZYNQMP_NUM_ACPUS];
+    ARMCPU rcpu[XLNX_ZYNQMP_NUM_RCPUS];
     GICState gic;
     MemoryRegion gic_mr[XLNX_ZYNQMP_GIC_REGIONS][XLNX_ZYNQMP_GIC_ALIASES];
     CadenceGEMState gem[XLNX_ZYNQMP_NUM_GEMS];
-- 
2.4.2.3.g2ffcb72

Re: [Qemu-devel] [PATCH target-arm v1 9/9] arm: xlnx-zynqmp: Add 2xCortexR5 CPUs

[Alistair Francis]

On Tue, Jun 2, 2015 at 4:04 AM, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> Add the 2xCortexR5 CPUs to zynqmp board. They are powered off on reset
> (this is true of real hardware).
>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
>  hw/arm/xlnx-zynqmp.c         | 26 ++++++++++++++++++++++++++
>  include/hw/arm/xlnx-zynqmp.h |  2 ++
>  2 files changed, 28 insertions(+)
>
> diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
> index 6faa578..bf46f7c 100644
> --- a/hw/arm/xlnx-zynqmp.c
> +++ b/hw/arm/xlnx-zynqmp.c
> @@ -71,6 +71,13 @@ static void xlnx_zynqmp_init(Object *obj)
>                                    &error_abort);
>      }
>
> +    for (i = 0; i < XLNX_ZYNQMP_NUM_RCPUS; i++) {
> +        object_initialize(&s->rcpu[i], sizeof(s->rcpu[i]),
> +                          "cortex-r5-" TYPE_ARM_CPU);
> +        object_property_add_child(obj, "rcpu[*]", OBJECT(&s->rcpu[i]),
> +                                  &error_abort);
> +    }
> +

Hey Peter,

Same comment about rcpu, I think it should just be rpu.

>      object_initialize(&s->gic, sizeof(s->gic), TYPE_ARM_GIC);
>      qdev_set_parent_bus(DEVICE(&s->gic), sysbus_get_default());
>
> @@ -155,6 +162,25 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
>          qdev_connect_gpio_out(DEVICE(&s->acpu[i]), 1, irq);
>      }
>
> +    for (i = 0; i < XLNX_ZYNQMP_NUM_RCPUS; i++) {
> +        /* RCPUs and held in reset on startup, by the reset controller */
> +        object_property_set_bool(OBJECT(&s->rcpu[i]), true,
> +                                 "start-powered-off", &error_abort);
> +
> +        object_property_set_bool(OBJECT(&s->rcpu[i]), true, "reset-hivecs",
> +                                 &err);
> +        if (err != NULL) {

You don't need the '!= NULL'.

> +            error_propagate(errp, err);
> +            return;
> +        }
> +
> +        object_property_set_bool(OBJECT(&s->rcpu[i]), true, "realized", &err);
> +        if (err) {
> +            error_propagate((errp), (err));

You don't need the brackets around the errp and err.

Although looking above they all have brackets around them, so I guess
you are conforming with the reset of the style.

Thanks,

Alistair

> +            return;
> +        }
> +    }
> +
>      for (i = 0; i < GIC_NUM_SPI_INTR; i++) {
>          gic_spi[i] = qdev_get_gpio_in(DEVICE(&s->gic), i);
>      }
> diff --git a/include/hw/arm/xlnx-zynqmp.h b/include/hw/arm/xlnx-zynqmp.h
> index bb67ef6..1272be3 100644
> --- a/include/hw/arm/xlnx-zynqmp.h
> +++ b/include/hw/arm/xlnx-zynqmp.h
> @@ -28,6 +28,7 @@
>                                         TYPE_XLNX_ZYNQMP)
>
>  #define XLNX_ZYNQMP_NUM_ACPUS 4
> +#define XLNX_ZYNQMP_NUM_RCPUS 2
>  #define XLNX_ZYNQMP_NUM_GEMS 4
>  #define XLNX_ZYNQMP_NUM_UARTS 2
>
> @@ -48,6 +49,7 @@ typedef struct XlnxZynqMPState {
>
>      /*< public >*/
>      ARMCPU acpu[XLNX_ZYNQMP_NUM_ACPUS];
> +    ARMCPU rcpu[XLNX_ZYNQMP_NUM_RCPUS];
>      GICState gic;
>      MemoryRegion gic_mr[XLNX_ZYNQMP_GIC_REGIONS][XLNX_ZYNQMP_GIC_ALIASES];
>      CadenceGEMState gem[XLNX_ZYNQMP_NUM_GEMS];
> --
> 2.4.2.3.g2ffcb72
>
>