[Qemu-devel] [PATCH 0/4] Improve the CPU hot plug tests

[Qemu-devel] [PATCH 0/4] Improve the CPU hot plug tests

[Thomas Huth]

So far the CPU hot-plug qtest was only checking "cpu-add" on x86. With
these patches, we now test "device_add" for hot-plugging CPUs on x86,
and enable the test on ppc64 and s390x, too.

Thomas Huth (4):
  tests: Rename pc-cpu-test.c to cpu-plug-test.c
  tests/cpu-plug-test: Check the CPU hot-plugging with device_add, too
  tests/cpu-plug-test: Check CPU hot-plugging on ppc64, too
  tests/cpu-plug-test: Test CPU hot-plugging on s390x

 tests/Makefile.include |   6 +-
 tests/cpu-plug-test.c  | 261 +++++++++++++++++++++++++++++++++++++++++++++++++
 tests/pc-cpu-test.c    | 135 -------------------------
 3 files changed, 265 insertions(+), 137 deletions(-)
 create mode 100644 tests/cpu-plug-test.c
 delete mode 100644 tests/pc-cpu-test.c

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/4] tests: Rename pc-cpu-test.c to cpu-plug-test.c

[Thomas Huth]

The test will be extended to work on other architectures, too, so let's
use a more generic name for the file and the functions in here first.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 tests/Makefile.include                   |  4 ++--
 tests/{pc-cpu-test.c => cpu-plug-test.c} | 24 ++++++++++++------------
 2 files changed, 14 insertions(+), 14 deletions(-)
 rename tests/{pc-cpu-test.c => cpu-plug-test.c} (87%)

diff --git a/tests/Makefile.include b/tests/Makefile.include
index 4ca15e6..3d7e814 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -276,7 +276,7 @@ gcov-files-i386-y += hw/usb/dev-hid.c
 gcov-files-i386-y += hw/usb/dev-storage.c
 check-qtest-i386-y += tests/usb-hcd-xhci-test$(EXESUF)
 gcov-files-i386-y += hw/usb/hcd-xhci.c
-check-qtest-i386-y += tests/pc-cpu-test$(EXESUF)
+check-qtest-i386-y += tests/cpu-plug-test$(EXESUF)
 check-qtest-i386-y += tests/q35-test$(EXESUF)
 check-qtest-i386-y += tests/vmgenid-test$(EXESUF)
 gcov-files-i386-y += hw/pci-host/q35.c
@@ -780,7 +780,7 @@ tests/usb-hcd-ohci-test$(EXESUF): tests/usb-hcd-ohci-test.o $(libqos-usb-obj-y)
 tests/usb-hcd-uhci-test$(EXESUF): tests/usb-hcd-uhci-test.o $(libqos-usb-obj-y)
 tests/usb-hcd-ehci-test$(EXESUF): tests/usb-hcd-ehci-test.o $(libqos-usb-obj-y)
 tests/usb-hcd-xhci-test$(EXESUF): tests/usb-hcd-xhci-test.o $(libqos-usb-obj-y)
-tests/pc-cpu-test$(EXESUF): tests/pc-cpu-test.o
+tests/cpu-plug-test$(EXESUF): tests/cpu-plug-test.o
 tests/postcopy-test$(EXESUF): tests/postcopy-test.o
 tests/vhost-user-test$(EXESUF): tests/vhost-user-test.o $(test-util-obj-y) \
 	$(qtest-obj-y) $(test-io-obj-y) $(libqos-virtio-obj-y) $(libqos-pc-obj-y) \
diff --git a/tests/pc-cpu-test.c b/tests/cpu-plug-test.c
similarity index 87%
rename from tests/pc-cpu-test.c
rename to tests/cpu-plug-test.c
index 11d3e81..4579119 100644
--- a/tests/pc-cpu-test.c
+++ b/tests/cpu-plug-test.c
@@ -1,5 +1,5 @@
 /*
- * QTest testcase for PC CPUs
+ * QTest testcase for CPU plugging
  *
  * Copyright (c) 2015 SUSE Linux GmbH
  *
@@ -13,7 +13,7 @@
 #include "libqtest.h"
 #include "qapi/qmp/types.h"
 
-struct PCTestData {
+struct PlugTestData {
     char *machine;
     const char *cpu_model;
     unsigned sockets;
@@ -21,11 +21,11 @@ struct PCTestData {
     unsigned threads;
     unsigned maxcpus;
 };
-typedef struct PCTestData PCTestData;
+typedef struct PlugTestData PlugTestData;
 
-static void test_pc_with_cpu_add(gconstpointer data)
+static void test_plug_with_cpu_add(gconstpointer data)
 {
-    const PCTestData *s = data;
+    const PlugTestData *s = data;
     char *args;
     QDict *response;
     unsigned int i;
@@ -48,9 +48,9 @@ static void test_pc_with_cpu_add(gconstpointer data)
     g_free(args);
 }
 
-static void test_pc_without_cpu_add(gconstpointer data)
+static void test_plug_without_cpu_add(gconstpointer data)
 {
-    const PCTestData *s = data;
+    const PlugTestData *s = data;
     char *args;
     QDict *response;
 
@@ -73,7 +73,7 @@ static void test_pc_without_cpu_add(gconstpointer data)
 
 static void test_data_free(gpointer data)
 {
-    PCTestData *pc = data;
+    PlugTestData *pc = data;
 
     g_free(pc->machine);
     g_free(pc);
@@ -82,12 +82,12 @@ static void test_data_free(gpointer data)
 static void add_pc_test_case(const char *mname)
 {
     char *path;
-    PCTestData *data;
+    PlugTestData *data;
 
     if (!g_str_has_prefix(mname, "pc-")) {
         return;
     }
-    data = g_new(PCTestData, 1);
+    data = g_new(PlugTestData, 1);
     data->machine = g_strdup(mname);
     data->cpu_model = "Haswell"; /* 1.3+ theoretically */
     data->sockets = 1;
