[Qemu-devel] [PATCH] qemu will core dump with "-smp 254, sockets=2, cores=3, threads=2"

[Qemu-devel] [PATCH] qemu will core dump with "-smp 254, sockets=2, cores=3, threads=2"

[lijun]

Hi all,

when set "-smp" more than 160, qemu will give the following warning:
Warning: Number of SMP cpus requested (161) exceeds the recommended cpus 
supported by KVM (160)
As the above warning, when set "-smp 160,sockets=2,cores=3,threads=2", 
but find that apic_id(hw/i386/acpi-build.c) is 259 not 159 and 
id(hw/acpi/piix4.c) is 259 not 159.

As the above warning, when set "-smp 254,sockets=2,cores=3,threads=2", 
but find that apic_id(hw/i386/acpi-build.c) is 513 not 253 and 
id(hw/acpi/piix4.c) is 513 not 253.

Based on above reasons, we have two methods to fix this issue.
1, Delete "assert(apic_id <= MAX_CPUMASK_BITS)" in file 
"hw/i386/acpi-build.c" and delete "g_assert((id / 8) < PIIX4_PROC_LEN)" 
in file "hw/acpi/piix4.c".
2, Detect the values of "sockets,cores,threads" when get them from 
command line. And modify smp_parse function in file vl.c to do some 
restrictions on these parameters when boot qemu.

I will submit the code patch later.

Best Regards,
Jun Li

Re: [Qemu-devel] [PATCH] qemu will core dump with "-smp 254, sockets=2, cores=3, threads=2"

[lijun]

Hi all,
     As qemu core dump cause by "sockets=2,cores=3,threads=2", so add 
this patch to check whether cores and threads is a power of 2.
     The following is the realization of apicid_from_topo_ids function 
in file target-i386/topology.h. It uses shift to get the values of 
pkg_id and core_id. nr_cores and nr_threads is related to this shift.
static inline apic_id_t apicid_from_topo_ids(unsigned nr_cores,
                                              unsigned nr_threads,
                                              unsigned pkg_id,
                                              unsigned core_id,
                                              unsigned smt_id)
{
     return (pkg_id  << apicid_pkg_offset(nr_cores, nr_threads)) |
            (core_id << apicid_core_offset(nr_cores, nr_threads)) |
            smt_id;
}
----
So should add a check for smp_cores and smp_threads in smp_parse 
function in file vl.c. Check whether smp_cores and smp_threads is a 
power of 2, so nr_cores and nr_threads is a power of 2.  When return 
from apicid_from_topo_ids function, apic_id and id could get the correct 
values(apic_id is in file "hw/i386/acpi-build.c" and id is in file 
"hw/acpi/piix4.c") .

Without this check for smp_cores and smp_threads, specify "-smp 
160,sockets=2,cores=3,threads=2", qemu will core dump too.


--- a/vl.c    2013-12-14 23:46:58.991076467 +0800
+++ b/vl.c    2013-12-15 00:40:31.653800907 +0800
@@ -1384,6 +1384,19 @@
      },
  };

+/**
+ * This function will return whether @num is power of 2.
+ *
+ * Returns: 1 indicate @num is power of 2, 0 indicate @num is not.
+ */
+static int is_2_power(int num)
+{
+    if (num < 0 || num > 256)
+        return 1;
+
+    return !(num & (num - 1));
+}
+
  static void smp_parse(QemuOpts *opts)
  {
      if (opts) {
@@ -1418,6 +1431,12 @@

      }

+    /* check whether smp_cores and smp_threads is a power of 2 */
+    if (!is_2_power(smp_cores) || !is_2_power(smp_threads)) {
+        smp_cores = 1;
+        smp_threads = 1;
+    }
+
      if (max_cpus == 0) {
          max_cpus = smp_cpus;
      }


Best Regards,
Jun Li

Re: [Qemu-devel] [PATCH] qemu will core dump with "-smp 254, sockets=2, cores=3, threads=2"

[Peter Maydell]

On 14 December 2013 17:21, lijun <junmuzi@gmail.com> wrote:
> Hi all,
>     As qemu core dump cause by "sockets=2,cores=3,threads=2", so add this
> patch to check whether cores and threads is a power of 2.
>     The following is the realization of apicid_from_topo_ids function in
> file target-i386/topology.h. It uses shift to get the values of pkg_id and
> core_id. nr_cores and nr_threads is related to this shift.
> static inline apic_id_t apicid_from_topo_ids(unsigned nr_cores,
>                                              unsigned nr_threads,
>                                              unsigned pkg_id,
>                                              unsigned core_id,
>                                              unsigned smt_id)
> {
>     return (pkg_id  << apicid_pkg_offset(nr_cores, nr_threads)) |
>            (core_id << apicid_core_offset(nr_cores, nr_threads)) |
>            smt_id;
> }
> ----
> So should add a check for smp_cores and smp_threads in smp_parse function in
> file vl.c. Check whether smp_cores and smp_threads is a power of 2, so
> nr_cores and nr_threads is a power of 2.  When return from
> apicid_from_topo_ids function, apic_id and id could get the correct
> values(apic_id is in file "hw/i386/acpi-build.c" and id is in file
> "hw/acpi/piix4.c") .

vl.c is cross platform code so I'm not sure it's the right place to make
this check. x86 ACPI may not be able to handle a non-power-of-2
number of cores in a socket, but it's a v alid ARM SMP configuration,
for instance.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] qemu will core dump with "-smp 254, sockets=2, cores=3, threads=2"

