diff mbox series

[RFC,16/40] target/arm: Represent the entire MPIDR_EL1

Message ID 20230103181646.55711-17-richard.henderson@linaro.org
State New
Headers show
Series Toward class init of cpu features | expand

Commit Message

Richard Henderson Jan. 3, 2023, 6:16 p.m. UTC
Replace ARMCPU.mp_affinity with CPUARMState.cp15.mpidr_el1,
setting the additional bits as required.  In particular,
always set the U bit when there is only one cpu in the system.
Remove the mp_is_up bit which attempted to do the same thing.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/cpu.h     |  7 ++--
 target/arm/cpu.c     | 80 +++++++++++++++++++++++++++++++++++++-------
 target/arm/cpu_tcg.c |  1 -
 target/arm/helper.c  | 25 ++------------
 target/arm/hvf/hvf.c |  2 +-
 target/arm/kvm64.c   |  4 +--
 6 files changed, 75 insertions(+), 44 deletions(-)

Comments

Peter Maydell Jan. 6, 2023, 7:16 p.m. UTC | #1
On Tue, 3 Jan 2023 at 18:24, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Replace ARMCPU.mp_affinity with CPUARMState.cp15.mpidr_el1,
> setting the additional bits as required.  In particular,
> always set the U bit when there is only one cpu in the system.
> Remove the mp_is_up bit which attempted to do the same thing.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/cpu.h     |  7 ++--
>  target/arm/cpu.c     | 80 +++++++++++++++++++++++++++++++++++++-------
>  target/arm/cpu_tcg.c |  1 -
>  target/arm/helper.c  | 25 ++------------
>  target/arm/hvf/hvf.c |  2 +-
>  target/arm/kvm64.c   |  4 +--
>  6 files changed, 75 insertions(+), 44 deletions(-)

Based purely on the diffstat it's not super-obvious why this
is an improvement. What's the rationale ?

Side note, we don't currently handle the MT bit, where some
CPUs end up putting the cpu number in the Aff1 field rather
than Aff0. We ideally ought to handle that too.

thanks
-- PMM
Richard Henderson Jan. 6, 2023, 7:33 p.m. UTC | #2
On 1/6/23 11:16, Peter Maydell wrote:
> On Tue, 3 Jan 2023 at 18:24, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> Replace ARMCPU.mp_affinity with CPUARMState.cp15.mpidr_el1,
>> setting the additional bits as required.  In particular,
>> always set the U bit when there is only one cpu in the system.
>> Remove the mp_is_up bit which attempted to do the same thing.
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>   target/arm/cpu.h     |  7 ++--
>>   target/arm/cpu.c     | 80 +++++++++++++++++++++++++++++++++++++-------
>>   target/arm/cpu_tcg.c |  1 -
>>   target/arm/helper.c  | 25 ++------------
>>   target/arm/hvf/hvf.c |  2 +-
>>   target/arm/kvm64.c   |  4 +--
>>   6 files changed, 75 insertions(+), 44 deletions(-)
> 
> Based purely on the diffstat it's not super-obvious why this
> is an improvement. What's the rationale ?

It gets rid of cpu->mp_is_up, set only by cortex-r5, and then generalizes.

> Side note, we don't currently handle the MT bit, where some
> CPUs end up putting the cpu number in the Aff1 field rather
> than Aff0. We ideally ought to handle that too.

Would that be set by the board as well?  I don't think we model cpu 
packages/clusters/whatchacallums at that level currently.


r~
Peter Maydell Jan. 6, 2023, 10:14 p.m. UTC | #3
On Fri, 6 Jan 2023 at 19:33, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 1/6/23 11:16, Peter Maydell wrote:
> > On Tue, 3 Jan 2023 at 18:24, Richard Henderson
> > <richard.henderson@linaro.org> wrote:
> >>
> >> Replace ARMCPU.mp_affinity with CPUARMState.cp15.mpidr_el1,
> >> setting the additional bits as required.  In particular,
> >> always set the U bit when there is only one cpu in the system.
> >> Remove the mp_is_up bit which attempted to do the same thing.
> >>
> >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> >> ---
> >>   target/arm/cpu.h     |  7 ++--
> >>   target/arm/cpu.c     | 80 +++++++++++++++++++++++++++++++++++++-------
> >>   target/arm/cpu_tcg.c |  1 -
> >>   target/arm/helper.c  | 25 ++------------
> >>   target/arm/hvf/hvf.c |  2 +-
> >>   target/arm/kvm64.c   |  4 +--
> >>   6 files changed, 75 insertions(+), 44 deletions(-)
> >
> > Based purely on the diffstat it's not super-obvious why this
> > is an improvement. What's the rationale ?
>
> It gets rid of cpu->mp_is_up, set only by cortex-r5, and then generalizes.
>
> > Side note, we don't currently handle the MT bit, where some
> > CPUs end up putting the cpu number in the Aff1 field rather
> > than Aff0. We ideally ought to handle that too.
>
> Would that be set by the board as well?  I don't think we model cpu
> packages/clusters/whatchacallums at that level currently.

