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

[David Gibson]

On Wed, Sep 28, 2016 at 09:54:14AM +0200, Laurent Vivier wrote:
> 
> 
> 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.

Hmm.. so the existing code in qpci_device_foreach(), amongst others is
currently broken if host endian != guest endian?

Actually, though, regardless of what happens now, there's only one
sane approach here.  the pci_io_read*() functions know the length of
the value they're reading and return a word type.  Having it return
anything except native endianness is just bogus (host native that is,
because the code is executing on the host, although it's simulating
execution of a guest).

Endian conversions should *only* occur at the points we read or write
to guest memory or hardware (from either the framework or accel side).
That means if host and guest endian are different, we're likely to
swap the value as it's stored into guest memory, then swap it right
back as we pull it into the qtest accel, but that's fine.

> 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.

Basically all PCI devices (in fact, nearly vaguely modern all hardware
of any sort) are LE.  So I think it's correct for the PCI accessors to
assume the hardware is LE - the handful of BE devices out there can
jump through hoops to handle the exception.

Virtio pre-1.0 will require extra hacks because of the (stupid) "guest
native" behaviour.  But we should design the general interfaces around
the quirks of one device.

> 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
> 

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature