[4/4] tests: enable ohci/uhci/xhci tests on PPC64

Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
 tests/Makefile.include    |  8 +++++++-
 tests/libqos/usb.c        | 14 ++++++++++++++
 tests/usb-hcd-uhci-test.c | 24 ++++++++++++++++--------
 3 files changed, 37 insertions(+), 9 deletions(-)

On Mon, Sep 26, 2016 at 04:10:49PM +0200, Laurent Vivier wrote:
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
>  tests/Makefile.include    |  8 +++++++-
>  tests/libqos/usb.c        | 14 ++++++++++++++
>  tests/usb-hcd-uhci-test.c | 24 ++++++++++++++++--------
>  3 files changed, 37 insertions(+), 9 deletions(-)
> 
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index 967426a..aaf264f 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..6168a2f 100644
> --- a/tests/libqos/usb.c
> +++ b/tests/libqos/usb.c
> @@ -12,10 +12,17 @@
>   * See the COPYING file in the top-level directory.
>   */
>  #include "qemu/osdep.h"
> +#include "qemu/bswap.h"
>  #include "libqtest.h"
>  #include "hw/usb/uhci-regs.h"
>  #include "libqos/usb.h"
>  
> +#ifdef HOST_WORDS_BIGENDIAN
> +static const bool host_big_endian = true;
> +#else
> +static const bool host_big_endian = false;
> +#endif
> +
>  void qusb_pci_init_one(QPCIBus *pcibus, struct qhc *hc, uint32_t devfn, int bar)
>  {
>      hc->dev = qpci_device_find(pcibus, devfn);
> @@ -31,6 +38,13 @@ void uhci_port_test(struct qhc *hc, int port, uint16_t expect)
>      uint16_t value = qpci_io_readw(hc->dev, addr);
>      uint16_t mask = ~(UHCI_PORT_WRITE_CLEAR | UHCI_PORT_RSVD1);
>  
> +    if (qtest_big_endian() && host_big_endian) {
> +        /* little endian device on big endian guest
> +         * must be swapped on big endian host
> +         */
> +        value = bswap16(value);
> +    }
> +

Hm.. should the qpci_io_*() helpers handle the endian conversion?

>      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");
> +    }
>      ret = g_test_run();
> -    qtest_end();
> +    qtest_shutdown(qs);
>  
>      return ret;
>  }

On 27/09/2016 05:53, David Gibson wrote:
> On Mon, Sep 26, 2016 at 04:10:49PM +0200, Laurent Vivier wrote:

>>  void qusb_pci_init_one(QPCIBus *pcibus, struct qhc *hc, uint32_t devfn, int bar)
>>  {
>>      hc->dev = qpci_device_find(pcibus, devfn);
>> @@ -31,6 +38,13 @@ void uhci_port_test(struct qhc *hc, int port, uint16_t expect)
>>      uint16_t value = qpci_io_readw(hc->dev, addr);
>>      uint16_t mask = ~(UHCI_PORT_WRITE_CLEAR | UHCI_PORT_RSVD1);
>>  
>> +    if (qtest_big_endian() && host_big_endian) {
>> +        /* little endian device on big endian guest
>> +         * must be swapped on big endian host
>> +         */
>> +        value = bswap16(value);
>> +    }
>> +
> 
> Hm.. should the qpci_io_*() helpers handle the endian conversion?

I'm really wondering how to manage correctly this case (I've the same
kind of issue with virtio).

The protocol between guest and test program reads/writes data using the
guest CPU endianess, so it works in the overall case. But in this case,
hcd-uhci is a little-endian device (.endianness = DEVICE_LITTLE_ENDIAN)
on a big endian machine, so I think in the linux driver we should have a
"le16_to_cpu()". But in our case we can't use "le16_to_cpu()" because
endianess of the host cpu is not the same has the one of the guest CPU.
Perhaps I should add a "target_le16_to_cpu()"?

Thanks,
Laurent