@@ -108,14 +108,14 @@ static void add_pc_test_case(const char *mname)
         path = g_strdup_printf("cpu/%s/init/%ux%ux%u&maxcpus=%u",
                                mname, data->sockets, data->cores,
                                data->threads, data->maxcpus);
-        qtest_add_data_func_full(path, data, test_pc_without_cpu_add,
+        qtest_add_data_func_full(path, data, test_plug_without_cpu_add,
                                  test_data_free);
         g_free(path);
     } else {
         path = g_strdup_printf("cpu/%s/add/%ux%ux%u&maxcpus=%u",
                                mname, data->sockets, data->cores,
                                data->threads, data->maxcpus);
-        qtest_add_data_func_full(path, data, test_pc_with_cpu_add,
+        qtest_add_data_func_full(path, data, test_plug_with_cpu_add,
                                  test_data_free);
         g_free(path);
     }
-- 
1.8.3.1

[Qemu-devel] [PATCH 2/4] tests/cpu-plug-test: Check the CPU hot-plugging with device_add, too

[Thomas Huth]

Using 'device_add' instead of 'cpu-add' is the new way for
hot-plugging CPUs, so we should test this regularly, too.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 tests/cpu-plug-test.c | 42 ++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 40 insertions(+), 2 deletions(-)

diff --git a/tests/cpu-plug-test.c b/tests/cpu-plug-test.c
index 4579119..5fef297 100644
--- a/tests/cpu-plug-test.c
+++ b/tests/cpu-plug-test.c
@@ -16,6 +16,7 @@
 struct PlugTestData {
     char *machine;
     const char *cpu_model;
+    const char *device_model;
     unsigned sockets;
     unsigned cores;
     unsigned threads;
@@ -71,6 +72,34 @@ static void test_plug_without_cpu_add(gconstpointer data)
     g_free(args);
 }
 
+static void test_plug_with_device_add_x86(gconstpointer data)
+{
+    const PlugTestData *td = data;
+    char *args;
+    unsigned int s, c, t;
+
+    args = g_strdup_printf("-machine %s -cpu %s "
+                           "-smp sockets=%u,cores=%u,threads=%u,maxcpus=%u",
+                           td->machine, td->cpu_model,
+                           td->sockets, td->cores, td->threads, td->maxcpus);
+    qtest_start(args);
+
+    for (s = td->sockets; s < td->maxcpus / td->cores / td->threads; s++) {
+        for (c = 0; c < td->cores; c++) {
+            for (t = 0; t < td->threads; t++) {
+                char *id = g_strdup_printf("id-%i-%i-%i", s, c, t);
+                qtest_qmp_device_add(td->device_model, id, "'socket-id':'%i', "
+                                     "'core-id':'%i', 'thread-id':'%i'",
+                                     s, c, t);
+                g_free(id);
+            }
+        }
+    }
+
+    qtest_end();
+    g_free(args);
+}
+
 static void test_data_free(gpointer data)
 {
     PlugTestData *pc = data;
@@ -90,6 +119,7 @@ static void add_pc_test_case(const char *mname)
     data = g_new(PlugTestData, 1);
     data->machine = g_strdup(mname);
     data->cpu_model = "Haswell"; /* 1.3+ theoretically */
+    data->device_model = "Haswell-x86_64-cpu";
     data->sockets = 1;
     data->cores = 3;
     data->threads = 2;
@@ -105,19 +135,27 @@ static void add_pc_test_case(const char *mname)
         (strcmp(mname, "pc-0.12") == 0) ||
         (strcmp(mname, "pc-0.11") == 0) ||
         (strcmp(mname, "pc-0.10") == 0)) {
-        path = g_strdup_printf("cpu/%s/init/%ux%ux%u&maxcpus=%u",
+        path = g_strdup_printf("cpu-plug/%s/init/%ux%ux%u&maxcpus=%u",
                                mname, data->sockets, data->cores,
                                data->threads, data->maxcpus);
         qtest_add_data_func_full(path, data, test_plug_without_cpu_add,
                                  test_data_free);
         g_free(path);
     } else {
-        path = g_strdup_printf("cpu/%s/add/%ux%ux%u&maxcpus=%u",
+        PlugTestData *data2 = g_memdup(data, sizeof(PlugTestData));
+        data2->machine = g_strdup(mname);
+        path = g_strdup_printf("cpu-plug/%s/cpu-add/%ux%ux%u&maxcpus=%u",
                                mname, data->sockets, data->cores,
                                data->threads, data->maxcpus);
         qtest_add_data_func_full(path, data, test_plug_with_cpu_add,
                                  test_data_free);
         g_free(path);
+        path = g_strdup_printf("cpu-plug/%s/device-add/%ux%ux%u&maxcpus=%u",
+                               mname, data2->sockets, data2->cores,
+                               data2->threads, data2->maxcpus);
+        qtest_add_data_func_full(path, data2, test_plug_with_device_add_x86,
+                                 test_data_free);
+        g_free(path);
     }
 }
 
-- 
1.8.3.1

[Qemu-devel] [PATCH 3/4] tests/cpu-plug-test: Check CPU hot-plugging on ppc64, too

[Thomas Huth]

Hot plugging on ppc64 is possible via "device_add", too. Unlike x86,
we must not specify a 'socket-id' and 'thread-id' here, so this needs
to be done with a separate function that just specifies the 'core-id'
during the "device_add".

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 tests/Makefile.include |  1 +
 tests/cpu-plug-test.c  | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 51 insertions(+)

diff --git a/tests/Makefile.include b/tests/Makefile.include
index 3d7e814..cbec19f 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -332,6 +332,7 @@ check-qtest-ppc64-$(CONFIG_POSIX) += tests/test-filter-redirector$(EXESUF)
 check-qtest-ppc64-y += tests/display-vga-test$(EXESUF)
 check-qtest-ppc64-y += tests/numa-test$(EXESUF)
 check-qtest-ppc64-$(CONFIG_IVSHMEM) += tests/ivshmem-test$(EXESUF)
+check-qtest-ppc64-y += tests/cpu-plug-test$(EXESUF)
 
 check-qtest-sh4-y = tests/endianness-test$(EXESUF)
 
diff --git a/tests/cpu-plug-test.c b/tests/cpu-plug-test.c
index 5fef297..714a69c 100644
--- a/tests/cpu-plug-test.c
+++ b/tests/cpu-plug-test.c
@@ -100,6 +100,28 @@ static void test_plug_with_device_add_x86(gconstpointer data)
     g_free(args);
 }
 
+static void test_plug_with_device_add_coreid(gconstpointer data)
+{
+    const PlugTestData *td = data;
+    char *args;
+    unsigned int c;
+
+    args = g_strdup_printf("-machine %s -cpu %s "
+                           "-smp 1,sockets=%u,cores=%u,threads=%u,maxcpus=%u",
+                           td->machine, td->cpu_model,
+                           td->sockets, td->cores, td->threads, td->maxcpus);
+    qtest_start(args);
+
+    for (c = td->cores; c < td->maxcpus / td->sockets / td->threads; c++) {
+        char *id = g_strdup_printf("id-%i", c);
+        qtest_qmp_device_add(td->device_model, id, "'core-id':'%i'", c);
+        g_free(id);
+    }
+
+    qtest_end();
+    g_free(args);
+}
+
 static void test_data_free(gpointer data)
 {
     PlugTestData *pc = data;
@@ -159,6 +181,32 @@ static void add_pc_test_case(const char *mname)
     }
 }
 
