diff mbox series

[v1,03/27] target/s390x: get rid of next_core_id

Message ID 20170918160012.4317-4-david@redhat.com
State New
Headers show
Series s390x: SMP for TCG (+ cleanups) | expand

Commit Message

David Hildenbrand Sept. 18, 2017, 3:59 p.m. UTC
core_id is not needed by linux-user, as the core_id a.k.a. CPU address
is only accessible from kernel space.

Therefore, drop next_core_id and make cpu_index get autoassigned again
for linux-user.

While at it, shield core_id and cpuid completely from linux-user. cpuid
can also only be queried from kernel space.

Suggested-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/cpu-qom.h    |  2 --
 target/s390x/cpu.c        | 14 +++++++-------
 target/s390x/cpu.h        |  2 ++
 target/s390x/cpu_models.c |  2 ++
 4 files changed, 11 insertions(+), 9 deletions(-)

Comments

Igor Mammedov Sept. 21, 2017, 1:28 p.m. UTC | #1
On Mon, 18 Sep 2017 17:59:48 +0200
David Hildenbrand <david@redhat.com> wrote:

> core_id is not needed by linux-user, as the core_id a.k.a. CPU address
> is only accessible from kernel space.
> 
> Therefore, drop next_core_id and make cpu_index get autoassigned again
> for linux-user.
> 
> While at it, shield core_id and cpuid completely from linux-user. cpuid
> can also only be queried from kernel space.
> 
> Suggested-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>

[...]
Richard Henderson Sept. 25, 2017, 11:14 p.m. UTC | #2
On 09/18/2017 08:59 AM, David Hildenbrand wrote:
>      /* sync cs->cpu_index and env->core_id. The latter is needed for TCG. */
> -    cs->cpu_index = env->core_id;
> +    cs->cpu_index = cpu->env.core_id;
> +#endif