It's a fixed property of the CPU implementation, same as
mp_is_up. We currently mismodel Cortex-A55, Cortex-A76
and Neoverse-N1, which should all be setting the MT bit in
MPIDR and having the cpu number in Aff1. See this thread:
 https://lore.kernel.org/qemu-devel/CAFEAcA9P2-v94p8H8+ktnf-Yf-rucbGySXE6AGPdwvDxXfP=ZA@mail.gmail.com/

+    if (arm_feature(env, ARM_FEATURE_V7MP)) {
+        cpu->mpidr_el1 |= (1u << 31);   /* M */
+        if (cpu->core_count == 1) {
+            cpu->mpidr_el1 |= 1 << 30;  /* U */
+        }
+    }

This is wrong, incidentally -- a single Cortex A9, A53, etc does
not set the U bit. (It's "a cluster with 1 core in it", not
"a uniprocessor system".) The reason mp_is_up is set only
by the R5 model is because "we implement the MP extensions
but consider ourselves to be not part of a cluster" is a
behaviour of only this CPU. I forget why I didn't implement
it as an ARM_FEATURE_FOO bit.

thanks
-- PMM
Richard Henderson Jan. 6, 2023, 10:36 p.m. UTC | #4
On 1/6/23 14:14, Peter Maydell wrote:
> +    if (arm_feature(env, ARM_FEATURE_V7MP)) {
> +        cpu->mpidr_el1 |= (1u << 31);   /* M */
> +        if (cpu->core_count == 1) {
> +            cpu->mpidr_el1 |= 1 << 30;  /* U */
> +        }
> +    }
> 
> This is wrong, incidentally -- a single Cortex A9, A53, etc does
> not set the U bit. (It's "a cluster with 1 core in it", not
> "a uniprocessor system".)

Hmph.  It would have been handy to have the "uniprocessor" term defined somewhere in the 
architecture manual, since they appear to be using it in a specialized way.


r~
diff mbox series