+static void add_pseries_test_case(const char *mname)
+{
+    char *path;
+    PlugTestData *data;
+
+    if (!g_str_has_prefix(mname, "pseries-") ||
+        (g_str_has_prefix(mname, "pseries-2.") && atoi(&mname[10]) < 7)) {
+        return;
+    }
+    data = g_new(PlugTestData, 1);
+    data->machine = g_strdup(mname);
+    data->cpu_model = "POWER8_v2.0";
+    data->device_model = "power8_v2.0-spapr-cpu-core";
+    data->sockets = 2;
+    data->cores = 3;
+    data->threads = 1;
+    data->maxcpus = data->sockets * data->cores * data->threads * 2;
+
+    path = g_strdup_printf("cpu-plug/%s/device-add/%ux%ux%u&maxcpus=%u",
+                           mname, data->sockets, data->cores,
+                           data->threads, data->maxcpus);
+    qtest_add_data_func_full(path, data, test_plug_with_device_add_coreid,
+                             test_data_free);
+    g_free(path);
+}
+
 int main(int argc, char **argv)
 {
     const char *arch = qtest_get_arch();
@@ -167,6 +215,8 @@ int main(int argc, char **argv)
 
     if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
         qtest_cb_for_every_machine(add_pc_test_case);
+    } else if (g_str_equal(arch, "ppc64")) {
+        qtest_cb_for_every_machine(add_pseries_test_case);
     }
 
     return g_test_run();
-- 
1.8.3.1

[Qemu-devel] [PATCH 4/4] tests/cpu-plug-test: Test CPU hot-plugging on s390x

[Thomas Huth]

CPU hot-plugging on s390x is possible with both, "cpu-add"
and "device_add", so test both.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 tests/Makefile.include |  1 +
 tests/cpu-plug-test.c  | 38 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 39 insertions(+)

diff --git a/tests/Makefile.include b/tests/Makefile.include
index cbec19f..85cf9ea 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -368,6 +368,7 @@ check-qtest-s390x-$(CONFIG_SLIRP) += tests/test-netfilter$(EXESUF)
 check-qtest-s390x-$(CONFIG_POSIX) += tests/test-filter-mirror$(EXESUF)
 check-qtest-s390x-$(CONFIG_POSIX) += tests/test-filter-redirector$(EXESUF)
 check-qtest-s390x-y += tests/drive_del-test$(EXESUF)
+check-qtest-s390x-y += tests/cpu-plug-test$(EXESUF)
 
 check-qtest-generic-y += tests/qom-test$(EXESUF)
 check-qtest-generic-y += tests/test-hmp$(EXESUF)
diff --git a/tests/cpu-plug-test.c b/tests/cpu-plug-test.c
index 714a69c..58edaf6 100644
--- a/tests/cpu-plug-test.c
+++ b/tests/cpu-plug-test.c
@@ -207,6 +207,42 @@ static void add_pseries_test_case(const char *mname)
     g_free(path);
 }
 
+static void add_s390x_test_case(const char *mname)
+{
+    char *path;
+    PlugTestData *data, *data2;
+
+    if (!g_str_has_prefix(mname, "s390-ccw-virtio-")) {
+        return;
+    }
+
+    data = g_new(PlugTestData, 1);
+    data->machine = g_strdup(mname);
+    data->cpu_model = "qemu";
+    data->device_model = "qemu-s390-cpu";
+    data->sockets = 1;
+    data->cores = 3;
+    data->threads = 1;
+    data->maxcpus = data->sockets * data->cores * data->threads * 2;
+
+    data2 = g_memdup(data, sizeof(PlugTestData));
+    data2->machine = g_strdup(mname);
+
+    path = g_strdup_printf("cpu-plug/%s/cpu-add/%ux%ux%u&maxcpus=%u",
+                           mname, data->sockets, data->cores,
+                           data->threads, data->maxcpus);
+    qtest_add_data_func_full(path, data, test_plug_with_cpu_add,
+                             test_data_free);
+    g_free(path);
+
+    path = g_strdup_printf("cpu-plug/%s/device-add/%ux%ux%u&maxcpus=%u",
+                           mname, data2->sockets, data2->cores,
+                           data2->threads, data2->maxcpus);
+    qtest_add_data_func_full(path, data2, test_plug_with_device_add_coreid,
+                             test_data_free);
+    g_free(path);
+}
+
 int main(int argc, char **argv)
 {
     const char *arch = qtest_get_arch();
@@ -217,6 +253,8 @@ int main(int argc, char **argv)
         qtest_cb_for_every_machine(add_pc_test_case);
     } else if (g_str_equal(arch, "ppc64")) {
         qtest_cb_for_every_machine(add_pseries_test_case);
+    } else if (g_str_equal(arch, "s390x")) {
+        qtest_cb_for_every_machine(add_s390x_test_case);
     }
 
     return g_test_run();
