Bug 212021 - xhci(4) broken with qemu-devel
Summary: xhci(4) broken with qemu-devel
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: usb (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Some People
Assignee: Hans Petter Selasky
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-08-21 03:21 UTC by Nathan Whitehorn
Modified: 2018-11-06 04:19 UTC (History)
1 user (show)

See Also:


Attachments
Log from ppc64 QEMU with an xhci controller. usb_debug and xhcidebug have been set to 10 and QEMU's usb tracing has also been enabled. (186.94 KB, text/plain)
2016-08-21 03:21 UTC, Nathan Whitehorn
no flags Details
Fix XHCI compatibility with QEMU (797 bytes, patch)
2016-08-21 18:53 UTC, Hans Petter Selasky
no flags Details | Diff
Fix bug in QEMU XHCI emulation (519 bytes, patch)
2016-08-22 19:36 UTC, Hans Petter Selasky
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nathan Whitehorn freebsd_committer 2016-08-21 03:21:03 UTC
Created attachment 173901 [details]
Log from ppc64 QEMU with an xhci controller. usb_debug and xhcidebug have been set to 10 and QEMU's usb tracing has also been enabled.

xhci(4) times out after interrupts turn on while probing the USB bus on big-endian hardware, notably newer POWER8 servers that come with USB3.

The problem is reproducible on qemu-system-ppc64 with an emulated xhci device for debugging without access to the hardware. I've attached a boot log with all the USB debugging in FreeBSD and QEMU turned on. Nothing was immediately obvious as the source of the problem but I'm not that familiar with USB.
Comment 1 Hans Petter Selasky freebsd_committer 2016-08-21 08:45:51 UTC
Hi,

The FreeBSD XHCI driver only supports little endian PCI interface / register set and DMA descriptors.

There are no wrappers or flags to support big-endian XHCI yet like with the EHCI driver.

--HPS
Comment 2 Nathan Whitehorn freebsd_committer 2016-08-21 15:24:23 UTC
(In reply to Hans Petter Selasky from comment #1)

You misunderstand, I think. This is a bog-standard PCI-E XHCI card with little-endian registers. The driver just doesn't work when the CPU is big-endian (or, at least, it doesn't work on ppc64 -- I'm assuming the problem is an endianness one).
Comment 3 Hans Petter Selasky freebsd_committer 2016-08-21 17:57:32 UTC
Got it. OK, Look at this error:

xhci: output context at 831a000
xhci: invalid input context control 00000000 00000003
13236@1471749323.871077:usb_xhci_queue_event v 0, idx 8, ER_COMMAND_COMPLETE, CC_TRB_ERROR, p 000000000499fdf0, s 05000000, c 0x01008401
13236@1471749323.871080:usb_xhci_irq_intx level 1

And this piece of code which I simply Googled:

hcd-xhci.c:

    cpu_physical_memory_read(ictx, (uint8_t *) ictl_ctx, sizeof(ictl_ctx));

    if (ictl_ctx[0] != 0x0 || ictl_ctx[1] != 0x3) {
        fprintf(stderr, "xhci: invalid input context control %08x %08x\n",
                ictl_ctx[0], ictl_ctx[1]);
        return CC_TRB_ERROR;
    }

It looks like the ictl_ctx[1] read should have a le32_to_cpu() wrapper around it.

--HPS
Comment 4 Nathan Whitehorn freebsd_committer 2016-08-21 18:14:59 UTC
(In reply to Hans Petter Selasky from comment #3)

That is probably true, but I think that can't be the problem in this case, for two reasons:
1. The host CPU for my QEMU is little-endian, so that change would be a no-op.
2. I have exactly the same problem on real hardware.
Comment 5 Hans Petter Selasky freebsd_committer 2016-08-21 18:28:00 UTC
The qemu prints the values x=0 and y=3 in hex. And the check is (x != 0 || y != 3) which I find very odd.

I'll check if we are writing a 32-bit field with 64-bit ops.

--HPS
Comment 6 Nathan Whitehorn freebsd_committer 2016-08-21 18:43:31 UTC
(In reply to Hans Petter Selasky from comment #5)

The QEMU code that is printing the error is this in xhci_configure_slot():


    if ((ictl_ctx[0] & 0x3) != 0x0 || (ictl_ctx[1] & 0x3) != 0x1) {
        DPRINTF("xhci: invalid input context control %08x %08x\n",
                ictl_ctx[0], ictl_ctx[1]);
        return CC_TRB_ERROR;
    }

Unfortunately, they have several identical error messages from different places. Does QEMU's emulated xhci work with the amd64 target?
Comment 7 Hans Petter Selasky freebsd_committer 2016-08-21 18:53:43 UTC
Created attachment 173916 [details]
Fix XHCI compatibility with QEMU

Hi,

Can you test the attached patch. Looks like a XHCI compatibility issue.

--HPS
Comment 8 Nathan Whitehorn freebsd_committer 2016-08-22 03:48:09 UTC
(In reply to Hans Petter Selasky from comment #7)

That doesn't seem to help. The ppc64 QEMU behavior seems identical.
Comment 9 Hans Petter Selasky freebsd_committer 2016-08-22 07:28:24 UTC
Hi,

I looked into this yesterday and there are some missing pieces in the QEMU control endpoint handling code. FreeBSD sends the SETUP, DATA and STATUS stage as separate events, while QEMU expects them all at the same time. That's why the getting the device descriptor times out. This part should be fixed in QEMU. I might be able to provide a patch.

--HPS
Comment 10 commit-hook freebsd_committer 2016-08-22 10:21:58 UTC
A commit references this bug:

Author: hselasky
Date: Mon Aug 22 10:21:25 UTC 2016
New revision: 304597
URL: https://svnweb.freebsd.org/changeset/base/304597

Log:
  Fix for invalid use of bits in input context. Basically split
  configuring of EP0 and non-EP0 into xhci_cmd_evaluate_ctx() and
  xhci_cmd_configure_ep() respectivly. This resolves some errors when
  using XHCI under QEMU and gets is more in line with the XHCI
  specification.

  PR:		212021
  MFC after:	1 week

Changes:
  head/sys/dev/usb/controller/xhci.c
Comment 11 commit-hook freebsd_committer 2016-08-22 19:33:13 UTC
A commit references this bug:

Author: hselasky
Date: Mon Aug 22 19:32:50 UTC 2016
New revision: 304629
URL: https://svnweb.freebsd.org/changeset/base/304629

Log:
  Don't separate the status stage of the XHCI USB control transfers into
  its own job because this breaks the simplified QEMU XHCI TRB parser,
  which expects the complete USB control transfer as a series of back to
  back TRBs. The old behaviour is kept under #ifdef in case this change
  breaks enumeration of any USB devices.

  PR:		212021
  MFC after:	1 week

Changes:
  head/sys/dev/usb/controller/xhci.c
Comment 12 Hans Petter Selasky freebsd_committer 2016-08-22 19:36:00 UTC
Created attachment 173956 [details]
Fix bug in QEMU XHCI emulation
Comment 13 Hans Petter Selasky freebsd_committer 2016-08-22 19:37:17 UTC
Running the latest FreeBSD 12-current and applying the attached patch to QEMU, the XHCI driver enumerates w/o any issues so far for amd64.
Comment 14 Nathan Whitehorn freebsd_committer 2016-08-22 20:05:01 UTC
(In reply to Hans Petter Selasky from comment #13)

Still broken for me on PPC64 with QEMU, though in a different way:

xhci_process_commands()
xhci: input context at 8302000
xhci: output context at 8405000
xhci: input slot context: 08300000 00070000 00000000 00000000
xhci: output slot context: 08300000 00070000 00000000 10000001
xhci: input ep0 context: 00000000 00400026 07bbe001 00000000 00000008
xhci: output ep0 context: 00000003 00400026 07bbe001 00000000 00000008
xhci: set epctx: 8405020 state=1 dequeue=0000000007bbe001
xhci: setup packet pid 0x69 addr 1 ep 0
xhci: set epctx: 8405020 state=1 dequeue=0000000007be9311
Root mount waiting for: usbus0
xhci: set epctx: 8405020 state=1 dequeue=0000000007be9311
xhci: setup packet pid 0x69 addr 1 ep 0
xhci: set epctx: 8405020 state=1 dequeue=0000000007bfc311
xhci: set epctx: 8405020 state=1 dequeue=0000000007bfc311
xhci: setup packet pid 0x69 addr 1 ep 0
xhci: set epctx: 8405020 state=1 dequeue=0000000007be9311
xhci: set epctx: 8405020 state=1 dequeue=0000000007be9311
xhci: setup packet pid 0x69 addr 1 ep 0
xhci: set epctx: 8405020 state=1 dequeue=0000000007bfc311
xhci: set epctx: 8405020 state=1 dequeue=0000000007bfc311
xhci: setup packet pid 0x69 addr 1 ep 0
xhci: set epctx: 8405020 state=1 dequeue=0000000007be9311
xhci: set epctx: 8405020 state=1 dequeue=0000000007be9311
xhci: setup packet pid 0x69 addr 1 ep 0
xhci: set epctx: 8405020 state=1 dequeue=0000000007bfc311
xhci: set epctx: 8405020 state=1 dequeue=0000000007bfc311
xhci: setup packet pid 0x69 addr 1 ep 0
xhci: set epctx: 8405020 state=1 dequeue=0000000007be9311
Root mount waiting for: usbus0
xhci: set epctx: 8405020 state=1 dequeue=0000000007be9311
xhci: setup packet pid 0x69 addr 1 ep 0
xhci: set epctx: 8405020 state=1 dequeue=0000000007bfc311
usbd_setup_device_desc: getting device descriptor at addr 1 failed, USB_ERR_SHORT_XFER
Comment 15 Hans Petter Selasky freebsd_committer 2016-08-23 07:24:40 UTC
Hi,

Did you patch QEMU? The short message indicates not.

--HPS
Comment 16 Nathan Whitehorn freebsd_committer 2016-08-23 14:49:31 UTC
I did not. Thanks for your help!
Comment 17 commit-hook freebsd_committer 2016-08-29 08:40:39 UTC
A commit references this bug:

Author: hselasky
Date: Mon Aug 29 08:39:53 UTC 2016
New revision: 304992
URL: https://svnweb.freebsd.org/changeset/base/304992

Log:
  MFC r304597:
  Fix for invalid use of bits in input context. Basically split
  configuring of EP0 and non-EP0 into xhci_cmd_evaluate_ctx() and
  xhci_cmd_configure_ep() respectivly. This resolves some errors when
  using XHCI under QEMU and gets is more in line with the XHCI
  specification.

  PR:		212021

Changes:
_U  stable/11/
  stable/11/sys/dev/usb/controller/xhci.c
Comment 18 commit-hook freebsd_committer 2016-08-29 08:42:41 UTC
A commit references this bug:

Author: hselasky
Date: Mon Aug 29 08:42:37 UTC 2016
New revision: 304993
URL: https://svnweb.freebsd.org/changeset/base/304993

Log:
  MFC r304597:
  Fix for invalid use of bits in input context. Basically split
  configuring of EP0 and non-EP0 into xhci_cmd_evaluate_ctx() and
  xhci_cmd_configure_ep() respectivly. This resolves some errors when
  using XHCI under QEMU and gets is more in line with the XHCI
  specification.

  PR:		212021

Changes:
_U  stable/10/
  stable/10/sys/dev/usb/controller/xhci.c
Comment 19 commit-hook freebsd_committer 2016-08-29 08:44:43 UTC
A commit references this bug:

Author: hselasky
Date: Mon Aug 29 08:44:35 UTC 2016
New revision: 304994
URL: https://svnweb.freebsd.org/changeset/base/304994

Log:
  MFC r304597:
  Fix for invalid use of bits in input context. Basically split
  configuring of EP0 and non-EP0 into xhci_cmd_evaluate_ctx() and
  xhci_cmd_configure_ep() respectivly. This resolves some errors when
  using XHCI under QEMU and gets is more in line with the XHCI
  specification.

  PR:		212021

Changes:
_U  stable/9/sys/
  stable/9/sys/dev/usb/controller/xhci.c
Comment 20 commit-hook freebsd_committer 2016-08-29 08:46:44 UTC
A commit references this bug:

Author: hselasky
Date: Mon Aug 29 08:46:15 UTC 2016
New revision: 304995
URL: https://svnweb.freebsd.org/changeset/base/304995

Log:
  MFC r304597:
  Fix for invalid use of bits in input context. Basically split
  configuring of EP0 and non-EP0 into xhci_cmd_evaluate_ctx() and
  xhci_cmd_configure_ep() respectivly. This resolves some errors when
  using XHCI under QEMU and gets is more in line with the XHCI
  specification.

  PR:		212021

Changes:
_U  stable/8/sys/
_U  stable/8/sys/dev/
_U  stable/8/sys/dev/usb/
  stable/8/sys/dev/usb/controller/xhci.c
Comment 21 commit-hook freebsd_committer 2016-08-29 08:51:46 UTC
A commit references this bug:

Author: hselasky
Date: Mon Aug 29 08:51:28 UTC 2016
New revision: 304998
URL: https://svnweb.freebsd.org/changeset/base/304998

Log:
  MFC r304629:
  Don't separate the status stage of the XHCI USB control transfers into
  its own job because this breaks the simplified QEMU XHCI TRB parser,
  which expects the complete USB control transfer as a series of back to
  back TRBs. The old behaviour is kept under #ifdef in case this change
  breaks enumeration of any USB devices.

  PR:		212021

Changes:
_U  stable/10/
  stable/10/sys/dev/usb/controller/xhci.c
Comment 22 commit-hook freebsd_committer 2016-08-29 08:53:48 UTC
A commit references this bug:

Author: hselasky
Date: Mon Aug 29 08:52:53 UTC 2016
New revision: 304999
URL: https://svnweb.freebsd.org/changeset/base/304999

Log:
  MFC r304629:
  Don't separate the status stage of the XHCI USB control transfers into
  its own job because this breaks the simplified QEMU XHCI TRB parser,
  which expects the complete USB control transfer as a series of back to
  back TRBs. The old behaviour is kept under #ifdef in case this change
  breaks enumeration of any USB devices.

  PR:		212021

Changes:
_U  stable/11/
  stable/11/sys/dev/usb/controller/xhci.c
Comment 23 commit-hook freebsd_committer 2016-08-29 08:54:49 UTC
A commit references this bug:

Author: hselasky
Date: Mon Aug 29 08:54:31 UTC 2016
New revision: 305000
URL: https://svnweb.freebsd.org/changeset/base/305000

Log:
  MFC r304629:
  Don't separate the status stage of the XHCI USB control transfers into
  its own job because this breaks the simplified QEMU XHCI TRB parser,
  which expects the complete USB control transfer as a series of back to
  back TRBs. The old behaviour is kept under #ifdef in case this change
  breaks enumeration of any USB devices.

  PR:		212021

Changes:
_U  stable/9/sys/
  stable/9/sys/dev/usb/controller/xhci.c
Comment 24 commit-hook freebsd_committer 2016-08-29 08:56:51 UTC
A commit references this bug:

Author: hselasky
Date: Mon Aug 29 08:55:59 UTC 2016
New revision: 305001
URL: https://svnweb.freebsd.org/changeset/base/305001

Log:
  MFC r304629:
  Don't separate the status stage of the XHCI USB control transfers into
  its own job because this breaks the simplified QEMU XHCI TRB parser,
  which expects the complete USB control transfer as a series of back to
  back TRBs. The old behaviour is kept under #ifdef in case this change
  breaks enumeration of any USB devices.

  PR:		212021

Changes:
_U  stable/8/sys/
_U  stable/8/sys/dev/
_U  stable/8/sys/dev/usb/
  stable/8/sys/dev/usb/controller/xhci.c
Comment 25 Hans Petter Selasky freebsd_committer 2016-09-14 13:34:39 UTC
FYI:

Patches for QEMU are proceeding:
http://lists.nongnu.org/archive/html/qemu-devel/2016-09/msg03124.html