[Qemu-devel] [PATCH] Use Aff1 with mpidr

[Qemu-devel] [PATCH] Use Aff1 with mpidr

[Pavel Fedin]

 This is an improved and KVM-aware alternative to
https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg00942.html. Changes are:
1. MPIDR value (minus feature bits) is now directly stored in CPU instance.
2. Default assignment is based on original rule (8 CPUs per cluster).
3. If KVM is used, MPIDR values are overridden with those from KVM. This is necessary for
proper KVM PSCI functioning.
4. Some cosmetic changes which would make further expansion of this code easier.
5. Added some debugging macros because CPU ID assignment is tricky part, and if there are
some problems, i think there should be a quick way to make sure they are correct.
 This does not have an RFC mark because it is perfectly okay to be committed alone, it
does not break anything. Commit message follows. Cc'ed to all interested parties because i
really hope to get things going somewhere this time.

In order to support up to 128 cores with GIC-500 (GICv3 implementation)
affinity1 must be used. GIC-500 support up to 32 clusters with up to
8 cores in a cluster. So for example, if one wishes to have 16 cores,
the options are: 2 clusters of 8 cores each, 4 clusters with 4 cores each
Currently only the first option is supported for TCG. However, KVM expects
to have the same clusters layout as host machine has. Therefore, if we use
64-bit KVM we override CPU IDs with those provided by the KVM. For 32-bit
systems it is OK to leave the default because so far we do not have more
than 8 CPUs on any of them.
This implementation has a potential to be extended with some methods which
would define cluster layout instead of hardcoded CPUS_PER_CLUSTER
definition. This would allow to emulate real machines with different
layouts. However, in case of KVM we would still have to inherit IDs from
the host.

Signed-off-by: Shlomo Pongratz <shlomo.pongratz@huawei.com>
Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
---
 hw/arm/virt.c        |  6 +++++-
 target-arm/cpu-qom.h |  3 +++
 target-arm/cpu.c     | 17 +++++++++++++++++
 target-arm/helper.c  |  9 +++------
 target-arm/kvm64.c   | 25 +++++++++++++++++++++++++
 target-arm/psci.c    | 20 ++++++++++++++++++--
 6 files changed, 71 insertions(+), 9 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index a7f9a10..a1186c5 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -317,7 +317,11 @@ static void fdt_add_cpu_nodes(const VirtBoardInfo *vbi)
                                         "enable-method", "psci");
         }
 
-        qemu_fdt_setprop_cell(vbi->fdt, nodename, "reg", cpu);
+        /*
+         * If cpus node's #address-cells property is set to 1
+         * The reg cell bits [23:0] must be set to bits [23:0] of MPIDR_EL1.
+         */
+        qemu_fdt_setprop_cell(vbi->fdt, nodename, "reg", armcpu->mpidr);
         g_free(nodename);
     }
 }
