[PATCH v3] target/arm: Use correct variable for setting 'max' cpu's MIDR_EL1

[PATCH v3] target/arm: Use correct variable for setting 'max' cpu's MIDR_EL1

[Philippe Mathieu-Daudé]

MIDR_EL1 a 64-bit system register with the top 32-bit being RES0.

This fixes when compiling with -Werror=conversion:

  target/arm/cpu64.c: In function ‘aarch64_max_initfn’:
  target/arm/cpu64.c:628:21: error: conversion from ‘uint64_t’ {aka ‘long unsigned int’} to ‘uint32_t’ {aka ‘unsigned int’} may change value [-Werror=conversion]
    628 |         cpu->midr = t;
        |                     ^

Suggested-by: Laurent Desnogues <laurent.desnogues@gmail.com>
Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
Since v2: Do not use RESERVED bits.
Since v1: Follow Laurent and Peter suggestion.
---
 target/arm/cpu.h | 2 +-
 target/arm/cpu.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 8b9f2961ba..592fb217d6 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -894,7 +894,7 @@ struct ARMCPU {
         uint64_t id_aa64dfr0;
         uint64_t id_aa64dfr1;
     } isar;
-    uint32_t midr;
+    uint64_t midr;
     uint32_t revidr;
     uint32_t reset_fpsid;
     uint32_t ctr;
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index a79f233b17..7ff80894b6 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -2757,7 +2757,7 @@ static const ARMCPUInfo arm_cpus[] = {
 static Property arm_cpu_properties[] = {
     DEFINE_PROP_BOOL("start-powered-off", ARMCPU, start_powered_off, false),
     DEFINE_PROP_UINT32("psci-conduit", ARMCPU, psci_conduit, 0),
-    DEFINE_PROP_UINT32("midr", ARMCPU, midr, 0),
+    DEFINE_PROP_UINT64("midr", ARMCPU, midr, 0),
     DEFINE_PROP_UINT64("mp-affinity", ARMCPU,
                         mp_affinity, ARM64_AFFINITY_INVALID),
     DEFINE_PROP_INT32("node-id", ARMCPU, node_id, CPU_UNSET_NUMA_NODE_ID),
-- 
2.21.1

Re: [PATCH v3] target/arm: Use correct variable for setting 'max' cpu's MIDR_EL1

[Laurent Desnogues]

On Tue, Apr 28, 2020 at 7:26 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> MIDR_EL1 a 64-bit system register with the top 32-bit being RES0.
>
> This fixes when compiling with -Werror=conversion:
>
>   target/arm/cpu64.c: In function ‘aarch64_max_initfn’:
>   target/arm/cpu64.c:628:21: error: conversion from ‘uint64_t’ {aka ‘long unsigned int’} to ‘uint32_t’ {aka ‘unsigned int’} may change value [-Werror=conversion]
>     628 |         cpu->midr = t;
>         |                     ^
>
> Suggested-by: Laurent Desnogues <laurent.desnogues@gmail.com>
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Laurent Desnogues <laurent.desnogues@gmail.com>

Thanks,

Laurent

> ---
> Since v2: Do not use RESERVED bits.
> Since v1: Follow Laurent and Peter suggestion.
> ---
>  target/arm/cpu.h | 2 +-
>  target/arm/cpu.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 8b9f2961ba..592fb217d6 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -894,7 +894,7 @@ struct ARMCPU {
>          uint64_t id_aa64dfr0;
>          uint64_t id_aa64dfr1;
>      } isar;
> -    uint32_t midr;
> +    uint64_t midr;
>      uint32_t revidr;
>      uint32_t reset_fpsid;
>      uint32_t ctr;
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index a79f233b17..7ff80894b6 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -2757,7 +2757,7 @@ static const ARMCPUInfo arm_cpus[] = {
>  static Property arm_cpu_properties[] = {
>      DEFINE_PROP_BOOL("start-powered-off", ARMCPU, start_powered_off, false),
>      DEFINE_PROP_UINT32("psci-conduit", ARMCPU, psci_conduit, 0),
> -    DEFINE_PROP_UINT32("midr", ARMCPU, midr, 0),
> +    DEFINE_PROP_UINT64("midr", ARMCPU, midr, 0),
>      DEFINE_PROP_UINT64("mp-affinity", ARMCPU,
>                          mp_affinity, ARM64_AFFINITY_INVALID),
>      DEFINE_PROP_INT32("node-id", ARMCPU, node_id, CPU_UNSET_NUMA_NODE_ID),
> --
> 2.21.1
>

Re: [PATCH v3] target/arm: Use correct variable for setting 'max' cpu's MIDR_EL1

[Peter Maydell]

On Tue, 28 Apr 2020 at 18:26, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> MIDR_EL1 a 64-bit system register with the top 32-bit being RES0.
>
> This fixes when compiling with -Werror=conversion:
>
>   target/arm/cpu64.c: In function ‘aarch64_max_initfn’:
>   target/arm/cpu64.c:628:21: error: conversion from ‘uint64_t’ {aka ‘long unsigned int’} to ‘uint32_t’ {aka ‘unsigned int’} may change value [-Werror=conversion]
>     628 |         cpu->midr = t;
>         |                     ^
>
> Suggested-by: Laurent Desnogues <laurent.desnogues@gmail.com>
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Applied to target-arm.next, with the commit message fixed
up to match the patch contents:

    target/arm: Use uint64_t for midr field in CPU state struct

    MIDR_EL1 is a 64-bit system register with the top 32-bit being RES0.
    Represent it in QEMU's ARMCPU struct with a uint64_t, not a
    uint32_t.

    This fixes an error when compiling with -Werror=conversion
    because we were manipulating the register value using a
    local uint64_t variable:

      target/arm/cpu64.c: In function ‘aarch64_max_initfn’:
      target/arm/cpu64.c:628:21: error: conversion from ‘uint64_t’
{aka ‘long unsigned int’} to ‘uint32_t      ’ {aka ‘unsigned int’} may
change value [-Werror=conversion]
        628 |         cpu->midr = t;
            |                     ^

    and future-proofs us against a possible future architecture
    change using some of the top 32 bits.

thanks
-- PMM

Re: [PATCH v3] target/arm: Use correct variable for setting 'max' cpu's MIDR_EL1

[Philippe Mathieu-Daudé]

On 4/30/20 5:59 PM, Peter Maydell wrote:
> On Tue, 28 Apr 2020 at 18:26, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> MIDR_EL1 a 64-bit system register with the top 32-bit being RES0.
>>
>> This fixes when compiling with -Werror=conversion:
>>
>>    target/arm/cpu64.c: In function ‘aarch64_max_initfn’:
>>    target/arm/cpu64.c:628:21: error: conversion from ‘uint64_t’ {aka ‘long unsigned int’} to ‘uint32_t’ {aka ‘unsigned int’} may change value [-Werror=conversion]
>>      628 |         cpu->midr = t;
>>          |                     ^
>>
>> Suggested-by: Laurent Desnogues <laurent.desnogues@gmail.com>
>> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> 
> Applied to target-arm.next, with the commit message fixed
> up to match the patch contents:
> 
>      target/arm: Use uint64_t for midr field in CPU state struct
> 
>      MIDR_EL1 is a 64-bit system register with the top 32-bit being RES0.
>      Represent it in QEMU's ARMCPU struct with a uint64_t, not a
>      uint32_t.
> 
>      This fixes an error when compiling with -Werror=conversion
>      because we were manipulating the register value using a
>      local uint64_t variable:
> 
>        target/arm/cpu64.c: In function ‘aarch64_max_initfn’:
>        target/arm/cpu64.c:628:21: error: conversion from ‘uint64_t’
> {aka ‘long unsigned int’} to ‘uint32_t      ’ {aka ‘unsigned int’} may
> change value [-Werror=conversion]
>          628 |         cpu->midr = t;
>              |                     ^
> 
>      and future-proofs us against a possible future architecture
>      change using some of the top 32 bits.

Thanks Peter for updating the description.

> 
> thanks
> -- PMM
>