Any reason not to drop core_id entirely in favour of cpu_index?
(Since cpu_index itself is generic and can't be dropped.)


r~
Igor Mammedov Sept. 26, 2017, 8:49 a.m. UTC | #3
On Mon, 25 Sep 2017 16:14:16 -0700
Richard Henderson <richard.henderson@linaro.org> wrote:

> On 09/18/2017 08:59 AM, David Hildenbrand wrote:
> >      /* sync cs->cpu_index and env->core_id. The latter is needed for TCG. */
> > -    cs->cpu_index = env->core_id;
> > +    cs->cpu_index = cpu->env.core_id;
> > +#endif  
> 
> Any reason not to drop core_id entirely in favour of cpu_index?
> (Since cpu_index itself is generic and can't be dropped.)
I prefer core_id vs cpu_index as the later is too generic
(and we were trying to decouple target specific attributes
/coreid/cpuid/whatnot from cpu_index as much as possible
so it would be more clear what they mean and if possible
make them in-depended of QEMU internal cpu_index which is
essentially not owned by target).
David Hildenbrand Sept. 26, 2017, 12:40 p.m. UTC | #4
On 26.09.2017 10:49, Igor Mammedov wrote:
> On Mon, 25 Sep 2017 16:14:16 -0700
> Richard Henderson <richard.henderson@linaro.org> wrote:
> 
>> On 09/18/2017 08:59 AM, David Hildenbrand wrote:
>>>      /* sync cs->cpu_index and env->core_id. The latter is needed for TCG. */
>>> -    cs->cpu_index = env->core_id;
>>> +    cs->cpu_index = cpu->env.core_id;
>>> +#endif  
>>
>> Any reason not to drop core_id entirely in favour of cpu_index?
>> (Since cpu_index itself is generic and can't be dropped.)
> I prefer core_id vs cpu_index as the later is too generic
> (and we were trying to decouple target specific attributes
> /coreid/cpuid/whatnot from cpu_index as much as possible
> so it would be more clear what they mean and if possible
> make them in-depended of QEMU internal cpu_index which is
> essentially not owned by target).
> 

As x86 also has this split I also prefer to keep it for now as it is.

cpu_index just has to match our core id, otherwise e.g. migration could
break (cpu_index is used as identifier for cpus in the migration stream).

If we would drop core_id, we would have to implement a handler for the
STAP instruction, as the CPU object is not accessible from translate.c.
(not a big problem, just another reason to simply leave it as is for
now), and also a new property handler, so only a uint16_t can be set
using "core-id" onto a CPU object.
diff mbox series

Patch

diff --git a/target/s390x/cpu-qom.h b/target/s390x/cpu-qom.h
index 2e446fab51..0510c49e07 100644
--- a/target/s390x/cpu-qom.h
+++ b/target/s390x/cpu-qom.h
@@ -54,8 +54,6 @@  typedef struct S390CPUClass {
     bool is_migration_safe;
     const char *desc;
 
-    uint32_t next_core_id;
-
     DeviceRealize parent_realize;
     void (*parent_reset)(CPUState *cpu);
     void (*load_normal)(CPUState *cpu);
diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index 4920da3625..f42e28ea25 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -178,8 +178,9 @@  static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
 {
     CPUState *cs = CPU(dev);
     S390CPUClass *scc = S390_CPU_GET_CLASS(dev);
+#if !defined(CONFIG_USER_ONLY)
     S390CPU *cpu = S390_CPU(dev);
-    CPUS390XState *env = &cpu->env;
+#endif
     Error *err = NULL;
 
     /* the model has to be realized before qemu_init_vcpu() due to kvm */
@@ -195,11 +196,6 @@  static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
                    max_cpus - 1);
         goto out;
     }
-#else
-    /* implicitly set for linux-user only */
-    cpu->env.core_id = scc->next_core_id;
-    scc->next_core_id++;
-#endif
 
     if (cpu_exists(cpu->env.core_id)) {
         error_setg(&err, "Unable to add CPU with core-id: %" PRIu32
@@ -208,7 +204,9 @@  static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
     }
 
     /* sync cs->cpu_index and env->core_id. The latter is needed for TCG. */
-    cs->cpu_index = env->core_id;
+    cs->cpu_index = cpu->env.core_id;
+#endif
+
     cpu_exec_realizefn(cs, &err);
     if (err != NULL) {
         goto out;
@@ -440,7 +438,9 @@  static gchar *s390_gdb_arch_name(CPUState *cs)
 }
 
 static Property s390x_cpu_properties[] = {
+#if !defined(CONFIG_USER_ONLY)
     DEFINE_PROP_UINT32("core-id", S390CPU, env.core_id, 0),
+#endif
     DEFINE_PROP_END_OF_LIST()
 };
 
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 9b549dc491..f13aea3dd8 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -150,8 +150,10 @@  struct CPUS390XState {
 
     CPU_COMMON
 
+#if !defined(CONFIG_USER_ONLY)
     uint32_t core_id; /* PoP "CPU address", same as cpu_index */
     uint64_t cpuid;
+#endif
 
     uint64_t tod_offset;
     uint64_t tod_basetime;
diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
index 5169379db5..191aee6dd4 100644
--- a/target/s390x/cpu_models.c
+++ b/target/s390x/cpu_models.c
@@ -941,11 +941,13 @@  void s390_realize_cpu_model(CPUState *cs, Error **errp)
 
     apply_cpu_model(cpu->model, errp);
 
+#if !defined(CONFIG_USER_ONLY)
     cpu->env.cpuid = s390_cpuid_from_cpu_model(cpu->model);
     if (tcg_enabled()) {
         /* basic mode, write the cpu address into the first 4 bit of the ID */
         cpu->env.cpuid = deposit64(cpu->env.cpuid, 54, 4, cpu->env.core_id);
     }
+#endif
 }
 
 static void get_feature(Object *obj, Visitor *v, const char *name,