Re: [Qemu-devel] [PATCH 4/4] qtest: add ivshmem-test for ppc64

[Laurent Vivier]

On 13/12/2016 08:55, Markus Armbruster wrote:
> Laurent Vivier <address@hidden> writes:
> 
>> Signed-off-by: Laurent Vivier <address@hidden>
>> ---
>>  tests/Makefile.include |  3 ++-
>>  tests/ivshmem-test.c   | 46 +++++++++++++++++++++++++++++-----------------
>>  2 files changed, 31 insertions(+), 18 deletions(-)
>>
>> diff --git a/tests/Makefile.include b/tests/Makefile.include
>> index b574964..09fe5b6 100644
>> --- a/tests/Makefile.include
>> +++ b/tests/Makefile.include
>> @@ -287,6 +287,7 @@ check-qtest-ppc64-y += tests/test-netfilter$(EXESUF)
>>  check-qtest-ppc64-y += tests/test-filter-mirror$(EXESUF)
>>  check-qtest-ppc64-y += tests/test-filter-redirector$(EXESUF)
>>  check-qtest-ppc64-y += tests/display-vga-test$(EXESUF)
>> +check-qtest-ppc64-y += tests/ivshmem-test$(EXESUF)
>>  
>>  check-qtest-sh4-y = tests/endianness-test$(EXESUF)
>>  
>> @@ -690,7 +691,7 @@ tests/test-netfilter$(EXESUF): tests/test-netfilter.o 
>> $(qtest-obj-y)
>>  tests/test-filter-mirror$(EXESUF): tests/test-filter-mirror.o $(qtest-obj-y)
>>  tests/test-filter-redirector$(EXESUF): tests/test-filter-redirector.o 
>> $(qtest-obj-y)
>>  tests/test-x86-cpuid-compat$(EXESUF): tests/test-x86-cpuid-compat.o 
>> $(qtest-obj-y)
>> -tests/ivshmem-test$(EXESUF): tests/ivshmem-test.o 
>> contrib/ivshmem-server/ivshmem-server.o $(libqos-pc-obj-y)
>> +tests/ivshmem-test$(EXESUF): tests/ivshmem-test.o 
>> contrib/ivshmem-server/ivshmem-server.o $(libqos-pc-obj-y) 
>> $(libqos-spapr-obj-y)
>>  tests/vhost-user-bridge$(EXESUF): tests/vhost-user-bridge.o
>>  tests/test-uuid$(EXESUF): tests/test-uuid.o $(test-util-obj-y)
>>  tests/test-arm-mptimer$(EXESUF): tests/test-arm-mptimer.o
>> diff --git a/tests/ivshmem-test.c b/tests/ivshmem-test.c
>> index 04a5c5d..5c6dc38 100644
>> --- a/tests/ivshmem-test.c
>> +++ b/tests/ivshmem-test.c
>> @@ -11,8 +11,9 @@
>>  #include "qemu/osdep.h"
>>  #include <glib/gstdio.h>
>>  #include "contrib/ivshmem-server/ivshmem-server.h"
>> -#include "libqos/pci-pc.h"
>>  #include "libqtest.h"
>> +#include "libqos/libqos-pc.h"
>> +#include "libqos/libqos-spapr.h"
>>  #include "qemu-common.h"
>>  
>>  #define TMPSHMSIZE (1 << 20)
>> @@ -40,9 +41,8 @@ static QPCIDevice *get_device(QPCIBus *pcibus)
>>  }
>>  
>>  typedef struct _IVState {
>> -    QTestState *qtest;
>> +    QOSState *qs;
>>      QPCIBar reg_bar, mem_bar;
>> -    QPCIBus *pcibus;
>>      QPCIDevice *dev;
>>  } IVState;
>>  
>> @@ -74,7 +74,7 @@ static inline unsigned in_reg(IVState *s, enum Reg reg)
>>      QTestState *qtest = global_qtest;
>>      unsigned res;
>>  
>> -    global_qtest = s->qtest;
>> +    global_qtest = s->qs->qts;
>>      res = qpci_io_readl(s->dev, s->reg_bar, reg);
>>      g_test_message("*%s -> %x\n", name, res);
>>      global_qtest = qtest;
>> @@ -87,7 +87,7 @@ static inline void out_reg(IVState *s, enum Reg reg, 
>> unsigned v)
>>      const char *name = reg2str(reg);
>>      QTestState *qtest = global_qtest;
>>  
>> -    global_qtest = s->qtest;
>> +    global_qtest = s->qs->qts;
>>      g_test_message("%x -> *%s\n", v, name);
>>      qpci_io_writel(s->dev, s->reg_bar, reg, v);
>>      global_qtest = qtest;
>> @@ -97,7 +97,7 @@ static inline void read_mem(IVState *s, uint64_t off, void 
>> *buf, size_t len)
>>  {
>>      QTestState *qtest = global_qtest;
>>  
>> -    global_qtest = s->qtest;
>> +    global_qtest = s->qs->qts;
>>      qpci_memread(s->dev, s->mem_bar, off, buf, len);
>>      global_qtest = qtest;
>>  }
>> @@ -107,7 +107,7 @@ static inline void write_mem(IVState *s, uint64_t off,
>>  {
>>      QTestState *qtest = global_qtest;
>>  
>> -    global_qtest = s->qtest;
>> +    global_qtest = s->qs->qts;
>>      qpci_memwrite(s->dev, s->mem_bar, off, buf, len);
>>      global_qtest = qtest;
>>  }
>> @@ -115,17 +115,23 @@ static inline void write_mem(IVState *s, uint64_t off,
>>  static void cleanup_vm(IVState *s)
>>  {
>>      g_free(s->dev);
>> -    qpci_free_pc(s->pcibus);
>> -    qtest_quit(s->qtest);
>> +    qtest_shutdown(s->qs);
>>  }
>>  
>>  static void setup_vm_cmd(IVState *s, const char *cmd, bool msix)
>>  {
>>      uint64_t barsize;
>> +    const char *arch = qtest_get_arch();
>>  
>> -    s->qtest = qtest_start(cmd);
>> -    s->pcibus = qpci_init_pc(NULL);
>> -    s->dev = get_device(s->pcibus);
>> +    if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
>> +        s->qs = qtest_pc_boot(cmd);
>> +    } else if (strcmp(arch, "ppc64") == 0) {
>> +        s->qs = qtest_spapr_boot(cmd);
>> +    } else {
>> +        g_printerr("vshmem-test tests are only available on x86 or 
>> ppc64\n");
> 
> Typo: s/vshmem/ivshmem
> 
> To get here, you need to build and run this manually, as make check only
> builds and runs this for targets that work.  But the check doesn't hurt,
> and documents expectations.
> 
>> +        exit(EXIT_FAILURE);
>> +    }
>> +    s->dev = get_device(s->qs->pcibus);
>>  
>>      s->reg_bar = qpci_iomap(s->dev, 0, &barsize);
>>      g_assert_cmpuint(barsize, ==, 256);
>> @@ -347,7 +353,7 @@ static void test_ivshmem_server(bool msi)
>>      g_assert_cmpint(vm1, !=, vm2);
>>  
>>      /* check number of MSI-X vectors */
>> -    global_qtest = s1->qtest;
>> +    global_qtest = s1->qs->qts;
>>      if (msi) {
>>          ret = qpci_msix_table_size(s1->dev);
>>          g_assert_cmpuint(ret, ==, nvectors);
>> @@ -370,7 +376,7 @@ static void test_ivshmem_server(bool msi)
>>      g_assert_cmpuint(ret, !=, 0);
>>  
>>      /* ping vm1 -> vm2 on vector 1 */
>> -    global_qtest = s2->qtest;
>> +    global_qtest = s2->qs->qts;
>>      if (msi) {
>>          ret = qpci_msix_pending(s2->dev, 1);
>>          g_assert_cmpuint(ret, ==, 0);
>> @@ -412,6 +418,7 @@ static void test_ivshmem_server_irq(void)
>>  
>>  static void test_ivshmem_hotplug(void)
>>  {
>> +    const char *arch = qtest_get_arch();
>>      gchar *opts;
>>  
>>      qtest_start("");
> 
> Unlike the other tests, this one doesn't refuse to run on unexpected
> targets.  Intentional?

Yes, because there is no dependencies on libqos PCI implementation, so
it should always work.

>> @@ -419,7 +426,9 @@ static void test_ivshmem_hotplug(void)
>>      opts = g_strdup_printf("'shm': '%s', 'size': '1M'", tmpshm);
>>  
>>      qpci_plug_device_test("ivshmem", "iv1", PCI_SLOT_HP, opts);
>> -    qpci_unplug_acpi_device_test("iv1", PCI_SLOT_HP);
>> +    if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
>> +        qpci_unplug_acpi_device_test("iv1", PCI_SLOT_HP);
>> +    }
> 
> Disables unplug test for ppc64.
> 
>>  
>>      qtest_end();
>>      g_free(opts);
>> @@ -491,6 +500,7 @@ static gchar *mktempshm(int size, int *fd)
>>  int main(int argc, char **argv)
>>  {
>>      int ret, fd;
>> +    const char *arch = qtest_get_arch();
>>      gchar dir[] = "/tmp/ivshmem-test.XXXXXX";
>>  
>>  #if !GLIB_CHECK_VERSION(2, 31, 0)
>> @@ -521,8 +531,10 @@ int main(int argc, char **argv)
>>      qtest_add_func("/ivshmem/memdev", test_ivshmem_memdev);
>>      if (g_test_slow()) {
>>          qtest_add_func("/ivshmem/pair", test_ivshmem_pair);
>> -        qtest_add_func("/ivshmem/server-msi", test_ivshmem_server_msi);
>> -        qtest_add_func("/ivshmem/server-irq", test_ivshmem_server_irq);
>> +        if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
>> +            qtest_add_func("/ivshmem/server-msi", test_ivshmem_server_msi);
>> +            qtest_add_func("/ivshmem/server-irq", test_ivshmem_server_irq);
>> +        }
>>      }
>>  
>>      ret = g_test_run();
> 
> The commit message should explain things in a bit more detail: the test
> works only for x86, and is only compiled and executed for x86.  The
> patch converts it to libqos, adds ppc64, and makes the test fail for
> targets that don't work.  It also disables the unplug part of test
> /ivshmem/hotplug for ppc64; please explain why.
> 
> Please make the conversion to libqos a separate patch, for easier
> review.
> 

Thank you for the review.

I'm going to change this patch according to your comments.

Laurent