Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 6/6] tests: enable ohci/uhci/xhci tests on PPC64

[Laurent Vivier]

On 28/09/2016 04:45, David Gibson wrote:
> On Tue, Sep 27, 2016 at 08:55:59PM +0200, Laurent Vivier wrote:
>> Signed-off-by: Laurent Vivier <address@hidden>
>> ---
>>  tests/Makefile.include    |  8 +++++++-
>>  tests/libqos/usb.c        |  2 +-
>>  tests/usb-hcd-uhci-test.c | 24 ++++++++++++++++--------
>>  3 files changed, 24 insertions(+), 10 deletions(-)
>>
>> diff --git a/tests/Makefile.include b/tests/Makefile.include
>> index bca2cbc..4136959 100644
>> --- a/tests/Makefile.include
>> +++ b/tests/Makefile.include
>> @@ -270,6 +270,12 @@ check-qtest-ppc64-y += tests/drive_del-test$(EXESUF)
>>  check-qtest-ppc64-y += tests/postcopy-test$(EXESUF)
>>  check-qtest-ppc64-y += tests/boot-serial-test$(EXESUF)
>>  check-qtest-ppc64-y += tests/rtas-test$(EXESUF)
>> +check-qtest-ppc64-y += tests/usb-hcd-ohci-test$(EXESUF)
>> +gcov-files-ppc64-y += hw/usb/hcd-ohci.c
>> +check-qtest-ppc64-y += tests/usb-hcd-uhci-test$(EXESUF)
>> +gcov-files-ppc64-y += hw/usb/hcd-uhci.c
>> +check-qtest-ppc64-y += tests/usb-hcd-xhci-test$(EXESUF)
>> +gcov-files-ppc64-y += hw/usb/hcd-xhci.c
>>  
>>  check-qtest-sh4-y = tests/endianness-test$(EXESUF)
>>  
>> @@ -595,7 +601,7 @@ libqos-pc-obj-y += tests/libqos/malloc-pc.o 
>> tests/libqos/libqos-pc.o
>>  libqos-pc-obj-y += tests/libqos/ahci.o
>>  libqos-omap-obj-y = $(libqos-obj-y) tests/libqos/i2c-omap.o
>>  libqos-imx-obj-y = $(libqos-obj-y) tests/libqos/i2c-imx.o
>> -libqos-usb-obj-y = $(libqos-pc-obj-y) tests/libqos/usb.o
>> +libqos-usb-obj-y = $(libqos-spapr-obj-y) $(libqos-pc-obj-y) 
>> tests/libqos/usb.o
>>  libqos-virtio-obj-y = $(libqos-pc-obj-y) tests/libqos/virtio.o 
>> tests/libqos/virtio-pci.o tests/libqos/virtio-mmio.o 
>> tests/libqos/malloc-generic.o
>>  
>>  tests/device-introspect-test$(EXESUF): tests/device-introspect-test.o
>> diff --git a/tests/libqos/usb.c b/tests/libqos/usb.c
>> index f794d92..25e5f38 100644
>> --- a/tests/libqos/usb.c
>> +++ b/tests/libqos/usb.c
>> @@ -28,7 +28,7 @@ void qusb_pci_init_one(QPCIBus *pcibus, struct qhc *hc, 
>> uint32_t devfn, int bar)
>>  void uhci_port_test(struct qhc *hc, int port, uint16_t expect)
>>  {
>>      void *addr = hc->base + 0x10 + 2 * port;
>> -    uint16_t value = qpci_io_readw(hc->dev, addr);
>> +    uint16_t value = target_le16_to_cpu(qpci_io_readw(hc->dev, addr));
> 
> This doesn't look right.  Judging by the code using qpci_io_readw() in
> qpci_device_foreach() and in other tests I'm looking at,
> qpci_io_readw() (and the others) are expected to return results in
> *host native* order - i.e. suitable for immediate comparisons and
> masks within qtest code executing on the host.

Its true when the endianness of the device is the same as the endianness
of the guest.

This case is particular because the endianness of the device is
little-endian (all USB controller are little-endian). On a little-endian
guest it is read correctly, on a big-endian guest it is not.

So either we add a command in the protocol to ask a big-endian guest to
read in a little-endian mode, or we can just convert it on the host
(knowing endianness of the guest).

> 
>>      uint16_t mask = ~(UHCI_PORT_WRITE_CLEAR | UHCI_PORT_RSVD1);
>>  
>>      g_assert((value & mask) == (expect & mask));
>> diff --git a/tests/usb-hcd-uhci-test.c b/tests/usb-hcd-uhci-test.c
>> index c24063e..4b951ce 100644
>> --- a/tests/usb-hcd-uhci-test.c
>> +++ b/tests/usb-hcd-uhci-test.c
>> @@ -9,9 +9,13 @@
>>  
>>  #include "qemu/osdep.h"
>>  #include "libqtest.h"
>> +#include "libqos/libqos.h"
>>  #include "libqos/usb.h"
>> +#include "libqos/libqos-pc.h"
>> +#include "libqos/libqos-spapr.h"
>>  #include "hw/usb/uhci-regs.h"
>>  
>> +static QOSState *qs;
>>  
>>  static void test_uhci_init(void)
>>  {
>> @@ -19,13 +23,10 @@ static void test_uhci_init(void)
>>  
>>  static void test_port(int port)
>>  {
>> -    QPCIBus *pcibus;
>>      struct qhc uhci;
>>  
>>      g_assert(port > 0);
>> -    pcibus = qpci_init_pc(NULL);
>> -    g_assert(pcibus != NULL);
>> -    qusb_pci_init_one(pcibus, &uhci, QPCI_DEVFN(0x1d, 0), 4);
>> +    qusb_pci_init_one(qs->pcibus, &uhci, QPCI_DEVFN(0x1d, 0), 4);
>>      uhci_port_test(&uhci, port - 1, UHCI_PORT_CCS);
>>  }
>>  
>> @@ -75,6 +76,7 @@ static void test_usb_storage_hotplug(void)
>>  
>>  int main(int argc, char **argv)
>>  {
>> +    const char *arch = qtest_get_arch();
>>      int ret;
>>  
>>      g_test_init(&argc, &argv, NULL);
>> @@ -84,11 +86,17 @@ int main(int argc, char **argv)
>>      qtest_add_func("/uhci/pci/hotplug", test_uhci_hotplug);
>>      qtest_add_func("/uhci/pci/hotplug/usb-storage", 
>> test_usb_storage_hotplug);
>>  
>> -    qtest_start("-device piix3-usb-uhci,id=uhci,addr=1d.0"
>> -                " -drive id=drive0,if=none,file=/dev/null,format=raw"
>> -                " -device usb-tablet,bus=uhci.0,port=1");
>> +    if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
>> +        qs = qtest_pc_boot("-device piix3-usb-uhci,id=uhci,addr=1d.0"
>> +                           " -drive 
>> id=drive0,if=none,file=/dev/null,format=raw"
>> +                           " -device usb-tablet,bus=uhci.0,port=1");
>> +    } else if (strcmp(arch, "ppc64") == 0) {
>> +        qs = qtest_spapr_boot("-device piix3-usb-uhci,id=uhci,addr=1d.0"
>> +                           " -drive 
>> id=drive0,if=none,file=/dev/null,format=raw"
>> +                           " -device usb-tablet,bus=uhci.0,port=1");
> 
> Why aren't there similar ifs needed for ohci, ehci and/or xhci?

ohci and xhci don't test the card is working correctly, they just test
we can plug it, so we don't need PCI framework and we don't have to
initialize it with qtest_XXX_boot(), and the machine are started in a
generic way using qtest_start().

I din't add ehci because I was not able to make it working for the moment.
My goal is to test virtio, so USB is just here to test new qtest SPAPR
PCI implementation. We can improve/add more PCI tests later.

> 
>> +    }
>>      ret = g_test_run();
>> -    qtest_end();
>> +    qtest_shutdown(qs);
>>  
>>      return ret;
>>  }
> 

thansk,
Laurent