On 27/09/2016 09:43, Laurent Vivier wrote:
> 
> 
> On 27/09/2016 05:53, David Gibson wrote:
>> On Mon, Sep 26, 2016 at 04:10:49PM +0200, Laurent Vivier wrote:
> 
>>>  void qusb_pci_init_one(QPCIBus *pcibus, struct qhc *hc, uint32_t devfn, int bar)
>>>  {
>>>      hc->dev = qpci_device_find(pcibus, devfn);
>>> @@ -31,6 +38,13 @@ void uhci_port_test(struct qhc *hc, int port, uint16_t expect)
>>>      uint16_t value = qpci_io_readw(hc->dev, addr);
>>>      uint16_t mask = ~(UHCI_PORT_WRITE_CLEAR | UHCI_PORT_RSVD1);
>>>  
>>> +    if (qtest_big_endian() && host_big_endian) {
>>> +        /* little endian device on big endian guest
>>> +         * must be swapped on big endian host
>>> +         */
>>> +        value = bswap16(value);
>>> +    }
>>> +
>>
>> Hm.. should the qpci_io_*() helpers handle the endian conversion?
> 
> I'm really wondering how to manage correctly this case (I've the same
> kind of issue with virtio).
> 
> The protocol between guest and test program reads/writes data using the
> guest CPU endianess, so it works in the overall case. But in this case,
> hcd-uhci is a little-endian device (.endianness = DEVICE_LITTLE_ENDIAN)
> on a big endian machine, so I think in the linux driver we should have a
> "le16_to_cpu()". But in our case we can't use "le16_to_cpu()" because
> endianess of the host cpu is not the same has the one of the guest CPU.
> Perhaps I should add a "target_le16_to_cpu()"?

I think the "&& host_big_endian" is a side effect of a wrong
byte-swapping done in pci-spapr.c. As we read the value with the
endianess of the target CPU, this should be not needed.

I'm going to rework this part and define some
target_leXX_to_cpu()/target_cpu_to_leXX() macros.

Laurent

On Tue, Sep 27, 2016 at 09:43:27AM +0200, Laurent Vivier wrote:
> 
> 
> On 27/09/2016 05:53, David Gibson wrote:
> > On Mon, Sep 26, 2016 at 04:10:49PM +0200, Laurent Vivier wrote:
> 
> >>  void qusb_pci_init_one(QPCIBus *pcibus, struct qhc *hc, uint32_t devfn, int bar)
> >>  {
> >>      hc->dev = qpci_device_find(pcibus, devfn);
> >> @@ -31,6 +38,13 @@ void uhci_port_test(struct qhc *hc, int port, uint16_t expect)
> >>      uint16_t value = qpci_io_readw(hc->dev, addr);
> >>      uint16_t mask = ~(UHCI_PORT_WRITE_CLEAR | UHCI_PORT_RSVD1);
> >>  
> >> +    if (qtest_big_endian() && host_big_endian) {
> >> +        /* little endian device on big endian guest
> >> +         * must be swapped on big endian host
> >> +         */
> >> +        value = bswap16(value);
> >> +    }
> >> +
> > 
> > Hm.. should the qpci_io_*() helpers handle the endian conversion?
> 
> I'm really wondering how to manage correctly this case (I've the same
> kind of issue with virtio).
> 
> The protocol between guest and test program reads/writes data using the
> guest CPU endianess, so it works in the overall case.

Oh.. That seems bogus; maybe we should change that.  IIUC the protocol
is transferring whole word values as strings.  In which case we should
be transferring whole word values - endianness should only come into
play if we're associating bytes in the value with addresses.

So using (poorly defined) guest endianness is a design error here - we
should use "host endianness"; really "no endianness".

> But in this case,
> hcd-uhci is a little-endian device (.endianness = DEVICE_LITTLE_ENDIAN)
> on a big endian machine, so I think in the linux driver we should have a
> "le16_to_cpu()". But in our case we can't use "le16_to_cpu()" because
> endianess of the host cpu is not the same has the one of the guest CPU.
> Perhaps I should add a "target_le16_to_cpu()"?
> 
> Thanks,
> Laurent
>