| Message ID | 1474899049-12506-5-git-send-email-lvivier@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 >
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); + } + 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; }
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(-)