ARM Cortex R5 Support
cover ARM Cortex R5 Support
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(-)
Signature · 1 line
2.4.2.3.g2ffcb72
PATCH 1/9 target-arm: Prepare support for Cortex-R5
Introduce a CPU model for the Cortex R5 processor. ARMv7 with MPU, and both thumb and ARM div instructions.
--- target-arm/cpu.c | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+)
target-arm/cpu.c +27 -0
@@ -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 },
Signature · 1 line
2.4.2.3.g2ffcb72
Re: [Qemu-devel] [PATCH target-arm v1 1/9] target-arm: Prepare support for Cortex-R5
Quoted 4 lines · 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 <[email protected]>
This patch needs to go last, not first -- implement all the support the R5 needs, and only then let somebody create one.
-- PMM
Quoted 9 lines
--- target-arm/cpu.c | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+)target-arm/cpu.c +0
-0
target-arm/cpu.c static void arm_v7m_class_init(ObjectClass *oc, void *data) · line 802
Quoted 9 lines
target-arm/cpu.c +6
-0@@ -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.
target-arm/cpu.c static void arm_v7m_class_init(ObjectClass *oc, void *data) · line 804
Quoted 2 lines
target-arm/cpu.c +2
-0+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...
Quoted 30 lines
target-arm/cpu.c +19
-1+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 }, --
Quoted 2 lines
2.4.2.3.g2ffcb72
thanks
-- PMM
Re: [Qemu-devel] [PATCH target-arm v1 1/9] target-arm: Prepare support for Cortex-R5
Quoted 10 lines · Peter Maydell
Quoted 4 lines · 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 <[email protected]>This patch needs to go last, not first -- implement all the support the R5 needs, and only then let somebody create one.
OK
Quoted 14 lines
-- PMM
Quoted 12 lines
--- target-arm/cpu.c | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+)target-arm/cpu.c +0
-0@@ -794,6 +794,32 @@ static void arm_v7m_class_init(ObjectClass *oc, void *data) cc->cpu_exec_interrupt = arm_v7m_cpu_exec_interrupt; }
Quoted 11 lines
target-arm/cpu.c static void arm_v7m_class_init(ObjectClass *oc, void *data) · line 802
Quoted 6 lines
target-arm/cpu.c +6
-0+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_DIVis 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
Quoted 52 lines
target-arm/cpu.c static void arm_v7m_class_init(ObjectClass *oc, void *data) · line 804
Quoted 2 lines
target-arm/cpu.c +2
-0+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_reginfoassumes 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...
Quoted 30 lines
target-arm/cpu.c +19
-1+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 }, --Quoted 2 lines
2.4.2.3.g2ffcb72
thanks
-- PMM
Re: [Qemu-devel] [PATCH target-arm v1 1/9] target-arm: Prepare support for Cortex-R5
Quoted 12 lines · Peter Crosthwaite
Quoted 10 lines · Peter Maydell
target-arm/cpu.c static void arm_v7m_class_init(ObjectClass *oc, void *data) · line 802
Quoted 6 lines
target-arm/cpu.c +6
-0+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_DIVis mandatory for v7R and so we could infer it in cpu realize.
Quoted 3 lines
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
In reply to Peter MaydellQuoted 35 lines · Peter Maydell
Quoted 4 lines · 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 <[email protected]>This patch needs to go last, not first -- implement all the support the R5 needs, and only then let somebody create one.
-- PMM
Quoted 9 lines
--- target-arm/cpu.c | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+)target-arm/cpu.c +0
-0target-arm/cpu.c static void arm_v7m_class_init(ObjectClass *oc, void *data) · line 802
Quoted 9 lines
target-arm/cpu.c +6
-0@@ -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_DIVis mandatory for v7R and so we could infer it in cpu realize.
Quoted 6 lines
target-arm/cpu.c static void arm_v7m_class_init(ObjectClass *oc, void *data) · line 804
Quoted 2 lines
target-arm/cpu.c +2
-0+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_reginfoassumes VMSA).
Knocked those out with a !ARM_FEATURE_MPU
Quoted 8 lines
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
Quoted 38 lines
The R5 (from r1p0 and up) is such a core...
Quoted 30 lines
target-arm/cpu.c +19
-1+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 }, --Quoted 2 lines
2.4.2.3.g2ffcb72
thanks
-- PMM
PATCH 2/9 arm: helper: Factor out CP regs common to [pv]msa
In reply to Peter CrosthwaiteV6+ 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.
--- target-arm/helper.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)
target-arm/helper.c +10 -5
@@ -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 )) {
Signature · 1 line
2.4.2.3.g2ffcb72
Re: [Qemu-devel] [PATCH target-arm v1 2/9] arm: helper: Factor out CP regs common to [pv]msa
Quoted 23 lines · 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 <[email protected]>--- target-arm/helper.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)target-arm/helper.c +1
-1@@ -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),
target-arm/helper.c static const ARMCPRegInfo vmsa_cp_reginfo[] = { · line 1862
Quoted 7 lines
target-arm/helper.c +4
-0@@ -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
Quoted 27 lines · Peter Maydell
Quoted 26 lines · 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 <[email protected]>--- target-arm/helper.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)target-arm/helper.c +1
-1@@ -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) } },
Quoted 9 lines
target-arm/helper.c static const ARMCPRegInfo vmsa_cp_reginfo[] = { · line 1862
Quoted 4 lines
target-arm/helper.c +4
-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) } },Can you move the
FAR_EL1reginfo as well, please? They should stay together because they're the 32 and 64 bit versions of the same register.
Done.
Quoted 2 lines
(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
Quoted 4 lines
Otherwise looks OK.
-- PMM
Re: [Qemu-devel] [PATCH target-arm v1 2/9] arm: helper: Factor out CP regs common to [pv]msa
Quoted 21 lines · Peter Crosthwaite
Quoted 10 lines · Peter Maydell
Quoted 10 lines
target-arm/helper.c +1
-1-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) } },Quoted 9 lines
target-arm/helper.c static const ARMCPRegInfo vmsa_cp_reginfo[] = { · line 1862
Quoted 4 lines
target-arm/helper.c +4
-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) } },Can you move the
FAR_EL1reginfo as well, please? They should stay together because they're the 32 and 64 bit versions of the same register.
Quoted 7 lines
Done.
Quoted 2 lines
(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
PATCH 3/9 target-arm/helper.c: define MPUIR register
In reply to Peter CrosthwaiteJust hardcoded to 16way unified MPU.
--- target-arm/cpu.h | 2 ++ target-arm/helper.c | 4 ++++ 2 files changed, 6 insertions(+)
target-arm/cpu.h +2 -0
@@ -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;
target-arm/helper.c +4 -0
@@ -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 = {
Signature · 1 line
2.4.2.3.g2ffcb72
Re: [Qemu-devel] [PATCH target-arm v1 3/9] target-arm/helper.c: define MPUIR register
Quoted 26 lines · Peter Crosthwaite
Just hardcoded to 16way unified MPU.
Signed-off-by Peter Crosthwaite <[email protected]>--- target-arm/cpu.h | 2 ++ target-arm/helper.c | 4 ++++ 2 files changed, 6 insertions(+)target-arm/cpu.h +2
-0@@ -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;target-arm/helper.c +0
-0
target-arm/helper.c void register_cp_regs_for_features(ARMCPU *cpu) · line 3396
Quoted 10 lines
target-arm/helper.c +4
-0@@ -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
Quoted 31 lines · Peter Maydell
Quoted 30 lines · Peter Crosthwaite
Just hardcoded to 16way unified MPU.
Signed-off-by Peter Crosthwaite <[email protected]>--- target-arm/cpu.h | 2 ++ target-arm/helper.c | 4 ++++ 2 files changed, 6 insertions(+)target-arm/cpu.h +2
-0@@ -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;target-arm/helper.c +1
-0@@ -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",
Quoted 9 lines
target-arm/helper.c void register_cp_regs_for_features(ARMCPU *cpu) · line 3396
Quoted 6 lines
target-arm/helper.c +3
-0+ .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
Quoted 2 lines
-- PMM
Re: [Qemu-devel] [PATCH target-arm v1 3/9] target-arm/helper.c: define MPUIR register
Quoted 42 lines · Peter Crosthwaite
Quoted 31 lines · Peter Maydell
Quoted 30 lines · Peter Crosthwaite
Just hardcoded to 16way unified MPU.
Signed-off-by Peter Crosthwaite <[email protected]>--- target-arm/cpu.h | 2 ++ target-arm/helper.c | 4 ++++ 2 files changed, 6 insertions(+)target-arm/cpu.h +2
-0@@ -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;target-arm/helper.c +1
-0@@ -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",Quoted 9 lines
target-arm/helper.c void register_cp_regs_for_features(ARMCPU *cpu) · line 3396
Quoted 6 lines
target-arm/helper.c +3
-0+ .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?
Quoted 2 lines
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
Quoted 50 lines · Peter Maydell
Quoted 42 lines · Peter Crosthwaite
Quoted 31 lines · Peter Maydell
Quoted 30 lines · Peter Crosthwaite
Just hardcoded to 16way unified MPU.
Signed-off-by Peter Crosthwaite <[email protected]>--- target-arm/cpu.h | 2 ++ target-arm/helper.c | 4 ++++ 2 files changed, 6 insertions(+)target-arm/cpu.h +2
-0@@ -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;target-arm/helper.c +1
-0@@ -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",Quoted 9 lines
target-arm/helper.c void register_cp_regs_for_features(ARMCPU *cpu) · line 3396
Quoted 6 lines
target-arm/helper.c +3
-0+ .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?
Quoted 2 lines
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.
Quoted 6 lines
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
Quoted 2 lines
-- PMM
Re: [Qemu-devel] [PATCH target-arm v1 3/9] target-arm/helper.c: define MPUIR register
In reply to Peter MaydellQuoted 50 lines · Peter Maydell
Quoted 42 lines · Peter Crosthwaite
Quoted 31 lines · Peter Maydell
Quoted 30 lines · Peter Crosthwaite
Just hardcoded to 16way unified MPU.
Signed-off-by Peter Crosthwaite <[email protected]>--- target-arm/cpu.h | 2 ++ target-arm/helper.c | 4 ++++ 2 files changed, 6 insertions(+)target-arm/cpu.h +2
-0@@ -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;target-arm/helper.c +1
-0@@ -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",Quoted 9 lines
target-arm/helper.c void register_cp_regs_for_features(ARMCPU *cpu) · line 3396
Quoted 6 lines
target-arm/helper.c +3
-0+ .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?
Quoted 2 lines
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.
Quoted 6 lines
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
Quoted 2 lines
-- PMM
PATCH 4/9 target-arm: Add registers for PMSAv7
In reply to Peter Crosthwaitedefine the arm CP registers for PMSAv7 and their accessor functions.
--- target-arm/cpu.h | 6 ++++++ target-arm/helper.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+)
target-arm/cpu.h +6 -0
@@ -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 */
target-arm/helper.c +48 -0
54 lines not on this page · raw message
Signature · 1 line
2.4.2.3.g2ffcb72
Re: [Qemu-devel] [PATCH target-arm v1 4/9] target-arm: Add registers for PMSAv7
Quoted 30 lines · Peter Crosthwaite
define the arm CP registers for PMSAv7 and their accessor functions.
Signed-off-by Peter Crosthwaite <[email protected]>--- target-arm/cpu.h | 6 ++++++ target-arm/helper.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+)target-arm/cpu.h +6
-0@@ -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 */ target-arm/helper.c +0
-0
target-arm/helper.c static uint64_t pmsav5_insn_ap_read(CPUARMState *env, const ARMCPRegInfo *ri) · line 1714
Quoted 6 lines
target-arm/helper.c +3
-0@@ -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);
Quoted 4 lines
target-arm/helper.c +4
-0+ + u32p += env->cp15.c6_rgnr; +return *u32p; +}
target-arm/helper.c static uint64_t pmsav5_insn_ap_read(CPUARMState *env, const ARMCPRegInfo *ri) · line 1728
Quoted 10 lines
target-arm/helper.c +10
-0+ +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...
target-arm/helper.c static uint64_t pmsav5_insn_ap_read(CPUARMState *env, const ARMCPRegInfo *ri) · line 1736
Quoted 8 lines
target-arm/helper.c +8
-0+} + +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.
Quoted 12 lines
target-arm/helper.c +12
-0+ 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]),
target-arm/helper.c static uint64_t pmsav5_insn_ap_read(CPUARMState *env, const ARMCPRegInfo *ri) · line 1758
Quoted 10 lines
target-arm/helper.c +10
-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
Quoted 34 lines · Peter Maydell
Quoted 33 lines · Peter Crosthwaite
define the arm CP registers for PMSAv7 and their accessor functions.
Signed-off-by Peter Crosthwaite <[email protected]>--- target-arm/cpu.h | 6 ++++++ target-arm/helper.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+)target-arm/cpu.h +6
-0@@ -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 */ target-arm/helper.c +0
-0@@ -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); }
Quoted 6 lines
target-arm/helper.c static uint64_t pmsav5_insn_ap_read(CPUARMState *env, const ARMCPRegInfo *ri) · line 1714
Quoted 3 lines
target-arm/helper.c +3
-0+static uint64_t pmsav7_read (CPUARMState *env,const ARMCPRegInfo *ri) +{ +uint32_t *u32p = (void *)env + ri->fieldoffset;This is
raw_ptr(env, ri);
Fixed.
Quoted 11 lines
Quoted 11 lines
target-arm/helper.c +11
-0+ + 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; +
Quoted 9 lines
target-arm/helper.c static uint64_t pmsav5_insn_ap_read(CPUARMState *env, const ARMCPRegInfo *ri) · line 1728
Quoted 3 lines
target-arm/helper.c +3
-0+ u32p += env->cp15.c6_rgnr; +tlb_flush (CPU (cpu),1 );/* Mappings may have changed - purge! */ + *u32p = value;Since you're not boundary-checking
c6_rgnreither 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.
Quoted 4 lines
Quoted 4 lines
target-arm/helper.c +4
-0+} + +static void pmsav7_reset (CPUARMState *env,const ARMCPRegInfo *ri) +{
Quoted 9 lines
target-arm/helper.c static uint64_t pmsav5_insn_ap_read(CPUARMState *env, const ARMCPRegInfo *ri) · line 1736
Quoted 4 lines
target-arm/helper.c +4
-0+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 justmemset()the whole thing rather than looping.
Memset added. Hard constant removed.
Quoted 16 lines
Quoted 16 lines
target-arm/helper.c +16
-0+ 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]),
Quoted 9 lines
target-arm/helper.c static uint64_t pmsav5_insn_ap_read(CPUARMState *env, const ARMCPRegInfo *ri) · line 1758
Quoted 6 lines
target-arm/helper.c +6
-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
Quoted 2 lines
-- PMM
PATCH 5/9 arm: helper: rename get_phys_addr_mpu
In reply to Peter CrosthwaiteThis get_phys_addr is really for pmsav5. Rename it accordingly.
--- target-arm/helper.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
target-arm/helper.c +5 -5
@@ -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)) {
Signature · 1 line
2.4.2.3.g2ffcb72
Re: [Qemu-devel] [PATCH target-arm v1 5/9] arm: helper: rename get_phys_addr_mpu
Quoted 36 lines · Peter Crosthwaite
This
get_phys_addris really for pmsav5. Rename it accordingly.Signed-off-by Peter Crosthwaite <[email protected]>--- target-arm/helper.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)target-arm/helper.c +5
-6@@ -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)) { --
Quoted 2 lines
2.4.2.3.g2ffcb72
thanks
-- PMM
PATCH 6/9 target-arm: Implement PMSAv7 MPU
In reply to Peter CrosthwaiteUnified 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.
--- target-arm/cpu.h | 1 + target-arm/helper.c | 153 ++++++++++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 144 insertions(+), 10 deletions(-)
target-arm/cpu.h +1 -0
@@ -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)
target-arm/helper.c +143 -10
189 lines not on this page · raw message
Signature · 1 line
2.4.2.3.g2ffcb72
Re: [Qemu-devel] [PATCH target-arm v1 6/9] target-arm: Implement PMSAv7 MPU
Quoted 32 lines · Peter Crosthwaite
Unified MPU only. Uses ARM architecture major revision to switch between PMSAv5 and v7 when
ARM_FEATURE_MPUis set. PMSA v6 remains unsupported and is asserted against.The return code from
get_phys_addrhas to patched to handle the case of a 0 FSR with error. Use -1 to indicate this condition.Signed-off-by Peter Crosthwaite <[email protected]>--- target-arm/cpu.h | 1 + target-arm/helper.c | 153 ++++++++++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 144 insertions(+), 10 deletions(-)target-arm/cpu.h +1
-0@@ -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)target-arm/helper.c +0
-0@@ -3323,13 +3323,13 @@ void register_cp_regs_for_features(ARMCPU *cpu)define_one_arm_cp_reg (cpu, &rvbar); }
target-arm/helper.c void register_cp_regs_for_features(ARMCPU *cpu) · line 3327
Quoted 10 lines
target-arm/helper.c +2
-7if (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 */
target-arm/helper.c void register_cp_regs_for_features(ARMCPU *cpu) · line 3328
Quoted 1 line
target-arm/helper.c +1
-0+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.
Quoted 31 lines
target-arm/helper.c +24
-0+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,
target-arm/helper.c do_fault: · line 5752
Quoted 10 lines
target-arm/helper.c +10
-0+ 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.
target-arm/helper.c do_fault: · line 5763
Quoted 11 lines
target-arm/helper.c +11
-0+ }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.
target-arm/helper.c do_fault: · line 5764
Quoted 1 line
target-arm/helper.c +1
-0+ }
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.
target-arm/helper.c do_fault: · line 5776
Quoted 12 lines
target-arm/helper.c +12
-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 )) {
I think I would find these easier to read with more parens so I didn't have to look up & vs << precedence.
Quoted 21 lines
target-arm/helper.c +21
-0+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 */
target-arm/helper.c do_fault: · line 5807
Quoted 10 lines
target-arm/helper.c +10
-0+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.
Quoted 12 lines
target-arm/helper.c +12
-0+ } + }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 :
target-arm/helper.c do_fault: · line 5829
Quoted 10 lines
target-arm/helper.c +10
-0+ *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.
target-arm/helper.c do_fault: · line 5839
Quoted 10 lines
target-arm/helper.c +10
-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 ;
This is
if (!(*prot & (1 << access_type))) {
return 0xD; /* Permission fault */
} else {
return 0;}
isn't it?
target-arm/helper.c do_fault: · line 5842
Quoted 3 lines
target-arm/helper.c +3
-0+default : +abort ();/* should be unreachable */ +return 0 ;
The usual way to write this is g_assert_not_reached(), and the return isn't needed.
target-arm/helper.c static int get_phys_addr_pmsav5(CPUARMState *env, uint32_t address, · line 5922
Quoted 13 lines
target-arm/helper.c +5
-1+ } + +} +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.
target-arm/helper.c static int get_phys_addr_pmsav5(CPUARMState *env, uint32_t address, · line 5928
Quoted 6 lines
target-arm/helper.c +0
-0* 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?
Quoted 5 lines
target-arm/helper.c +1
-0+* * -1 return value indicates a 0 FSR. * * @env: CPUARMState * @address: virtual address to get physical address for
target-arm/helper.c static inline int get_phys_addr(CPUARMState *env, target_ulong address, · line 5988
Quoted 9 lines
target-arm/helper.c +4
-2@@ -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.)
Quoted 8 lines
target-arm/helper.c +4
-0+ }else { +return get_phys_addr_pmsav5 (env, address, access_type, mmu_idx, + phys_ptr, prot); + } }if (regime_using_lpae_format (env, mmu_idx)) {
target-arm/helper.c int arm_tlb_fill(CPUState *cs, vaddr address, · line 6035
Quoted 8 lines
target-arm/helper.c +2
-0@@ -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
Quoted 37 lines · Peter Maydell
Quoted 36 lines · Peter Crosthwaite
Unified MPU only. Uses ARM architecture major revision to switch between PMSAv5 and v7 when
ARM_FEATURE_MPUis set. PMSA v6 remains unsupported and is asserted against.The return code from
get_phys_addrhas to patched to handle the case of a 0 FSR with error. Use -1 to indicate this condition.Signed-off-by Peter Crosthwaite <[email protected]>--- target-arm/cpu.h | 1 + target-arm/helper.c | 153 ++++++++++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 144 insertions(+), 10 deletions(-)target-arm/cpu.h +1
-0@@ -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)target-arm/helper.c +0
-3@@ -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
Quoted 9 lines
target-arm/helper.c void register_cp_regs_for_features(ARMCPU *cpu) · line 3327
Quoted 6 lines
target-arm/helper.c +2
-4-* 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.
Quoted 7 lines
target-arm/helper.c void register_cp_regs_for_features(ARMCPU *cpu) · line 3328
Quoted 1 line
target-arm/helper.c +1
-0+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.
Quoted 36 lines
Quoted 36 lines
target-arm/helper.c +29
-0+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); +
Quoted 9 lines
target-arm/helper.c do_fault: · line 5752
Quoted 5 lines
target-arm/helper.c +5
-0+ *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.
Quoted 12 lines
target-arm/helper.c do_fault: · line 5763
Quoted 11 lines
target-arm/helper.c +11
-0+ }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" );
Quoted 4 lines
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.
Quoted 7 lines
target-arm/helper.c do_fault: · line 5764
Quoted 1 line
target-arm/helper.c +1
-0+ }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.
Quoted 3 lines
Base address not aligned to the region size is also UNPREDICTABLE, incidentally.
Check added. region is ignored (continue) in this case.
Quoted 7 lines
Quoted 7 lines
target-arm/helper.c +7
-0+ +if (address < base || address > base -1 + (1ull << rsize)) { +continue ; + } + +if (rsize <8 ) {/* no subregions for regions < 256 bytes */ +break ;
Quoted 9 lines
target-arm/helper.c do_fault: · line 5776
Quoted 5 lines
target-arm/helper.c +5
-0+ } + + 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.
Quoted 26 lines
Quoted 26 lines
target-arm/helper.c +26
-0+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 :
Quoted 9 lines
target-arm/helper.c do_fault: · line 5807
Quoted 5 lines
target-arm/helper.c +5
-0+ *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.
Quoted 9 lines
Quoted 9 lines
target-arm/helper.c +9
-0+ } + }else {/* Priv. mode AP bits decoding */ +switch (ap) { +case 0 : +break ;/* no access */ +case 1 : +case 2 : +case 3 : + *prot |=PAGE_WRITE ;
Quoted 8 lines
target-arm/helper.c do_fault: · line 5824
Quoted 8 lines
target-arm/helper.c +8
-0+/* 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.
Quoted 8 lines
target-arm/helper.c do_fault: · line 5829
Quoted 5 lines
target-arm/helper.c +5
-0+ } + } + +/* 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.
Quoted 11 lines
target-arm/helper.c do_fault: · line 5839
Quoted 10 lines
target-arm/helper.c +10
-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 ;
Quoted 9 lines
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.
Quoted 7 lines
target-arm/helper.c do_fault: · line 5842
Quoted 3 lines
target-arm/helper.c +3
-0+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.
Quoted 7 lines
Quoted 7 lines
target-arm/helper.c +4
-0+ } + +} +static int get_phys_addr_pmsav5 (CPUARMState *env,uint32_t address,int access_type,ARMMMUIdx mmu_idx,hwaddr *phys_ptr,int *prot)
Quoted 9 lines
target-arm/helper.c static int get_phys_addr_pmsav5(CPUARMState *env, uint32_t address, · line 5922
Quoted 6 lines
target-arm/helper.c +1
-1@@ -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.
Quoted 8 lines
target-arm/helper.c static int get_phys_addr_pmsav5(CPUARMState *env, uint32_t address, · line 5928
Quoted 6 lines
target-arm/helper.c +0
-0* 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
Quoted 3 lines
If it is, is that a bug
we now need to fix?
Don't think so. Should be ok.
Quoted 6 lines
Quoted 6 lines
target-arm/helper.c +1
-0+* * -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,
Quoted 13 lines
target-arm/helper.c static inline int get_phys_addr(CPUARMState *env, target_ulong address, · line 5988
Quoted 8 lines
target-arm/helper.c +4
-2if (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?
Quoted 24 lines
(We don't actually implement the M profile MPU right now, but it would be nice to avoid leaving beartraps for whoever does.)
Quoted 8 lines
target-arm/helper.c +4
-0+ }else { +return get_phys_addr_pmsav5 (env, address, access_type, mmu_idx, + phys_ptr, prot); + } }if (regime_using_lpae_format (env, mmu_idx)) {target-arm/helper.c int arm_tlb_fill(CPUState *cs, vaddr address, · line 6035
Quoted 8 lines
target-arm/helper.c +2
-0@@ -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.
Quoted 4 lines
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
Quoted 3 lines
thanks
-- PMM
Re: [Qemu-devel] [PATCH target-arm v1 6/9] target-arm: Implement PMSAv7 MPU
Quoted 17 lines · Peter Crosthwaite
Quoted 16 lines · Peter Maydell
target-arm/helper.c do_fault: · line 5839
Quoted 7 lines · Peter Crosthwaite
target-arm/helper.c +7
-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 ;This is
if (!(*prot & (1 << access_type))) { return 0xD; /* Permission fault */ } else { return 0;}
Quoted 6 lines
Quoted 2 lines
isn't it?
Yes but that assumes that
access_typeencoding is correlated toPAGE_FOOmasks 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.
Quoted 20 lines
Quoted 6 lines
Quoted 6 lines
+ * * -1 return value indicates a 0 FSR.
*
* @env: CPUARMState
* @address: virtual address to get physical address for(quoted diff) +0
-0@@ -5857,8 +5982,14 @@ static inline int get_phys_addr(CPUARMState *env, target_ulong address,Quoted 13 lines
target-arm/helper.c static inline int get_phys_addr(CPUARMState *env, target_ulong address, · line 5988
Quoted 8 lines
target-arm/helper.c +4
-2if (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?
Quoted 1 line
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
Quoted 19 lines · Peter Maydell
Quoted 17 lines · Peter Crosthwaite
Quoted 16 lines · Peter Maydell
target-arm/helper.c do_fault: · line 5839
Quoted 7 lines · Peter Crosthwaite
target-arm/helper.c +7
-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 ;This is
if (!(*prot & (1 << access_type))) { return 0xD; /* Permission fault */ } else { return 0;}
Quoted 9 lines
Quoted 6 lines
Quoted 2 lines
isn't it?
Yes but that assumes that
access_typeencoding is correlated toPAGE_FOOmasks 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
PATCH 7/9 arm: r5: Implement dummy ATCM, BTCM and D-cache invalidate
In reply to Peter CrosthwaiteThese 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.
--- target-arm/cpu.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
target-arm/cpu.c +12 -0
@@ -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[] = {
Signature · 1 line
2.4.2.3.g2ffcb72
Re: [Qemu-devel] [PATCH target-arm v1 7/9] arm: r5: Implement dummy ATCM, BTCM and D-cache invalidate
Quoted 14 lines · 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 <[email protected]>--- target-arm/cpu.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)target-arm/cpu.c +0
-0
target-arm/cpu.c static void arm_v7m_class_init(ObjectClass *oc, void *data) · line 802
Quoted 9 lines
target-arm/cpu.c +6
-0@@ -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).
target-arm/cpu.c static void arm_v7m_class_init(ObjectClass *oc, void *data) · line 804
Quoted 2 lines
target-arm/cpu.c +2
-0+ { .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?
Quoted 7 lines
target-arm/cpu.c +3
-0+ REGINFO_SENTINEL +}; +static void cortex_r5_initfn (Object *obj) {ARMCPU *cpu =ARM_CPU (obj);
target-arm/cpu.c static void cortex_r5_initfn(Object *obj) · line 835
Quoted 7 lines
target-arm/cpu.c +1
-0@@ -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
PATCH 8/9 arm: xlnx-zynqmp: Preface CPU variables with "A"
In reply to Peter CrosthwaiteThe 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.
--- 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(-)
hw/arm/xlnx-ep108.c +1 -1
@@ -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 = {
hw/arm/xlnx-zynqmp.c +12 -12
@@ -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++) {
include/hw/arm/xlnx-zynqmp.h +2 -2
@@ -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 ];
Signature · 1 line
2.4.2.3.g2ffcb72
Re: [Qemu-devel] [PATCH target-arm v1 8/9] arm: xlnx-zynqmp: Preface CPU variables with "A"
Quoted 6 lines · 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 <[email protected]>
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"
In reply to Peter CrosthwaiteQuoted 17 lines · 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 <[email protected]>--- 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(-)hw/arm/xlnx-ep108.c +0
-0
hw/arm/xlnx-ep108.c static void xlnx_ep108_init(MachineState *machine) · line 70
Quoted 7 lines
hw/arm/xlnx-ep108.c +1
-1@@ -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
Quoted 101 lines
hw/arm/xlnx-ep108.c +0
-0static QEMUMachine xlnx_ep108_machine = {hw/arm/xlnx-zynqmp.c +12
-12@@ -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++) {include/hw/arm/xlnx-zynqmp.h +2
-3@@ -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"
Quoted 29 lines · Alistair Francis
Quoted 17 lines · 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 <[email protected]>--- 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(-)hw/arm/xlnx-ep108.c +0
-0hw/arm/xlnx-ep108.c static void xlnx_ep108_init(MachineState *machine) · line 70
Quoted 7 lines
hw/arm/xlnx-ep108.c +1
-1@@ -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,
Quoted 4 lines
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
Quoted 106 lines
Thanks,
Alistair
Quoted 101 lines
hw/arm/xlnx-ep108.c +0
-0static QEMUMachine xlnx_ep108_machine = {hw/arm/xlnx-zynqmp.c +12
-12@@ -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++) {include/hw/arm/xlnx-zynqmp.h +2
-3@@ -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"
Quoted 36 lines · Peter Crosthwaite
Quoted 29 lines · Alistair Francis
Quoted 17 lines · 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 <[email protected]>--- 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(-)hw/arm/xlnx-ep108.c +0
-0hw/arm/xlnx-ep108.c static void xlnx_ep108_init(MachineState *machine) · line 70
Quoted 7 lines
hw/arm/xlnx-ep108.c +1
-1@@ -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,
Quoted 4 lines
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.
Quoted 8 lines
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
Quoted 111 lines
Regards,
PeterQuoted 106 lines
Thanks,
Alistair
Quoted 101 lines
hw/arm/xlnx-ep108.c +0
-0static QEMUMachine xlnx_ep108_machine = {hw/arm/xlnx-zynqmp.c +12
-12@@ -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++) {include/hw/arm/xlnx-zynqmp.h +2
-3@@ -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
PATCH 9/9 arm: xlnx-zynqmp: Add 2xCortexR5 CPUs
In reply to Peter CrosthwaiteAdd the 2xCortexR5 CPUs to zynqmp board. They are powered off on reset (this is true of real hardware).
--- hw/arm/xlnx-zynqmp.c | 26 ++++++++++++++++++++++++++ include/hw/arm/xlnx-zynqmp.h | 2 ++ 2 files changed, 28 insertions(+)
hw/arm/xlnx-zynqmp.c +26 -0
@@ -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); }
include/hw/arm/xlnx-zynqmp.h +2 -0
@@ -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 ];
Signature · 1 line
2.4.2.3.g2ffcb72
Re: [Qemu-devel] [PATCH target-arm v1 9/9] arm: xlnx-zynqmp: Add 2xCortexR5 CPUs
Quoted 14 lines · 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 <[email protected]>--- hw/arm/xlnx-zynqmp.c | 26 ++++++++++++++++++++++++++ include/hw/arm/xlnx-zynqmp.h | 2 ++ 2 files changed, 28 insertions(+)hw/arm/xlnx-zynqmp.c +0
-0
hw/arm/xlnx-zynqmp.c static void xlnx_zynqmp_init(Object *obj) · line 80
Quoted 10 lines
hw/arm/xlnx-zynqmp.c +7
-0@@ -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.
Quoted 5 lines
hw/arm/xlnx-zynqmp.c +0
-0object_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);
hw/arm/xlnx-zynqmp.c static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp) · line 172
Quoted 10 lines
hw/arm/xlnx-zynqmp.c +8
-0} +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'.
hw/arm/xlnx-zynqmp.c static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp) · line 179
Quoted 7 lines
hw/arm/xlnx-zynqmp.c +7
-0+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
Quoted 31 lines
hw/arm/xlnx-zynqmp.c +4
-0+return ; + } + } +for (i =0 ; i <GIC_NUM_SPI_INTR ; i++) { gic_spi[i] =qdev_get_gpio_in (DEVICE (&s->gic), i); }include/hw/arm/xlnx-zynqmp.h +2
-1@@ -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