[Qemu-devel] [PATCH] target-arm: fix REVIDR reset value

[Qemu-devel] [PATCH] target-arm: fix REVIDR reset value

[Sergey Fedorov]

According to ARM Cortex-A57 TRM, REVIDR reset value should be zero. So
let REVIDR reset value be specified by CPU model and fix it for
Cortex-A57.

Signed-off-by: Sergey Fedorov <serge.fdrv@gmail.com>
---
 target-arm/cpu-qom.h | 1 +
 target-arm/cpu64.c   | 1 +
 target-arm/helper.c  | 2 +-
 3 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/target-arm/cpu-qom.h b/target-arm/cpu-qom.h
index ed5a644..c80381d 100644
--- a/target-arm/cpu-qom.h
+++ b/target-arm/cpu-qom.h
@@ -127,6 +127,7 @@ typedef struct ARMCPU {
      * prefix means a constant register.
      */
     uint32_t midr;
+    uint32_t revidr;
     uint32_t reset_fpsid;
     uint32_t mvfr0;
     uint32_t mvfr1;
diff --git a/target-arm/cpu64.c b/target-arm/cpu64.c
index bf7dd68..e330160 100644
--- a/target-arm/cpu64.c
+++ b/target-arm/cpu64.c
@@ -110,6 +110,7 @@ static void aarch64_a57_initfn(Object *obj)
     set_feature(&cpu->env, ARM_FEATURE_CRC);
     cpu->kvm_target = QEMU_KVM_ARM_TARGET_CORTEX_A57;
     cpu->midr = 0x411fd070;
+    cpu->revidr = 0x00000000;
     cpu->reset_fpsid = 0x41034070;
     cpu->mvfr0 = 0x10110222;
     cpu->mvfr1 = 0x12111111;
diff --git a/target-arm/helper.c b/target-arm/helper.c
index b2b377a..89940d6 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -3363,7 +3363,7 @@ void register_cp_regs_for_features(ARMCPU *cpu)
               .access = PL1_R, .type = ARM_CP_CONST, .resetvalue = cpu->midr },
             { .name = "REVIDR_EL1", .state = ARM_CP_STATE_BOTH,
               .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 0, .opc2 = 6,
-              .access = PL1_R, .type = ARM_CP_CONST, .resetvalue = cpu->midr },
+              .access = PL1_R, .type = ARM_CP_CONST, .resetvalue = cpu->revidr },
             REGINFO_SENTINEL
         };
         ARMCPRegInfo id_cp_reginfo[] = {
-- 
1.9.1

Re: [Qemu-devel] [PATCH] target-arm: fix REVIDR reset value

[Shannon Zhao]

Hi,

On 2015/6/2 20:21, Sergey Fedorov wrote:
> According to ARM Cortex-A57 TRM, REVIDR reset value should be zero. So
> let REVIDR reset value be specified by CPU model and fix it for
> Cortex-A57.
> 

Also need to fix it for Cortex-A53?

> Signed-off-by: Sergey Fedorov <serge.fdrv@gmail.com>
> ---
>  target-arm/cpu-qom.h | 1 +
>  target-arm/cpu64.c   | 1 +
>  target-arm/helper.c  | 2 +-
>  3 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/target-arm/cpu-qom.h b/target-arm/cpu-qom.h
> index ed5a644..c80381d 100644
> --- a/target-arm/cpu-qom.h
> +++ b/target-arm/cpu-qom.h
> @@ -127,6 +127,7 @@ typedef struct ARMCPU {
>       * prefix means a constant register.
>       */
>      uint32_t midr;
> +    uint32_t revidr;
>      uint32_t reset_fpsid;
>      uint32_t mvfr0;
>      uint32_t mvfr1;
> diff --git a/target-arm/cpu64.c b/target-arm/cpu64.c
> index bf7dd68..e330160 100644
> --- a/target-arm/cpu64.c
> +++ b/target-arm/cpu64.c
> @@ -110,6 +110,7 @@ static void aarch64_a57_initfn(Object *obj)
>      set_feature(&cpu->env, ARM_FEATURE_CRC);
>      cpu->kvm_target = QEMU_KVM_ARM_TARGET_CORTEX_A57;
>      cpu->midr = 0x411fd070;
> +    cpu->revidr = 0x00000000;
>      cpu->reset_fpsid = 0x41034070;
>      cpu->mvfr0 = 0x10110222;
>      cpu->mvfr1 = 0x12111111;
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index b2b377a..89940d6 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -3363,7 +3363,7 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>                .access = PL1_R, .type = ARM_CP_CONST, .resetvalue = cpu->midr },
>              { .name = "REVIDR_EL1", .state = ARM_CP_STATE_BOTH,
>                .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 0, .opc2 = 6,
> -              .access = PL1_R, .type = ARM_CP_CONST, .resetvalue = cpu->midr },
> +              .access = PL1_R, .type = ARM_CP_CONST, .resetvalue = cpu->revidr },
>              REGINFO_SENTINEL
>          };
>          ARMCPRegInfo id_cp_reginfo[] = {
> 

-- 
Shannon

Re: [Qemu-devel] [PATCH] target-arm: fix REVIDR reset value

[Peter Maydell]

On 2 June 2015 at 13:51, Shannon Zhao <shannon.zhao@linaro.org> wrote:
> Hi,
>
> On 2015/6/2 20:21, Sergey Fedorov wrote:
>> According to ARM Cortex-A57 TRM, REVIDR reset value should be zero. So
>> let REVIDR reset value be specified by CPU model and fix it for
>> Cortex-A57.
>>
>
> Also need to fix it for Cortex-A53?

Yes, please.

Also you need to update the comment:
            /* v8 MIDR -- the wildcard isn't necessary, and nor is the
             * variable-MIDR TI925 behaviour. Instead we have a single
             * (strictly speaking IMPDEF) alias of the MIDR, REVIDR.
             */

since the REVIDR isn't an alias of the MIDR any more.

NB: a bug that's been on my todo list for ages is that the comment
is incorrect about the wildcard being unnecessary -- this was a
misreading of the ARM ARM by me when I wrote that code. v8 *does*
retain the aliases of the MIDR at any unoccupied opc2 values
in the 0, c0, c0, x space, and we should provide the wildcard
encoding here.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] target-arm: fix REVIDR reset value

