[Qemu-devel] [PATCH 1/2] linux-user-i386: fix crash on cpuid

[Qemu-devel] [PATCH 1/2] linux-user-i386: fix crash on cpuid

[Marc-André Lureau]

Running cpuid instructions with a simple run like:
i386-linux-user/qemu-i386 tests/tcg/sha1-i386

Results in the following assert:
 #0  0x00007ffff64246f5 in raise () from /lib64/libc.so.6
 #1  0x00007ffff64262fa in abort () from /lib64/libc.so.6
 #2  0x00007ffff7937ec5 in g_assertion_message () from /lib64/libglib-2.0.so.0
 #3  0x00007ffff7937f5a in g_assertion_message_expr () from /lib64/libglib-2.0.so.0
 #4  0x000055555561b54c in apicid_bitwidth_for_count (count=0) at /home/elmarco/src/qemu/include/hw/i386/topology.h:58
 #5  0x000055555561b58a in apicid_smt_width (nr_cores=0, nr_threads=0) at /home/elmarco/src/qemu/include/hw/i386/topology.h:67
 #6  0x000055555561b5c3 in apicid_core_offset (nr_cores=0, nr_threads=0) at /home/elmarco/src/qemu/include/hw/i386/topology.h:82
 #7  0x000055555561b5e3 in apicid_pkg_offset (nr_cores=0, nr_threads=0) at /home/elmarco/src/qemu/include/hw/i386/topology.h:89
 #8  0x000055555561dd86 in cpu_x86_cpuid (env=0x555557999550, index=4, count=3, eax=0x7fffffffcae8, ebx=0x7fffffffcaec, ecx=0x7fffffffcaf0, edx=0x7fffffffcaf4) at /home/elmarco/src/qemu/target-i386/cpu.c:2405
 #9  0x0000555555638e8e in helper_cpuid (env=0x555557999550) at /home/elmarco/src/qemu/target-i386/misc_helper.c:106
 #10 0x000055555599dc5e in static_code_gen_buffer ()
 #11 0x00005555555952f8 in cpu_tb_exec (cpu=0x5555579912d0, itb=0x7ffff4371ab0) at /home/elmarco/src/qemu/cpu-exec.c:166
 #12 0x0000555555595c8e in cpu_loop_exec_tb (cpu=0x5555579912d0, tb=0x7ffff4371ab0, last_tb=0x7fffffffd088, tb_exit=0x7fffffffd084, sc=0x7fffffffd0a0) at /home/elmarco/src/qemu/cpu-exec.c:517
 #13 0x0000555555595e50 in cpu_exec (cpu=0x5555579912d0) at /home/elmarco/src/qemu/cpu-exec.c:612
 #14 0x00005555555c065b in cpu_loop (env=0x555557999550) at /home/elmarco/src/qemu/linux-user/main.c:297
 #15 0x00005555555c25b2 in main (argc=2, argv=0x7fffffffd848, envp=0x7fffffffd860) at /home/elmarco/src/qemu/linux-user/main.c:4803

The fields are set in qemu_init_vcpu() with softmmu, but it's a stub
with linux-user.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 qom/cpu.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/qom/cpu.c b/qom/cpu.c
index 2553247..f783b5a 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -342,6 +342,11 @@ static void cpu_common_initfn(Object *obj)
 
     cpu->cpu_index = UNASSIGNED_CPU_INDEX;
     cpu->gdb_num_regs = cpu->gdb_num_g_regs = cc->gdb_num_core_regs;
+    /* *-user doesn't have configurable SMP topology */
+    /* the default value is changed by qemu_init_vcpu() for softmmu */
+    cpu->nr_cores = 1;
+    cpu->nr_threads = 1;
+
     qemu_mutex_init(&cpu->work_mutex);
     QTAILQ_INIT(&cpu->breakpoints);
     QTAILQ_INIT(&cpu->watchpoints);
-- 
2.10.0

[Qemu-devel] [PATCH 2/2] linux-user: remove #define smp_{cores, threads}

