1. Patchew
  2. QEMU
  3. View series

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

Philippe Mathieu-Daudé posted 1 patch 4 years ago
Test docker-mingw@fedora passed
Test checkpatch passed
Test docker-quick@centos7 passed
Test FreeBSD passed
Test asan passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20200428155005.25537-1-f4bug@amsat.org
There is a newer version of this series
target/arm/cpu.h | 3 ++-
target/arm/cpu.c | 4 +++-
2 files changed, 5 insertions(+), 2 deletions(-)
[PATCH v2] target/arm: Use correct variable for setting 'max' cpu's MIDR_EL1
Posted by Philippe Mathieu-Daudé 4 years ago 
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>
---
I suppose cp15.c0_cpuid register in target/arm/cpu.h as uint32_t is OK.

Since v1: Follow Laurent and Peter suggestion.
---
 target/arm/cpu.h | 3 ++-
 target/arm/cpu.c | 4 +++-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 8b9f2961ba..4d1be56df9 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;
@@ -1685,6 +1685,7 @@ FIELD(MIDR_EL1, PARTNUM, 4, 12)
 FIELD(MIDR_EL1, ARCHITECTURE, 16, 4)
 FIELD(MIDR_EL1, VARIANT, 20, 4)
 FIELD(MIDR_EL1, IMPLEMENTER, 24, 8)
+FIELD(MIDR_EL1, RESERVED, 32, 32)
 
 FIELD(ID_ISAR0, SWAP, 0, 4)
 FIELD(ID_ISAR0, BITCOUNT, 4, 4)
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index a79f233b17..aaa48e06ac 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1182,6 +1182,8 @@ void arm_cpu_post_init(Object *obj)
 {
     ARMCPU *cpu = ARM_CPU(obj);
 
+    assert(FIELD_EX64(cpu->midr, MIDR_EL1, RESERVED) == 0);
+
     /* M profile implies PMSA. We have to do this here rather than
      * in realize with the other feature-implication checks because
      * we look at the PMSA bit to see if we should add some properties.
@@ -2757,7 +2759,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 v2] target/arm: Use correct variable for setting 'max' cpu's MIDR_EL1
Posted by Peter Maydell 4 years ago 
On Tue, 28 Apr 2020 at 16:50, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:

Looks like you forgot to edit the commit message subject line.

> 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>
> ---
> I suppose cp15.c0_cpuid register in target/arm/cpu.h as uint32_t is OK.

Yes, that's a 32-bit aarch32 system register.

>
> Since v1: Follow Laurent and Peter suggestion.
> ---
>  target/arm/cpu.h | 3 ++-
>  target/arm/cpu.c | 4 +++-
>  2 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 8b9f2961ba..4d1be56df9 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;
> @@ -1685,6 +1685,7 @@ FIELD(MIDR_EL1, PARTNUM, 4, 12)
>  FIELD(MIDR_EL1, ARCHITECTURE, 16, 4)
>  FIELD(MIDR_EL1, VARIANT, 20, 4)
>  FIELD(MIDR_EL1, IMPLEMENTER, 24, 8)
> +FIELD(MIDR_EL1, RESERVED, 32, 32)
>
>  FIELD(ID_ISAR0, SWAP, 0, 4)
>  FIELD(ID_ISAR0, BITCOUNT, 4, 4)
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index a79f233b17..aaa48e06ac 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -1182,6 +1182,8 @@ void arm_cpu_post_init(Object *obj)
>  {
>      ARMCPU *cpu = ARM_CPU(obj);
>
> +    assert(FIELD_EX64(cpu->midr, MIDR_EL1, RESERVED) == 0);

I wouldn't bother with the assert; we don't generally do that
kind of check on ID register values.

> +
>      /* M profile implies PMSA. We have to do this here rather than
>       * in realize with the other feature-implication checks because
>       * we look at the PMSA bit to see if we should add some properties.
> @@ -2757,7 +2759,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

Otherwise looks good.

-- PMM
Re: [PATCH v2] target/arm: Use correct variable for setting 'max' cpu's MIDR_EL1
Posted by Laurent Desnogues 4 years ago 
On Tue, Apr 28, 2020 at 5:50 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>
> ---
> I suppose cp15.c0_cpuid register in target/arm/cpu.h as uint32_t is OK.
>
> Since v1: Follow Laurent and Peter suggestion.
> ---
>  target/arm/cpu.h | 3 ++-
>  target/arm/cpu.c | 4 +++-
>  2 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 8b9f2961ba..4d1be56df9 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;
> @@ -1685,6 +1685,7 @@ FIELD(MIDR_EL1, PARTNUM, 4, 12)
>  FIELD(MIDR_EL1, ARCHITECTURE, 16, 4)
>  FIELD(MIDR_EL1, VARIANT, 20, 4)
>  FIELD(MIDR_EL1, IMPLEMENTER, 24, 8)
> +FIELD(MIDR_EL1, RESERVED, 32, 32)

If you follow Peter advice to not check these 32-bits, you could
remove that field definition and if you keep it please rename it RES0
:-).

Thanks,

Laurent

>
>  FIELD(ID_ISAR0, SWAP, 0, 4)
>  FIELD(ID_ISAR0, BITCOUNT, 4, 4)
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index a79f233b17..aaa48e06ac 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -1182,6 +1182,8 @@ void arm_cpu_post_init(Object *obj)
>  {
>      ARMCPU *cpu = ARM_CPU(obj);
>
> +    assert(FIELD_EX64(cpu->midr, MIDR_EL1, RESERVED) == 0);
> +
>      /* M profile implies PMSA. We have to do this here rather than
>       * in realize with the other feature-implication checks because
>       * we look at the PMSA bit to see if we should add some properties.
> @@ -2757,7 +2759,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
>

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.