-- 
1.8.3.1

Re: [Qemu-devel] [Qemu-ppc] [PATCH 3/4] tests/cpu-plug-test: Check CPU hot-plugging on ppc64, too

[Daniel Henrique Barboza]

On 10/17/2017 11:32 AM, Thomas Huth wrote:
> Hot plugging on ppc64 is possible via "device_add", too. Unlike x86,
> we must not specify a 'socket-id' and 'thread-id' here, so this needs
> to be done with a separate function that just specifies the 'core-id'
> during the "device_add".
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>

Reviewed-by: Daniel Barboza <danielhb@linux.vnet.ibm.com>
Tested-by: Daniel Barboza <danielhb@linux.vnet.ibm.com>

> ---
>   tests/Makefile.include |  1 +
>   tests/cpu-plug-test.c  | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 51 insertions(+)
>
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index 3d7e814..cbec19f 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -332,6 +332,7 @@ check-qtest-ppc64-$(CONFIG_POSIX) += tests/test-filter-redirector$(EXESUF)
>   check-qtest-ppc64-y += tests/display-vga-test$(EXESUF)
>   check-qtest-ppc64-y += tests/numa-test$(EXESUF)
>   check-qtest-ppc64-$(CONFIG_IVSHMEM) += tests/ivshmem-test$(EXESUF)
> +check-qtest-ppc64-y += tests/cpu-plug-test$(EXESUF)
>
>   check-qtest-sh4-y = tests/endianness-test$(EXESUF)
>
> diff --git a/tests/cpu-plug-test.c b/tests/cpu-plug-test.c
> index 5fef297..714a69c 100644
> --- a/tests/cpu-plug-test.c
> +++ b/tests/cpu-plug-test.c
> @@ -100,6 +100,28 @@ static void test_plug_with_device_add_x86(gconstpointer data)
>       g_free(args);
>   }
>
> +static void test_plug_with_device_add_coreid(gconstpointer data)
> +{
> +    const PlugTestData *td = data;
> +    char *args;
> +    unsigned int c;
> +
> +    args = g_strdup_printf("-machine %s -cpu %s "
> +                           "-smp 1,sockets=%u,cores=%u,threads=%u,maxcpus=%u",
> +                           td->machine, td->cpu_model,
> +                           td->sockets, td->cores, td->threads, td->maxcpus);
> +    qtest_start(args);
> +
> +    for (c = td->cores; c < td->maxcpus / td->sockets / td->threads; c++) {
> +        char *id = g_strdup_printf("id-%i", c);
> +        qtest_qmp_device_add(td->device_model, id, "'core-id':'%i'", c);
> +        g_free(id);
> +    }
> +
> +    qtest_end();
> +    g_free(args);
> +}
> +
>   static void test_data_free(gpointer data)
>   {
>       PlugTestData *pc = data;
> @@ -159,6 +181,32 @@ static void add_pc_test_case(const char *mname)
>       }
>   }
>
> +static void add_pseries_test_case(const char *mname)
> +{
> +    char *path;
> +    PlugTestData *data;
> +
> +    if (!g_str_has_prefix(mname, "pseries-") ||
> +        (g_str_has_prefix(mname, "pseries-2.") && atoi(&mname[10]) < 7)) {
> +        return;
> +    }
> +    data = g_new(PlugTestData, 1);
> +    data->machine = g_strdup(mname);
> +    data->cpu_model = "POWER8_v2.0";
> +    data->device_model = "power8_v2.0-spapr-cpu-core";
> +    data->sockets = 2;
> +    data->cores = 3;
> +    data->threads = 1;
> +    data->maxcpus = data->sockets * data->cores * data->threads * 2;
> +
> +    path = g_strdup_printf("cpu-plug/%s/device-add/%ux%ux%u&maxcpus=%u",
> +                           mname, data->sockets, data->cores,
> +                           data->threads, data->maxcpus);
> +    qtest_add_data_func_full(path, data, test_plug_with_device_add_coreid,
> +                             test_data_free);
> +    g_free(path);
> +}
> +
>   int main(int argc, char **argv)
>   {
>       const char *arch = qtest_get_arch();
> @@ -167,6 +215,8 @@ int main(int argc, char **argv)
>
>       if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
>           qtest_cb_for_every_machine(add_pc_test_case);
> +    } else if (g_str_equal(arch, "ppc64")) {
> +        qtest_cb_for_every_machine(add_pseries_test_case);
>       }
>
>       return g_test_run();

Re: [Qemu-devel] [Qemu-ppc] [PATCH 0/4] Improve the CPU hot plug tests

[Daniel Henrique Barboza]

On 10/17/2017 11:32 AM, Thomas Huth wrote:
> So far the CPU hot-plug qtest was only checking "cpu-add" on x86. With
> these patches, we now test "device_add" for hot-plugging CPUs on x86,
> and enable the test on ppc64 and s390x, too.

Question: is there a reason other than "no one bothered" to not have a 
CPU hot
unplug test, something around the lines of the now cpu-plug-test.c? I might
give it a shot if there is no known roadblocks against it.


Thanks,

Daniel