[Marc-André Lureau]

Those are unneeded now that CPUState nr_{cores,threads} is always
initialized.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 target-i386/cpu.c           | 8 ++++----
 target-ppc/translate_init.c | 3 ++-
 include/sysemu/cpus.h       | 5 +----
 3 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 5a5299a..e863bea 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2490,13 +2490,13 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
 
         switch (count) {
         case 0:
-            *eax = apicid_core_offset(smp_cores, smp_threads);
-            *ebx = smp_threads;
+            *eax = apicid_core_offset(cs->nr_cores, cs->nr_threads);
+            *ebx = cs->nr_threads;
             *ecx |= CPUID_TOPOLOGY_LEVEL_SMT;
             break;
         case 1:
-            *eax = apicid_pkg_offset(smp_cores, smp_threads);
-            *ebx = smp_cores * smp_threads;
+            *eax = apicid_pkg_offset(cs->nr_cores, cs->nr_threads);
+            *ebx = cs->nr_cores * cs->nr_threads;
             *ecx |= CPUID_TOPOLOGY_LEVEL_CORE;
             break;
         default:
diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
index 407ccb9..b66b40b 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -9943,7 +9943,8 @@ static void ppc_cpu_unrealizefn(DeviceState *dev, Error **errp)
 
 int ppc_get_compat_smt_threads(PowerPCCPU *cpu)
 {
-    int ret = MIN(smp_threads, kvmppc_smt_threads());
+    CPUState *cs = CPU(cpu);
+    int ret = MIN(cs->nr_threads, kvmppc_smt_threads());
 
     switch (cpu->cpu_version) {
     case CPU_POWERPC_LOGICAL_2_05:
diff --git a/include/sysemu/cpus.h b/include/sysemu/cpus.h
index fe992a8..3728a1e 100644
--- a/include/sysemu/cpus.h
+++ b/include/sysemu/cpus.h
@@ -29,12 +29,9 @@ void qtest_clock_warp(int64_t dest);
 
 #ifndef CONFIG_USER_ONLY
 /* vl.c */
+/* *-user doesn't have configurable SMP topology */
 extern int smp_cores;
 extern int smp_threads;
-#else
-/* *-user doesn't have configurable SMP topology */
-#define smp_cores   1
-#define smp_threads 1
 #endif
 
 void list_cpus(FILE *f, fprintf_function cpu_fprintf, const char *optarg);
-- 
2.10.0

Re: [Qemu-devel] [PATCH 2/2] linux-user: remove #define smp_{cores, threads}

[Eduardo Habkost]

On Fri, Sep 16, 2016 at 07:50:24PM +0400, Marc-André Lureau wrote:
> Those are unneeded now that CPUState nr_{cores,threads} is always
> initialized.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

I will wait for at least an Acked-by from the PPC maintainers
before I merge it, though.

> ---
>  target-i386/cpu.c           | 8 ++++----
>  target-ppc/translate_init.c | 3 ++-
>  include/sysemu/cpus.h       | 5 +----
>  3 files changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 5a5299a..e863bea 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -2490,13 +2490,13 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>  
>          switch (count) {
>          case 0:
> -            *eax = apicid_core_offset(smp_cores, smp_threads);
> -            *ebx = smp_threads;
> +            *eax = apicid_core_offset(cs->nr_cores, cs->nr_threads);
> +            *ebx = cs->nr_threads;
>              *ecx |= CPUID_TOPOLOGY_LEVEL_SMT;
>              break;
>          case 1:
> -            *eax = apicid_pkg_offset(smp_cores, smp_threads);
> -            *ebx = smp_cores * smp_threads;
> +            *eax = apicid_pkg_offset(cs->nr_cores, cs->nr_threads);
> +            *ebx = cs->nr_cores * cs->nr_threads;
>              *ecx |= CPUID_TOPOLOGY_LEVEL_CORE;
>              break;
>          default:
> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
> index 407ccb9..b66b40b 100644
> --- a/target-ppc/translate_init.c
> +++ b/target-ppc/translate_init.c
> @@ -9943,7 +9943,8 @@ static void ppc_cpu_unrealizefn(DeviceState *dev, Error **errp)
>  
>  int ppc_get_compat_smt_threads(PowerPCCPU *cpu)
>  {
> -    int ret = MIN(smp_threads, kvmppc_smt_threads());
> +    CPUState *cs = CPU(cpu);
> +    int ret = MIN(cs->nr_threads, kvmppc_smt_threads());
>  
>      switch (cpu->cpu_version) {
>      case CPU_POWERPC_LOGICAL_2_05:
> diff --git a/include/sysemu/cpus.h b/include/sysemu/cpus.h
> index fe992a8..3728a1e 100644
> --- a/include/sysemu/cpus.h
> +++ b/include/sysemu/cpus.h
> @@ -29,12 +29,9 @@ void qtest_clock_warp(int64_t dest);
>  
>  #ifndef CONFIG_USER_ONLY
>  /* vl.c */
> +/* *-user doesn't have configurable SMP topology */
>  extern int smp_cores;
>  extern int smp_threads;
> -#else
> -/* *-user doesn't have configurable SMP topology */
> -#define smp_cores   1
> -#define smp_threads 1
>  #endif
>  
>  void list_cpus(FILE *f, fprintf_function cpu_fprintf, const char *optarg);
> -- 
> 2.10.0
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 2/2] linux-user: remove #define smp_{cores, threads}

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3277 bytes --]

On Fri, Sep 16, 2016 at 04:36:48PM -0300, Eduardo Habkost wrote:
> On Fri, Sep 16, 2016 at 07:50:24PM +0400, Marc-André Lureau wrote:
> > Those are unneeded now that CPUState nr_{cores,threads} is always
> > initialized.
> > 
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> 
> I will wait for at least an Acked-by from the PPC maintainers
> before I merge it, though.

As I've said elsewhere, I'm not entirely convinced that the vcpu
structure is the place to keep this info.

But since it's there, we might as well use it.

Acked-by: David Gibson <david@gibson.dropbear.id.au>

> 
> > ---
> >  target-i386/cpu.c           | 8 ++++----
> >  target-ppc/translate_init.c | 3 ++-
> >  include/sysemu/cpus.h       | 5 +----
> >  3 files changed, 7 insertions(+), 9 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 5a5299a..e863bea 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -2490,13 +2490,13 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
> >  
> >          switch (count) {
> >          case 0:
> > -            *eax = apicid_core_offset(smp_cores, smp_threads);
> > -            *ebx = smp_threads;
> > +            *eax = apicid_core_offset(cs->nr_cores, cs->nr_threads);
> > +            *ebx = cs->nr_threads;
> >              *ecx |= CPUID_TOPOLOGY_LEVEL_SMT;
> >              break;
> >          case 1:
> > -            *eax = apicid_pkg_offset(smp_cores, smp_threads);
> > -            *ebx = smp_cores * smp_threads;
> > +            *eax = apicid_pkg_offset(cs->nr_cores, cs->nr_threads);
> > +            *ebx = cs->nr_cores * cs->nr_threads;
> >              *ecx |= CPUID_TOPOLOGY_LEVEL_CORE;
> >              break;
> >          default:
> > diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
> > index 407ccb9..b66b40b 100644
> > --- a/target-ppc/translate_init.c
> > +++ b/target-ppc/translate_init.c
> > @@ -9943,7 +9943,8 @@ static void ppc_cpu_unrealizefn(DeviceState *dev, Error **errp)
> >  
> >  int ppc_get_compat_smt_threads(PowerPCCPU *cpu)
> >  {
> > -    int ret = MIN(smp_threads, kvmppc_smt_threads());
> > +    CPUState *cs = CPU(cpu);
> > +    int ret = MIN(cs->nr_threads, kvmppc_smt_threads());
> >  
> >      switch (cpu->cpu_version) {
> >      case CPU_POWERPC_LOGICAL_2_05:
> > diff --git a/include/sysemu/cpus.h b/include/sysemu/cpus.h
> > index fe992a8..3728a1e 100644
> > --- a/include/sysemu/cpus.h
> > +++ b/include/sysemu/cpus.h
> > @@ -29,12 +29,9 @@ void qtest_clock_warp(int64_t dest);
> >  
> >  #ifndef CONFIG_USER_ONLY
> >  /* vl.c */
> > +/* *-user doesn't have configurable SMP topology */
> >  extern int smp_cores;
> >  extern int smp_threads;
> > -#else
> > -/* *-user doesn't have configurable SMP topology */
> > -#define smp_cores   1
> > -#define smp_threads 1
> >  #endif
> >  
> >  void list_cpus(FILE *f, fprintf_function cpu_fprintf, const char *optarg);
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH 1/2] linux-user-i386: fix crash on cpuid

[Eduardo Habkost]

On Fri, Sep 16, 2016 at 07:50:23PM +0400, Marc-André Lureau wrote:
> Running cpuid instructions with a simple run like:
> i386-linux-user/qemu-i386 tests/tcg/sha1-i386
> 
> Results in the following assert:
>  #0  0x00007ffff64246f5 in raise () from /lib64/libc.so.6
>  #1  0x00007ffff64262fa in abort () from /lib64/libc.so.6
>  #2  0x00007ffff7937ec5 in g_assertion_message () from /lib64/libglib-2.0.so.0
>  #3  0x00007ffff7937f5a in g_assertion_message_expr () from /lib64/libglib-2.0.so.0
>  #4  0x000055555561b54c in apicid_bitwidth_for_count (count=0) at /home/elmarco/src/qemu/include/hw/i386/topology.h:58
>  #5  0x000055555561b58a in apicid_smt_width (nr_cores=0, nr_threads=0) at /home/elmarco/src/qemu/include/hw/i386/topology.h:67
>  #6  0x000055555561b5c3 in apicid_core_offset (nr_cores=0, nr_threads=0) at /home/elmarco/src/qemu/include/hw/i386/topology.h:82
>  #7  0x000055555561b5e3 in apicid_pkg_offset (nr_cores=0, nr_threads=0) at /home/elmarco/src/qemu/include/hw/i386/topology.h:89
>  #8  0x000055555561dd86 in cpu_x86_cpuid (env=0x555557999550, index=4, count=3, eax=0x7fffffffcae8, ebx=0x7fffffffcaec, ecx=0x7fffffffcaf0, edx=0x7fffffffcaf4) at /home/elmarco/src/qemu/target-i386/cpu.c:2405
>  #9  0x0000555555638e8e in helper_cpuid (env=0x555557999550) at /home/elmarco/src/qemu/target-i386/misc_helper.c:106
>  #10 0x000055555599dc5e in static_code_gen_buffer ()
>  #11 0x00005555555952f8 in cpu_tb_exec (cpu=0x5555579912d0, itb=0x7ffff4371ab0) at /home/elmarco/src/qemu/cpu-exec.c:166
>  #12 0x0000555555595c8e in cpu_loop_exec_tb (cpu=0x5555579912d0, tb=0x7ffff4371ab0, last_tb=0x7fffffffd088, tb_exit=0x7fffffffd084, sc=0x7fffffffd0a0) at /home/elmarco/src/qemu/cpu-exec.c:517
>  #13 0x0000555555595e50 in cpu_exec (cpu=0x5555579912d0) at /home/elmarco/src/qemu/cpu-exec.c:612
>  #14 0x00005555555c065b in cpu_loop (env=0x555557999550) at /home/elmarco/src/qemu/linux-user/main.c:297
>  #15 0x00005555555c25b2 in main (argc=2, argv=0x7fffffffd848, envp=0x7fffffffd860) at /home/elmarco/src/qemu/linux-user/main.c:4803
> 
> The fields are set in qemu_init_vcpu() with softmmu, but it's a stub
> with linux-user.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

Applied to x86-next. Thanks.

-- 
Eduardo