[Peter Maydell]

On 2 June 2015 at 14:18, Peter Maydell <peter.maydell@linaro.org> wrote:
> NB: a bug that's been on my todo list for ages is that the comment
> is incorrect about the wildcard being unnecessary -- this was a
> misreading of the ARM ARM by me when I wrote that code. v8 *does*
> retain the aliases of the MIDR at any unoccupied opc2 values
> in the 0, c0, c0, x space, and we should provide the wildcard
> encoding here.

...*but* this only applies (as far as I can tell) to v8 AArch32;
v8 AArch64 MIDR has no aliases.

-- PMM

Re: [Qemu-devel] [PATCH] target-arm: fix REVIDR reset value

[Sergey Fedorov]

On 02.06.2015 16:28, Peter Maydell wrote:
> On 2 June 2015 at 14:18, Peter Maydell <peter.maydell@linaro.org> wrote:
>> NB: a bug that's been on my todo list for ages is that the comment
>> is incorrect about the wildcard being unnecessary -- this was a
>> misreading of the ARM ARM by me when I wrote that code. v8 *does*
>> retain the aliases of the MIDR at any unoccupied opc2 values
>> in the 0, c0, c0, x space, and we should provide the wildcard
>> encoding here.
> ...*but* this only applies (as far as I can tell) to v8 AArch32;
> v8 AArch64 MIDR has no aliases.
>
> -- PMM

OK, I'm going to fix that as well.

Regards,
Sergey

Re: [Qemu-devel] [PATCH] target-arm: fix REVIDR reset value

[Peter Maydell]

On 2 June 2015 at 13:21, Sergey Fedorov <serge.fdrv@gmail.com> wrote:
> According to ARM Cortex-A57 TRM, REVIDR reset value should be zero. So
> let REVIDR reset value be specified by CPU model and fix it for
> Cortex-A57.
>
> Signed-off-by: Sergey Fedorov <serge.fdrv@gmail.com>
> --

Do you have guest software that actually cares about the REVIDR?
What's it doing with the value?

thanks
-- PMM

Re: [Qemu-devel] [PATCH] target-arm: fix REVIDR reset value

[Sergey Fedorov]

On 02.06.2015 16:54, Peter Maydell wrote:
> On 2 June 2015 at 13:21, Sergey Fedorov <serge.fdrv@gmail.com> wrote:
>> According to ARM Cortex-A57 TRM, REVIDR reset value should be zero. So
>> let REVIDR reset value be specified by CPU model and fix it for
>> Cortex-A57.
>>
>> Signed-off-by: Sergey Fedorov <serge.fdrv@gmail.com>
>> --
> Do you have guest software that actually cares about the REVIDR?
> What's it doing with the value?
>
> thanks
> -- PMM

Linux kernel reads this register. I just saw a strange value and checked
it against TRM.

Regards,
Sergey