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

[Laurent Vivier <lvivier@redhat.com>]

On 04/10/2016 15:20, Thomas Huth wrote:
> On 28.09.2016 20:51, Laurent Vivier wrote:
>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> 
> Meta-question: Do we need to test UHCI on ppc at all? AFAIK most (all?)
> ppc-based machines were rather based on OHCI instead...

No, but this is the only test using PCI. OHCI test tests only if we can
plug and hotplug a device.

> 
>> ---
>>  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/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");
>> +    }
> 
> The "-device ..." string looks the same for both machine types ... so
> IMHO it would be somewhat nicer to define this only once and then use
> the common string for both machine instead of specifying it here twice.

yes.

Thanks,
Laurent