diff --git a/target-arm/cpu-qom.h b/target-arm/cpu-qom.h
index ed5a644..a382a09 100644
--- a/target-arm/cpu-qom.h
+++ b/target-arm/cpu-qom.h
@@ -159,6 +159,7 @@ typedef struct ARMCPU {
     uint64_t id_aa64mmfr1;
     uint32_t dbgdidr;
     uint32_t clidr;
+    uint64_t mpidr; /* Without feature bits */
     /* The elements of this array are the CCSIDR values for each cache,
      * in the order L1DCache, L1ICache, L2DCache, L2ICache, etc.
      */
@@ -171,6 +172,8 @@ typedef struct ARMCPU {
     uint64_t rvbar;
 } ARMCPU;
 
+#define CPUS_PER_CLUSTER 8
+
 #define TYPE_AARCH64_CPU "aarch64-cpu"
 #define AARCH64_CPU_CLASS(klass) \
     OBJECT_CLASS_CHECK(AArch64CPUClass, (klass), TYPE_AARCH64_CPU)
diff --git a/target-arm/cpu.c b/target-arm/cpu.c
index 3ca3fa8..7dc2595 100644
--- a/target-arm/cpu.c
+++ b/target-arm/cpu.c
@@ -31,6 +31,12 @@
 #include "sysemu/kvm.h"
 #include "kvm_arm.h"
 
+#ifdef DEBUG
+# define DPRINTF(format, ...) printf("armcpu: " format , ## __VA_ARGS__)
+#else
+# define DPRINTF(format, ...) do { } while (0)
+#endif
+
 static void arm_cpu_set_pc(CPUState *cs, vaddr value)
 {
     ARMCPU *cpu = ARM_CPU(cs);
@@ -367,12 +373,23 @@ static void arm_cpu_initfn(Object *obj)
     CPUState *cs = CPU(obj);
     ARMCPU *cpu = ARM_CPU(obj);
     static bool inited;
+    uint32_t Aff1, Aff0;
 
     cs->env_ptr = &cpu->env;
     cpu_exec_init(&cpu->env);
     cpu->cp_regs = g_hash_table_new_full(g_int_hash, g_int_equal,
                                          g_free, g_free);
 
+    /*
+     * We don't support setting cluster ID ([16..23]) (known as Aff2
+     * in later ARM ARM versions), or any of the higher affinity level fields,
+     * so these bits always RAZ.
+     */
+    Aff1 = cs->cpu_index / CPUS_PER_CLUSTER;
+    Aff0 = cs->cpu_index % CPUS_PER_CLUSTER;
+    cpu->mpidr = (Aff1 << 8) | Aff0;
+    DPRINTF("Init(%p): index %d mpidr 0x%jX\n", cpu, cs->cpu_index, cpu->mpidr);
+
 #ifndef CONFIG_USER_ONLY
     /* Our inbound IRQ and FIQ lines */
     if (kvm_enabled()) {
diff --git a/target-arm/helper.c b/target-arm/helper.c
index 5d0f011..9535290 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -2036,12 +2036,9 @@ static const ARMCPRegInfo strongarm_cp_reginfo[] = {
 
 static uint64_t mpidr_read(CPUARMState *env, const ARMCPRegInfo *ri)
 {
-    CPUState *cs = CPU(arm_env_get_cpu(env));
-    uint32_t mpidr = cs->cpu_index;
-    /* We don't support setting cluster ID ([8..11]) (known as Aff1
-     * in later ARM ARM versions), or any of the higher affinity level fields,
-     * so these bits always RAZ.
-     */
+    ARMCPU *cpu = ARM_CPU(arm_env_get_cpu(env));
+    uint64_t mpidr = cpu->mpidr;
+
     if (arm_feature(env, ARM_FEATURE_V7MP)) {
         mpidr |= (1U << 31);
         /* Cores which are uniprocessor (non-coherent)
diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
index 93c1ca8..99fa34e 100644
--- a/target-arm/kvm64.c
+++ b/target-arm/kvm64.c
@@ -25,6 +25,12 @@
 #include "internals.h"
 #include "hw/arm/arm.h"
 
+#ifdef DEBUG
+# define DPRINTF(format, ...) printf("arm-kvm64: " format , ## __VA_ARGS__)
+#else
+# define DPRINTF(format, ...) do { } while (0)
+#endif
+
 static inline void set_feature(uint64_t *features, int feature)
 {
     *features |= 1ULL << feature;
@@ -77,6 +83,10 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUClass *ahcc)
     return true;
 }
 
+#define ARM_MPIDR_HWID_BITMASK 0xFF00FFFFFFULL
+#define ARM_CPU_ID             3, 0, 0, 0
+#define ARM_CPU_ID_MPIDR       5
+
 int kvm_arch_init_vcpu(CPUState *cs)
 {
     int ret;
@@ -107,6 +117,21 @@ int kvm_arch_init_vcpu(CPUState *cs)
         return ret;
     }
 
+    /*
+     * When KVM is in use, psci is emulated in-kernel and not by qemu.
+     * In order for it to work correctly we should use correct MPIDR values,
+     * which appear to be inherited from the host.
+     * So we override our defaults by asking KVM about these values.
+     */
+    ret = kvm_get_one_reg(cs, ARM64_SYS_REG(ARM_CPU_ID, ARM_CPU_ID_MPIDR),
+                          &cpu->mpidr);
+    if (ret) {
+        return ret;
+    }
+    cpu->mpidr &= ARM_MPIDR_HWID_BITMASK;
+    DPRINTF("Init(%p): index %d MPIDR 0x%jX\n", cpu,
+            cs->cpu_index, cpu->mpidr);
+
     return kvm_arm_init_cpreg_list(cpu);
 }
 
diff --git a/target-arm/psci.c b/target-arm/psci.c
index d8fafab..2905be6 100644
--- a/target-arm/psci.c
+++ b/target-arm/psci.c
@@ -72,6 +72,22 @@ bool arm_is_psci_call(ARMCPU *cpu, int excp_type)
     }
 }
 
+static uint32_t mpidr_to_idx(uint64_t mpidr)
+{
+    uint32_t Aff1, Aff0;
+    
+    /*
+     * MPIDR_EL1 [RES0:affinity-3:RES1:U:RES0:MT:affinity-1:affinity-0]
+     * GIC 500 code currently supports 32 clusters with 8 cores each
+     * but no more than 128 cores. Future version will have flexible
+     * affinity selection
+     * GIC 400 supports 8 cores so 0x7 for Aff0 is O.K. too
+     */
+     Aff1 = (mpidr & 0xff00) >> 8;
+     Aff0 = mpidr & 0xff;
+     return Aff1 * CPUS_PER_CLUSTER + Aff0;
+}
+
 void arm_handle_psci_call(ARMCPU *cpu)
 {
     /*
@@ -121,7 +137,7 @@ void arm_handle_psci_call(ARMCPU *cpu)
 
         switch (param[2]) {
         case 0:
-            target_cpu_state = qemu_get_cpu(mpidr & 0xff);
+            target_cpu_state = qemu_get_cpu(mpidr_to_idx(mpidr));
             if (!target_cpu_state) {
                 ret = QEMU_PSCI_RET_INVALID_PARAMS;
                 break;
@@ -153,7 +169,7 @@ void arm_handle_psci_call(ARMCPU *cpu)
         context_id = param[3];
 
         /* change to the cpu we are powering up */
-        target_cpu_state = qemu_get_cpu(mpidr & 0xff);
+        target_cpu_state = qemu_get_cpu(mpidr_to_idx(mpidr));
         if (!target_cpu_state) {
             ret = QEMU_PSCI_RET_INVALID_PARAMS;
             break;
-- 
1.9.5.msysgit.0

Re: [Qemu-devel] [PATCH] Use Aff1 with mpidr

[Shlomo Pongratz]

Hi,

I see that you added mpidr to ARMCpu and initialized it in virt.c then you use it in mpidr_read.
Thus you must fix all other virtual machines in hw/arm not just virt.c as it is not initialized for them.

I have no other comments.

Best regards,

S.P.


> -----Original Message-----
> From: Pavel Fedin [mailto:p.fedin@samsung.com]
> Sent: Friday, 22 May, 2015 1:33 PM
> To: qemu-devel@nongnu.org
> Cc: peter.maydell@linaro.org; 'Ashok Kumar'; Shlomo Pongratz
> Subject: [PATCH] Use Aff1 with mpidr
> 
>  This is an improved and KVM-aware alternative to
> https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg00942.html.
> Changes are:
> 1. MPIDR value (minus feature bits) is now directly stored in CPU instance.
> 2. Default assignment is based on original rule (8 CPUs per cluster).
> 3. If KVM is used, MPIDR values are overridden with those from KVM. This is
> necessary for proper KVM PSCI functioning.
> 4. Some cosmetic changes which would make further expansion of this code
> easier.
> 5. Added some debugging macros because CPU ID assignment is tricky part,
> and if there are some problems, i think there should be a quick way to make
> sure they are correct.
>  This does not have an RFC mark because it is perfectly okay to be committed
> alone, it does not break anything. Commit message follows. Cc'ed to all
> interested parties because i really hope to get things going somewhere this
> time.
> 
> In order to support up to 128 cores with GIC-500 (GICv3 implementation)
> affinity1 must be used. GIC-500 support up to 32 clusters with up to
> 8 cores in a cluster. So for example, if one wishes to have 16 cores, the
> options are: 2 clusters of 8 cores each, 4 clusters with 4 cores each Currently
> only the first option is supported for TCG. However, KVM expects to have
> the same clusters layout as host machine has. Therefore, if we use 64-bit
> KVM we override CPU IDs with those provided by the KVM. For 32-bit
> systems it is OK to leave the default because so far we do not have more
> than 8 CPUs on any of them.
> This implementation has a potential to be extended with some methods
> which would define cluster layout instead of hardcoded CPUS_PER_CLUSTER
> definition. This would allow to emulate real machines with different layouts.
> However, in case of KVM we would still have to inherit IDs from the host.
> 
> Signed-off-by: Shlomo Pongratz <shlomo.pongratz@huawei.com>
> Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
> ---
>  hw/arm/virt.c        |  6 +++++-
>  target-arm/cpu-qom.h |  3 +++
>  target-arm/cpu.c     | 17 +++++++++++++++++
>  target-arm/helper.c  |  9 +++------
>  target-arm/kvm64.c   | 25 +++++++++++++++++++++++++
>  target-arm/psci.c    | 20 ++++++++++++++++++--
>  6 files changed, 71 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c index a7f9a10..a1186c5 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -317,7 +317,11 @@ static void fdt_add_cpu_nodes(const VirtBoardInfo
> *vbi)
>                                          "enable-method", "psci");
>          }
> 
> -        qemu_fdt_setprop_cell(vbi->fdt, nodename, "reg", cpu);
> +        /*
> +         * If cpus node's #address-cells property is set to 1
> +         * The reg cell bits [23:0] must be set to bits [23:0] of MPIDR_EL1.
> +         */
> +        qemu_fdt_setprop_cell(vbi->fdt, nodename, "reg",
> + armcpu->mpidr);
>          g_free(nodename);
>      }
>  }
> diff --git a/target-arm/cpu-qom.h b/target-arm/cpu-qom.h index
> ed5a644..a382a09 100644
> --- a/target-arm/cpu-qom.h
> +++ b/target-arm/cpu-qom.h
> @@ -159,6 +159,7 @@ typedef struct ARMCPU {
>      uint64_t id_aa64mmfr1;
>      uint32_t dbgdidr;
>      uint32_t clidr;
> +    uint64_t mpidr; /* Without feature bits */
>      /* The elements of this array are the CCSIDR values for each cache,
>       * in the order L1DCache, L1ICache, L2DCache, L2ICache, etc.
>       */
> @@ -171,6 +172,8 @@ typedef struct ARMCPU {
>      uint64_t rvbar;
>  } ARMCPU;
> 
> +#define CPUS_PER_CLUSTER 8
> +
>  #define TYPE_AARCH64_CPU "aarch64-cpu"
>  #define AARCH64_CPU_CLASS(klass) \
>      OBJECT_CLASS_CHECK(AArch64CPUClass, (klass), TYPE_AARCH64_CPU)
> diff --git a/target-arm/cpu.c b/target-arm/cpu.c index 3ca3fa8..7dc2595
> 100644
> --- a/target-arm/cpu.c
> +++ b/target-arm/cpu.c
> @@ -31,6 +31,12 @@
>  #include "sysemu/kvm.h"
>  #include "kvm_arm.h"
> 
> +#ifdef DEBUG
> +# define DPRINTF(format, ...) printf("armcpu: " format , ##
> +__VA_ARGS__) #else # define DPRINTF(format, ...) do { } while (0)
> +#endif
> +
>  static void arm_cpu_set_pc(CPUState *cs, vaddr value)  {
>      ARMCPU *cpu = ARM_CPU(cs);
> @@ -367,12 +373,23 @@ static void arm_cpu_initfn(Object *obj)
>      CPUState *cs = CPU(obj);
>      ARMCPU *cpu = ARM_CPU(obj);
>      static bool inited;
> +    uint32_t Aff1, Aff0;
> 
>      cs->env_ptr = &cpu->env;
>      cpu_exec_init(&cpu->env);
>      cpu->cp_regs = g_hash_table_new_full(g_int_hash, g_int_equal,
>                                           g_free, g_free);
> 
> +    /*
> +     * We don't support setting cluster ID ([16..23]) (known as Aff2
> +     * in later ARM ARM versions), or any of the higher affinity level fields,
> +     * so these bits always RAZ.
> +     */
> +    Aff1 = cs->cpu_index / CPUS_PER_CLUSTER;
> +    Aff0 = cs->cpu_index % CPUS_PER_CLUSTER;
> +    cpu->mpidr = (Aff1 << 8) | Aff0;
> +    DPRINTF("Init(%p): index %d mpidr 0x%jX\n", cpu, cs->cpu_index,
> + cpu->mpidr);
> +
>  #ifndef CONFIG_USER_ONLY
>      /* Our inbound IRQ and FIQ lines */
>      if (kvm_enabled()) {
> diff --git a/target-arm/helper.c b/target-arm/helper.c index
> 5d0f011..9535290 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -2036,12 +2036,9 @@ static const ARMCPRegInfo
> strongarm_cp_reginfo[] = {
> 
>  static uint64_t mpidr_read(CPUARMState *env, const ARMCPRegInfo *ri)  {
> -    CPUState *cs = CPU(arm_env_get_cpu(env));
> -    uint32_t mpidr = cs->cpu_index;
> -    /* We don't support setting cluster ID ([8..11]) (known as Aff1
> -     * in later ARM ARM versions), or any of the higher affinity level fields,
> -     * so these bits always RAZ.
> -     */
> +    ARMCPU *cpu = ARM_CPU(arm_env_get_cpu(env));
> +    uint64_t mpidr = cpu->mpidr;
> +
>      if (arm_feature(env, ARM_FEATURE_V7MP)) {
>          mpidr |= (1U << 31);
>          /* Cores which are uniprocessor (non-coherent) diff --git a/target-
> arm/kvm64.c b/target-arm/kvm64.c index 93c1ca8..99fa34e 100644
> --- a/target-arm/kvm64.c
> +++ b/target-arm/kvm64.c
> @@ -25,6 +25,12 @@
>  #include "internals.h"
>  #include "hw/arm/arm.h"
> 
> +#ifdef DEBUG
> +# define DPRINTF(format, ...) printf("arm-kvm64: " format , ##
> +__VA_ARGS__) #else # define DPRINTF(format, ...) do { } while (0)
> +#endif
> +
>  static inline void set_feature(uint64_t *features, int feature)  {
>      *features |= 1ULL << feature;
> @@ -77,6 +83,10 @@ bool
> kvm_arm_get_host_cpu_features(ARMHostCPUClass *ahcc)
>      return true;
>  }
> 
> +#define ARM_MPIDR_HWID_BITMASK 0xFF00FFFFFFULL
> +#define ARM_CPU_ID             3, 0, 0, 0
> +#define ARM_CPU_ID_MPIDR       5
> +
>  int kvm_arch_init_vcpu(CPUState *cs)
>  {
>      int ret;
> @@ -107,6 +117,21 @@ int kvm_arch_init_vcpu(CPUState *cs)
>          return ret;
>      }
> 
> +    /*
> +     * When KVM is in use, psci is emulated in-kernel and not by qemu.
> +     * In order for it to work correctly we should use correct MPIDR values,
> +     * which appear to be inherited from the host.
> +     * So we override our defaults by asking KVM about these values.
> +     */
> +    ret = kvm_get_one_reg(cs, ARM64_SYS_REG(ARM_CPU_ID,
> ARM_CPU_ID_MPIDR),
> +                          &cpu->mpidr);
> +    if (ret) {
> +        return ret;
> +    }
> +    cpu->mpidr &= ARM_MPIDR_HWID_BITMASK;
> +    DPRINTF("Init(%p): index %d MPIDR 0x%jX\n", cpu,
> +            cs->cpu_index, cpu->mpidr);
> +
>      return kvm_arm_init_cpreg_list(cpu);  }
> 
> diff --git a/target-arm/psci.c b/target-arm/psci.c index d8fafab..2905be6
> 100644
> --- a/target-arm/psci.c
> +++ b/target-arm/psci.c
> @@ -72,6 +72,22 @@ bool arm_is_psci_call(ARMCPU *cpu, int excp_type)
>      }
>  }
> 
> +static uint32_t mpidr_to_idx(uint64_t mpidr) {
> +    uint32_t Aff1, Aff0;
> +
> +    /*
> +     * MPIDR_EL1 [RES0:affinity-3:RES1:U:RES0:MT:affinity-1:affinity-0]
> +     * GIC 500 code currently supports 32 clusters with 8 cores each
> +     * but no more than 128 cores. Future version will have flexible
> +     * affinity selection
> +     * GIC 400 supports 8 cores so 0x7 for Aff0 is O.K. too
> +     */
> +     Aff1 = (mpidr & 0xff00) >> 8;
> +     Aff0 = mpidr & 0xff;
> +     return Aff1 * CPUS_PER_CLUSTER + Aff0; }
> +
>  void arm_handle_psci_call(ARMCPU *cpu)
>  {
>      /*
> @@ -121,7 +137,7 @@ void arm_handle_psci_call(ARMCPU *cpu)
> 
>          switch (param[2]) {
>          case 0:
> -            target_cpu_state = qemu_get_cpu(mpidr & 0xff);
> +            target_cpu_state = qemu_get_cpu(mpidr_to_idx(mpidr));
>              if (!target_cpu_state) {
>                  ret = QEMU_PSCI_RET_INVALID_PARAMS;
>                  break;
> @@ -153,7 +169,7 @@ void arm_handle_psci_call(ARMCPU *cpu)
>          context_id = param[3];
> 
>          /* change to the cpu we are powering up */
> -        target_cpu_state = qemu_get_cpu(mpidr & 0xff);
> +        target_cpu_state = qemu_get_cpu(mpidr_to_idx(mpidr));
>          if (!target_cpu_state) {
>              ret = QEMU_PSCI_RET_INVALID_PARAMS;
>              break;
> --
> 1.9.5.msysgit.0
> 

Re: [Qemu-devel] [PATCH] Use Aff1 with mpidr

[Pavel Fedin]

 Hi!

> I see that you added mpidr to ARMCpu and initialized it in virt.c then you use it in
mpidr_read.
> Thus you must fix all other virtual machines in hw/arm not just virt.c as it is not
initialized for
> them.

 The only change in virt.c is:
--- cut ---
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index a7f9a10..a1186c5 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -317,7 +317,11 @@ static void fdt_add_cpu_nodes(const VirtBoardInfo *vbi)
                                         "enable-method", "psci");
         }
 
-        qemu_fdt_setprop_cell(vbi->fdt, nodename, "reg", cpu);
+        /*
+         * If cpus node's #address-cells property is set to 1
+         * The reg cell bits [23:0] must be set to bits [23:0] of MPIDR_EL1.
+         */
+        qemu_fdt_setprop_cell(vbi->fdt, nodename, "reg", armcpu->mpidr);
         g_free(nodename);
     }
 }
--- cut ---
 So that it takes MPIDR instead of just CPU index. Theoretically - yes, may be other
machines should also be changed, but:
1. They are 32-bit, so MPIDR == index for them, because there are no more than 8 CPUs.
2. Those machines AFAIK do not compose device tree by themselves. They use pre-made ones
instead, coming for example with kernel.
 Only virt currently can be a 64-bit machine and cares about more than 8 CPUs.
 As to MPIDR initialization, it is done in arm_cpu_initfn(), therefore all ARM CPUs get
this automatically. There's no need to modify code for every machine.
 I would kindly ask you to use my patch in your next series, or base something on it, if
you dislike the implementation, but it's crucial for KVM that MPIDR values can be obtained
from the host. Your original implementation cannot do this by design, this is why i made
my own solution. See kvm_arch_init_vcpu() in my patch - without this CPUs beyond #7 will
not power up in KVM.
 And when are you planning to post v3? I am waiting for it to be integrated, in order to
add KVM vGICv3. Otherwise i'm afraid there are too many collisions.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

Re: [Qemu-devel] [PATCH] Use Aff1 with mpidr

[Igor Mammedov]

On Fri, 22 May 2015 13:32:54 +0300
Pavel Fedin <p.fedin@samsung.com> wrote:

>  This is an improved and KVM-aware alternative to
> https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg00942.html. Changes are:
> 1. MPIDR value (minus feature bits) is now directly stored in CPU instance.
> 2. Default assignment is based on original rule (8 CPUs per cluster).
> 3. If KVM is used, MPIDR values are overridden with those from KVM. This is necessary for
> proper KVM PSCI functioning.
> 4. Some cosmetic changes which would make further expansion of this code easier.
> 5. Added some debugging macros because CPU ID assignment is tricky part, and if there are
> some problems, i think there should be a quick way to make sure they are correct.
>  This does not have an RFC mark because it is perfectly okay to be committed alone, it
> does not break anything. Commit message follows. Cc'ed to all interested parties because i
> really hope to get things going somewhere this time.
> 
> In order to support up to 128 cores with GIC-500 (GICv3 implementation)
> affinity1 must be used. GIC-500 support up to 32 clusters with up to
> 8 cores in a cluster. So for example, if one wishes to have 16 cores,
> the options are: 2 clusters of 8 cores each, 4 clusters with 4 cores each
> Currently only the first option is supported for TCG. However, KVM expects
> to have the same clusters layout as host machine has. Therefore, if we use
> 64-bit KVM we override CPU IDs with those provided by the KVM. For 32-bit
> systems it is OK to leave the default because so far we do not have more
> than 8 CPUs on any of them.
> This implementation has a potential to be extended with some methods which
> would define cluster layout instead of hardcoded CPUS_PER_CLUSTER
> definition. This would allow to emulate real machines with different
> layouts. However, in case of KVM we would still have to inherit IDs from
> the host.
> 
> Signed-off-by: Shlomo Pongratz <shlomo.pongratz@huawei.com>
> Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
> ---
>  hw/arm/virt.c        |  6 +++++-
>  target-arm/cpu-qom.h |  3 +++
>  target-arm/cpu.c     | 17 +++++++++++++++++
>  target-arm/helper.c  |  9 +++------
>  target-arm/kvm64.c   | 25 +++++++++++++++++++++++++
>  target-arm/psci.c    | 20 ++++++++++++++++++--
>  6 files changed, 71 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index a7f9a10..a1186c5 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -317,7 +317,11 @@ static void fdt_add_cpu_nodes(const VirtBoardInfo *vbi)
>                                          "enable-method", "psci");
>          }
>  
> -        qemu_fdt_setprop_cell(vbi->fdt, nodename, "reg", cpu);
> +        /*
> +         * If cpus node's #address-cells property is set to 1
> +         * The reg cell bits [23:0] must be set to bits [23:0] of MPIDR_EL1.
> +         */
> +        qemu_fdt_setprop_cell(vbi->fdt, nodename, "reg", armcpu->mpidr);
>          g_free(nodename);
>      }
>  }
> diff --git a/target-arm/cpu-qom.h b/target-arm/cpu-qom.h
> index ed5a644..a382a09 100644
> --- a/target-arm/cpu-qom.h
> +++ b/target-arm/cpu-qom.h
> @@ -159,6 +159,7 @@ typedef struct ARMCPU {
>      uint64_t id_aa64mmfr1;
>      uint32_t dbgdidr;
>      uint32_t clidr;
> +    uint64_t mpidr; /* Without feature bits */
>      /* The elements of this array are the CCSIDR values for each cache,
>       * in the order L1DCache, L1ICache, L2DCache, L2ICache, etc.
>       */
> @@ -171,6 +172,8 @@ typedef struct ARMCPU {
>      uint64_t rvbar;
>  } ARMCPU;
>  
> +#define CPUS_PER_CLUSTER 8
> +
>  #define TYPE_AARCH64_CPU "aarch64-cpu"
>  #define AARCH64_CPU_CLASS(klass) \
>      OBJECT_CLASS_CHECK(AArch64CPUClass, (klass), TYPE_AARCH64_CPU)
> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> index 3ca3fa8..7dc2595 100644
> --- a/target-arm/cpu.c
> +++ b/target-arm/cpu.c
> @@ -31,6 +31,12 @@
>  #include "sysemu/kvm.h"
>  #include "kvm_arm.h"
>  
> +#ifdef DEBUG
> +# define DPRINTF(format, ...) printf("armcpu: " format , ## __VA_ARGS__)
> +#else
> +# define DPRINTF(format, ...) do { } while (0)
> +#endif
> +
>  static void arm_cpu_set_pc(CPUState *cs, vaddr value)
>  {
>      ARMCPU *cpu = ARM_CPU(cs);
> @@ -367,12 +373,23 @@ static void arm_cpu_initfn(Object *obj)
>      CPUState *cs = CPU(obj);
>      ARMCPU *cpu = ARM_CPU(obj);
>      static bool inited;
> +    uint32_t Aff1, Aff0;
>  
>      cs->env_ptr = &cpu->env;
>      cpu_exec_init(&cpu->env);
>      cpu->cp_regs = g_hash_table_new_full(g_int_hash, g_int_equal,
>                                           g_free, g_free);
>  
> +    /*
> +     * We don't support setting cluster ID ([16..23]) (known as Aff2
> +     * in later ARM ARM versions), or any of the higher affinity level fields,
> +     * so these bits always RAZ.
> +     */
> +    Aff1 = cs->cpu_index / CPUS_PER_CLUSTER;
> +    Aff0 = cs->cpu_index % CPUS_PER_CLUSTER;
> +    cpu->mpidr = (Aff1 << 8) | Aff0;
> +    DPRINTF("Init(%p): index %d mpidr 0x%jX\n", cpu, cs->cpu_index, cpu->mpidr);
> +
>  #ifndef CONFIG_USER_ONLY
>      /* Our inbound IRQ and FIQ lines */
>      if (kvm_enabled()) {
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 5d0f011..9535290 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -2036,12 +2036,9 @@ static const ARMCPRegInfo strongarm_cp_reginfo[] = {
>  
>  static uint64_t mpidr_read(CPUARMState *env, const ARMCPRegInfo *ri)
>  {
> -    CPUState *cs = CPU(arm_env_get_cpu(env));
> -    uint32_t mpidr = cs->cpu_index;
> -    /* We don't support setting cluster ID ([8..11]) (known as Aff1
> -     * in later ARM ARM versions), or any of the higher affinity level fields,
> -     * so these bits always RAZ.
> -     */
> +    ARMCPU *cpu = ARM_CPU(arm_env_get_cpu(env));
> +    uint64_t mpidr = cpu->mpidr;
> +
>      if (arm_feature(env, ARM_FEATURE_V7MP)) {
>          mpidr |= (1U << 31);
>          /* Cores which are uniprocessor (non-coherent)
> diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
> index 93c1ca8..99fa34e 100644
> --- a/target-arm/kvm64.c
> +++ b/target-arm/kvm64.c
> @@ -25,6 +25,12 @@
>  #include "internals.h"
>  #include "hw/arm/arm.h"
>  
> +#ifdef DEBUG
> +# define DPRINTF(format, ...) printf("arm-kvm64: " format , ## __VA_ARGS__)
> +#else
> +# define DPRINTF(format, ...) do { } while (0)
> +#endif
> +
>  static inline void set_feature(uint64_t *features, int feature)
>  {
>      *features |= 1ULL << feature;
> @@ -77,6 +83,10 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUClass *ahcc)
>      return true;
>  }
>  
> +#define ARM_MPIDR_HWID_BITMASK 0xFF00FFFFFFULL
> +#define ARM_CPU_ID             3, 0, 0, 0
> +#define ARM_CPU_ID_MPIDR       5
> +
>  int kvm_arch_init_vcpu(CPUState *cs)
>  {
>      int ret;
> @@ -107,6 +117,21 @@ int kvm_arch_init_vcpu(CPUState *cs)
>          return ret;
>      }
>  
> +    /*
> +     * When KVM is in use, psci is emulated in-kernel and not by qemu.
> +     * In order for it to work correctly we should use correct MPIDR values,
> +     * which appear to be inherited from the host.
why it must be inherited from host?
Could you point out to KVM code that depends on it?

> +     * So we override our defaults by asking KVM about these values.
> +     */
> +    ret = kvm_get_one_reg(cs, ARM64_SYS_REG(ARM_CPU_ID, ARM_CPU_ID_MPIDR),
> +                          &cpu->mpidr);
> +    if (ret) {
> +        return ret;
> +    }
> +    cpu->mpidr &= ARM_MPIDR_HWID_BITMASK;
> +    DPRINTF("Init(%p): index %d MPIDR 0x%jX\n", cpu,
> +            cs->cpu_index, cpu->mpidr);
> +
>      return kvm_arm_init_cpreg_list(cpu);
>  }
>  
> diff --git a/target-arm/psci.c b/target-arm/psci.c
> index d8fafab..2905be6 100644
> --- a/target-arm/psci.c
> +++ b/target-arm/psci.c
> @@ -72,6 +72,22 @@ bool arm_is_psci_call(ARMCPU *cpu, int excp_type)
>      }
>  }
>  
> +static uint32_t mpidr_to_idx(uint64_t mpidr)
> +{
> +    uint32_t Aff1, Aff0;
> +    
> +    /*
> +     * MPIDR_EL1 [RES0:affinity-3:RES1:U:RES0:MT:affinity-1:affinity-0]
> +     * GIC 500 code currently supports 32 clusters with 8 cores each
> +     * but no more than 128 cores. Future version will have flexible
> +     * affinity selection
> +     * GIC 400 supports 8 cores so 0x7 for Aff0 is O.K. too
> +     */
> +     Aff1 = (mpidr & 0xff00) >> 8;
> +     Aff0 = mpidr & 0xff;
> +     return Aff1 * CPUS_PER_CLUSTER + Aff0;
> +}
> +
>  void arm_handle_psci_call(ARMCPU *cpu)
>  {
>      /*
> @@ -121,7 +137,7 @@ void arm_handle_psci_call(ARMCPU *cpu)
>  
>          switch (param[2]) {
>          case 0:
> -            target_cpu_state = qemu_get_cpu(mpidr & 0xff);
> +            target_cpu_state = qemu_get_cpu(mpidr_to_idx(mpidr));
>              if (!target_cpu_state) {
>                  ret = QEMU_PSCI_RET_INVALID_PARAMS;
>                  break;
> @@ -153,7 +169,7 @@ void arm_handle_psci_call(ARMCPU *cpu)
>          context_id = param[3];
>  
>          /* change to the cpu we are powering up */
> -        target_cpu_state = qemu_get_cpu(mpidr & 0xff);
> +        target_cpu_state = qemu_get_cpu(mpidr_to_idx(mpidr));
>          if (!target_cpu_state) {
>              ret = QEMU_PSCI_RET_INVALID_PARAMS;
>              break;

Re: [Qemu-devel] [PATCH] Use Aff1 with mpidr

[Pavel Fedin]

 Hello!

> > +    /*
> > +     * When KVM is in use, psci is emulated in-kernel and not by qemu.
> > +     * In order for it to work correctly we should use correct MPIDR values,
> > +     * which appear to be inherited from the host.
> why it must be inherited from host?
> Could you point out to KVM code that depends on it?

 It is PSCI, i have tested this and experienced this problem. In KVM guest PSCI is handled
by "hvc" call and is done entirely in kernel, no qemu code is involved. If i supply wrong
IDs the processors will simply not power up.
 I have checked how kvmtool (found here: git://linux-arm.org/linux-ap.git, tools/kvm)
handles this. Related parts are:
tools/kvm/arm/fdt.c - generate_cpu_nodes() - "reg" property assignment:
--- cut ---
static void generate_cpu_nodes(void *fdt, struct kvm *kvm)
{
	int cpu;

	_FDT(fdt_begin_node(fdt, "cpus"));
	_FDT(fdt_property_cell(fdt, "#address-cells", 0x1));
	_FDT(fdt_property_cell(fdt, "#size-cells", 0x0));

	for (cpu = 0; cpu < kvm->nrcpus; ++cpu) {
		char cpu_name[CPU_NAME_MAX_LEN];
		struct kvm_cpu *vcpu = kvm->cpus[cpu];
		unsigned long mpidr = kvm_cpu__get_vcpu_mpidr(vcpu);

		mpidr &= ARM_MPIDR_HWID_BITMASK;
		snprintf(cpu_name, CPU_NAME_MAX_LEN, "cpu@%lx", mpidr);

		_FDT(fdt_begin_node(fdt, cpu_name));
		_FDT(fdt_property_string(fdt, "device_type", "cpu"));
		_FDT(fdt_property_string(fdt, "compatible", vcpu->cpu_compatible));

		if (kvm->nrcpus > 1)
			_FDT(fdt_property_string(fdt, "enable-method", "psci"));

		_FDT(fdt_property_cell(fdt, "reg", mpidr));
		_FDT(fdt_end_node(fdt));
	}

	_FDT(fdt_end_node(fdt));
}
--- cut ---
 tools/kvm/arm/aarch64/kvm-cpu.c - kvm_cpu__get_vcpu_mpidr() - obtains CPU ID from the
kernel:
--- cut ---
unsigned long kvm_cpu__get_vcpu_mpidr(struct kvm_cpu *vcpu)
{
	struct kvm_one_reg reg;
	u64 mpidr;

	reg.id = ARM64_SYS_REG(ARM_CPU_ID, ARM_CPU_ID_MPIDR);
	reg.addr = (u64)&mpidr;
	if (ioctl(vcpu->vcpu_fd, KVM_GET_ONE_REG, &reg) < 0)
		die("KVM_GET_ONE_REG failed (get_mpidr vcpu%ld", vcpu->cpu_id);

	return mpidr;
}
--- cut ---

 So i just decided to do the same thing in KVM. However, to tell the truth, i actually do
not know whether it is possible to do it in reverse and assign MPIDR to VCPUs using
KVM_SET_ONE_REG ioctl instead. I can try it if you think it's more appropriate.
 
Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

Re: [Qemu-devel] [PATCH] Use Aff1 with mpidr

[Igor Mammedov]

On Fri, 29 May 2015 11:53:38 +0300
Pavel Fedin <p.fedin@samsung.com> wrote:

>  Hello!
> 
> > > +    /*
> > > +     * When KVM is in use, psci is emulated in-kernel and not by qemu.
> > > +     * In order for it to work correctly we should use correct MPIDR values,
> > > +     * which appear to be inherited from the host.
> > why it must be inherited from host?
> > Could you point out to KVM code that depends on it?
> 
>  It is PSCI, i have tested this and experienced this problem. In KVM guest PSCI is handled
> by "hvc" call and is done entirely in kernel, no qemu code is involved. If i supply wrong
> IDs the processors will simply not power up.
>  I have checked how kvmtool (found here: git://linux-arm.org/linux-ap.git, tools/kvm)
> handles this. Related parts are:
> tools/kvm/arm/fdt.c - generate_cpu_nodes() - "reg" property assignment:
> --- cut ---
> static void generate_cpu_nodes(void *fdt, struct kvm *kvm)
> {
> 	int cpu;
> 
> 	_FDT(fdt_begin_node(fdt, "cpus"));
> 	_FDT(fdt_property_cell(fdt, "#address-cells", 0x1));
> 	_FDT(fdt_property_cell(fdt, "#size-cells", 0x0));
> 
> 	for (cpu = 0; cpu < kvm->nrcpus; ++cpu) {
> 		char cpu_name[CPU_NAME_MAX_LEN];
> 		struct kvm_cpu *vcpu = kvm->cpus[cpu];
> 		unsigned long mpidr = kvm_cpu__get_vcpu_mpidr(vcpu);
> 
> 		mpidr &= ARM_MPIDR_HWID_BITMASK;
> 		snprintf(cpu_name, CPU_NAME_MAX_LEN, "cpu@%lx", mpidr);
> 
> 		_FDT(fdt_begin_node(fdt, cpu_name));
> 		_FDT(fdt_property_string(fdt, "device_type", "cpu"));
> 		_FDT(fdt_property_string(fdt, "compatible", vcpu->cpu_compatible));
> 
> 		if (kvm->nrcpus > 1)
> 			_FDT(fdt_property_string(fdt, "enable-method", "psci"));
> 
> 		_FDT(fdt_property_cell(fdt, "reg", mpidr));
> 		_FDT(fdt_end_node(fdt));
> 	}
> 
> 	_FDT(fdt_end_node(fdt));
> }
> --- cut ---
>  tools/kvm/arm/aarch64/kvm-cpu.c - kvm_cpu__get_vcpu_mpidr() - obtains CPU ID from the
> kernel:
> --- cut ---
> unsigned long kvm_cpu__get_vcpu_mpidr(struct kvm_cpu *vcpu)
> {
> 	struct kvm_one_reg reg;
> 	u64 mpidr;
> 
> 	reg.id = ARM64_SYS_REG(ARM_CPU_ID, ARM_CPU_ID_MPIDR);
> 	reg.addr = (u64)&mpidr;
> 	if (ioctl(vcpu->vcpu_fd, KVM_GET_ONE_REG, &reg) < 0)
> 		die("KVM_GET_ONE_REG failed (get_mpidr vcpu%ld", vcpu->cpu_id);
> 
> 	return mpidr;
> }
> --- cut ---
> 
>  So i just decided to do the same thing in KVM. However, to tell the truth, i actually do
> not know whether it is possible to do it in reverse and assign MPIDR to VCPUs using
> KVM_SET_ONE_REG ioctl instead. I can try it if you think it's more appropriate.
That's what I see as correct way to go
and along the way teach KVM to not derive mpidr encoding from vcpuid
and use QEMU provided mpidr value, that way mpidr would be consistent.

As it currently stands QEMU notion of mpidr and KVM's will diverge
onece CPU count goes above 16 CPUs.

>  
> Kind regards,
> Pavel Fedin
> Expert Engineer
> Samsung Electronics Research center Russia
> 
> 

Re: [Qemu-devel] [PATCH] Use Aff1 with mpidr

[Pavel Fedin]

 Hello!

> That's what I see as correct way to go
> and along the way teach KVM to not derive mpidr encoding from vcpuid
> and use QEMU provided mpidr value, that way mpidr would be consistent.

 Yes, and this would make modelling of real hardware working much better. However,
unfortunately, i have just tested it, and it does not work. MPIDR can be set for the vCPU,
and i believe it will even give back the value when read. But PSCI completely ignores this
setting.

> As it currently stands QEMU notion of mpidr and KVM's will diverge
> onece CPU count goes above 16 CPUs.

 Yes, but fortunately the code which cares about it (PSCI and Shlomo's GICv3 software
emulation) is not used with KVM. So i think we have no other choice if we want to be
compatible with current KVM APIs.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

Re: [Qemu-devel] [PATCH] Use Aff1 with mpidr

[Igor Mammedov]

On Fri, 29 May 2015 15:26:57 +0300
Pavel Fedin <p.fedin@samsung.com> wrote:

>  Hello!
> 
> > That's what I see as correct way to go
> > and along the way teach KVM to not derive mpidr encoding from vcpuid
> > and use QEMU provided mpidr value, that way mpidr would be consistent.
> 
>  Yes, and this would make modelling of real hardware working much better. However,
> unfortunately, i have just tested it, and it does not work. MPIDR can be set for the vCPU,
> and i believe it will even give back the value when read. But PSCI completely ignores this
> setting.
> 
> > As it currently stands QEMU notion of mpidr and KVM's will diverge
> > onece CPU count goes above 16 CPUs.
> 
>  Yes, but fortunately the code which cares about it (PSCI and Shlomo's GICv3 software
> emulation) is not used with KVM. So i think we have no other choice if we want to be
> compatible with current KVM APIs.
Well KVM side should be fixed instead of driving us along wrong route.

> 
> Kind regards,
> Pavel Fedin
> Expert Engineer
> Samsung Electronics Research center Russia
> 
> 

Re: [Qemu-devel] [PATCH] Use Aff1 with mpidr

[Pavel Fedin]

 Hello!

> Well KVM side should be fixed instead of driving us along wrong route.

 It can be fixed... Perhaps... If kernel developers acknowledge this is a problem, which
might not happen.
 But still we will have older kernels, and what? Don't you want 64-bit ARM KVM work on
them?

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

Re: [Qemu-devel] [PATCH] Use Aff1 with mpidr

[Pavel Fedin]

 Hi!

> Well KVM side should be fixed instead of driving us along wrong route.

 I have studied the question a bit more, and i discovered that MPIDR access on ARM is not
trapped by KVM. And guest would always get the same value as host would. Theoretically you
could modify the kernel so that PSCI accepts IDs set by qemu, but in this case we would
have inconsistency between device tree and real IDs. And there seems to be no way round.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

Re: [Qemu-devel] [PATCH] Use Aff1 with mpidr

[Shannon Zhao]

On 2015/5/30 1:37, Pavel Fedin wrote:
>  Hi!
> 
>> Well KVM side should be fixed instead of driving us along wrong route.
> 
>  I have studied the question a bit more, and i discovered that MPIDR access on ARM is not
> trapped by KVM. And guest would always get the same value as host would. Theoretically you

Yes, it doesn't trap but there is one register "vmpidr_el2" which is
used for virtualization. When guest reads mpidr, it will get the value
of vmpidr_el2. And when context switching, hyp will restore the value of
vmpidr_el2 and the value is got from MPIDR_EL1 which is set by
reset_mpidr().

hyp.s:

.macro restore_sysregs // x2: base address for cpu context
	// x3: tmp register
	add	x3, x2, #CPU_SYSREG_OFFSET(MPIDR_EL1)
----cut----
	msr	vmpidr_el2,	x4
----cut----
.endm


-- 
Shannon

Re: [Qemu-devel] [PATCH] Use Aff1 with mpidr

[Shlomo Pongratz]

Hi,

See below.

> -----Original Message-----
> From: Pavel Fedin [mailto:p.fedin@samsung.com]
> Sent: Friday, 29 May, 2015 9:45 AM
> To: Shlomo Pongratz; qemu-devel@nongnu.org
> Cc: peter.maydell@linaro.org; 'Ashok Kumar'
> Subject: RE: [PATCH] Use Aff1 with mpidr
> 
>  Hi!
> 
> > I see that you added mpidr to ARMCpu and initialized it in virt.c then
> > you use it in
> mpidr_read.
> > Thus you must fix all other virtual machines in hw/arm not just virt.c
> > as it is not
> initialized for
> > them.
> 
>  The only change in virt.c is:
> --- cut ---
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c index a7f9a10..a1186c5 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -317,7 +317,11 @@ static void fdt_add_cpu_nodes(const VirtBoardInfo
> *vbi)
>                                          "enable-method", "psci");
>          }
> 
> -        qemu_fdt_setprop_cell(vbi->fdt, nodename, "reg", cpu);
> +        /*
> +         * If cpus node's #address-cells property is set to 1
> +         * The reg cell bits [23:0] must be set to bits [23:0] of MPIDR_EL1.
> +         */
> +        qemu_fdt_setprop_cell(vbi->fdt, nodename, "reg",
> + armcpu->mpidr);
>          g_free(nodename);
>      }
>  }
> --- cut ---
>  So that it takes MPIDR instead of just CPU index. Theoretically - yes, may be
> other machines should also be changed, but:
> 1. They are 32-bit, so MPIDR == index for them, because there are no more
> than 8 CPUs.
> 2. Those machines AFAIK do not compose device tree by themselves. They
> use pre-made ones instead, coming for example with kernel.
>  Only virt currently can be a 64-bit machine and cares about more than 8
> CPUs.
>  As to MPIDR initialization, it is done in arm_cpu_initfn(), therefore all ARM
> CPUs get this automatically. There's no need to modify code for every
> machine.

I think we should take Igor's comment into account. The CPUS_PER_CLUSTER should not be a constant, and maybe should be initialized in arm_cpu_initfn and aarch64_{a53|a57}_initfn, as psci need to know it.

>  I would kindly ask you to use my patch in your next series, or base
> something on it, if you dislike the implementation, but it's crucial for KVM
> that MPIDR values can be obtained from the host. Your original
> implementation cannot do this by design, this is why i made my own solution.
> See kvm_arch_init_vcpu() in my patch - without this CPUs beyond #7 will not
> power up in KVM.

I understand that you suggest that I'll not use my 1 & 4 patches. I have no problem with that, but how do I commit? Should I write that the patch series is pending until your patch series is integrated?

>  And when are you planning to post v3? I am waiting for it to be integrated, in
> order to add KVM vGICv3. Otherwise i'm afraid there are too many collisions.

I intend to release the V3 version during this week.
I have some issues with the new groups code that was introduce to GICv2 but they are minor.

> 
> Kind regards,
> Pavel Fedin
> Expert Engineer
> Samsung Electronics Research center Russia
> 

Re: [Qemu-devel] [PATCH] Use Aff1 with mpidr

[Pavel Fedin]

 Hi!

> I think we should take Igor's comment into account. The CPUS_PER_CLUSTER should not be a
> constant, and maybe should be initialized in arm_cpu_initfn and
aarch64_{a53|a57}_initfn, as
> psci need to know it.

 Yes, of course. Just replace a #define with something like:

 int cpus_per_cluster = 8;

 and you're done with this. This provides good backwards compatibility as well as
possibility to change this on a per-machine basis.

> I understand that you suggest that I'll not use my 1 & 4 patches. I have no problem with
that,
> but how do I commit? Should I write that the patch series is pending until your patch
series is
> integrated?

 Just make it a part of your V3. I'm perfectly OK with that. Looks like maintainers don't
want to integrate it alone.

> I intend to release the V3 version during this week.

 Good.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

Re: [Qemu-devel] [PATCH] Use Aff1 with mpidr

[Pavel Fedin]

 Hi again!

> I intend to release the V3 version during this week.

 One more note. I remember that i suggested to merge ITS_CONTROL and ITS_TRANSLATION into
one region in virt memory map. I was wrong, don't do this and leave them as they are.
There is in-kernel ITS implementation suggested on ARM-Linux mailing list, and qemu
support will rely on this (it will have to pass CONTROL region to KVM and intercept
TRANSLATION region).

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

Re: [Qemu-devel] [PATCH] Use Aff1 with mpidr

[Shlomo Pongratz]

Hi

> -----Original Message-----
> From: Pavel Fedin [mailto:p.fedin@samsung.com]
> Sent: Monday, 01 June, 2015 12:04 PM
> To: Shlomo Pongratz; qemu-devel@nongnu.org
> Cc: peter.maydell@linaro.org; 'Ashok Kumar'
> Subject: RE: [Qemu-devel] [PATCH] Use Aff1 with mpidr
> 
>  Hi again!
> 
> > I intend to release the V3 version during this week.
> 
>  One more note. I remember that i suggested to merge ITS_CONTROL and
> ITS_TRANSLATION into one region in virt memory map. I was wrong, don't do
> this and leave them as they are.
> There is in-kernel ITS implementation suggested on ARM-Linux mailing list,
> and qemu support will rely on this (it will have to pass CONTROL region to
> KVM and intercept TRANSLATION region).
> 

Hi Pavel,

You mean that I put your patch https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg04495.html
The first patch of my V3 patch series, with the sole exception of fixing the ITS control & ITS translation.

Best regards,

S.P.


> Kind regards,
> Pavel Fedin
> Expert Engineer
> Samsung Electronics Research center Russia

Re: [Qemu-devel] [PATCH] Use Aff1 with mpidr

[Pavel Fedin]

 Hi!

> You mean that I put your patch
https://lists.gnu.org/archive/html/qemu-devel/201505/msg04495.html

 Yes. You can of course further modify it, like changing #define to a variable which can
be set from outside.

> The first patch of my V3 patch series, with the sole exception of fixing the ITS control
& ITS
> translation.

 ITS memory regions is another thing, it is in virt machine definition.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

Re: [Qemu-devel] [PATCH] Use Aff1 with mpidr

[Igor Mammedov]

On Fri, 29 May 2015 16:41:01 +0300
Pavel Fedin <p.fedin@samsung.com> wrote:

>  Hello!
> 
> > Well KVM side should be fixed instead of driving us along wrong route.
> 
>  It can be fixed... Perhaps... If kernel developers acknowledge this is a problem, which
> might not happen.
>  But still we will have older kernels, and what? Don't you want 64-bit ARM KVM work on
> them?
I think arm target is not counted as stable  yet, so we don't have to keep
compatibility layer yet.

> 
> Kind regards,
> Pavel Fedin
> Expert Engineer
> Samsung Electronics Research center Russia
> 
> 

Re: [Qemu-devel] [PATCH] Use Aff1 with mpidr

[Igor Mammedov]

On Sun, 31 May 2015 11:03:53 +0000
Shlomo Pongratz <shlomo.pongratz@huawei.com> wrote:

> Hi,
> 
> See below.
> 
> > -----Original Message-----
> > From: Pavel Fedin [mailto:p.fedin@samsung.com]
> > Sent: Friday, 29 May, 2015 9:45 AM
> > To: Shlomo Pongratz; qemu-devel@nongnu.org
> > Cc: peter.maydell@linaro.org; 'Ashok Kumar'
> > Subject: RE: [PATCH] Use Aff1 with mpidr
> > 
> >  Hi!
> > 
> > > I see that you added mpidr to ARMCpu and initialized it in virt.c then
> > > you use it in
> > mpidr_read.
> > > Thus you must fix all other virtual machines in hw/arm not just virt.c
> > > as it is not
> > initialized for
> > > them.
> > 
> >  The only change in virt.c is:
> > --- cut ---
> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c index a7f9a10..a1186c5 100644
> > --- a/hw/arm/virt.c
> > +++ b/hw/arm/virt.c
> > @@ -317,7 +317,11 @@ static void fdt_add_cpu_nodes(const VirtBoardInfo
> > *vbi)
> >                                          "enable-method", "psci");
> >          }
> > 
> > -        qemu_fdt_setprop_cell(vbi->fdt, nodename, "reg", cpu);
> > +        /*
> > +         * If cpus node's #address-cells property is set to 1
> > +         * The reg cell bits [23:0] must be set to bits [23:0] of MPIDR_EL1.
> > +         */
> > +        qemu_fdt_setprop_cell(vbi->fdt, nodename, "reg",
> > + armcpu->mpidr);
> >          g_free(nodename);
> >      }
> >  }
> > --- cut ---
> >  So that it takes MPIDR instead of just CPU index. Theoretically - yes, may be
> > other machines should also be changed, but:
> > 1. They are 32-bit, so MPIDR == index for them, because there are no more
> > than 8 CPUs.
> > 2. Those machines AFAIK do not compose device tree by themselves. They
> > use pre-made ones instead, coming for example with kernel.
> >  Only virt currently can be a 64-bit machine and cares about more than 8
> > CPUs.
> >  As to MPIDR initialization, it is done in arm_cpu_initfn(), therefore all ARM
> > CPUs get this automatically. There's no need to modify code for every
> > machine.
> 
> I think we should take Igor's comment into account. The CPUS_PER_CLUSTER should not be a constant, and maybe should be initialized in arm_cpu_initfn and aarch64_{a53|a57}_initfn, as psci need to know it.