>
> Thomas Huth (4):
>    tests: Rename pc-cpu-test.c to cpu-plug-test.c
>    tests/cpu-plug-test: Check the CPU hot-plugging with device_add, too
>    tests/cpu-plug-test: Check CPU hot-plugging on ppc64, too
>    tests/cpu-plug-test: Test CPU hot-plugging on s390x
>
>   tests/Makefile.include |   6 +-
>   tests/cpu-plug-test.c  | 261 +++++++++++++++++++++++++++++++++++++++++++++++++
>   tests/pc-cpu-test.c    | 135 -------------------------
>   3 files changed, 265 insertions(+), 137 deletions(-)
>   create mode 100644 tests/cpu-plug-test.c
>   delete mode 100644 tests/pc-cpu-test.c
>

Re: [Qemu-devel] [Qemu-ppc] [PATCH 0/4] Improve the CPU hot plug tests

[Thomas Huth]

On 17.10.2017 18:27, Daniel Henrique Barboza wrote:
> 
> 
> On 10/17/2017 11:32 AM, Thomas Huth wrote:
>> So far the CPU hot-plug qtest was only checking "cpu-add" on x86. With
>> these patches, we now test "device_add" for hot-plugging CPUs on x86,
>> and enable the test on ppc64 and s390x, too.
> 
> Question: is there a reason other than "no one bothered" to not have a
> CPU hot
> unplug test, something around the lines of the now cpu-plug-test.c? I might
> give it a shot if there is no known roadblocks against it.

AFAIK on x86, you need some ACPI magic on the guest side to do this
(something similar to qpci_unplug_acpi_device_test() in
tests/libqos/pci-pc.c I guess). And on s390x, CPU unplug is not
supported at all. But it might work on ppc64 without guest intervention,
not sure - maybe Bharata can comment on this...

 Thomas

Re: [Qemu-devel] [PATCH 1/4] tests: Rename pc-cpu-test.c to cpu-plug-test.c

[Philippe Mathieu-Daudé]

