illumos gate

Thanks for reporting this. I'm taking a look at the QEMU source code. It does appear to report version 1, but what's not clear is how the emulation is supposed to handle a request to read the capability section at offset 0x2. If you look at http://git.qemu.org/?p=qemu.git;a=blob;f=hw/usb/hcd-xhci.c;hb=HEAD#l2902, it doesn't seem to have an explicit offset for 0x2, which is treated as separate registers here. I need to spend some more time looking at their emulation to understand how our request for a 2 byte read at offset two is treated here.

Michal Nowak, do you have the ability to run / deploy qemu with its tracing features? If so, I'd be curious to see if we hit either:

http://git.qemu.org/?p=qemu.git;a=blob;f=hw/usb/hcd-xhci.c;hb=HEAD#l2963

or

http://git.qemu.org/?p=qemu.git;a=blob;f=hw/usb/hcd-xhci.c;hb=HEAD#l2967

So, from looking around. I suspect this is an issue with QEMU, though we can attempt to work around in the OS. I think what's happening is that the way their logic for combining reads into the memory access means that they'll fail a read at offset 0x2, which is what we're doing for the identification information, which is why we end up getting a version of zero, which isn't what happens in hardware or VMware Fusion.

From looking at other OSes, it looks like Linux does a four byte read of the initial capacity register which is allowed, but also means they're reading reserved bits and then just mask it off. FreeBSD only seems to read the version for debug purposes; however, OpenBSD will be in the same place.

Michal, if you're able to confirm that, that'd be useful information. Depending on what we find there, we can go and take a look at working around this in the OS and also working with QEMU upstream to get it fixed.

Robert Mustacchi wrote:

Michal Nowak, do you have the ability to run / deploy qemu with its tracing features?

Thanks for looking into this, Robert. This time I run QEMU with -trace enable='usb_xhci*',file=MyEvents.dump. Resulting dump is attached (50 MB after extraction). Let me know if you need trace with all events (*), or anything else.

That actually had exactly what I was looking for -- thanks. That confirms it and actually shows it for another register as well. So, given this, I think we can work around this in QEMU and I may try to work with the QEMU folks to file a bug.

If I was able to put together a patch to test this, do you think that you'd be able to test that or would it help to have a full OI image put together. If that's the case, I'll have to work with some other folks to help produce that.

Let me know bugref if you decide to file bug to upstream QEMU, please.

Never build illumos myself before, but I guess I can challenge myself to do so :), if you provide a patch against illumos-gate.

I filed a QEMU bug: https://bugs.launchpad.net/qemu/+bug/1693050. Here's a patch. Would you mind trying this out:

diff --git a/usr/src/uts/common/io/usb/hcd/xhci/xhci.c b/usr/src/uts/common/io/usb/hcd/xhci/xhci.c
index 17d190f..b43d7df 100644
--- a/usr/src/uts/common/io/usb/hcd/xhci/xhci.c
+++ b/usr/src/uts/common/io/usb/hcd/xhci/xhci.c
@@ -1170,14 +1170,20 @@ static boolean_t
 xhci_read_params(xhci_t *xhcip)
 {
        uint8_t usb;
-       uint16_t vers;
        uint32_t struc1, struc2, struc3, cap1, cap2, pgsz;
-       uint32_t psize, pbit;
+       uint32_t psize, pbit, vers;
        xhci_capability_t *xcap;
        unsigned long ps;

+       /*
+        * While it's tempting to do a 16-bit read at offset 0x2, unfortunately,
+        * a few emulated systems don't support reading at offset 0x2 for the
+        * version. Instead we need to read the caplength register and get the
+        * upper two bytes.
+        */
+       vers = xhci_get32(xhcip, XHCI_R_CAP, XHCI_CAPLENGTH);
+       vers = XHCI_VERSION_MASK(vers);
        usb = pci_config_get8(xhcip->xhci_cfg_handle, PCI_XHCI_USBREV);
-       vers = xhci_get16(xhcip, XHCI_R_CAP, XHCI_HCIVERSION);
        struc1 = xhci_get32(xhcip, XHCI_R_CAP, XHCI_HCSPARAMS1);
        struc2 = xhci_get32(xhcip, XHCI_R_CAP, XHCI_HCSPARAMS2);
        struc3 = xhci_get32(xhcip, XHCI_R_CAP, XHCI_HCSPARAMS3);
diff --git a/usr/src/uts/common/sys/usb/hcd/xhci/xhcireg.h b/usr/src/uts/common/sys/usb/hcd/xhci/xhcireg.h
index 7e23463..a5a295d 100644
--- a/usr/src/uts/common/sys/usb/hcd/xhci/xhcireg.h
+++ b/usr/src/uts/common/sys/usb/hcd/xhci/xhcireg.h
@@ -58,6 +58,8 @@ extern "C" {
 #define        XHCI_HCIVERSION_0_9     0x0090  /* xHCI version 0.9 */
 #define        XHCI_HCIVERSION_1_0     0x0100  /* xHCI version 1.0 */

+#define        XHCI_VERSION_MASK(x)    (((x) >> 16) & 0xffff)
+
 /*
  * Structural Parameters 1 - xHCI 1.1 / 5.3.3
  */

Robert Mustacchi wrote:

[...] Here's a patch. Would you mind trying this out:

I just verified with emulated USB tablet connected to xHCI, that the patch against current illumos works. Thank you!

OK, great. Thank you very much for testing this. If you watch illumos-developer, I'll get this sent out for review shortly.