[Qemu-devel] [PATCH] target/arm: Fix register definitions for VMIDR and VMPIDR

[Qemu-devel] [PATCH] target/arm: Fix register definitions for VMIDR and VMPIDR

[Peter Maydell]

The register definitions for VMIDR and VMPIDR have separate
reginfo structs for the AArch32 and AArch64 registers. However
the 32-bit versions are wrong:
 * they use offsetof instead of offsetoflow32 to mark where
   the 32-bit value lives in the uint64_t CPU state field
 * they don't mark themselves as ARM_CP_ALIAS

In particular this means that if you try to use an Arm guest CPU
which enables EL2 on a big-endian host it will assert at reset:
 target/arm/cpu.c:114: cp_reg_check_reset: Assertion `oldvalue == newvalue' failed.

because the reset of the 32-bit register writes to the top
half of the uint64_t.

Correct the errors in the structures.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
This is necessary for 'make check' to pass on big endian
systems with the 'raspi3' board enabled, which is the
first board which has an EL2-enabled-by-default CPU.
---
 target/arm/helper.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index e7586fcf6c..e27957df38 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -5068,8 +5068,8 @@ void register_cp_regs_for_features(ARMCPU *cpu)
             { .name = "VPIDR", .state = ARM_CP_STATE_AA32,
               .cp = 15, .opc1 = 4, .crn = 0, .crm = 0, .opc2 = 0,
               .access = PL2_RW, .accessfn = access_el3_aa32ns,
-              .resetvalue = cpu->midr,
-              .fieldoffset = offsetof(CPUARMState, cp15.vpidr_el2) },
+              .resetvalue = cpu->midr, .type = ARM_CP_ALIAS,
+              .fieldoffset = offsetoflow32(CPUARMState, cp15.vpidr_el2) },
             { .name = "VPIDR_EL2", .state = ARM_CP_STATE_AA64,
               .opc0 = 3, .opc1 = 4, .crn = 0, .crm = 0, .opc2 = 0,
               .access = PL2_RW, .resetvalue = cpu->midr,
@@ -5077,8 +5077,8 @@ void register_cp_regs_for_features(ARMCPU *cpu)
             { .name = "VMPIDR", .state = ARM_CP_STATE_AA32,
               .cp = 15, .opc1 = 4, .crn = 0, .crm = 0, .opc2 = 5,
               .access = PL2_RW, .accessfn = access_el3_aa32ns,
-              .resetvalue = vmpidr_def,
-              .fieldoffset = offsetof(CPUARMState, cp15.vmpidr_el2) },
+              .resetvalue = vmpidr_def, .type = ARM_CP_ALIAS,
+              .fieldoffset = offsetoflow32(CPUARMState, cp15.vmpidr_el2) },
             { .name = "VMPIDR_EL2", .state = ARM_CP_STATE_AA64,
               .opc0 = 3, .opc1 = 4, .crn = 0, .crm = 0, .opc2 = 5,
               .access = PL2_RW,
-- 
2.16.1

Re: [Qemu-devel] [Qemu-arm] [PATCH] target/arm: Fix register definitions for VMIDR and VMPIDR

[Philippe Mathieu-Daudé]

On 02/15/2018 03:24 PM, Peter Maydell wrote:
> The register definitions for VMIDR and VMPIDR have separate
> reginfo structs for the AArch32 and AArch64 registers. However
> the 32-bit versions are wrong:
>  * they use offsetof instead of offsetoflow32 to mark where
>    the 32-bit value lives in the uint64_t CPU state field

Maybe we can replace all offsetof() by offsetof64() and forbid the use
of offsetof() in this file, this will make A32 endian/alignment errors
more apparent.

>  * they don't mark themselves as ARM_CP_ALIAS
> 
> In particular this means that if you try to use an Arm guest CPU
> which enables EL2 on a big-endian host it will assert at reset:
>  target/arm/cpu.c:114: cp_reg_check_reset: Assertion `oldvalue == newvalue' failed.
> 
> because the reset of the 32-bit register writes to the top
> half of the uint64_t.
> 
> Correct the errors in the structures.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> This is necessary for 'make check' to pass on big endian
> systems with the 'raspi3' board enabled, which is the
> first board which has an EL2-enabled-by-default CPU.
> ---
>  target/arm/helper.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index e7586fcf6c..e27957df38 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -5068,8 +5068,8 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>              { .name = "VPIDR", .state = ARM_CP_STATE_AA32,
>                .cp = 15, .opc1 = 4, .crn = 0, .crm = 0, .opc2 = 0,
>                .access = PL2_RW, .accessfn = access_el3_aa32ns,
> -              .resetvalue = cpu->midr,
> -              .fieldoffset = offsetof(CPUARMState, cp15.vpidr_el2) },
> +              .resetvalue = cpu->midr, .type = ARM_CP_ALIAS,
> +              .fieldoffset = offsetoflow32(CPUARMState, cp15.vpidr_el2) },
>              { .name = "VPIDR_EL2", .state = ARM_CP_STATE_AA64,
>                .opc0 = 3, .opc1 = 4, .crn = 0, .crm = 0, .opc2 = 0,
>                .access = PL2_RW, .resetvalue = cpu->midr,
> @@ -5077,8 +5077,8 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>              { .name = "VMPIDR", .state = ARM_CP_STATE_AA32,
>                .cp = 15, .opc1 = 4, .crn = 0, .crm = 0, .opc2 = 5,
>                .access = PL2_RW, .accessfn = access_el3_aa32ns,
> -              .resetvalue = vmpidr_def,
> -              .fieldoffset = offsetof(CPUARMState, cp15.vmpidr_el2) },
> +              .resetvalue = vmpidr_def, .type = ARM_CP_ALIAS,
> +              .fieldoffset = offsetoflow32(CPUARMState, cp15.vmpidr_el2) },
>              { .name = "VMPIDR_EL2", .state = ARM_CP_STATE_AA64,
>                .opc0 = 3, .opc1 = 4, .crn = 0, .crm = 0, .opc2 = 5,
>                .access = PL2_RW,
> 

After enjoying scrolling a 6666p specs,
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>