On 10/17/2017 10:32 AM, Thomas Huth wrote:
> The test will be extended to work on other architectures, too, so let's
> use a more generic name for the file and the functions in here first.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  tests/Makefile.include                   |  4 ++--
>  tests/{pc-cpu-test.c => cpu-plug-test.c} | 24 ++++++++++++------------
>  2 files changed, 14 insertions(+), 14 deletions(-)
>  rename tests/{pc-cpu-test.c => cpu-plug-test.c} (87%)
> 
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index 4ca15e6..3d7e814 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -276,7 +276,7 @@ gcov-files-i386-y += hw/usb/dev-hid.c
>  gcov-files-i386-y += hw/usb/dev-storage.c
>  check-qtest-i386-y += tests/usb-hcd-xhci-test$(EXESUF)
>  gcov-files-i386-y += hw/usb/hcd-xhci.c
> -check-qtest-i386-y += tests/pc-cpu-test$(EXESUF)
> +check-qtest-i386-y += tests/cpu-plug-test$(EXESUF)
>  check-qtest-i386-y += tests/q35-test$(EXESUF)
>  check-qtest-i386-y += tests/vmgenid-test$(EXESUF)
>  gcov-files-i386-y += hw/pci-host/q35.c
> @@ -780,7 +780,7 @@ tests/usb-hcd-ohci-test$(EXESUF): tests/usb-hcd-ohci-test.o $(libqos-usb-obj-y)
>  tests/usb-hcd-uhci-test$(EXESUF): tests/usb-hcd-uhci-test.o $(libqos-usb-obj-y)
>  tests/usb-hcd-ehci-test$(EXESUF): tests/usb-hcd-ehci-test.o $(libqos-usb-obj-y)
>  tests/usb-hcd-xhci-test$(EXESUF): tests/usb-hcd-xhci-test.o $(libqos-usb-obj-y)
> -tests/pc-cpu-test$(EXESUF): tests/pc-cpu-test.o
> +tests/cpu-plug-test$(EXESUF): tests/cpu-plug-test.o
>  tests/postcopy-test$(EXESUF): tests/postcopy-test.o
>  tests/vhost-user-test$(EXESUF): tests/vhost-user-test.o $(test-util-obj-y) \
>  	$(qtest-obj-y) $(test-io-obj-y) $(libqos-virtio-obj-y) $(libqos-pc-obj-y) \
> diff --git a/tests/pc-cpu-test.c b/tests/cpu-plug-test.c
> similarity index 87%
> rename from tests/pc-cpu-test.c
> rename to tests/cpu-plug-test.c
> index 11d3e81..4579119 100644
> --- a/tests/pc-cpu-test.c
> +++ b/tests/cpu-plug-test.c
> @@ -1,5 +1,5 @@
>  /*
> - * QTest testcase for PC CPUs
> + * QTest testcase for CPU plugging
>   *
>   * Copyright (c) 2015 SUSE Linux GmbH
>   *
> @@ -13,7 +13,7 @@
>  #include "libqtest.h"
>  #include "qapi/qmp/types.h"
>  
> -struct PCTestData {
> +struct PlugTestData {
>      char *machine;
>      const char *cpu_model;
>      unsigned sockets;
> @@ -21,11 +21,11 @@ struct PCTestData {
>      unsigned threads;
>      unsigned maxcpus;
>  };
> -typedef struct PCTestData PCTestData;
> +typedef struct PlugTestData PlugTestData;
>  
> -static void test_pc_with_cpu_add(gconstpointer data)
> +static void test_plug_with_cpu_add(gconstpointer data)
>  {
> -    const PCTestData *s = data;
> +    const PlugTestData *s = data;
>      char *args;
>      QDict *response;
>      unsigned int i;
> @@ -48,9 +48,9 @@ static void test_pc_with_cpu_add(gconstpointer data)
>      g_free(args);
>  }
>  
> -static void test_pc_without_cpu_add(gconstpointer data)
> +static void test_plug_without_cpu_add(gconstpointer data)
>  {
> -    const PCTestData *s = data;
> +    const PlugTestData *s = data;
>      char *args;
>      QDict *response;
>  
> @@ -73,7 +73,7 @@ static void test_pc_without_cpu_add(gconstpointer data)
>  
>  static void test_data_free(gpointer data)
>  {
> -    PCTestData *pc = data;
> +    PlugTestData *pc = data;
>  
>      g_free(pc->machine);
>      g_free(pc);
> @@ -82,12 +82,12 @@ static void test_data_free(gpointer data)
>  static void add_pc_test_case(const char *mname)
>  {
>      char *path;
> -    PCTestData *data;
> +    PlugTestData *data;
>  
>      if (!g_str_has_prefix(mname, "pc-")) {
>          return;
>      }
> -    data = g_new(PCTestData, 1);
> +    data = g_new(PlugTestData, 1);
>      data->machine = g_strdup(mname);
>      data->cpu_model = "Haswell"; /* 1.3+ theoretically */
>      data->sockets = 1;
> @@ -108,14 +108,14 @@ static void add_pc_test_case(const char *mname)
>          path = g_strdup_printf("cpu/%s/init/%ux%ux%u&maxcpus=%u",
>                                 mname, data->sockets, data->cores,
>                                 data->threads, data->maxcpus);
> -        qtest_add_data_func_full(path, data, test_pc_without_cpu_add,
> +        qtest_add_data_func_full(path, data, test_plug_without_cpu_add,
>                                   test_data_free);
>          g_free(path);
>      } else {
>          path = g_strdup_printf("cpu/%s/add/%ux%ux%u&maxcpus=%u",
>                                 mname, data->sockets, data->cores,
>                                 data->threads, data->maxcpus);
> -        qtest_add_data_func_full(path, data, test_pc_with_cpu_add,
> +        qtest_add_data_func_full(path, data, test_plug_with_cpu_add,
>                                   test_data_free);
>          g_free(path);
>      }
> 

Re: [Qemu-devel] [Qemu-ppc] [PATCH 0/4] Improve the CPU hot plug tests

[Daniel Henrique Barboza]

On 10/17/2017 02:36 PM, Thomas Huth wrote:
> On 17.10.2017 18:27, Daniel Henrique Barboza wrote:
>>
>> On 10/17/2017 11:32 AM, Thomas Huth wrote:
>>> So far the CPU hot-plug qtest was only checking "cpu-add" on x86. With
>>> these patches, we now test "device_add" for hot-plugging CPUs on x86,
>>> and enable the test on ppc64 and s390x, too.
>> Question: is there a reason other than "no one bothered" to not have a
>> CPU hot
>> unplug test, something around the lines of the now cpu-plug-test.c? I might
>> give it a shot if there is no known roadblocks against it.
> AFAIK on x86, you need some ACPI magic on the guest side to do this
> (something similar to qpci_unplug_acpi_device_test() in
> tests/libqos/pci-pc.c I guess). And on s390x, CPU unplug is not
> supported at all. But it might work on ppc64 without guest intervention,
> not sure - maybe Bharata can comment on this...

Good point, it won't work in spapr machine without guest intervention too.
Both CPU and LMB unplug relies on guest-side callbacks to complete the
operation.

I'll see how this ACPI magic you mentioned is done and see how hard it is to
do it for ppc64.


Daniel

>
>   Thomas
>

Re: [Qemu-devel] [PATCH 1/4] tests: Rename pc-cpu-test.c to cpu-plug-test.c

[David Hildenbrand]

On 17.10.2017 15:32, Thomas Huth wrote:
> The test will be extended to work on other architectures, too, so let's
> use a more generic name for the file and the functions in here first.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  tests/Makefile.include                   |  4 ++--
>  tests/{pc-cpu-test.c => cpu-plug-test.c} | 24 ++++++++++++------------
>  2 files changed, 14 insertions(+), 14 deletions(-)
>  rename tests/{pc-cpu-test.c => cpu-plug-test.c} (87%)
> 
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index 4ca15e6..3d7e814 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -276,7 +276,7 @@ gcov-files-i386-y += hw/usb/dev-hid.c
>  gcov-files-i386-y += hw/usb/dev-storage.c
>  check-qtest-i386-y += tests/usb-hcd-xhci-test$(EXESUF)
>  gcov-files-i386-y += hw/usb/hcd-xhci.c
> -check-qtest-i386-y += tests/pc-cpu-test$(EXESUF)
> +check-qtest-i386-y += tests/cpu-plug-test$(EXESUF)
>  check-qtest-i386-y += tests/q35-test$(EXESUF)
>  check-qtest-i386-y += tests/vmgenid-test$(EXESUF)
>  gcov-files-i386-y += hw/pci-host/q35.c
> @@ -780,7 +780,7 @@ tests/usb-hcd-ohci-test$(EXESUF): tests/usb-hcd-ohci-test.o $(libqos-usb-obj-y)
>  tests/usb-hcd-uhci-test$(EXESUF): tests/usb-hcd-uhci-test.o $(libqos-usb-obj-y)
>  tests/usb-hcd-ehci-test$(EXESUF): tests/usb-hcd-ehci-test.o $(libqos-usb-obj-y)
>  tests/usb-hcd-xhci-test$(EXESUF): tests/usb-hcd-xhci-test.o $(libqos-usb-obj-y)
> -tests/pc-cpu-test$(EXESUF): tests/pc-cpu-test.o
> +tests/cpu-plug-test$(EXESUF): tests/cpu-plug-test.o
>  tests/postcopy-test$(EXESUF): tests/postcopy-test.o
>  tests/vhost-user-test$(EXESUF): tests/vhost-user-test.o $(test-util-obj-y) \
>  	$(qtest-obj-y) $(test-io-obj-y) $(libqos-virtio-obj-y) $(libqos-pc-obj-y) \
> diff --git a/tests/pc-cpu-test.c b/tests/cpu-plug-test.c
> similarity index 87%
> rename from tests/pc-cpu-test.c
> rename to tests/cpu-plug-test.c
> index 11d3e81..4579119 100644
> --- a/tests/pc-cpu-test.c
> +++ b/tests/cpu-plug-test.c
> @@ -1,5 +1,5 @@
>  /*
> - * QTest testcase for PC CPUs
> + * QTest testcase for CPU plugging
>   *
>   * Copyright (c) 2015 SUSE Linux GmbH
>   *
> @@ -13,7 +13,7 @@
>  #include "libqtest.h"
>  #include "qapi/qmp/types.h"
>  
> -struct PCTestData {
> +struct PlugTestData {
>      char *machine;
>      const char *cpu_model;
>      unsigned sockets;
> @@ -21,11 +21,11 @@ struct PCTestData {
>      unsigned threads;
>      unsigned maxcpus;
>  };
> -typedef struct PCTestData PCTestData;
> +typedef struct PlugTestData PlugTestData;
>  
> -static void test_pc_with_cpu_add(gconstpointer data)
> +static void test_plug_with_cpu_add(gconstpointer data)
>  {
> -    const PCTestData *s = data;
> +    const PlugTestData *s = data;
>      char *args;
>      QDict *response;
>      unsigned int i;
> @@ -48,9 +48,9 @@ static void test_pc_with_cpu_add(gconstpointer data)
>      g_free(args);
>  }
>  
> -static void test_pc_without_cpu_add(gconstpointer data)
> +static void test_plug_without_cpu_add(gconstpointer data)
>  {
> -    const PCTestData *s = data;
> +    const PlugTestData *s = data;
>      char *args;
>      QDict *response;
>  
> @@ -73,7 +73,7 @@ static void test_pc_without_cpu_add(gconstpointer data)
>  
>  static void test_data_free(gpointer data)
>  {
> -    PCTestData *pc = data;
> +    PlugTestData *pc = data;
>  
>      g_free(pc->machine);
>      g_free(pc);
> @@ -82,12 +82,12 @@ static void test_data_free(gpointer data)
>  static void add_pc_test_case(const char *mname)
>  {
>      char *path;
> -    PCTestData *data;
> +    PlugTestData *data;
>  
>      if (!g_str_has_prefix(mname, "pc-")) {
>          return;
>      }
> -    data = g_new(PCTestData, 1);
> +    data = g_new(PlugTestData, 1);
>      data->machine = g_strdup(mname);
>      data->cpu_model = "Haswell"; /* 1.3+ theoretically */
>      data->sockets = 1;
> @@ -108,14 +108,14 @@ static void add_pc_test_case(const char *mname)
>          path = g_strdup_printf("cpu/%s/init/%ux%ux%u&maxcpus=%u",
>                                 mname, data->sockets, data->cores,
>                                 data->threads, data->maxcpus);
> -        qtest_add_data_func_full(path, data, test_pc_without_cpu_add,
> +        qtest_add_data_func_full(path, data, test_plug_without_cpu_add,
>                                   test_data_free);
>          g_free(path);
>      } else {
>          path = g_strdup_printf("cpu/%s/add/%ux%ux%u&maxcpus=%u",
>                                 mname, data->sockets, data->cores,
>                                 data->threads, data->maxcpus);
> -        qtest_add_data_func_full(path, data, test_pc_with_cpu_add,
> +        qtest_add_data_func_full(path, data, test_plug_with_cpu_add,
>                                   test_data_free);
>          g_free(path);
>      }
> 

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 

Thanks,

David

Re: [Qemu-devel] [PATCH 2/4] tests/cpu-plug-test: Check the CPU hot-plugging with device_add, too

[David Hildenbrand]

On 17.10.2017 15:32, Thomas Huth wrote:
> Using 'device_add' instead of 'cpu-add' is the new way for
> hot-plugging CPUs, so we should test this regularly, too.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  tests/cpu-plug-test.c | 42 ++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 40 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/cpu-plug-test.c b/tests/cpu-plug-test.c
> index 4579119..5fef297 100644
> --- a/tests/cpu-plug-test.c
> +++ b/tests/cpu-plug-test.c
> @@ -16,6 +16,7 @@
>  struct PlugTestData {
>      char *machine;
>      const char *cpu_model;
> +    const char *device_model;
>      unsigned sockets;
>      unsigned cores;
>      unsigned threads;
> @@ -71,6 +72,34 @@ static void test_plug_without_cpu_add(gconstpointer data)
>      g_free(args);
>  }
>  
> +static void test_plug_with_device_add_x86(gconstpointer data)
> +{
> +    const PlugTestData *td = data;
> +    char *args;
> +    unsigned int s, c, t;
> +
> +    args = g_strdup_printf("-machine %s -cpu %s "
> +                           "-smp sockets=%u,cores=%u,threads=%u,maxcpus=%u",
> +                           td->machine, td->cpu_model,
> +                           td->sockets, td->cores, td->threads, td->maxcpus);
> +    qtest_start(args);
> +

I wonder if we should also query the hotpluggable CPUs via
query-hotpluggable-cpus and try to add all (maybe even in random order).
Could also be a second test case.

> +    for (s = td->sockets; s < td->maxcpus / td->cores / td->threads; s++) {
> +        for (c = 0; c < td->cores; c++) {
> +            for (t = 0; t < td->threads; t++) {
> +                char *id = g_strdup_printf("id-%i-%i-%i", s, c, t);
> +                qtest_qmp_device_add(td->device_model, id, "'socket-id':'%i', "
> +                                     "'core-id':'%i', 'thread-id':'%i'",
> +                                     s, c, t);
> +                g_free(id);
> +            }
> +        }
> +    }
> +
> +    qtest_end();
> +    g_free(args);
> +}
> +
>  static void test_data_free(gpointer data)
>  {
>      PlugTestData *pc = data;
> @@ -90,6 +119,7 @@ static void add_pc_test_case(const char *mname)
>      data = g_new(PlugTestData, 1);
>      data->machine = g_strdup(mname);
>      data->cpu_model = "Haswell"; /* 1.3+ theoretically */
> +    data->device_model = "Haswell-x86_64-cpu";
>      data->sockets = 1;
>      data->cores = 3;
>      data->threads = 2;
> @@ -105,19 +135,27 @@ static void add_pc_test_case(const char *mname)
>          (strcmp(mname, "pc-0.12") == 0) ||
>          (strcmp(mname, "pc-0.11") == 0) ||
>          (strcmp(mname, "pc-0.10") == 0)) {
> -        path = g_strdup_printf("cpu/%s/init/%ux%ux%u&maxcpus=%u",
> +        path = g_strdup_printf("cpu-plug/%s/init/%ux%ux%u&maxcpus=%u",
>                                 mname, data->sockets, data->cores,
>                                 data->threads, data->maxcpus);
>          qtest_add_data_func_full(path, data, test_plug_without_cpu_add,
>                                   test_data_free);
>          g_free(path);
>      } else {
> -        path = g_strdup_printf("cpu/%s/add/%ux%ux%u&maxcpus=%u",
> +        PlugTestData *data2 = g_memdup(data, sizeof(PlugTestData));

add an empty line?

> +        data2->machine = g_strdup(mname);
> +        path = g_strdup_printf("cpu-plug/%s/cpu-add/%ux%ux%u&maxcpus=%u",
>                                 mname, data->sockets, data->cores,
>                                 data->threads, data->maxcpus);
>          qtest_add_data_func_full(path, data, test_plug_with_cpu_add,
>                                   test_data_free);
>          g_free(path);
> +        path = g_strdup_printf("cpu-plug/%s/device-add/%ux%ux%u&maxcpus=%u",
> +                               mname, data2->sockets, data2->cores,
> +                               data2->threads, data2->maxcpus);
> +        qtest_add_data_func_full(path, data2, test_plug_with_device_add_x86,
> +                                 test_data_free);
> +        g_free(path);
>      }
>  }
>  
> 

device_model can usually be created from cpu_model (although not 100%
clean, e.g. s390 vs. s390x). But the distinction here actually allows us
to test that we can't plug mixed CPU models.

So we might add a test case later that tries to hotplug a different CPU.

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 

Thanks,

David

Re: [Qemu-devel] [PATCH 4/4] tests/cpu-plug-test: Test CPU hot-plugging on s390x

[David Hildenbrand]

On 17.10.2017 15:32, Thomas Huth wrote:
> CPU hot-plugging on s390x is possible with both, "cpu-add"
> and "device_add", so test both.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  tests/Makefile.include |  1 +
>  tests/cpu-plug-test.c  | 38 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 39 insertions(+)
> 
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index cbec19f..85cf9ea 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -368,6 +368,7 @@ check-qtest-s390x-$(CONFIG_SLIRP) += tests/test-netfilter$(EXESUF)
>  check-qtest-s390x-$(CONFIG_POSIX) += tests/test-filter-mirror$(EXESUF)
>  check-qtest-s390x-$(CONFIG_POSIX) += tests/test-filter-redirector$(EXESUF)
>  check-qtest-s390x-y += tests/drive_del-test$(EXESUF)
> +check-qtest-s390x-y += tests/cpu-plug-test$(EXESUF)
>  
>  check-qtest-generic-y += tests/qom-test$(EXESUF)
>  check-qtest-generic-y += tests/test-hmp$(EXESUF)
> diff --git a/tests/cpu-plug-test.c b/tests/cpu-plug-test.c
> index 714a69c..58edaf6 100644
> --- a/tests/cpu-plug-test.c
> +++ b/tests/cpu-plug-test.c
> @@ -207,6 +207,42 @@ static void add_pseries_test_case(const char *mname)
>      g_free(path);
>  }
>  
> +static void add_s390x_test_case(const char *mname)
> +{
> +    char *path;
> +    PlugTestData *data, *data2;
> +
> +    if (!g_str_has_prefix(mname, "s390-ccw-virtio-")) {
> +        return;
> +    }
> +
> +    data = g_new(PlugTestData, 1);
> +    data->machine = g_strdup(mname);
> +    data->cpu_model = "qemu";
> +    data->device_model = "qemu-s390-cpu";
> +    data->sockets = 1;
> +    data->cores = 3;
> +    data->threads = 1;
> +    data->maxcpus = data->sockets * data->cores * data->threads * 2;
> +
> +    data2 = g_memdup(data, sizeof(PlugTestData));
> +    data2->machine = g_strdup(mname);
> +
> +    path = g_strdup_printf("cpu-plug/%s/cpu-add/%ux%ux%u&maxcpus=%u",
> +                           mname, data->sockets, data->cores,
> +                           data->threads, data->maxcpus);
> +    qtest_add_data_func_full(path, data, test_plug_with_cpu_add,
> +                             test_data_free);
> +    g_free(path);
> +
> +    path = g_strdup_printf("cpu-plug/%s/device-add/%ux%ux%u&maxcpus=%u",
> +                           mname, data2->sockets, data2->cores,
> +                           data2->threads, data2->maxcpus);
> +    qtest_add_data_func_full(path, data2, test_plug_with_device_add_coreid,
> +                             test_data_free);
> +    g_free(path);
> +}
> +
>  int main(int argc, char **argv)
>  {
>      const char *arch = qtest_get_arch();
> @@ -217,6 +253,8 @@ int main(int argc, char **argv)
>          qtest_cb_for_every_machine(add_pc_test_case);
>      } else if (g_str_equal(arch, "ppc64")) {
>          qtest_cb_for_every_machine(add_pseries_test_case);
> +    } else if (g_str_equal(arch, "s390x")) {
> +        qtest_cb_for_every_machine(add_s390x_test_case);
>      }
>  
>      return g_test_run();
> 

Looks sane to me, haven't tested it, though.

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 

Thanks,

David