[jun muzi]

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

As Peter's suggestion, mv this patch to file target-i386/topology.h. If
someone has any good idea, please give me a commit in the followings.
Thanks.
---
 target-i386/topology.h | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/target-i386/topology.h b/target-i386/topology.h
index 07a6c5f..cda6bf5 100644
--- a/target-i386/topology.h
+++ b/target-i386/topology.h
@@ -117,6 +117,19 @@ static inline void x86_topo_ids_from_idx(unsigned
nr_cores,
     *pkg_id = core_index / nr_cores;
 }

+/* This function will return whether @num is power of 2.
+ *
+ * Returns: 1 indicate @num is power of 2, 0 indicate @num is not.
+ */
+static int is_2_power(int num)
+{
+    if (num < 0 || num > 256) {
+        return 1;
+    }
+
+    return !(num & (num - 1));
+}
+
 /* Make APIC ID for the CPU 'cpu_index'
  *
  * 'cpu_index' is a sequential, contiguous ID for the CPU.
@@ -126,6 +139,13 @@ static inline apic_id_t
x86_apicid_from_cpu_idx(unsigned nr_cores,
                                                 unsigned cpu_index)
 {
     unsigned pkg_id, core_id, smt_id;
+
+    /* check whether nr_cores and nr_threads is a power of 2 */
+    if (!is_2_power(smp_cores) || !is_2_power(smp_threads)) {
+        nr_cores = 1;
+        nr_threads = 1;
+    }
+
     x86_topo_ids_from_idx(nr_cores, nr_threads, cpu_index,
                           &pkg_id, &core_id, &smt_id);
     return apicid_from_topo_ids(nr_cores, nr_threads, pkg_id, core_id,
smt_id);
-- 
1.8.1.4
-- 
Jun Li <junmuzi@gmail.com>