I has been suggesting something like following to allow every CPU type override
affinity definitions:

diff --git a/target-arm/cpu-qom.h b/target-arm/cpu-qom.h
index ed5a644..f309699 100644
--- a/target-arm/cpu-qom.h
+++ b/target-arm/cpu-qom.h
@@ -181,6 +181,7 @@ typedef struct AArch64CPUClass {
     /*< private >*/
     ARMCPUClass parent_class;
     /*< public >*/
+    void (*get_affinity)(ARMCPU *cpu, uint8_t *aff1, uint8_t *aff2, uint8_t *aff3)
 } AArch64CPUClass;
 
 static inline ARMCPU *arm_env_get_cpu(CPUARMState *env)
diff --git a/target-arm/cpu64.c b/target-arm/cpu64.c
index 270bc2f..bf19efe 100644
--- a/target-arm/cpu64.c
+++ b/target-arm/cpu64.c
@@ -235,7 +235,9 @@ static void aarch64_cpu_set_pc(CPUState *cs, vaddr value)
 static void aarch64_cpu_class_init(ObjectClass *oc, void *data)
 {
     CPUClass *cc = CPU_CLASS(oc);
+    AArch64CPUClass *acc = ARMCPUClass(oc);
 
+    acc->get_affinity = coretex_a[57|53|..]_get_affinity;
 #if !defined(CONFIG_USER_ONLY)
     cc->do_interrupt = aarch64_cpu_do_interrupt;
 #endif

Re: [Qemu-devel] [PATCH] Use Aff1 with mpidr

[Peter Maydell]

On 2 June 2015 at 16:32, Igor Mammedov <imammedo@redhat.com> wrote:
> On Fri, 29 May 2015 16:41:01 +0300
> Pavel Fedin <p.fedin@samsung.com> wrote:
>
>>  Hello!
>>
>> > Well KVM side should be fixed instead of driving us along wrong route.
>>
>>  It can be fixed... Perhaps... If kernel developers acknowledge this is a problem, which
>> might not happen.
>>  But still we will have older kernels, and what? Don't you want 64-bit ARM KVM work on
>> them?
> I think arm target is not counted as stable  yet, so we don't have to keep
> compatibility layer yet.

We don't want to break running KVM on older kernels on ARM.
It's OK if an older kernel means you don't get access to shiny
new features (like GICv3 support, maybe), but QEMU binaries
and configurations that used to work on those kernels should
continue to work on those kernels.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] Use Aff1 with mpidr

[Igor Mammedov]

On Tue, 2 Jun 2015 16:42:31 +0100
Peter Maydell <peter.maydell@linaro.org> wrote:

> On 2 June 2015 at 16:32, Igor Mammedov <imammedo@redhat.com> wrote:
> > On Fri, 29 May 2015 16:41:01 +0300
> > Pavel Fedin <p.fedin@samsung.com> wrote:
> >
> >>  Hello!
> >>
> >> > Well KVM side should be fixed instead of driving us along wrong route.
> >>
> >>  It can be fixed... Perhaps... If kernel developers acknowledge this is a problem, which
> >> might not happen.
> >>  But still we will have older kernels, and what? Don't you want 64-bit ARM KVM work on
> >> them?
> > I think arm target is not counted as stable  yet, so we don't have to keep
> > compatibility layer yet.
> 
> We don't want to break running KVM on older kernels on ARM.
> It's OK if an older kernel means you don't get access to shiny
> new features (like GICv3 support, maybe), but QEMU binaries
> and configurations that used to work on those kernels should
> continue to work on those kernels.
perhaps we would need to start using machine types then like we do for x86
or something like this.

> 
> thanks
> -- PMM