Patch

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 499d2a6028..0c5b942ed0 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -935,9 +935,6 @@  struct ArchCPU {
     /* KVM steal time */
     OnOffAuto kvm_steal_time;
 
-    /* Uniprocessor system with MP extensions */
-    bool mp_is_up;
-
     /* True if we tried kvm_arm_host_cpu_features() during CPU instance_init
      * and the probe failed (so we need to report the error in realize)
      */
@@ -977,7 +974,7 @@  struct ArchCPU {
     uint64_t id_aa64afr0;
     uint64_t id_aa64afr1;
     uint64_t clidr;
-    uint64_t mp_affinity; /* MP ID without feature bits */
+    uint64_t mpidr_el1;
     /* The elements of this array are the CCSIDR values for each cache,
      * in the order L1DCache, L1ICache, L2DCache, L2ICache, etc.
      */
@@ -1041,7 +1038,7 @@  uint64_t arm_build_mp_affinity(int idx, uint8_t clustersz);
 
 static inline uint64_t arm_cpu_mp_affinity(ARMCPU *cpu)
 {
-    return cpu->mp_affinity;
+    return cpu->mpidr_el1 & ARM64_AFFINITY_MASK;
 }
 
 #ifndef CONFIG_USER_ONLY
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index a104a77165..a46fa424d3 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1231,6 +1231,9 @@  static void arm_cpu_initfn(Object *obj)
     cpu->sme_default_vq = 2;
 # endif
 #else
+    /* To be set properly by either the board or by realize. */
+    cpu->mpidr_el1 = ARM64_AFFINITY_INVALID;
+
     /* Our inbound IRQ and FIQ lines */
     if (kvm_enabled()) {
         /* VIRQ and VFIQ are unused with KVM but we add them to maintain
@@ -1921,16 +1924,6 @@  static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
         return;
     }
 
-    /* This cpu-id-to-MPIDR affinity is used only for TCG; KVM will override it.
-     * 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.
-     */
-    if (cpu->mp_affinity == ARM64_AFFINITY_INVALID) {
-        cpu->mp_affinity = arm_build_mp_affinity(cs->cpu_index,
-                                                 ARM_DEFAULT_CPUS_PER_CLUSTER);
-    }
-
     if (cpu->reset_hivecs) {
             cpu->reset_sctlr |= (1 << 13);
     }
@@ -2116,7 +2109,27 @@  static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
     if (cpu->core_count == -1) {
         cpu->core_count = smp_cpus;
     }
-#endif
+
+    /*
+     * Provide a default cpu-id-to-MPIDR affinity; we don't support setting
+     * Aff2 or Aff3.  This has already set by KVM and by some board models,
+     * which will have cleared our internal invalid bit.
+     */
+    if (cpu->mpidr_el1 == ARM64_AFFINITY_INVALID) {
+        assert(!kvm_enabled());
+        assert(cs->cpu_index < 256 * ARM_DEFAULT_CPUS_PER_CLUSTER);
+        cpu->mpidr_el1 = arm_build_mp_affinity(cs->cpu_index,
+                                               ARM_DEFAULT_CPUS_PER_CLUSTER);
+    }
+#endif /* !CONFIG_USER_ONLY */
+
+    /* Linux exposes M to userland, so still need to set it for user-only. */
+    if (arm_feature(env, ARM_FEATURE_V7MP)) {
+        cpu->mpidr_el1 |= (1u << 31);   /* M */
+        if (cpu->core_count == 1) {
+            cpu->mpidr_el1 |= 1 << 30;  /* U */
+        }
+    }
 
     if (tcg_enabled()) {
         int dcz_blocklen = 4 << cpu->dcz_blocksize;
@@ -2176,10 +2189,47 @@  static ObjectClass *arm_cpu_class_by_name(const char *cpu_model)
     return oc;
 }
 
+static void cpu_arm_set_mp_affinity(Object *obj, Visitor *v, const char *name,
+                                    void *opaque, Error **errp)
+{
+    ARMCPU *cpu = ARM_CPU(obj);
+    CPUARMState *env = &cpu->env;
+    uint64_t value;
+
+    if (!visit_type_uint64(v, name, &value, errp)) {
+        return;
+    }
+
+    if (arm_feature(env, ARM_FEATURE_AARCH64)) {
+        value &= ARM64_AFFINITY_MASK;
+    } else {
+        value &= ARM32_AFFINITY_MASK;
+    }
+    if (cpu->mpidr_el1 == ARM64_AFFINITY_INVALID) {
+        cpu->mpidr_el1 = value;
+    } else {
+        cpu->mpidr_el1 &= ~ARM64_AFFINITY_MASK;
+        cpu->mpidr_el1 |= value;
+    }
+}
+
+static void cpu_arm_get_mp_affinity(Object *obj, Visitor *v, const char *name,
+                                    void *opaque, Error **errp)
+{
+    ARMCPU *cpu = ARM_CPU(obj);
+    uint64_t value;
+
+    /*
+     * Note that the arm64 mask is a superset of the arm32 mask,
+     * and we will have limited the value upon setting.
+     * Here we simply want to return the Aff[0-3] fields.
+     */
+    value = cpu->mpidr_el1 & ARM64_AFFINITY_MASK;
+    visit_type_uint64(v, name, &value, errp);
+}
+
 static Property arm_cpu_properties[] = {
     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),
     DEFINE_PROP_INT32("core-count", ARMCPU, core_count, -1),
     DEFINE_PROP_END_OF_LIST()
@@ -2244,6 +2294,10 @@  static void arm_cpu_class_init(ObjectClass *oc, void *data)
 
     device_class_set_props(dc, arm_cpu_properties);
 
+    object_class_property_add(oc, "mp-affinity", "uint64",
+                              cpu_arm_get_mp_affinity,
+                              cpu_arm_set_mp_affinity, NULL, NULL);
+
     resettable_class_set_parent_phases(rc, NULL, arm_cpu_reset_hold, NULL,
                                        &acc->parent_phases);
 
diff --git a/target/arm/cpu_tcg.c b/target/arm/cpu_tcg.c
index d566a815d3..7514065d5b 100644
--- a/target/arm/cpu_tcg.c
+++ b/target/arm/cpu_tcg.c
@@ -848,7 +848,6 @@  static void cortex_r5_initfn(Object *obj)
     cpu->isar.id_isar4 = 0x0010142;
     cpu->isar.id_isar5 = 0x0;
     cpu->isar.id_isar6 = 0x0;
-    cpu->mp_is_up = true;
     cpu->pmsav7_dregion = 16;
     cpu->isar.reset_pmcr_el0 = 0x41151800;
     define_arm_cp_regs(cpu, cortexr5_cp_reginfo);
diff --git a/target/arm/helper.c b/target/arm/helper.c
index bac2ea62c4..8f5097f995 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -4087,24 +4087,6 @@  static uint64_t midr_read(CPUARMState *env, const ARMCPRegInfo *ri)
     return raw_read(env, ri);
 }
 
-static uint64_t mpidr_read_val(CPUARMState *env)
-{
-    ARMCPU *cpu = env_archcpu(env);
-    uint64_t mpidr = cpu->mp_affinity;
-
-    if (arm_feature(env, ARM_FEATURE_V7MP)) {
-        mpidr |= (1U << 31);
-        /* Cores which are uniprocessor (non-coherent)
-         * but still implement the MP extensions set
-         * bit 30. (For instance, Cortex-R5).
-         */
-        if (cpu->mp_is_up) {
-            mpidr |= (1u << 30);
-        }
-    }
-    return mpidr;
-}
-
 static uint64_t mpidr_read(CPUARMState *env, const ARMCPRegInfo *ri)
 {
     unsigned int cur_el = arm_current_el(env);
@@ -4112,7 +4094,7 @@  static uint64_t mpidr_read(CPUARMState *env, const ARMCPRegInfo *ri)
     if (arm_is_el2_enabled(env) && cur_el == 1) {
         return env->cp15.vmpidr_el2;
     }
-    return mpidr_read_val(env);
+    return env_archcpu(env)->mpidr_el1;
 }
 
 static const ARMCPRegInfo lpae_cp_reginfo[] = {
@@ -7940,7 +7922,6 @@  void register_cp_regs_for_features(ARMCPU *cpu)
     if (arm_feature(env, ARM_FEATURE_EL2)
         || (arm_feature(env, ARM_FEATURE_EL3)
             && arm_feature(env, ARM_FEATURE_V8))) {
-        uint64_t vmpidr_def = mpidr_read_val(env);
         ARMCPRegInfo vpidr_regs[] = {
             { .name = "VPIDR", .state = ARM_CP_STATE_AA32,
               .cp = 15, .opc1 = 4, .crn = 0, .crm = 0, .opc2 = 0,
@@ -7956,12 +7937,12 @@  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,
+              .resetvalue = cpu->mpidr_el1,
               .type = ARM_CP_ALIAS | ARM_CP_EL3_NO_EL2_C_NZ,
               .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, .resetvalue = vmpidr_def,
+              .access = PL2_RW, .resetvalue = cpu->mpidr_el1,
               .type = ARM_CP_EL3_NO_EL2_C_NZ,
               .fieldoffset = offsetof(CPUARMState, cp15.vmpidr_el2) },
         };
diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c
index e0ba91f5c6..278a4b2ede 100644
--- a/target/arm/hvf/hvf.c
+++ b/target/arm/hvf/hvf.c
@@ -607,7 +607,7 @@  int hvf_arch_init_vcpu(CPUState *cpu)
     assert_hvf_ok(ret);
 
     ret = hv_vcpu_set_sys_reg(cpu->hvf->fd, HV_SYS_REG_MPIDR_EL1,
-                              arm_cpu->mp_affinity);
+                              arm_cpu->mpidr_el1);
     assert_hvf_ok(ret);
 
     ret = hv_vcpu_get_sys_reg(cpu->hvf->fd, HV_SYS_REG_ID_AA64PFR0_EL1, &pfr);
diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
index 1197253d12..2cdd7517b8 100644
--- a/target/arm/kvm64.c
+++ b/target/arm/kvm64.c
@@ -914,11 +914,11 @@  int kvm_arch_init_vcpu(CPUState *cs)
      * Currently KVM has its own idea about MPIDR assignment, so we
      * override our defaults with what we get from KVM.
      */
-    ret = kvm_get_one_reg(cs, ARM64_SYS_REG(ARM_CPU_ID_MPIDR), &mpidr);
+    ret = kvm_get_one_reg(cs, ARM64_SYS_REG(ARM_CPU_ID_MPIDR),
+                          &cpu->mpidr_el1);
     if (ret) {
         return ret;
     }
-    cpu->mp_affinity = mpidr & ARM64_AFFINITY_MASK;
 
     kvm_arm_init_debug(cs);