[-- Attachment #2: Type: text/html, Size: 1774 bytes --]

Re: [Qemu-devel] [PATCH] qemu will core dump with "-smp 254, sockets=2, cores=3, threads=2"

[jun muzi]

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

Type error, change smp_cores to nr_cores and change smp_threads to
nr_threads. But using smp_cores can work well. As it is not the same with
explanatory note, so change it.

Signed-off-by: Jun Li <junmuzi@gmail.com>
---
 target-i386/topology.h | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/target-i386/topology.h b/target-i386/topology.h
index 07a6c5f..49c6331 100644
--- a/target-i386/topology.h
+++ b/target-i386/topology.h
@@ -117,6 +117,19 @@ static inline void x86_topo_ids_from_idx(unsigned
nr_cores,
     *pkg_id = core_index / nr_cores;
 }

+/* This function will return whether @num is power of 2.
+ *
+ * Returns: 1 indicate @num is power of 2, 0 indicate @num is not.
+ */
+static int is_2_power(int num)
+{
+    if (num < 0 || num > 256) {
+        return 1;
+    }
+
+    return !(num & (num - 1));
+}
+
 /* Make APIC ID for the CPU 'cpu_index'
  *
  * 'cpu_index' is a sequential, contiguous ID for the CPU.
@@ -126,6 +139,13 @@ static inline apic_id_t
x86_apicid_from_cpu_idx(unsigned nr_cores,
                                                 unsigned cpu_index)
 {
     unsigned pkg_id, core_id, smt_id;
+
+    /* check whether nr_cores and nr_threads is a power of 2 */
+    if (!is_2_power(nr_cores) || !is_2_power(nr_threads)) {
+        nr_cores = 1;
+        nr_threads = 1;
+    }
+
     x86_topo_ids_from_idx(nr_cores, nr_threads, cpu_index,
                           &pkg_id, &core_id, &smt_id);
     return apicid_from_topo_ids(nr_cores, nr_threads, pkg_id, core_id,
smt_id);
-- 
1.8.1.4

[-- Attachment #2: Type: text/html, Size: 1805 bytes --]

Re: [Qemu-devel] [PATCH] qemu will core dump with "-smp 254, sockets=2, cores=3, threads=2"

[Eric Blake]

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

On 12/14/2013 10:21 AM, lijun wrote:
> Hi all,
>     As qemu core dump cause by "sockets=2,cores=3,threads=2", so add
> this patch to check whether cores and threads is a power of 2.

> +/**
> + * This function will return whether @num is power of 2.
> + *
> + * Returns: 1 indicate @num is power of 2, 0 indicate @num is not.
> + */
> +static int is_2_power(int num)
> +{
> +    if (num < 0 || num > 256)
> +        return 1;
> +
> +    return !(num & (num - 1));
> +}

Please don't reinvent qemu-common.h's is_power_of_2.  Furthermore, your
function is more than just a power-of-2 check, it is also a range check.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH] qemu will core dump with "-smp 254, sockets=2, cores=3, threads=2"

[Eduardo Habkost]

On Mon, Dec 16, 2013 at 03:54:57PM +0800, jun muzi wrote:
> Type error, change smp_cores to nr_cores and change smp_threads to
> nr_threads. But using smp_cores can work well. As it is not the same with
> explanatory note, so change it.
> 
> Signed-off-by: Jun Li <junmuzi@gmail.com>
> ---
>  target-i386/topology.h | 20 ++++++++++++++++++++
[...]
> @@ -126,6 +139,13 @@ static inline apic_id_t
> x86_apicid_from_cpu_idx(unsigned nr_cores,
>                                                  unsigned cpu_index)
>  {
>      unsigned pkg_id, core_id, smt_id;
> +
> +    /* check whether nr_cores and nr_threads is a power of 2 */
> +    if (!is_2_power(nr_cores) || !is_2_power(nr_threads)) {
> +        nr_cores = 1;
> +        nr_threads = 1;
> +    }

I don't understand what you are trying to do here. The whole point of
the topology.h file is to make sure the right CPU topology is exposed to
the guest when nr_cores and nr_threads are not powers of 2.

-- 
Eduardo

Re: [Qemu-devel] [PATCH] qemu will core dump with "-smp 254, sockets=2, cores=3, threads=2"

[Eduardo Habkost]

On Fri, Dec 13, 2013 at 02:10:20AM +0800, lijun wrote:
> Hi all,
> 
> when set "-smp" more than 160, qemu will give the following warning:
> Warning: Number of SMP cpus requested (161) exceeds the recommended
> cpus supported by KVM (160)
> As the above warning, when set "-smp
> 160,sockets=2,cores=3,threads=2", but find that
> apic_id(hw/i386/acpi-build.c) is 259 not 159 and id(hw/acpi/piix4.c)
> is 259 not 159.
> 
> As the above warning, when set "-smp
> 254,sockets=2,cores=3,threads=2", but find that
> apic_id(hw/i386/acpi-build.c) is 513 not 253 and id(hw/acpi/piix4.c)
> is 513 not 253.

"-smp 254,sockets=2,cores=3,threads=2" is invalid because you can't fit
254 VCPUs in 2 cores having 2*3 threads each. Setting both sockets and
cores makes QEMU ignore the "threads" value and set it to
smp/(cores*sockets) (42).

(42 is also an invalid value because two (42*3)-core can have only
252 VCPUs, but that's another bug.)

Anyway, your crash should be also reproducible if you simply run: "-smp
254,sockets=2,cores=3".

But in that case, the APIC ID is right because:

 * With threads=42, we need 6 bits for thread ID
 * With cores=3, we need 2 bits for core ID
   * Bit offset of core ID is 6
   * Bit offset of socket ID is 6+2 = 8
 * CPU index #253 will thread #1 on core #0 on socket #2
   (253 = 2*42*3 + 0*3 + 1)
 * APIC ID for socket #2 core #0 thread #1 is:
   (2<<8) | (0<<6) | 1 = 513

What we need to do to avoid this crash is to reject configurations where
apic_id(max_cpus-1) or apic_id(smp_cpus-1) is too large. I believe this
is what you mean on item 2 below (except that you will need to do that
outside vl.c because the restriction is x86-specific)

> 
> Based on above reasons, we have two methods to fix this issue.
> 1, Delete "assert(apic_id <= MAX_CPUMASK_BITS)" in file
> "hw/i386/acpi-build.c" and delete "g_assert((id / 8) <
> PIIX4_PROC_LEN)" in file "hw/acpi/piix4.c".
> 2, Detect the values of "sockets,cores,threads" when get them from
> command line. And modify smp_parse function in file vl.c to do some
> restrictions on these parameters when boot qemu.
> 
> I will submit the code patch later.
> 
> Best Regards,
> Jun Li
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH] qemu will core dump with "-smp 254, , sockets=2, cores=3, threads=2"

[lijun]

As Eric and Eduardo's suggestions, use is_power_of_2 to check whether 
nr_cores
and nr_threads is the power of 2 in function x86_apicid_from_cpu_idx in file
target-i386/topology.h. This check is very simple, I prefer add it in a
function to write a new function. Thanks for Eric and Eduardo.

Best Regards,
Jun Li

Signed-off-by: Jun Li <junmuzi@gmail.com>
---
  target-i386/topology.h | 8 ++++++++
  1 file changed, 8 insertions(+)

diff --git a/target-i386/topology.h b/target-i386/topology.h
index 07a6c5f..ff21a98 100644
--- a/target-i386/topology.h
+++ b/target-i386/topology.h
@@ -126,6 +126,14 @@ static inline apic_id_t 
x86_apicid_from_cpu_idx(unsigned nr_cores,
                                                  unsigned cpu_index)
  {
      unsigned pkg_id, core_id, smt_id;
+
+    /* Check whether nr_cores and nr_threads is a power of 2.
+     * If not, 1 is assigned to them.
+     */
+    nr_cores = is_power_of_2(nr_cores) > 0 ? nr_cores : 1;
+    nr_threads = is_power_of_2(nr_threads) > 0 ? nr_threads : 1;
+
+
      x86_topo_ids_from_idx(nr_cores, nr_threads, cpu_index,
                            &pkg_id, &core_id, &smt_id);
      return apicid_from_topo_ids(nr_cores, nr_threads, pkg_id, core_id, 
smt_id);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] qemu will core dump with "-smp 254, , sockets=2, cores=3, threads=2"

[Eduardo Habkost]

On Tue, Dec 17, 2013 at 11:16:30PM +0800, lijun wrote:
> As Eric and Eduardo's suggestions, use is_power_of_2 to check
> whether nr_cores
> and nr_threads is the power of 2 in function x86_apicid_from_cpu_idx in file
> target-i386/topology.h. This check is very simple, I prefer add it in a
> function to write a new function. Thanks for Eric and Eduardo.
> 
> Best Regards,
> Jun Li
> 
> Signed-off-by: Jun Li <junmuzi@gmail.com>
> ---
>  target-i386/topology.h | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/target-i386/topology.h b/target-i386/topology.h
> index 07a6c5f..ff21a98 100644
> --- a/target-i386/topology.h
> +++ b/target-i386/topology.h
> @@ -126,6 +126,14 @@ static inline apic_id_t
> x86_apicid_from_cpu_idx(unsigned nr_cores,
>                                                  unsigned cpu_index)
>  {
>      unsigned pkg_id, core_id, smt_id;
> +
> +    /* Check whether nr_cores and nr_threads is a power of 2.
> +     * If not, 1 is assigned to them.
> +     */
> +    nr_cores = is_power_of_2(nr_cores) > 0 ? nr_cores : 1;
> +    nr_threads = is_power_of_2(nr_threads) > 0 ? nr_threads : 1;

I still have the same questions from my previous messages. The whole
point of topology.h is to calculate correct APIC IDs when nr_cores or
nr_threads are not powers of 2. You are simply reintroducing the bugs
that were fixed when topology.h was created.

In other words: x86_apicid_from_cpu_idx(3, 42, 253) must return 513, and
you are breaking it.

See:
  http://article.gmane.org/gmane.comp.emulators.qemu/190390
  Message-Id: <1358886309-26258-1-git-send-email-ehabkost@redhat.com>
and:
  http://article.gmane.org/gmane.comp.emulators.qemu/164044
  Message-Id: <1344369413-9053-1-git-send-email-ehabkost@redhat.com>



> +
> +
>      x86_topo_ids_from_idx(nr_cores, nr_threads, cpu_index,
>                            &pkg_id, &core_id, &smt_id);
>      return apicid_from_topo_ids(nr_cores, nr_threads, pkg_id,
> core_id, smt_id);
> -- 
> 1.8.3.1
> 

-- 
Eduardo