Series comparison

-[RFC PATCH 0/5] Add XHCI TR NOOP support, plus PCI, MSIX changes
+[PATCH 0/8] Add XHCI TR NOOP support, plus PCI, MSIX changes
 Hi,
-This RFC is wide-ranging for such a simple addition of XHCI TR NOOP
+This series adds XHCI "TR NOOP" support, but adding qtests ran into
-command, because adding qtests for it caught up some PCI/MSIX issues.
+issues with PCI and MSIX, and fixing those irritated what looks like
 a bug in the e1000e/igb tests.
-Firstly, the XHCI host shares a BAR between device registers and MSIX
+Firstly, the XHCI NEC PCI host has a shared BAR for device registers
-space, which is not handled by qtest/pci. Keeping track of BAR mappings
+and MSIX space, which is not handled by qtest/pci. Keeping track of BAR
-in qtest and allowing MSIX to use an existing mapping seems to work.
+mappings in qtest and allowing MSIX to use an existing mapping seems to
 work.
-For XHCI qtest, I added a MSIX check to ensure interrupts are working
+For the XHCI qtest, I added a MSIX check to ensure interrupts are working
-correctly. As part of that I need to dequeue masked interrupts which
+correctly, similarly to e1000e it polls the PBA bit with the interrupt
-seems to require PBA writes to clear them, but I could be way off
+masked. Doing this never actually clears the pending msix irq, because
-base here. Adding that does cause e1000e qtest to break, because it
+that is not implemented in the hw/pci code. So a test that waits for an
-doesn't like clear pending via PBA.
+interrupt to be triggered multiple times will fail.
-The XHCI model change gets AIX's XHCI driver working, so that's another
+Adding the PBA write operations to clear/trigger pending interrupts
-data point beyond the simple qtest for it added here. Unfortunately it
+breaks e1000e/igb tests that wait for multiple triggers, because they
-is not so easy to independently test.
+are actually buggy and only triggering the irq once, but the missing
 MSIX write support was saving them because it made it look like the
 irq had triggered.
-Any guidance or input would be welcome.
+My XHCI qtest caught that issue because it has some code which
 asserts that the interrupt is *not* pending.
 Since RFC I fixed up some checkpatch issues as Fabiano pointed out,
 and also dug into the e1000e/igb test failure a bit more and hopefully
 fixed it properly.
 Thanks,
 Nick
-Nicholas Piggin (5):
+Nicholas Piggin (8):
   qtest/pci: Enforce balanced iomap/unmap
-  qtest/libqos/pci: Fix msix_enable sharing bar0
+  qtest/libqos/pci: Fix qpci_msix_enable sharing bar0
   pci/msix: Implement PBA writes
+  tests/qtest/e1000e|igb: Fix e1000e and igb tests to re-trigger
+    interrupts
+  hw/usb/xhci: Move HCD constants to a header and add register constants
   qtest/xhci: Add controller and device setup and ring tests
-  hw/usb: Support XHCI TR NOOP commands
+  hw/usb/xhci: Support TR NOOP commands
   qtest/xhci: add a test for TR NOOP commands
+ hw/usb/hcd-xhci.h               | 197 ++++++++++++
  tests/qtest/libqos/ahci.h       |   1 +
  tests/qtest/libqos/pci.h        |   4 +
  tests/qtest/libqos/virtio-pci.h |   1 +
- tests/qtest/usb-hcd-xhci-test.h | 232 ++++++++++++++
  hw/pci/msix.c                   |  16 +
- hw/usb/hcd-xhci.c               |  28 +-
+ hw/usb/hcd-xhci.c               | 318 ++++++-------------
  tests/qtest/ahci-test.c         |   2 +
+ tests/qtest/e1000e-test.c       |   8 +-
+ tests/qtest/igb-test.c          |   8 +-
  tests/qtest/libqos/ahci.c       |   6 +
- tests/qtest/libqos/pci.c        |  75 ++++-
+ tests/qtest/libqos/e1000e.c     |   2 +-
  tests/qtest/libqos/pci.c        |  92 +++++-
  tests/qtest/libqos/virtio-pci.c |   6 +-
- tests/qtest/usb-hcd-xhci-test.c | 540 +++++++++++++++++++++++++++++++-
+ tests/qtest/usb-hcd-xhci-test.c | 535 +++++++++++++++++++++++++++++++-
-files changed, 893 insertions(+), 18 deletions(-)
+files changed, 943 insertions(+), 253 deletions(-)
  create mode 100644 tests/qtest/usb-hcd-xhci-test.h
 --
 .45.2

-[RFC PATCH 1/5] qtest/pci: Enforce balanced iomap/unmap
+[PATCH 1/8] qtest/pci: Enforce balanced iomap/unmap
 Add assertions to ensure a BAR is not mapped twice, and only
 previously mapped BARs are unmapped. This can help catch some
 bugs.
+Cc: Michael S. Tsirkin <mst@redhat.com>
+Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
+Reviewed-by: Fabiano Rosas <farosas@suse.de>
 Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
 ---
  tests/qtest/libqos/ahci.h       |  1 +
  tests/qtest/libqos/pci.h        |  2 ++
  tests/qtest/libqos/virtio-pci.h |  1 +
 ...

-[RFC PATCH 2/5] qtest/libqos/pci: Fix msix_enable sharing bar0
+[PATCH 2/8] qtest/libqos/pci: Fix qpci_msix_enable sharing bar0
 Devices where the MSI-X addresses are shared with other MMIO on BAR0
 can not use msi_enable because it unmaps and remaps BAR0, which
 interferes with device MMIO mappings. xhci-nec is one such device we
 would like to test msix with.
-Keep track of each the BAR iomaps for each device and add code in msix
+Use the BAR iomap tracking structure introduced in the previous change
-to use existing iomap if the msix bars are already mapped.
+to have qpci_misx_enable() use existing iomaps if msix bars are
 already mapped.
+Cc: Michael S. Tsirkin <mst@redhat.com>
+Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
+Reviewed-by: Fabiano Rosas <farosas@suse.de>
 Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
 ---
  tests/qtest/libqos/pci.h |  1 +
- tests/qtest/libqos/pci.c | 22 ++++++++++++++++------
+ tests/qtest/libqos/pci.c | 40 ++++++++++++++++++++++++++++++++++------
-files changed, 17 insertions(+), 6 deletions(-)
+files changed, 35 insertions(+), 6 deletions(-)
 diff --git a/tests/qtest/libqos/pci.h b/tests/qtest/libqos/pci.h
 index XXXXXXX..XXXXXXX 100644
 --- a/tests/qtest/libqos/pci.h
 +++ b/tests/qtest/libqos/pci.h
 ...
 +        dev->msix_pba_bar_iomap = true;
 +        dev->msix_pba_bar = qpci_iomap(dev, bir_pba, NULL);
      }
      dev->msix_pba_off = table & ~PCI_MSIX_FLAGS_BIRMASK;
+@@ -XXX,XX +XXX,XX @@ void qpci_msix_disable(QPCIDevice *dev)
+ {
+     uint8_t addr;
+     uint16_t val;
++    uint32_t table;
+     g_assert(dev->msix_enabled);
+     addr = qpci_find_capability(dev, PCI_CAP_ID_MSIX, 0);
 @@ -XXX,XX +XXX,XX @@ void qpci_msix_disable(QPCIDevice *dev)
      qpci_config_writew(dev, addr + PCI_MSIX_FLAGS,
                                                  val & ~PCI_MSIX_FLAGS_ENABLE);
 -    if (dev->msix_pba_bar.addr != dev->msix_table_bar.addr) {
 +    if (dev->msix_pba_bar_iomap) {
 +        dev->msix_pba_bar_iomap = false;
          qpci_iounmap(dev, dev->msix_pba_bar);
-     }
++    } else {
--    qpci_iounmap(dev, dev->msix_table_bar);
++        /*
 +         * If we had reused an existing iomap, ensure it is still mapped
 +         * otherwise it would be a bug if it were unmapped before msix is
 +         * disabled. A refcounting iomap implementation could avoid this
 +         * issue entirely, but let's wait until that's needed.
 +         */
 +        uint8_t bir_pba;
 +        table = qpci_config_readl(dev, addr + PCI_MSIX_PBA);
 +        bir_pba = table & PCI_MSIX_FLAGS_BIRMASK;
 +        g_assert(dev->bars_mapped[bir_pba]);
 +    }
 +
 +    if (dev->msix_table_bar_iomap) {
 +        dev->msix_table_bar_iomap = false;
 +        qpci_iounmap(dev, dev->msix_table_bar);
-+    }
++    } else {
 +        uint8_t bir_table;
 +        table = qpci_config_readl(dev, addr + PCI_MSIX_TABLE);
 +        bir_table = table & PCI_MSIX_FLAGS_BIRMASK;
 +        g_assert(dev->bars_mapped[bir_table]);
      }
 -    qpci_iounmap(dev, dev->msix_table_bar);
      dev->msix_enabled = 0;
      dev->msix_table_off = 0;
 --
 .45.2

-[RFC PATCH 3/5] pci/msix: Implement PBA writes
+[PATCH 3/8] pci/msix: Implement PBA writes
-Implement PBA write 1 to trigger and 0 to clear. This is used by
+Implement MMIO PBA writes, 1 to trigger and 0 to clear.
 qtests which mask the MSI irq and so the bits remain pending and
 expect to be cleared with stores.
-Some devices like e1000e seem to have MSIX PBA pending tied to
+This functionality is used by some qtests, which keep the msix irq
-some device state level, as such they call msix_clr_pending()
+masked and test irq pending via the PBA bits, for simplicity. Some
-directly, and clearing pending via a store to PBA causes this
+tests expect to be able to clear the irq with a store, so a side-effect
-to go out of synch. So the qpci_msix_pending() function is
+of this is that qpci_msix_pending() would actually clear the pending
-changed to avoid clearing, and a new test-and-clear function is
+bit where it previously did not. This actually causes some [possibly
-added for tests that would like to clear.
+buggy] tests to fail. So to avoid breakage until tests are re-examined,
 prior behavior of qpci_msix_pending() is kept by changing it to avoid
 clearing PBA.
+A new function qpci_msix_test_clear_pending() is added for tests that
+do want the PBA clearing, and it will be used by XHCI and e1000e/igb
+tests in subsequent changes.
+Cc: Michael S. Tsirkin <mst@redhat.com>
+Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
+Cc: Dmitry Fleytman <dmitry.fleytman@gmail.com>
+Cc: Akihiko Odaki <akihiko.odaki@daynix.com>
+Cc: Sriram Yagnaraman <sriram.yagnaraman@ericsson.com>
 Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
 ---
  tests/qtest/libqos/pci.h |  1 +
  hw/pci/msix.c            | 16 ++++++++++++++++
- tests/qtest/libqos/pci.c | 21 ++++++++++++++++++---
+ tests/qtest/libqos/pci.c | 20 +++++++++++++++++---
-files changed, 35 insertions(+), 3 deletions(-)
+files changed, 34 insertions(+), 3 deletions(-)
 diff --git a/tests/qtest/libqos/pci.h b/tests/qtest/libqos/pci.h
 index XXXXXXX..XXXXXXX 100644
 --- a/tests/qtest/libqos/pci.h
 +++ b/tests/qtest/libqos/pci.h
 ...
      g_assert(dev->msix_enabled);
      pba_entry = qpci_io_readl(dev, dev->msix_pba_bar, dev->msix_pba_off + off);
 -    qpci_io_writel(dev, dev->msix_pba_bar, dev->msix_pba_off + off,
 -                   pba_entry & ~(1 << bit_n));
 -    return (pba_entry & (1 << bit_n)) != 0;
-+    return (pba_entry & (1 << bit_n));
++    return pba_entry & (1 << bit_n);
- }
++}
++
 +bool qpci_msix_test_clear_pending(QPCIDevice *dev, uint16_t entry)
 +{
 +    uint32_t pba_entry;
 +    uint8_t bit_n = entry % 32;
 +    uint64_t  off = (entry / 32) * PCI_MSIX_ENTRY_SIZE / 4;
 ...
 +        qpci_io_writel(dev, dev->msix_pba_bar, dev->msix_pba_off + off,
 +                       pba_entry & ~(1 << bit_n));
 +        return true;
 +    }
 +    return false;
-+}
+ }
-+
 +
  bool qpci_msix_masked(QPCIDevice *dev, uint16_t entry)
- {
-     uint8_t addr;
 --
 .45.2

-New patch
+[PATCH 4/8] tests/qtest/e1000e|igb: Fix e1000e and igb tests to re-trigger interrupts
+The e1000e and igb tests don't clear the msix pending bit after waiting
+for it sit is masked so the irq doesn't get delivered. Failing to clear
+the pending interrupt means all subsequent waits for interrupt after the
+first do not actually wait for an interrupt genreated by the device.
+Explicitly clearing the msix pending bit results in the
+multiple-transfers test hanging waiting for the second interrupt. This
+happens because the e1000e and igb tests do not clear (or set
+auto-clear) on queue interrupts, so the cause remains ste in ICR/EICR,
+which inhibits triggering of a new interrupt.
+Fix both these problems. Clear the msix pending bit explicitly after
+waiting for it; and clear the ICR/EICR cause bits after seeing and
+interrupt (also verify we saw the correct cause bit).
+Cc: Michael S. Tsirkin <mst@redhat.com>
+Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
+Cc: Dmitry Fleytman <dmitry.fleytman@gmail.com>
+Cc: Akihiko Odaki <akihiko.odaki@daynix.com>
+Cc: Sriram Yagnaraman <sriram.yagnaraman@ericsson.com>
+Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
+---
+ tests/qtest/e1000e-test.c   | 8 ++++++--
+ tests/qtest/igb-test.c      | 8 ++++++--
+ tests/qtest/libqos/e1000e.c | 2 +-
+files changed, 13 insertions(+), 5 deletions(-)
+diff --git a/tests/qtest/e1000e-test.c b/tests/qtest/e1000e-test.c
+index XXXXXXX..XXXXXXX 100644
+--- a/tests/qtest/e1000e-test.c
++++ b/tests/qtest/e1000e-test.c
+@@ -XXX,XX +XXX,XX @@ static void e1000e_send_verify(QE1000E *d, int *test_sockets, QGuestAllocator *a
+     /* Put descriptor to the ring */
+     e1000e_tx_ring_push(d, &descr);
+-    /* Wait for TX WB interrupt */
++    /* Wait for TX WB interrupt (this clears the MSIX PBA) */
+     e1000e_wait_isr(d, E1000E_TX0_MSG_ID);
++    /* Read ICR which clears it ready for next interrupt, assert TXQ0 cause */
++    g_assert(e1000e_macreg_read(d, E1000_ICR) & E1000_ICR_TXQ0);
+     /* Check DD bit */
+     g_assert_cmphex(le32_to_cpu(descr.upper.data) & E1000_TXD_STAT_DD, ==,
+@@ -XXX,XX +XXX,XX @@ static void e1000e_receive_verify(QE1000E *d, int *test_sockets, QGuestAllocator
+     /* Put descriptor to the ring */
+     e1000e_rx_ring_push(d, &descr);
+-    /* Wait for TX WB interrupt */
++    /* Wait for TX WB interrupt (this clears the MSIX PBA) */
+     e1000e_wait_isr(d, E1000E_RX0_MSG_ID);
++    /* Read ICR which clears it ready for next interrupt, assert RXQ0 cause */
++    g_assert(e1000e_macreg_read(d, E1000_ICR) & E1000_ICR_RXQ0);
+     /* Check DD bit */
+     g_assert_cmphex(le32_to_cpu(descr.wb.upper.status_error) &
+diff --git a/tests/qtest/igb-test.c b/tests/qtest/igb-test.c
+index XXXXXXX..XXXXXXX 100644
+--- a/tests/qtest/igb-test.c
++++ b/tests/qtest/igb-test.c
+@@ -XXX,XX +XXX,XX @@ static void igb_send_verify(QE1000E *d, int *test_sockets, QGuestAllocator *allo
+     /* Put descriptor to the ring */
+     e1000e_tx_ring_push(d, &descr);
+-    /* Wait for TX WB interrupt */
++    /* Wait for TX WB interrupt (this clears the MSIX PBA) */
+     e1000e_wait_isr(d, E1000E_TX0_MSG_ID);
++    /* Read EICR which clears it ready for next interrupt, assert TXQ0 cause */
++    g_assert(e1000e_macreg_read(d, E1000_EICR) & (1 << E1000E_TX0_MSG_ID));
+     /* Check DD bit */
+     g_assert_cmphex(le32_to_cpu(descr.wb.status) & E1000_TXD_STAT_DD, ==,
+@@ -XXX,XX +XXX,XX @@ static void igb_receive_verify(QE1000E *d, int *test_sockets, QGuestAllocator *a
+     /* Put descriptor to the ring */
+     e1000e_rx_ring_push(d, &descr);
+-    /* Wait for TX WB interrupt */
++    /* Wait for TX WB interrupt (this clears the MSIX PBA) */
+     e1000e_wait_isr(d, E1000E_RX0_MSG_ID);
++    /* Read EICR which clears it ready for next interrupt, assert RXQ0 cause */
++    g_assert(e1000e_macreg_read(d, E1000_EICR) & (1 << E1000E_RX0_MSG_ID));
+     /* Check DD bit */
+     g_assert_cmphex(le32_to_cpu(descr.wb.upper.status_error) &
+diff --git a/tests/qtest/libqos/e1000e.c b/tests/qtest/libqos/e1000e.c
+index XXXXXXX..XXXXXXX 100644
+--- a/tests/qtest/libqos/e1000e.c
++++ b/tests/qtest/libqos/e1000e.c
+@@ -XXX,XX +XXX,XX @@ void e1000e_wait_isr(QE1000E *d, uint16_t msg_id)
+     guint64 end_time = g_get_monotonic_time() + 5 * G_TIME_SPAN_SECOND;
+     do {
+-        if (qpci_msix_pending(&d_pci->pci_dev, msg_id)) {
++        if (qpci_msix_test_clear_pending(&d_pci->pci_dev, msg_id)) {
+             return;
+         }
+         qtest_clock_step(d_pci->pci_dev.bus->qts, 10000);
+--
+.45.2

-New patch
+[PATCH 5/8] hw/usb/xhci: Move HCD constants to a header and add register constants
+Prepare to use some of these constants in xhci qtest code.
+Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
+---
+ hw/usb/hcd-xhci.h | 190 +++++++++++++++++++++++++++++++
+ hw/usb/hcd-xhci.c | 283 ++++++++++------------------------------------
+files changed, 250 insertions(+), 223 deletions(-)
+diff --git a/hw/usb/hcd-xhci.h b/hw/usb/hcd-xhci.h
+index XXXXXXX..XXXXXXX 100644
+--- a/hw/usb/hcd-xhci.h
++++ b/hw/usb/hcd-xhci.h
+@@ -XXX,XX +XXX,XX @@ typedef enum TRBCCode {
+     CC_SPLIT_TRANSACTION_ERROR
+ } TRBCCode;
++/* Register definitions */
++#define XHCI_HCCAP_CAPLENGTH     0x00
++#define XHCI_HCCAP_HCIVERSION    0x02
++#define XHCI_HCCAP_HCSPARAMS1    0x04
++#define XHCI_HCCAP_HCSPARAMS2    0x08
++#define XHCI_HCCAP_HCSPARAMS3    0x0C
++#define XHCI_HCCAP_HCCPARAMS1    0x10
++#define XHCI_HCCAP_DBOFF         0x14
++#define XHCI_HCCAP_RTSOFF        0x18
++#define XHCI_HCCAP_HCCPARAMS2    0x1C
++#define XHCI_HCCAP_EXTCAP_START  0x20 /* SW-defined */
++
++#define XHCI_PORT_PORTSC         0x00
++#define   PORTSC_CCS             (1 << 0)
++#define   PORTSC_PED             (1 << 1)
++#define   PORTSC_OCA             (1 << 3)
++#define   PORTSC_PR              (1 << 4)
++#define   PORTSC_PLS_SHIFT           5
++#define   PORTSC_PLS_MASK        0xf
++#define   PORTSC_PP              (1 << 9)
++#define   PORTSC_SPEED_SHIFT        10
++#define   PORTSC_SPEED_MASK      0xf
++#define   PORTSC_SPEED_FULL      (1 << 10)
++#define   PORTSC_SPEED_LOW       (2 << 10)
++#define   PORTSC_SPEED_HIGH      (3 << 10)
++#define   PORTSC_SPEED_SUPER     (4 << 10)
++#define   PORTSC_PIC_SHIFT          14
++#define   PORTSC_PIC_MASK        0x3
++#define   PORTSC_LWS             (1 << 16)
++#define   PORTSC_CSC             (1 << 17)
++#define   PORTSC_PEC             (1 << 18)
++#define   PORTSC_WRC             (1 << 19)
++#define   PORTSC_OCC             (1 << 20)
++#define   PORTSC_PRC             (1 << 21)
++#define   PORTSC_PLC             (1 << 22)
++#define   PORTSC_CEC             (1 << 23)
++#define   PORTSC_CAS             (1 << 24)
++#define   PORTSC_WCE             (1 << 25)
++#define   PORTSC_WDE             (1 << 26)
++#define   PORTSC_WOE             (1 << 27)
++#define   PORTSC_DR              (1 << 30)
++#define   PORTSC_WPR             (1 << 31)
++#define XHCI_PORT_PORTPMSC       0x04
++#define XHCI_PORT_PORTLI         0x08
++#define XHCI_PORT_PORTHLPMC      0x0C
++
++#define XHCI_OPER_USBCMD         0x00
++#define   USBCMD_RS              (1 << 0)
++#define   USBCMD_HCRST           (1 << 1)
++#define   USBCMD_INTE            (1 << 2)
++#define   USBCMD_HSEE            (1 << 3)
++#define   USBCMD_LHCRST          (1 << 7)
++#define   USBCMD_CSS             (1 << 8)
++#define   USBCMD_CRS             (1 << 9)
++#define   USBCMD_EWE             (1 << 10)
++#define   USBCMD_EU3S            (1 << 11)
++#define XHCI_OPER_USBSTS         0x04
++#define   USBSTS_HCH             (1 << 0)
++#define   USBSTS_HSE             (1 << 2)
++#define   USBSTS_EINT            (1 << 3)
++#define   USBSTS_PCD             (1 << 4)
++#define   USBSTS_SSS             (1 << 8)
++#define   USBSTS_RSS             (1 << 9)
++#define   USBSTS_SRE             (1 << 10)
++#define   USBSTS_CNR             (1 << 11)
++#define   USBSTS_HCE             (1 << 12)
++#define XHCI_OPER_PAGESIZE       0x08
++#define XHCI_OPER_DNCTRL         0x14
++#define XHCI_OPER_CRCR_LO        0x18
++#define   CRCR_RCS              (1 << 0)
++#define   CRCR_CS               (1 << 1)
++#define   CRCR_CA               (1 << 2)
++#define   CRCR_CRR              (1 << 3)
++#define XHCI_OPER_CRCR_HI        0x1C
++#define XHCI_OPER_DCBAAP_LO      0x30
++#define XHCI_OPER_DCBAAP_HI      0x34
++#define XHCI_OPER_CONFIG         0x38
++
++#define XHCI_OPER_MFINDEX        0x00
++#define XHCI_OPER_IR0            0x20
++#define XHCI_OPER_IR_SZ          0x20
++
++#define XHCI_INTR_IMAN           0x00
++#define   IMAN_IP                (1 << 0)
++#define   IMAN_IE                (1 << 1)
++#define XHCI_INTR_IMOD           0x04
++#define XHCI_INTR_ERSTSZ         0x08
++#define XHCI_INTR_ERSTBA_LO      0x10
++#define XHCI_INTR_ERSTBA_HI      0x14
++#define XHCI_INTR_ERDP_LO        0x18
++#define   ERDP_EHB               (1 << 3)
++#define XHCI_INTR_ERDP_HI        0x1C
++
++#define TRB_SIZE 16
++typedef struct XHCITRB {
++    uint64_t parameter;
++    uint32_t status;
++    uint32_t control;
++    dma_addr_t addr;
++    bool ccs;
++} XHCITRB;
++
++enum {
++    PLS_U0              =  0,
++    PLS_U1              =  1,
++    PLS_U2              =  2,
++    PLS_U3              =  3,
++    PLS_DISABLED        =  4,
++    PLS_RX_DETECT       =  5,
++    PLS_INACTIVE        =  6,
++    PLS_POLLING         =  7,
++    PLS_RECOVERY        =  8,
++    PLS_HOT_RESET       =  9,
++    PLS_COMPILANCE_MODE = 10,
++    PLS_TEST_MODE       = 11,
++    PLS_RESUME          = 15,
++};
++
++#define CR_LINK TR_LINK
++
++#define TRB_C               (1 << 0)
++#define TRB_TYPE_SHIFT          10
++#define TRB_TYPE_MASK       0x3f
++#define TRB_TYPE(t)         (((t).control >> TRB_TYPE_SHIFT) & TRB_TYPE_MASK)
++
++#define TRB_EV_ED           (1 << 2)
++
++#define TRB_TR_ENT          (1 << 1)
++#define TRB_TR_ISP          (1 << 2)
++#define TRB_TR_NS           (1 << 3)
++#define TRB_TR_CH           (1 << 4)
++#define TRB_TR_IOC          (1 << 5)
++#define TRB_TR_IDT          (1 << 6)
++#define TRB_TR_TBC_SHIFT        7
++#define TRB_TR_TBC_MASK     0x3
++#define TRB_TR_BEI          (1 << 9)
++#define TRB_TR_TLBPC_SHIFT      16
++#define TRB_TR_TLBPC_MASK   0xf
++#define TRB_TR_FRAMEID_SHIFT    20
++#define TRB_TR_FRAMEID_MASK 0x7ff
++#define TRB_TR_SIA          (1 << 31)
++
++#define TRB_TR_DIR          (1 << 16)
++
++#define TRB_CR_SLOTID_SHIFT     24
++#define TRB_CR_SLOTID_MASK  0xff
++#define TRB_CR_EPID_SHIFT       16
++#define TRB_CR_EPID_MASK    0x1f
++
++#define TRB_CR_BSR          (1 << 9)
++#define TRB_CR_DC           (1 << 9)
++
++#define TRB_LK_TC           (1 << 1)
++
++#define TRB_INTR_SHIFT          22
++#define TRB_INTR_MASK       0x3ff
++#define TRB_INTR(t)         (((t).status >> TRB_INTR_SHIFT) & TRB_INTR_MASK)
++
++#define EP_TYPE_MASK        0x7
++#define EP_TYPE_SHIFT           3
++
++#define EP_STATE_MASK       0x7
++#define EP_DISABLED         (0 << 0)
++#define EP_RUNNING          (1 << 0)
++#define EP_HALTED           (2 << 0)
++#define EP_STOPPED          (3 << 0)
++#define EP_ERROR            (4 << 0)
++
++#define SLOT_STATE_MASK     0x1f
++#define SLOT_STATE_SHIFT        27
++#define SLOT_STATE(s)       (((s) >> SLOT_STATE_SHIFT) & SLOT_STATE_MASK)
++#define SLOT_ENABLED        0
++#define SLOT_DEFAULT        1
++#define SLOT_ADDRESSED      2
++#define SLOT_CONFIGURED     3
++
++#define SLOT_CONTEXT_ENTRIES_MASK 0x1f
++#define SLOT_CONTEXT_ENTRIES_SHIFT 27
++
++typedef enum EPType {
++    ET_INVALID = 0,
++    ET_ISO_OUT,
++    ET_BULK_OUT,
++    ET_INTR_OUT,
++    ET_CONTROL,
++    ET_ISO_IN,
++    ET_BULK_IN,
++    ET_INTR_IN,
++} EPType;
++
+ typedef struct XHCIRing {
+     dma_addr_t dequeue;
+     bool ccs;
+diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
+index XXXXXXX..XXXXXXX 100644
+--- a/hw/usb/hcd-xhci.c
++++ b/hw/usb/hcd-xhci.c
+@@ -XXX,XX +XXX,XX @@
+ # error Increase XHCI_LEN_REGS
+ #endif
+-/* bit definitions */
+-#define USBCMD_RS       (1<<0)
+-#define USBCMD_HCRST    (1<<1)
+-#define USBCMD_INTE     (1<<2)
+-#define USBCMD_HSEE     (1<<3)
+-#define USBCMD_LHCRST   (1<<7)
+-#define USBCMD_CSS      (1<<8)
+-#define USBCMD_CRS      (1<<9)
+-#define USBCMD_EWE      (1<<10)
+-#define USBCMD_EU3S     (1<<11)
+-
+-#define USBSTS_HCH      (1<<0)
+-#define USBSTS_HSE      (1<<2)
+-#define USBSTS_EINT     (1<<3)
+-#define USBSTS_PCD      (1<<4)
+-#define USBSTS_SSS      (1<<8)
+-#define USBSTS_RSS      (1<<9)
+-#define USBSTS_SRE      (1<<10)
+-#define USBSTS_CNR      (1<<11)
+-#define USBSTS_HCE      (1<<12)
+-
+-
+-#define PORTSC_CCS          (1<<0)
+-#define PORTSC_PED          (1<<1)
+-#define PORTSC_OCA          (1<<3)
+-#define PORTSC_PR           (1<<4)
+-#define PORTSC_PLS_SHIFT        5
+-#define PORTSC_PLS_MASK     0xf
+-#define PORTSC_PP           (1<<9)
+-#define PORTSC_SPEED_SHIFT      10
+-#define PORTSC_SPEED_MASK   0xf
+-#define PORTSC_SPEED_FULL   (1<<10)
+-#define PORTSC_SPEED_LOW    (2<<10)
+-#define PORTSC_SPEED_HIGH   (3<<10)
+-#define PORTSC_SPEED_SUPER  (4<<10)
+-#define PORTSC_PIC_SHIFT        14
+-#define PORTSC_PIC_MASK     0x3
+-#define PORTSC_LWS          (1<<16)
+-#define PORTSC_CSC          (1<<17)
+-#define PORTSC_PEC          (1<<18)
+-#define PORTSC_WRC          (1<<19)
+-#define PORTSC_OCC          (1<<20)
+-#define PORTSC_PRC          (1<<21)
+-#define PORTSC_PLC          (1<<22)
+-#define PORTSC_CEC          (1<<23)
+-#define PORTSC_CAS          (1<<24)
+-#define PORTSC_WCE          (1<<25)
+-#define PORTSC_WDE          (1<<26)
+-#define PORTSC_WOE          (1<<27)
+-#define PORTSC_DR           (1<<30)
+-#define PORTSC_WPR          (1<<31)
+-
+-#define CRCR_RCS        (1<<0)
+-#define CRCR_CS         (1<<1)
+-#define CRCR_CA         (1<<2)
+-#define CRCR_CRR        (1<<3)
+-
+-#define IMAN_IP         (1<<0)
+-#define IMAN_IE         (1<<1)
+-
+-#define ERDP_EHB        (1<<3)
+-
+-#define TRB_SIZE 16
+-typedef struct XHCITRB {
+-    uint64_t parameter;
+-    uint32_t status;
+-    uint32_t control;
+-    dma_addr_t addr;
+-    bool ccs;
+-} XHCITRB;
+-
+-enum {
+-    PLS_U0              =  0,
+-    PLS_U1              =  1,
+-    PLS_U2              =  2,
+-    PLS_U3              =  3,
+-    PLS_DISABLED        =  4,
+-    PLS_RX_DETECT       =  5,
+-    PLS_INACTIVE        =  6,
+-    PLS_POLLING         =  7,
+-    PLS_RECOVERY        =  8,
+-    PLS_HOT_RESET       =  9,
+-    PLS_COMPILANCE_MODE = 10,
+-    PLS_TEST_MODE       = 11,
+-    PLS_RESUME          = 15,
+-};
+-
+-#define CR_LINK TR_LINK
+-
+-#define TRB_C               (1<<0)
+-#define TRB_TYPE_SHIFT          10
+-#define TRB_TYPE_MASK       0x3f
+-#define TRB_TYPE(t)         (((t).control >> TRB_TYPE_SHIFT) & TRB_TYPE_MASK)
+-
+-#define TRB_EV_ED           (1<<2)
+-
+-#define TRB_TR_ENT          (1<<1)
+-#define TRB_TR_ISP          (1<<2)
+-#define TRB_TR_NS           (1<<3)
+-#define TRB_TR_CH           (1<<4)
+-#define TRB_TR_IOC          (1<<5)
+-#define TRB_TR_IDT          (1<<6)
+-#define TRB_TR_TBC_SHIFT        7
+-#define TRB_TR_TBC_MASK     0x3
+-#define TRB_TR_BEI          (1<<9)
+-#define TRB_TR_TLBPC_SHIFT      16
+-#define TRB_TR_TLBPC_MASK   0xf
+-#define TRB_TR_FRAMEID_SHIFT    20
+-#define TRB_TR_FRAMEID_MASK 0x7ff
+-#define TRB_TR_SIA          (1<<31)
+-
+-#define TRB_TR_DIR          (1<<16)
+-
+-#define TRB_CR_SLOTID_SHIFT     24
+-#define TRB_CR_SLOTID_MASK  0xff
+-#define TRB_CR_EPID_SHIFT       16
+-#define TRB_CR_EPID_MASK    0x1f
+-
+-#define TRB_CR_BSR          (1<<9)
+-#define TRB_CR_DC           (1<<9)
+-
+-#define TRB_LK_TC           (1<<1)
+-
+-#define TRB_INTR_SHIFT          22
+-#define TRB_INTR_MASK       0x3ff
+-#define TRB_INTR(t)         (((t).status >> TRB_INTR_SHIFT) & TRB_INTR_MASK)
+-
+-#define EP_TYPE_MASK        0x7
+-#define EP_TYPE_SHIFT           3
+-
+-#define EP_STATE_MASK       0x7
+-#define EP_DISABLED         (0<<0)
+-#define EP_RUNNING          (1<<0)
+-#define EP_HALTED           (2<<0)
+-#define EP_STOPPED          (3<<0)
+-#define EP_ERROR            (4<<0)
+-
+-#define SLOT_STATE_MASK     0x1f
+-#define SLOT_STATE_SHIFT        27
+-#define SLOT_STATE(s)       (((s)>>SLOT_STATE_SHIFT)&SLOT_STATE_MASK)
+-#define SLOT_ENABLED        0
+-#define SLOT_DEFAULT        1
+-#define SLOT_ADDRESSED      2
+-#define SLOT_CONFIGURED     3
+-
+-#define SLOT_CONTEXT_ENTRIES_MASK 0x1f
+-#define SLOT_CONTEXT_ENTRIES_SHIFT 27
+-
+ #define get_field(data, field)                  \
+     (((data) >> field##_SHIFT) & field##_MASK)
+@@ -XXX,XX +XXX,XX @@ enum {
+         *data = val_;                                           \
+     } while (0)
+-typedef enum EPType {
+-    ET_INVALID = 0,
+-    ET_ISO_OUT,
+-    ET_BULK_OUT,
+-    ET_INTR_OUT,
+-    ET_CONTROL,
+-    ET_ISO_IN,
+-    ET_BULK_IN,
+-    ET_INTR_IN,
+-} EPType;
+-
+ typedef struct XHCITransfer {
+     XHCIEPContext *epctx;
+     USBPacket packet;
+@@ -XXX,XX +XXX,XX @@ static uint64_t xhci_cap_read(void *ptr, hwaddr reg, unsigned size)
+     uint32_t ret;
+     switch (reg) {
+-    case 0x00: /* HCIVERSION, CAPLENGTH */
++    case XHCI_HCCAP_CAPLENGTH: /* Covers HCIVERSION and CAPLENGTH */
+         ret = 0x01000000 | LEN_CAP;
+         break;
+-    case 0x04: /* HCSPARAMS 1 */
++    case XHCI_HCCAP_HCSPARAMS1:
+         ret = ((xhci->numports_2+xhci->numports_3)<<24)
+             | (xhci->numintrs<<8) | xhci->numslots;
+         break;
+-    case 0x08: /* HCSPARAMS 2 */
++    case XHCI_HCCAP_HCSPARAMS2:
+         ret = 0x0000000f;
+         break;
+-    case 0x0c: /* HCSPARAMS 3 */
++    case XHCI_HCCAP_HCSPARAMS3:
+         ret = 0x00000000;
+         break;
+-    case 0x10: /* HCCPARAMS */
+-        if (sizeof(dma_addr_t) == 4) {
+-            ret = 0x00080000 | (xhci->max_pstreams_mask << 12);
+-        } else {
+-            ret = 0x00080001 | (xhci->max_pstreams_mask << 12);
++    case XHCI_HCCAP_HCCPARAMS1:
++        ret = (XHCI_HCCAP_EXTCAP_START >> 2) | (xhci->max_pstreams_mask << 12);
++        if (sizeof(dma_addr_t) == 8) {
++            ret |= 0x00000001; /* AC64 */
+         }
+         break;
+-    case 0x14: /* DBOFF */
++    case XHCI_HCCAP_DBOFF:
+         ret = OFF_DOORBELL;
+         break;
+-    case 0x18: /* RTSOFF */
++    case XHCI_HCCAP_RTSOFF:
+         ret = OFF_RUNTIME;
+         break;
+     /* extended capabilities */
+-    case 0x20: /* Supported Protocol:00 */
++    case XHCI_HCCAP_EXTCAP_START + 0x00: /* Supported Protocol:00 */
+         ret = 0x02000402; /* USB 2.0 */
+         break;
+-    case 0x24: /* Supported Protocol:04 */
++    case XHCI_HCCAP_EXTCAP_START + 0x04: /* Supported Protocol:04 */
+         ret = 0x20425355; /* "USB " */
+         break;
+-    case 0x28: /* Supported Protocol:08 */
++    case XHCI_HCCAP_EXTCAP_START + 0x08: /* Supported Protocol:08 */
+         ret = (xhci->numports_2 << 8) | (xhci->numports_3 + 1);
+         break;
+-    case 0x2c: /* Supported Protocol:0c */
++    case XHCI_HCCAP_EXTCAP_START + 0x0c: /* Supported Protocol:0c */
+         ret = 0x00000000; /* reserved */
+         break;
+-    case 0x30: /* Supported Protocol:00 */
++    case XHCI_HCCAP_EXTCAP_START + 0x10: /* Supported Protocol:00 */
+         ret = 0x03000002; /* USB 3.0 */
+         break;
+-    case 0x34: /* Supported Protocol:04 */
++    case XHCI_HCCAP_EXTCAP_START + 0x14: /* Supported Protocol:04 */
+         ret = 0x20425355; /* "USB " */
+         break;
+-    case 0x38: /* Supported Protocol:08 */
++    case XHCI_HCCAP_EXTCAP_START + 0x18: /* Supported Protocol:08 */
+         ret = (xhci->numports_3 << 8) | 1;
+         break;
+-    case 0x3c: /* Supported Protocol:0c */
++    case XHCI_HCCAP_EXTCAP_START + 0x1c: /* Supported Protocol:0c */
+         ret = 0x00000000; /* reserved */
+         break;
+     default:
+@@ -XXX,XX +XXX,XX @@ static uint64_t xhci_port_read(void *ptr, hwaddr reg, unsigned size)
+     uint32_t ret;
+     switch (reg) {
+-    case 0x00: /* PORTSC */
++    case XHCI_PORT_PORTSC:
+         ret = port->portsc;
+         break;
+-    case 0x04: /* PORTPMSC */
+-    case 0x08: /* PORTLI */
++    case XHCI_PORT_PORTPMSC:
++    case XHCI_PORT_PORTLI:
+         ret = 0;
+         break;
+-    case 0x0c: /* reserved */
+     default:
+         trace_usb_xhci_unimplemented("port read", reg);
+         ret = 0;
+@@ -XXX,XX +XXX,XX @@ static void xhci_port_write(void *ptr, hwaddr reg,
+     trace_usb_xhci_port_write(port->portnr, reg, val);
+     switch (reg) {
+-    case 0x00: /* PORTSC */
++    case XHCI_PORT_PORTSC:
+         /* write-1-to-start bits */
+         if (val & PORTSC_WPR) {
+             xhci_port_reset(port, true);
+@@ -XXX,XX +XXX,XX @@ static void xhci_port_write(void *ptr, hwaddr reg,
+             xhci_port_notify(port, notify);
+         }
+         break;
+-    case 0x04: /* PORTPMSC */
+-    case 0x08: /* PORTLI */
+     default:
+         trace_usb_xhci_unimplemented("port write", reg);
+     }
+@@ -XXX,XX +XXX,XX @@ static uint64_t xhci_oper_read(void *ptr, hwaddr reg, unsigned size)
+     uint32_t ret;
+     switch (reg) {
+-    case 0x00: /* USBCMD */
++    case XHCI_OPER_USBCMD:
+         ret = xhci->usbcmd;
+         break;
+-    case 0x04: /* USBSTS */
++    case XHCI_OPER_USBSTS:
+         ret = xhci->usbsts;
+         break;
+-    case 0x08: /* PAGESIZE */
++    case XHCI_OPER_PAGESIZE:
+         ret = 1; /* 4KiB */
+         break;
+-    case 0x14: /* DNCTRL */
++    case XHCI_OPER_DNCTRL:
+         ret = xhci->dnctrl;
+         break;
+-    case 0x18: /* CRCR low */
++    case XHCI_OPER_CRCR_LO:
+         ret = xhci->crcr_low & ~0xe;
+         break;
+-    case 0x1c: /* CRCR high */
++    case XHCI_OPER_CRCR_HI:
+         ret = xhci->crcr_high;
+         break;
+-    case 0x30: /* DCBAAP low */
++    case XHCI_OPER_DCBAAP_LO:
+         ret = xhci->dcbaap_low;
+         break;
+-    case 0x34: /* DCBAAP high */
++    case XHCI_OPER_DCBAAP_HI:
+         ret = xhci->dcbaap_high;
+         break;
+-    case 0x38: /* CONFIG */
++    case XHCI_OPER_CONFIG:
+         ret = xhci->config;
+         break;
+     default:
+@@ -XXX,XX +XXX,XX @@ static void xhci_oper_write(void *ptr, hwaddr reg,
+     trace_usb_xhci_oper_write(reg, val);
+     switch (reg) {
+-    case 0x00: /* USBCMD */
++    case XHCI_OPER_USBCMD:
+         if ((val & USBCMD_RS) && !(xhci->usbcmd & USBCMD_RS)) {
+             xhci_run(xhci);
+         } else if (!(val & USBCMD_RS) && (xhci->usbcmd & USBCMD_RS)) {
+@@ -XXX,XX +XXX,XX @@ static void xhci_oper_write(void *ptr, hwaddr reg,
+         xhci_intr_update(xhci, 0);
+         break;
+-    case 0x04: /* USBSTS */
++    case XHCI_OPER_USBSTS:
+         /* these bits are write-1-to-clear */
+         xhci->usbsts &= ~(val & (USBSTS_HSE|USBSTS_EINT|USBSTS_PCD|USBSTS_SRE));
+         xhci_intr_update(xhci, 0);
+         break;
+-    case 0x14: /* DNCTRL */
++    case XHCI_OPER_DNCTRL:
+         xhci->dnctrl = val & 0xffff;
+         break;
+-    case 0x18: /* CRCR low */
++    case XHCI_OPER_CRCR_LO:
+         xhci->crcr_low = (val & 0xffffffcf) | (xhci->crcr_low & CRCR_CRR);
+         break;
+-    case 0x1c: /* CRCR high */
++    case XHCI_OPER_CRCR_HI:
+         xhci->crcr_high = val;
+         if (xhci->crcr_low & (CRCR_CA|CRCR_CS) && (xhci->crcr_low & CRCR_CRR)) {
+             XHCIEvent event = {ER_COMMAND_COMPLETE, CC_COMMAND_RING_STOPPED};
+@@ -XXX,XX +XXX,XX @@ static void xhci_oper_write(void *ptr, hwaddr reg,
+         }
+         xhci->crcr_low &= ~(CRCR_CA | CRCR_CS);
+         break;
+-    case 0x30: /* DCBAAP low */
++    case XHCI_OPER_DCBAAP_LO:
+         xhci->dcbaap_low = val & 0xffffffc0;
+         break;
+-    case 0x34: /* DCBAAP high */
++    case XHCI_OPER_DCBAAP_HI:
+         xhci->dcbaap_high = val;
+         break;
+-    case 0x38: /* CONFIG */
++    case XHCI_OPER_CONFIG:
+         xhci->config = val & 0xff;
+         break;
+     default:
+@@ -XXX,XX +XXX,XX @@ static uint64_t xhci_runtime_read(void *ptr, hwaddr reg,
+     XHCIState *xhci = ptr;
+     uint32_t ret = 0;
+-    if (reg < 0x20) {
++    if (reg < XHCI_OPER_IR0) {
+         switch (reg) {
+-        case 0x00: /* MFINDEX */
++        case XHCI_OPER_MFINDEX:
+             ret = xhci_mfindex_get(xhci) & 0x3fff;
+             break;
+         default:
+@@ -XXX,XX +XXX,XX @@ static uint64_t xhci_runtime_read(void *ptr, hwaddr reg,
+             break;
+         }
+     } else {
+-        int v = (reg - 0x20) / 0x20;
++        int v = (reg - XHCI_OPER_IR0) / XHCI_OPER_IR_SZ;
+         XHCIInterrupter *intr = &xhci->intr[v];
+-        switch (reg & 0x1f) {
+-        case 0x00: /* IMAN */
++        switch (reg & (XHCI_OPER_IR_SZ - 1)) {
++        case XHCI_INTR_IMAN:
+             ret = intr->iman;
+             break;
+-        case 0x04: /* IMOD */
++        case XHCI_INTR_IMOD:
+             ret = intr->imod;
+             break;
+-        case 0x08: /* ERSTSZ */
++        case XHCI_INTR_ERSTSZ:
+             ret = intr->erstsz;
+             break;
+-        case 0x10: /* ERSTBA low */
++        case XHCI_INTR_ERSTBA_LO:
+             ret = intr->erstba_low;
+             break;
+-        case 0x14: /* ERSTBA high */
++        case XHCI_INTR_ERSTBA_HI:
+             ret = intr->erstba_high;
+             break;
+-        case 0x18: /* ERDP low */
++        case XHCI_INTR_ERDP_LO:
+             ret = intr->erdp_low;
+             break;
+-        case 0x1c: /* ERDP high */
++        case XHCI_INTR_ERDP_HI:
+             ret = intr->erdp_high;
+             break;
+         }
+@@ -XXX,XX +XXX,XX @@ static void xhci_runtime_write(void *ptr, hwaddr reg,
+     trace_usb_xhci_runtime_write(reg, val);
+-    if (reg < 0x20) {
++    if (reg < XHCI_OPER_IR0) {
+         trace_usb_xhci_unimplemented("runtime write", reg);
+         return;
+     }
+-    v = (reg - 0x20) / 0x20;
++    v = (reg - XHCI_OPER_IR0) / XHCI_OPER_IR_SZ;
+     intr = &xhci->intr[v];
+-    switch (reg & 0x1f) {
+-    case 0x00: /* IMAN */
++    switch (reg & (XHCI_OPER_IR_SZ - 1)) {
++    case XHCI_INTR_IMAN:
+         if (val & IMAN_IP) {
+             intr->iman &= ~IMAN_IP;
+         }
+@@ -XXX,XX +XXX,XX @@ static void xhci_runtime_write(void *ptr, hwaddr reg,
+         intr->iman |= val & IMAN_IE;
+         xhci_intr_update(xhci, v);
+         break;
+-    case 0x04: /* IMOD */
++    case XHCI_INTR_IMOD:
+         intr->imod = val;
+         break;
+-    case 0x08: /* ERSTSZ */
++    case XHCI_INTR_ERSTSZ:
+         intr->erstsz = val & 0xffff;
+         break;
+-    case 0x10: /* ERSTBA low */
++    case XHCI_INTR_ERSTBA_LO:
+         if (xhci->nec_quirks) {
+             /* NEC driver bug: it doesn't align this to 64 bytes */
+             intr->erstba_low = val & 0xfffffff0;
+@@ -XXX,XX +XXX,XX @@ static void xhci_runtime_write(void *ptr, hwaddr reg,
+             intr->erstba_low = val & 0xffffffc0;
+         }
+         break;
+-    case 0x14: /* ERSTBA high */
++    case XHCI_INTR_ERSTBA_HI:
+         intr->erstba_high = val;
+         xhci_er_reset(xhci, v);
+         break;
+-    case 0x18: /* ERDP low */
++    case XHCI_INTR_ERDP_LO:
+         if (val & ERDP_EHB) {
+             intr->erdp_low &= ~ERDP_EHB;
+         }
+@@ -XXX,XX +XXX,XX @@ static void xhci_runtime_write(void *ptr, hwaddr reg,
+             }
+         }
+         break;
+-    case 0x1c: /* ERDP high */
++    case XHCI_INTR_ERDP_HI:
+         intr->erdp_high = val;
+         break;
+     default:
+--
+.45.2

-[RFC PATCH 4/5] qtest/xhci: Add controller and device setup and ring tests
+[PATCH 6/8] qtest/xhci: Add controller and device setup and ring tests
-Add tests which init the host controller registers to the point
+Add tests which init the host controller registers to the point where
-where command and event rings, irqs are operational. Enumerate
+command and event rings, irqs are operational. Enumerate ports and set
-ports and set up an attached device context that enables device
+up an attached device context that enables device transfer ring to be
-transfer ring to be set up and tested.
+set up and tested.
-This test does a bunch of things at once and is yet well
+This test does a bunch of things at once and is not yet well librified,
-librified, but it allows testing basic mechanisms and gives a
+but it allows testing basic mechanisms and gives a starting point for
-starting point for further work.
+further work.
 Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
 ---
- tests/qtest/usb-hcd-xhci-test.h | 232 +++++++++++++++
+ hw/usb/hcd-xhci.h               |   7 +
- tests/qtest/usb-hcd-xhci-test.c | 506 +++++++++++++++++++++++++++++++-
+ hw/usb/hcd-xhci.c               |   7 -
-files changed, 732 insertions(+), 6 deletions(-)
+ tests/qtest/usb-hcd-xhci-test.c | 498 +++++++++++++++++++++++++++++++-
- create mode 100644 tests/qtest/usb-hcd-xhci-test.h
+files changed, 499 insertions(+), 13 deletions(-)
-diff --git a/tests/qtest/usb-hcd-xhci-test.h b/tests/qtest/usb-hcd-xhci-test.h
+diff --git a/hw/usb/hcd-xhci.h b/hw/usb/hcd-xhci.h
-new file mode 100644
+index XXXXXXX..XXXXXXX 100644
-index XXXXXXX..XXXXXXX
+--- a/hw/usb/hcd-xhci.h
---- /dev/null
++++ b/hw/usb/hcd-xhci.h
-+++ b/tests/qtest/usb-hcd-xhci-test.h
+@@ -XXX,XX +XXX,XX @@ typedef struct XHCIRing {
-@@ -XXX,XX +XXX,XX @@
+     bool ccs;
-+#ifndef TESTS_USB_HCD_XHCI_H
+ } XHCIRing;
-+#define TESTS_USB_HCD_XHCI_H
-+
++typedef struct XHCIEvRingSeg {
-+typedef enum TRBType {
++    uint32_t addr_low;
-+    TRB_RESERVED = 0,
++    uint32_t addr_high;
-+    TR_NORMAL,
++    uint32_t size;
-+    TR_SETUP,
++    uint32_t rsvd;
-+    TR_DATA,
++} XHCIEvRingSeg;
-+    TR_STATUS,
++
-+    TR_ISOCH,
+ typedef struct XHCIPort {
-+    TR_LINK,
+     XHCIState *xhci;
-+    TR_EVDATA,
+     uint32_t portsc;
-+    TR_NOOP,
+diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
-+    CR_ENABLE_SLOT,
+index XXXXXXX..XXXXXXX 100644
-+    CR_DISABLE_SLOT,
+--- a/hw/usb/hcd-xhci.c
-+    CR_ADDRESS_DEVICE,
++++ b/hw/usb/hcd-xhci.c
-+    CR_CONFIGURE_ENDPOINT,
+@@ -XXX,XX +XXX,XX @@ struct XHCIEPContext {
-+    CR_EVALUATE_CONTEXT,
+     QEMUTimer *kick_timer;
-+    CR_RESET_ENDPOINT,
+ };
-+    CR_STOP_ENDPOINT,
-+    CR_SET_TR_DEQUEUE,
+-typedef struct XHCIEvRingSeg {
-+    CR_RESET_DEVICE,
+-    uint32_t addr_low;
-+    CR_FORCE_EVENT,
+-    uint32_t addr_high;
-+    CR_NEGOTIATE_BW,
+-    uint32_t size;
-+    CR_SET_LATENCY_TOLERANCE,
+-    uint32_t rsvd;
-+    CR_GET_PORT_BANDWIDTH,
+-} XHCIEvRingSeg;
-+    CR_FORCE_HEADER,
+-
-+    CR_NOOP,
+ static void xhci_kick_ep(XHCIState *xhci, unsigned int slotid,
-+    ER_TRANSFER = 32,
+                          unsigned int epid, unsigned int streamid);
-+    ER_COMMAND_COMPLETE,
+ static void xhci_kick_epctx(XHCIEPContext *epctx, unsigned int streamid);
 +    ER_PORT_STATUS_CHANGE,
 +    ER_BANDWIDTH_REQUEST,
 +    ER_DOORBELL,
 +    ER_HOST_CONTROLLER,
 +    ER_DEVICE_NOTIFICATION,
 +    ER_MFINDEX_WRAP,
 +    /* vendor specific bits */
 +    CR_VENDOR_NEC_FIRMWARE_REVISION  = 49,
 +    CR_VENDOR_NEC_CHALLENGE_RESPONSE = 50,
 +} TRBType;
 +
 +typedef enum TRBCCode {
 +    CC_INVALID = 0,
 +    CC_SUCCESS,
 +    CC_DATA_BUFFER_ERROR,
 +    CC_BABBLE_DETECTED,
 +    CC_USB_TRANSACTION_ERROR,
 +    CC_TRB_ERROR,
 +    CC_STALL_ERROR,
 +    CC_RESOURCE_ERROR,
 +    CC_BANDWIDTH_ERROR,
 +    CC_NO_SLOTS_ERROR,
 +    CC_INVALID_STREAM_TYPE_ERROR,
 +    CC_SLOT_NOT_ENABLED_ERROR,
 +    CC_EP_NOT_ENABLED_ERROR,
 +    CC_SHORT_PACKET,
 +    CC_RING_UNDERRUN,
 +    CC_RING_OVERRUN,
 +    CC_VF_ER_FULL,
 +    CC_PARAMETER_ERROR,
 +    CC_BANDWIDTH_OVERRUN,
 +    CC_CONTEXT_STATE_ERROR,
 +    CC_NO_PING_RESPONSE_ERROR,
 +    CC_EVENT_RING_FULL_ERROR,
 +    CC_INCOMPATIBLE_DEVICE_ERROR,
 +    CC_MISSED_SERVICE_ERROR,
 +    CC_COMMAND_RING_STOPPED,
 +    CC_COMMAND_ABORTED,
 +    CC_STOPPED,
 +    CC_STOPPED_LENGTH_INVALID,
 +    CC_MAX_EXIT_LATENCY_TOO_LARGE_ERROR = 29,
 +    CC_ISOCH_BUFFER_OVERRUN = 31,
 +    CC_EVENT_LOST_ERROR,
 +    CC_UNDEFINED_ERROR,
 +    CC_INVALID_STREAM_ID_ERROR,
 +    CC_SECONDARY_BANDWIDTH_ERROR,
 +    CC_SPLIT_TRANSACTION_ERROR
 +} TRBCCode;
 +
 +/* bit definitions */
 +#define USBCMD_RS       (1<<0)
 +#define USBCMD_HCRST    (1<<1)
 +#define USBCMD_INTE     (1<<2)
 +#define USBCMD_HSEE     (1<<3)
 +#define USBCMD_LHCRST   (1<<7)
 +#define USBCMD_CSS      (1<<8)
 +#define USBCMD_CRS      (1<<9)
 +#define USBCMD_EWE      (1<<10)
 +#define USBCMD_EU3S     (1<<11)
 +
 +#define USBSTS_HCH      (1<<0)
 +#define USBSTS_HSE      (1<<2)
 +#define USBSTS_EINT     (1<<3)
 +#define USBSTS_PCD      (1<<4)
 +#define USBSTS_SSS      (1<<8)
 +#define USBSTS_RSS      (1<<9)
 +#define USBSTS_SRE      (1<<10)
 +#define USBSTS_CNR      (1<<11)
 +#define USBSTS_HCE      (1<<12)
 +
 +
 +#define PORTSC_CCS          (1<<0)
 +#define PORTSC_PED          (1<<1)
 +#define PORTSC_OCA          (1<<3)
 +#define PORTSC_PR           (1<<4)
 +#define PORTSC_PLS_SHIFT        5
 +#define PORTSC_PLS_MASK     0xf
 +#define PORTSC_PP           (1<<9)
 +#define PORTSC_SPEED_SHIFT      10
 +#define PORTSC_SPEED_MASK   0xf
 +#define PORTSC_SPEED_FULL   (1<<10)
 +#define PORTSC_SPEED_LOW    (2<<10)
 +#define PORTSC_SPEED_HIGH   (3<<10)
 +#define PORTSC_SPEED_SUPER  (4<<10)
 +#define PORTSC_PIC_SHIFT        14
 +#define PORTSC_PIC_MASK     0x3
 +#define PORTSC_LWS          (1<<16)
 +#define PORTSC_CSC          (1<<17)
 +#define PORTSC_PEC          (1<<18)
 +#define PORTSC_WRC          (1<<19)
 +#define PORTSC_OCC          (1<<20)
 +#define PORTSC_PRC          (1<<21)
 +#define PORTSC_PLC          (1<<22)
 +#define PORTSC_CEC          (1<<23)
 +#define PORTSC_CAS          (1<<24)
 +#define PORTSC_WCE          (1<<25)
 +#define PORTSC_WDE          (1<<26)
 +#define PORTSC_WOE          (1<<27)
 +#define PORTSC_DR           (1<<30)
 +#define PORTSC_WPR          (1<<31)
 +
 +#define CRCR_RCS        (1<<0)
 +#define CRCR_CS         (1<<1)
 +#define CRCR_CA         (1<<2)
 +#define CRCR_CRR        (1<<3)
 +
 +#define IMAN_IP         (1<<0)
 +#define IMAN_IE         (1<<1)
 +
 +#define ERDP_EHB        (1<<3)
 +
 +#define TRB_SIZE 16
 +
 +enum {
 +    PLS_U0              =  0,
 +    PLS_U1              =  1,
 +    PLS_U2              =  2,
 +    PLS_U3              =  3,
 +    PLS_DISABLED        =  4,
 +    PLS_RX_DETECT       =  5,
 +    PLS_INACTIVE        =  6,
 +    PLS_POLLING         =  7,
 +    PLS_RECOVERY        =  8,
 +    PLS_HOT_RESET       =  9,
 +    PLS_COMPILANCE_MODE = 10,
 +    PLS_TEST_MODE       = 11,
 +    PLS_RESUME          = 15,
 +};
 +
 +#define CR_LINK TR_LINK
 +
 +#define TRB_C               (1<<0)
 +#define TRB_TYPE_SHIFT          10
 +#define TRB_TYPE_MASK       0x3f
 +#define TRB_TYPE(t)         (((t).control >> TRB_TYPE_SHIFT) & TRB_TYPE_MASK)
 +
 +#define TRB_EV_ED           (1<<2)
 +
 +#define TRB_TR_ENT          (1<<1)
 +#define TRB_TR_ISP          (1<<2)
 +#define TRB_TR_NS           (1<<3)
 +#define TRB_TR_CH           (1<<4)
 +#define TRB_TR_IOC          (1<<5)
 +#define TRB_TR_IDT          (1<<6)
 +#define TRB_TR_TBC_SHIFT        7
 +#define TRB_TR_TBC_MASK     0x3
 +#define TRB_TR_BEI          (1<<9)
 +#define TRB_TR_TLBPC_SHIFT      16
 +#define TRB_TR_TLBPC_MASK   0xf
 +#define TRB_TR_FRAMEID_SHIFT    20
 +#define TRB_TR_FRAMEID_MASK 0x7ff
 +#define TRB_TR_SIA          (1<<31)
 +
 +#define TRB_TR_DIR          (1<<16)
 +
 +#define TRB_CR_SLOTID_SHIFT     24
 +#define TRB_CR_SLOTID_MASK  0xff
 +#define TRB_CR_EPID_SHIFT       16
 +#define TRB_CR_EPID_MASK    0x1f
 +
 +#define TRB_CR_BSR          (1<<9)
 +#define TRB_CR_DC           (1<<9)
 +
 +#define TRB_LK_TC           (1<<1)
 +
 +#define TRB_INTR_SHIFT          22
 +#define TRB_INTR_MASK       0x3ff
 +#define TRB_INTR(t)         (((t).status >> TRB_INTR_SHIFT) & TRB_INTR_MASK)
 +
 +#define EP_TYPE_MASK        0x7
 +#define EP_TYPE_SHIFT           3
 +
 +#define EP_STATE_MASK       0x7
 +#define EP_DISABLED         (0<<0)
 +#define EP_RUNNING          (1<<0)
 +#define EP_HALTED           (2<<0)
 +#define EP_STOPPED          (3<<0)
 +#define EP_ERROR            (4<<0)
 +
 +#define SLOT_STATE_MASK     0x1f
 +#define SLOT_STATE_SHIFT        27
 +#define SLOT_STATE(s)       (((s)>>SLOT_STATE_SHIFT)&SLOT_STATE_MASK)
 +#define SLOT_ENABLED        0
 +#define SLOT_DEFAULT        1
 +#define SLOT_ADDRESSED      2
 +#define SLOT_CONFIGURED     3
 +
 +#define SLOT_CONTEXT_ENTRIES_MASK 0x1f
 +#define SLOT_CONTEXT_ENTRIES_SHIFT 27
 +
 +typedef enum EPType {
 +    ET_INVALID = 0,
 +    ET_ISO_OUT,
 +    ET_BULK_OUT,
 +    ET_INTR_OUT,
 +    ET_CONTROL,
 +    ET_ISO_IN,
 +    ET_BULK_IN,
 +    ET_INTR_IN,
 +} EPType;
 +
 +#endif /* TESTS_USB_HCD_XHCI_H */
 diff --git a/tests/qtest/usb-hcd-xhci-test.c b/tests/qtest/usb-hcd-xhci-test.c
 index XXXXXXX..XXXXXXX 100644
 --- a/tests/qtest/usb-hcd-xhci-test.c
 +++ b/tests/qtest/usb-hcd-xhci-test.c
 @@ -XXX,XX +XXX,XX @@
 ...
 +#include "libqos/libqos-pc.h"
  #include "libqtest-single.h"
  #include "libqos/usb.h"
 +#include "hw/pci/pci_ids.h"
 +#include "hw/pci/pci_regs.h"
-+#include "usb-hcd-xhci-test.h"
++#include "hw/usb/hcd-xhci.h"
 +
 +/*** Test Setup & Teardown ***/
-+typedef struct XHCIEvRingSeg {
-+    uint32_t addr_low;
-+    uint32_t addr_high;
-+    uint32_t size;
-+    uint32_t rsvd;
-+} XHCIEvRingSeg;
-+
-+typedef struct XHCITRB {
-+    uint64_t parameter;
-+    uint32_t status;
-+    uint32_t control;
-+} XHCITRB;
-+
 +typedef struct XHCIQSlotState {
 +    /* In-memory arrays */
 +    uint64_t device_context;
 +    uint64_t transfer_ring;
 +
 ...
 + */
 +static void xhci_shutdown(XHCIQState *xhci)
 +{
 +    QOSState *qs = xhci->parent;
 +
-+//    xhci_clean_mem(xhci);
 +    free_xhci_device(xhci->dev);
 +    g_free(xhci);
 +    qtest_shutdown(qs);
 +}
 +
 ...
 +}
 +
 +static void xhci_rt_writel(XHCIQState *s, uint64_t addr, uint32_t value)
 +{
 +    qpci_io_writel(s->dev, s->bar, s->rtoff + addr, value);
-+}
+ }
-+
 +static void xhci_db_writel(XHCIQState *s, uint32_t db, uint32_t value)
 +{
 +    qpci_io_writel(s->dev, s->bar, s->dboff + db * 4, value);
 +}
 +
 ...
 +    /* Update ERDP to processed TRB addr and EHB bit, which clears EHB */
 +    er_addr = s->event_ring + s->er_trb_idx * sizeof(*trb);
 +    xhci_rt_writel(s, 0x38, (er_addr & 0xffffffff) | ERDP_EHB);
 +}
 +
-+static void set_link_trb(XHCIQState *s, uint64_t ring, uint32_t c, uint32_t entries)
++static void set_link_trb(XHCIQState *s, uint64_t ring, uint32_t c,
 +                         uint32_t entries)
 +{
 +    XHCITRB trb;
 +
 +    g_assert(entries > 1);
 +
 +    memset(&trb, 0, sizeof(trb));
 +    trb.parameter = cpu_to_le64(ring);
 +    trb.control = cpu_to_le32(c | /* C */
 +                              (TR_LINK << TRB_TYPE_SHIFT) |
 +                              TRB_LK_TC);
-+    qtest_memwrite(s->parent->qts, ring + sizeof(trb) * (entries - 1), &trb, sizeof(trb));
++    qtest_memwrite(s->parent->qts, ring + sizeof(trb) * (entries - 1),
- }
++                   &trb, sizeof(trb));
++}
 +
 +static void submit_cr_trb(XHCIQState *s, XHCITRB *trb)
 +{
 +    XHCITRB t;
 +    uint64_t cr_addr = s->command_ring + s->cr_trb_idx * sizeof(*trb);
 +
 ...
 +        s->cr_trb_c ^= 1;
 +    }
 +    xhci_db_writel(s, 0, 0); /* doorbell 0 */
 +}
 +
++/*
++ * This test brings up an endpoint and runs some noops through its command
++ * ring and gets responses back on the event ring.
++ *
++ * This could be librified in future (like AHCI0 to have a way to bring up
++ * an endpoint to test device protocols.
++ */
 +static void pci_xhci_stress_rings(void)
 +{
 +    XHCIQState *s;
 +    uint32_t value;
 +    uint64_t input_context;
 +    XHCIEvRingSeg ev_seg;
 +    XHCITRB trb;
 +    uint32_t hcsparams1;
 +    uint32_t slotid;
-+    void *mem;
++    g_autofree void *mem = g_malloc0(0x1000); /* buffer for writing to guest */
 +    int i;
-+
-+    mem = g_malloc(0x1000);
-+    memset(mem, 0, 0x1000);
 +
 +    s = xhci_boot("-M q35 "
 +            "-device nec-usb-xhci,id=xhci,bus=pcie.0,addr=1d.0 "
 +            "-device usb-storage,bus=xhci.0,drive=drive0 "
 +            "-drive id=drive0,if=none,file=null-co://,"
 +                "file.read-zeroes=on,format=raw "
 +            );
-+//            "-d trace:*xhci*,trace:*usb*,trace:*msi*");
 +
 +    hcsparams1 = xhci_cap_readl(s, 0x4); /* HCSPARAMS1 */
 +    s->maxports = (hcsparams1 >> 24) & 0xff;
 +    s->maxintrs = (hcsparams1 >> 8) & 0x3ff;
 +    s->maxslots = hcsparams1 & 0xff;
 ...
 +
 +        /* Only first port should be attached and enabled */
 +        if (i == 0) {
 +            g_assert(value & PORTSC_CCS);
 +            g_assert(value & PORTSC_PED);
-+        /* Port Speed must be identified */
++            /* Port Speed must be identified (non-zero) */
-+        g_assert(((value >> PORTSC_SPEED_SHIFT) & PORTSC_SPEED_MASK) != 0);
++            g_assert(((value >> PORTSC_SPEED_SHIFT) & PORTSC_SPEED_MASK) != 0);
 +        } else {
 +            g_assert(!(value & PORTSC_CCS));
 +            g_assert(!(value & PORTSC_PED));
 +            g_assert(((value >> PORTSC_PLS_SHIFT) & PORTSC_PLS_MASK) == 5);
 +        }
 ...
 +
 +    /* Set endpoint 0 context */
 +    ((uint32_t *)mem)[16] = 0;
 +    ((uint32_t *)mem)[17] = cpu_to_le32((ET_CONTROL << EP_TYPE_SHIFT) |
 +                                        (0x200 << 16)); /* max packet sz XXX? */
-+    ((uint32_t *)mem)[18] = cpu_to_le32((s->slots[slotid].transfer_ring & 0xffffffff) | 1); /* DCS=1 */
++    ((uint32_t *)mem)[18] = cpu_to_le32((s->slots[slotid].transfer_ring &
 +                                         0xffffffff) | 1); /* DCS=1 */
 +    ((uint32_t *)mem)[19] = cpu_to_le32(s->slots[slotid].transfer_ring >> 32);
 +    ((uint32_t *)mem)[20] = cpu_to_le32(0x200); /* Average TRB length */
 +    qtest_memwrite(s->parent->qts, input_context, mem, 0x420);
 +
 +    s->slots[slotid].device_context = xhci_guest_zalloc(s, 0x400);
 +
 +    ((uint64_t *)mem)[0] = cpu_to_le64(s->slots[slotid].device_context);
-+    qtest_memwrite(s->parent->qts, s->dc_base_array + 8*slotid, mem, 8);
++    qtest_memwrite(s->parent->qts, s->dc_base_array + 8 * slotid, mem, 8);
 +
 +    /* Issue a command ring address device */
 +    memset(&trb, 0, sizeof(trb));
 +    trb.parameter = input_context;
 +    trb.control |= CR_ADDRESS_DEVICE << TRB_TYPE_SHIFT;
 +    trb.control |= slotid << TRB_CR_SLOTID_SHIFT;
 +    submit_cr_trb(s, &trb);
 +    wait_event_trb(s, &trb);
 +
-+    /* XXX: Check EP state is running? */
++    /* XXX: Could check EP state is running */
 +
 +    /* Shut it down */
 +    qpci_msix_disable(s->dev);
 +
 +    guest_free(&s->parent->alloc, s->slots[slotid].device_context);
 ...

-[RFC PATCH 5/5] hw/usb: Support XHCI TR NOOP commands
+[PATCH 7/8] hw/usb/xhci: Support TR NOOP commands
-Implement TR NOOP commands by setting up then immediately completing
+Implement XHCI TR NOOP commands by setting up then immediately
-the packet. Add a TR NOOP test to the xhci qtest.
+completing the packet.
-The IBM AIX XHCI driver uses NOOP commands to check driver and
+The IBM AIX XHCI HCD driver uses NOOP commands to check driver and
-hardware health.
+hardware health, which works after this change.
 Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
 ---
- hw/usb/hcd-xhci.c               | 28 ++++++++++++++++++++++++++-
+ hw/usb/hcd-xhci.c | 28 +++++++++++++++++++++++++++-
- tests/qtest/usb-hcd-xhci-test.c | 34 +++++++++++++++++++++++++++++++++
+file changed, 27 insertions(+), 1 deletion(-)
 files changed, 61 insertions(+), 1 deletion(-)
 diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
 index XXXXXXX..XXXXXXX 100644
 --- a/hw/usb/hcd-xhci.c
 +++ b/hw/usb/hcd-xhci.c
 ...
 +            xhci_noop_transfer(xhci, xfer);
 +        } else if (epctx->epid == 1) {
              xhci_fire_ctl_transfer(xhci, xfer);
          } else {
              xhci_fire_transfer(xhci, xfer, epctx);
-diff --git a/tests/qtest/usb-hcd-xhci-test.c b/tests/qtest/usb-hcd-xhci-test.c
-index XXXXXXX..XXXXXXX 100644
---- a/tests/qtest/usb-hcd-xhci-test.c
-+++ b/tests/qtest/usb-hcd-xhci-test.c
-@@ -XXX,XX +XXX,XX @@ static void submit_cr_trb(XHCIQState *s, XHCITRB *trb)
-     xhci_db_writel(s, 0, 0); /* doorbell 0 */
- }
-+static void submit_tr_trb(XHCIQState *s, int slot, XHCITRB *trb)
-+{
-+    XHCITRB t;
-+    uint64_t tr_addr = s->slots[slot].transfer_ring + s->slots[slot].tr_trb_idx * sizeof(*trb);
-+
-+    trb->control |= s->slots[slot].tr_trb_c; /* C */
-+
-+    t.parameter = cpu_to_le64(trb->parameter);
-+    t.status = cpu_to_le32(trb->status);
-+    t.control = cpu_to_le32(trb->control);
-+
-+    qtest_memwrite(s->parent->qts, tr_addr, &t, sizeof(t));
-+    s->slots[slot].tr_trb_idx++;
-+    /* Last entry contains the link, so wrap back */
-+    if (s->slots[slot].tr_trb_idx == s->slots[slot].tr_trb_entries - 1) {
-+        set_link_trb(s, s->slots[slot].transfer_ring,
-+                        s->slots[slot].tr_trb_c,
-+                        s->slots[slot].tr_trb_entries);
-+        s->slots[slot].tr_trb_idx = 0;
-+        s->slots[slot].tr_trb_c ^= 1;
-+    }
-+    xhci_db_writel(s, slot, 1); /* doorbell slot, EP0 target */
-+}
-+
- static void pci_xhci_stress_rings(void)
- {
-     XHCIQState *s;
-@@ -XXX,XX +XXX,XX @@ static void pci_xhci_stress_rings(void)
-     /* XXX: Check EP state is running? */
-+    /* Wrap the transfer ring a few times */
-+    for (i = 0; i < 100; i++) {
-+        /* Issue a transfer ring slot 0 noop */
-+        memset(&trb, 0, sizeof(trb));
-+        trb.control |= TR_NOOP << TRB_TYPE_SHIFT;
-+        trb.control |= TRB_TR_IOC;
-+        submit_tr_trb(s, slotid, &trb);
-+        wait_event_trb(s, &trb);
-+    }
-+
-     /* Shut it down */
-     qpci_msix_disable(s->dev);
 --
 .45.2

-New patch
+[PATCH 8/8] qtest/xhci: add a test for TR NOOP commands
+Run some TR NOOP commands through the transfer ring.
+Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
+---
+ tests/qtest/usb-hcd-xhci-test.c | 41 +++++++++++++++++++++++++++++++--
+file changed, 39 insertions(+), 2 deletions(-)
+diff --git a/tests/qtest/usb-hcd-xhci-test.c b/tests/qtest/usb-hcd-xhci-test.c
+index XXXXXXX..XXXXXXX 100644
+--- a/tests/qtest/usb-hcd-xhci-test.c
++++ b/tests/qtest/usb-hcd-xhci-test.c
+@@ -XXX,XX +XXX,XX @@ static void set_link_trb(XHCIQState *s, uint64_t ring, uint32_t c,
+ static void submit_cr_trb(XHCIQState *s, XHCITRB *trb)
+ {
+     XHCITRB t;
+-    uint64_t cr_addr = s->command_ring + s->cr_trb_idx * sizeof(*trb);
++    uint64_t cr_addr = s->command_ring +
++                       s->cr_trb_idx * sizeof(*trb);
+     trb->control |= s->cr_trb_c; /* C */
+@@ -XXX,XX +XXX,XX @@ static void submit_cr_trb(XHCIQState *s, XHCITRB *trb)
+     xhci_db_writel(s, 0, 0); /* doorbell 0 */
+ }
++static void submit_tr_trb(XHCIQState *s, int slot, XHCITRB *trb)
++{
++    XHCITRB t;
++    uint64_t tr_addr = s->slots[slot].transfer_ring +
++                       s->slots[slot].tr_trb_idx * sizeof(*trb);
++
++    trb->control |= s->slots[slot].tr_trb_c; /* C */
++
++    t.parameter = cpu_to_le64(trb->parameter);
++    t.status = cpu_to_le32(trb->status);
++    t.control = cpu_to_le32(trb->control);
++
++    qtest_memwrite(s->parent->qts, tr_addr, &t, sizeof(t));
++    s->slots[slot].tr_trb_idx++;
++    /* Last entry contains the link, so wrap back */
++    if (s->slots[slot].tr_trb_idx == s->slots[slot].tr_trb_entries - 1) {
++        set_link_trb(s, s->slots[slot].transfer_ring,
++                        s->slots[slot].tr_trb_c,
++                        s->slots[slot].tr_trb_entries);
++        s->slots[slot].tr_trb_idx = 0;
++        s->slots[slot].tr_trb_c ^= 1;
++    }
++    xhci_db_writel(s, slot, 1); /* doorbell slot, EP0 target */
++}
++
+ /*
+  * This test brings up an endpoint and runs some noops through its command
+- * ring and gets responses back on the event ring.
++ * ring and gets responses back on the event ring, then brings up a device
++ * context and runs some noops through its transfer ring.
+  *
+  * This could be librified in future (like AHCI0 to have a way to bring up
+  * an endpoint to test device protocols.
+@@ -XXX,XX +XXX,XX @@ static void pci_xhci_stress_rings(void)
+     /* XXX: Could check EP state is running */
++    /* Wrap the transfer ring a few times */
++    for (i = 0; i < 100; i++) {
++        /* Issue a transfer ring slot 0 noop */
++        memset(&trb, 0, sizeof(trb));
++        trb.control |= TR_NOOP << TRB_TYPE_SHIFT;
++        trb.control |= TRB_TR_IOC;
++        submit_tr_trb(s, slotid, &trb);
++        wait_event_trb(s, &trb);
++    }
++
+     /* Shut it down */
+     qpci_msix_disable(s->dev);
+--
+.45.2

Hi,

This RFC is wide-ranging for such a simple addition of XHCI TR NOOP
command, because adding qtests for it caught up some PCI/MSIX issues.

Firstly, the XHCI host shares a BAR between device registers and MSIX
space, which is not handled by qtest/pci. Keeping track of BAR mappings
in qtest and allowing MSIX to use an existing mapping seems to work.

For XHCI qtest, I added a MSIX check to ensure interrupts are working
correctly. As part of that I need to dequeue masked interrupts which
seems to require PBA writes to clear them, but I could be way off
base here. Adding that does cause e1000e qtest to break, because it
doesn't like clear pending via PBA.

The XHCI model change gets AIX's XHCI driver working, so that's another
data point beyond the simple qtest for it added here. Unfortunately it
is not so easy to independently test.

Any guidance or input would be welcome.

Thanks,
Nick

Nicholas Piggin (5):
  qtest/pci: Enforce balanced iomap/unmap
  qtest/libqos/pci: Fix msix_enable sharing bar0
  pci/msix: Implement PBA writes
  qtest/xhci: Add controller and device setup and ring tests
  hw/usb: Support XHCI TR NOOP commands

-- 
2.45.2

Add assertions to ensure a BAR is not mapped twice, and only
previously mapped BARs are unmapped. This can help catch some
bugs.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 tests/qtest/libqos/ahci.h       |  1 +
 tests/qtest/libqos/pci.h        |  2 ++
 tests/qtest/libqos/virtio-pci.h |  1 +
 tests/qtest/ahci-test.c         |  2 ++
 tests/qtest/libqos/ahci.c       |  6 ++++++
 tests/qtest/libqos/pci.c        | 32 +++++++++++++++++++++++++++++++-
 tests/qtest/libqos/virtio-pci.c |  6 +++++-
 7 files changed, 48 insertions(+), 2 deletions(-)

diff --git a/tests/qtest/libqos/ahci.h b/tests/qtest/libqos/ahci.h
index XXXXXXX..XXXXXXX 100644
--- a/tests/qtest/libqos/ahci.h
+++ b/tests/qtest/libqos/ahci.h
@@ -XXX,XX +XXX,XX @@ QPCIDevice *get_ahci_device(QTestState *qts, uint32_t *fingerprint);
 void free_ahci_device(QPCIDevice *dev);
 void ahci_pci_enable(AHCIQState *ahci);
 void start_ahci_device(AHCIQState *ahci);
+void stop_ahci_device(AHCIQState *ahci);
 void ahci_hba_enable(AHCIQState *ahci);
 
 /* Port Management */
diff --git a/tests/qtest/libqos/pci.h b/tests/qtest/libqos/pci.h
index XXXXXXX..XXXXXXX 100644
--- a/tests/qtest/libqos/pci.h
+++ b/tests/qtest/libqos/pci.h
@@ -XXX,XX +XXX,XX @@ struct QPCIDevice
 {
     QPCIBus *bus;
     int devfn;
+    bool bars_mapped[6];
+    QPCIBar bars[6];
     bool msix_enabled;
     QPCIBar msix_table_bar, msix_pba_bar;
     uint64_t msix_table_off, msix_pba_off;
diff --git a/tests/qtest/libqos/virtio-pci.h b/tests/qtest/libqos/virtio-pci.h
index XXXXXXX..XXXXXXX 100644
--- a/tests/qtest/libqos/virtio-pci.h
+++ b/tests/qtest/libqos/virtio-pci.h
@@ -XXX,XX +XXX,XX @@ typedef struct QVirtioPCIDevice {
     uint64_t config_msix_addr;
     uint32_t config_msix_data;
 
+    bool enabled;
     int bar_idx;
 
     /* VIRTIO 1.0 */
diff --git a/tests/qtest/ahci-test.c b/tests/qtest/ahci-test.c
index XXXXXXX..XXXXXXX 100644
--- a/tests/qtest/ahci-test.c
+++ b/tests/qtest/ahci-test.c
@@ -XXX,XX +XXX,XX @@ static void test_reset_pending_callback(void)
     /* Wait for throttled write to finish. */
     sleep(1);
 
+    stop_ahci_device(ahci);
+
     /* Start again. */
     ahci_clean_mem(ahci);
     ahci_pci_enable(ahci);
diff --git a/tests/qtest/libqos/ahci.c b/tests/qtest/libqos/ahci.c
index XXXXXXX..XXXXXXX 100644
--- a/tests/qtest/libqos/ahci.c
+++ b/tests/qtest/libqos/ahci.c
@@ -XXX,XX +XXX,XX @@ void start_ahci_device(AHCIQState *ahci)
     qpci_device_enable(ahci->dev);
 }
 
+void stop_ahci_device(AHCIQState *ahci)
+{
+    /* Map AHCI's ABAR (BAR5) */
+    qpci_iounmap(ahci->dev, ahci->hba_bar);
+}
+
 /**
  * Test and initialize the AHCI's HBA memory areas.
  * Initialize and start any ports with devices attached.
diff --git a/tests/qtest/libqos/pci.c b/tests/qtest/libqos/pci.c
index XXXXXXX..XXXXXXX 100644
--- a/tests/qtest/libqos/pci.c
+++ b/tests/qtest/libqos/pci.c
@@ -XXX,XX +XXX,XX @@ QPCIDevice *qpci_device_find(QPCIBus *bus, int devfn)
 void qpci_device_init(QPCIDevice *dev, QPCIBus *bus, QPCIAddress *addr)
 {
     uint16_t vendor_id, device_id;
+    int i;
 
     qpci_device_set(dev, bus, addr->devfn);
     vendor_id = qpci_config_readw(dev, PCI_VENDOR_ID);
     device_id = qpci_config_readw(dev, PCI_DEVICE_ID);
     g_assert(!addr->vendor_id || vendor_id == addr->vendor_id);
     g_assert(!addr->device_id || device_id == addr->device_id);
+
+    for (i = 0; i < 6; i++) {
+        g_assert(!dev->bars_mapped[i]);
+    }
 }
 
 static uint8_t qpci_find_resource_reserve_capability(QPCIDevice *dev)
@@ -XXX,XX +XXX,XX @@ QPCIBar qpci_iomap(QPCIDevice *dev, int barno, uint64_t *sizeptr)
     uint64_t loc;
 
     g_assert(barno >= 0 && barno <= 5);
+    g_assert(!dev->bars_mapped[barno]);
+
     bar_reg = bar_reg_map[barno];
 
     qpci_config_writel(dev, bar_reg, 0xFFFFFFFF);
@@ -XXX,XX +XXX,XX @@ QPCIBar qpci_iomap(QPCIDevice *dev, int barno, uint64_t *sizeptr)
     }
 
     bar.addr = loc;
+
+    dev->bars_mapped[barno] = true;
+    dev->bars[barno] = bar;
+
     return bar;
 }
 
 void qpci_iounmap(QPCIDevice *dev, QPCIBar bar)
 {
-    /* FIXME */
+    static const int bar_reg_map[] = {
+        PCI_BASE_ADDRESS_0, PCI_BASE_ADDRESS_1, PCI_BASE_ADDRESS_2,
+        PCI_BASE_ADDRESS_3, PCI_BASE_ADDRESS_4, PCI_BASE_ADDRESS_5,
+    };
+    int bar_reg;
+    int i;
+
+    for (i = 0; i < 6; i++) {
+        if (!dev->bars_mapped[i]) {
+            continue;
+        }
+        if (dev->bars[i].addr == bar.addr) {
+            dev->bars_mapped[i] = false;
+            bar_reg = bar_reg_map[i];
+            qpci_config_writel(dev, bar_reg, 0xFFFFFFFF);
+            /* FIXME: the address space is leaked */
+            return;
+        }
+    }
+    g_assert_not_reached();
 }
 
 QPCIBar qpci_legacy_iomap(QPCIDevice *dev, uint16_t addr)
diff --git a/tests/qtest/libqos/virtio-pci.c b/tests/qtest/libqos/virtio-pci.c
index XXXXXXX..XXXXXXX 100644
--- a/tests/qtest/libqos/virtio-pci.c
+++ b/tests/qtest/libqos/virtio-pci.c
@@ -XXX,XX +XXX,XX @@ void qvirtio_pci_device_enable(QVirtioPCIDevice *d)
 {
     qpci_device_enable(d->pdev);
     d->bar = qpci_iomap(d->pdev, d->bar_idx, NULL);
+    d->enabled = true;
 }
 
 void qvirtio_pci_device_disable(QVirtioPCIDevice *d)
 {
-    qpci_iounmap(d->pdev, d->bar);
+    if (d->enabled) {
+        qpci_iounmap(d->pdev, d->bar);
+        d->enabled = false;
+    }
 }
 
 void qvirtqueue_pci_msix_setup(QVirtioPCIDevice *d, QVirtQueuePCI *vqpci,
-- 
2.45.2

Devices where the MSI-X addresses are shared with other MMIO on BAR0
can not use msi_enable because it unmaps and remaps BAR0, which
interferes with device MMIO mappings. xhci-nec is one such device we
would like to test msix with.

Keep track of each the BAR iomaps for each device and add code in msix
to use existing iomap if the msix bars are already mapped.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 tests/qtest/libqos/pci.h |  1 +
 tests/qtest/libqos/pci.c | 22 ++++++++++++++++------
 2 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/tests/qtest/libqos/pci.h b/tests/qtest/libqos/pci.h
index XXXXXXX..XXXXXXX 100644
--- a/tests/qtest/libqos/pci.h
+++ b/tests/qtest/libqos/pci.h
@@ -XXX,XX +XXX,XX @@ struct QPCIDevice
     bool bars_mapped[6];
     QPCIBar bars[6];
     bool msix_enabled;
+    bool msix_table_bar_iomap, msix_pba_bar_iomap;
     QPCIBar msix_table_bar, msix_pba_bar;
     uint64_t msix_table_off, msix_pba_off;
 };
diff --git a/tests/qtest/libqos/pci.c b/tests/qtest/libqos/pci.c
index XXXXXXX..XXXXXXX 100644
--- a/tests/qtest/libqos/pci.c
+++ b/tests/qtest/libqos/pci.c
@@ -XXX,XX +XXX,XX @@ void qpci_msix_enable(QPCIDevice *dev)
 
     table = qpci_config_readl(dev, addr + PCI_MSIX_TABLE);
     bir_table = table & PCI_MSIX_FLAGS_BIRMASK;
-    dev->msix_table_bar = qpci_iomap(dev, bir_table, NULL);
+    if (dev->bars_mapped[bir_table]) {
+        dev->msix_table_bar = dev->bars[bir_table];
+    } else {
+        dev->msix_table_bar_iomap = true;
+        dev->msix_table_bar = qpci_iomap(dev, bir_table, NULL);
+    }
     dev->msix_table_off = table & ~PCI_MSIX_FLAGS_BIRMASK;
 
     table = qpci_config_readl(dev, addr + PCI_MSIX_PBA);
     bir_pba = table & PCI_MSIX_FLAGS_BIRMASK;
-    if (bir_pba != bir_table) {
-        dev->msix_pba_bar = qpci_iomap(dev, bir_pba, NULL);
+    if (dev->bars_mapped[bir_pba]) {
+        dev->msix_pba_bar = dev->bars[bir_pba];
     } else {
-        dev->msix_pba_bar = dev->msix_table_bar;
+        dev->msix_pba_bar_iomap = true;
+        dev->msix_pba_bar = qpci_iomap(dev, bir_pba, NULL);
     }
     dev->msix_pba_off = table & ~PCI_MSIX_FLAGS_BIRMASK;
 
@@ -XXX,XX +XXX,XX @@ void qpci_msix_disable(QPCIDevice *dev)
     qpci_config_writew(dev, addr + PCI_MSIX_FLAGS,
                                                 val & ~PCI_MSIX_FLAGS_ENABLE);
 
-    if (dev->msix_pba_bar.addr != dev->msix_table_bar.addr) {
+    if (dev->msix_pba_bar_iomap) {
+        dev->msix_pba_bar_iomap = false;
         qpci_iounmap(dev, dev->msix_pba_bar);
     }
-    qpci_iounmap(dev, dev->msix_table_bar);
+    if (dev->msix_table_bar_iomap) {
+        dev->msix_table_bar_iomap = false;
+        qpci_iounmap(dev, dev->msix_table_bar);
+    }
 
     dev->msix_enabled = 0;
     dev->msix_table_off = 0;
-- 
2.45.2

Implement PBA write 1 to trigger and 0 to clear. This is used by
qtests which mask the MSI irq and so the bits remain pending and
expect to be cleared with stores.

Some devices like e1000e seem to have MSIX PBA pending tied to
some device state level, as such they call msix_clr_pending()
directly, and clearing pending via a store to PBA causes this
to go out of synch. So the qpci_msix_pending() function is
changed to avoid clearing, and a new test-and-clear function is
added for tests that would like to clear.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 tests/qtest/libqos/pci.h |  1 +
 hw/pci/msix.c            | 16 ++++++++++++++++
 tests/qtest/libqos/pci.c | 21 ++++++++++++++++++---
 3 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/tests/qtest/libqos/pci.h b/tests/qtest/libqos/pci.h
index XXXXXXX..XXXXXXX 100644
--- a/tests/qtest/libqos/pci.h
+++ b/tests/qtest/libqos/pci.h
@@ -XXX,XX +XXX,XX @@ uint8_t qpci_find_capability(QPCIDevice *dev, uint8_t id, uint8_t start_addr);
 void qpci_msix_enable(QPCIDevice *dev);
 void qpci_msix_disable(QPCIDevice *dev);
 bool qpci_msix_pending(QPCIDevice *dev, uint16_t entry);
+bool qpci_msix_test_clear_pending(QPCIDevice *dev, uint16_t entry);
 bool qpci_msix_masked(QPCIDevice *dev, uint16_t entry);
 uint16_t qpci_msix_table_size(QPCIDevice *dev);
 
diff --git a/hw/pci/msix.c b/hw/pci/msix.c
index XXXXXXX..XXXXXXX 100644
--- a/hw/pci/msix.c
+++ b/hw/pci/msix.c
@@ -XXX,XX +XXX,XX @@ static uint64_t msix_pba_mmio_read(void *opaque, hwaddr addr,
 static void msix_pba_mmio_write(void *opaque, hwaddr addr,
                                 uint64_t val, unsigned size)
 {
+    PCIDevice *dev = opaque;
+    unsigned vector_start = addr * 8;
+    unsigned vector_end = MIN(addr + size * 8, dev->msix_entries_nr);
+    unsigned i;
+
+    for (i = vector_start; i < vector_end; i++) {
+        if ((val >> i) & 1) {
+            if (!msix_is_pending(dev, i)) {
+                msix_notify(dev, i);
+            }
+        } else {
+            if (msix_is_pending(dev, i)) {
+                msix_clr_pending(dev, i);
+            }
+        }
+    }
 }
 
 static const MemoryRegionOps msix_pba_mmio_ops = {
diff --git a/tests/qtest/libqos/pci.c b/tests/qtest/libqos/pci.c
index XXXXXXX..XXXXXXX 100644
--- a/tests/qtest/libqos/pci.c
+++ b/tests/qtest/libqos/pci.c
@@ -XXX,XX +XXX,XX @@ bool qpci_msix_pending(QPCIDevice *dev, uint16_t entry)
 
     g_assert(dev->msix_enabled);
     pba_entry = qpci_io_readl(dev, dev->msix_pba_bar, dev->msix_pba_off + off);
-    qpci_io_writel(dev, dev->msix_pba_bar, dev->msix_pba_off + off,
-                   pba_entry & ~(1 << bit_n));
-    return (pba_entry & (1 << bit_n)) != 0;
+    return (pba_entry & (1 << bit_n));
 }
 
+bool qpci_msix_test_clear_pending(QPCIDevice *dev, uint16_t entry)
+{
+    uint32_t pba_entry;
+    uint8_t bit_n = entry % 32;
+    uint64_t  off = (entry / 32) * PCI_MSIX_ENTRY_SIZE / 4;
+
+    g_assert(dev->msix_enabled);
+    pba_entry = qpci_io_readl(dev, dev->msix_pba_bar, dev->msix_pba_off + off);
+    if (pba_entry & (1 << bit_n)) {
+        qpci_io_writel(dev, dev->msix_pba_bar, dev->msix_pba_off + off,
+                       pba_entry & ~(1 << bit_n));
+        return true;
+    }
+    return false;
+}
+
+
 bool qpci_msix_masked(QPCIDevice *dev, uint16_t entry)
 {
     uint8_t addr;
-- 
2.45.2

Add tests which init the host controller registers to the point
where command and event rings, irqs are operational. Enumerate
ports and set up an attached device context that enables device
transfer ring to be set up and tested.

This test does a bunch of things at once and is yet well
librified, but it allows testing basic mechanisms and gives a
starting point for further work.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 tests/qtest/usb-hcd-xhci-test.h | 232 +++++++++++++++
 tests/qtest/usb-hcd-xhci-test.c | 506 +++++++++++++++++++++++++++++++-
 2 files changed, 732 insertions(+), 6 deletions(-)
 create mode 100644 tests/qtest/usb-hcd-xhci-test.h

diff --git a/tests/qtest/usb-hcd-xhci-test.h b/tests/qtest/usb-hcd-xhci-test.h
new file mode 100644
index XXXXXXX..XXXXXXX
--- /dev/null
+++ b/tests/qtest/usb-hcd-xhci-test.h
@@ -XXX,XX +XXX,XX @@
+#ifndef TESTS_USB_HCD_XHCI_H
+#define TESTS_USB_HCD_XHCI_H
+
+typedef enum TRBType {
+    TRB_RESERVED = 0,
+    TR_NORMAL,
+    TR_SETUP,
+    TR_DATA,
+    TR_STATUS,
+    TR_ISOCH,
+    TR_LINK,
+    TR_EVDATA,
+    TR_NOOP,
+    CR_ENABLE_SLOT,
+    CR_DISABLE_SLOT,
+    CR_ADDRESS_DEVICE,
+    CR_CONFIGURE_ENDPOINT,
+    CR_EVALUATE_CONTEXT,
+    CR_RESET_ENDPOINT,
+    CR_STOP_ENDPOINT,
+    CR_SET_TR_DEQUEUE,
+    CR_RESET_DEVICE,
+    CR_FORCE_EVENT,
+    CR_NEGOTIATE_BW,
+    CR_SET_LATENCY_TOLERANCE,
+    CR_GET_PORT_BANDWIDTH,
+    CR_FORCE_HEADER,
+    CR_NOOP,
+    ER_TRANSFER = 32,
+    ER_COMMAND_COMPLETE,
+    ER_PORT_STATUS_CHANGE,
+    ER_BANDWIDTH_REQUEST,
+    ER_DOORBELL,
+    ER_HOST_CONTROLLER,
+    ER_DEVICE_NOTIFICATION,
+    ER_MFINDEX_WRAP,
+    /* vendor specific bits */
+    CR_VENDOR_NEC_FIRMWARE_REVISION  = 49,
+    CR_VENDOR_NEC_CHALLENGE_RESPONSE = 50,
+} TRBType;
+
+typedef enum TRBCCode {
+    CC_INVALID = 0,
+    CC_SUCCESS,
+    CC_DATA_BUFFER_ERROR,
+    CC_BABBLE_DETECTED,
+    CC_USB_TRANSACTION_ERROR,
+    CC_TRB_ERROR,
+    CC_STALL_ERROR,
+    CC_RESOURCE_ERROR,
+    CC_BANDWIDTH_ERROR,
+    CC_NO_SLOTS_ERROR,
+    CC_INVALID_STREAM_TYPE_ERROR,
+    CC_SLOT_NOT_ENABLED_ERROR,
+    CC_EP_NOT_ENABLED_ERROR,
+    CC_SHORT_PACKET,
+    CC_RING_UNDERRUN,
+    CC_RING_OVERRUN,
+    CC_VF_ER_FULL,
+    CC_PARAMETER_ERROR,
+    CC_BANDWIDTH_OVERRUN,
+    CC_CONTEXT_STATE_ERROR,
+    CC_NO_PING_RESPONSE_ERROR,
+    CC_EVENT_RING_FULL_ERROR,
+    CC_INCOMPATIBLE_DEVICE_ERROR,
+    CC_MISSED_SERVICE_ERROR,
+    CC_COMMAND_RING_STOPPED,
+    CC_COMMAND_ABORTED,
+    CC_STOPPED,
+    CC_STOPPED_LENGTH_INVALID,
+    CC_MAX_EXIT_LATENCY_TOO_LARGE_ERROR = 29,
+    CC_ISOCH_BUFFER_OVERRUN = 31,
+    CC_EVENT_LOST_ERROR,
+    CC_UNDEFINED_ERROR,
+    CC_INVALID_STREAM_ID_ERROR,
+    CC_SECONDARY_BANDWIDTH_ERROR,
+    CC_SPLIT_TRANSACTION_ERROR
+} TRBCCode;
+
+/* bit definitions */
+#define USBCMD_RS       (1<<0)
+#define USBCMD_HCRST    (1<<1)
+#define USBCMD_INTE     (1<<2)
+#define USBCMD_HSEE     (1<<3)
+#define USBCMD_LHCRST   (1<<7)
+#define USBCMD_CSS      (1<<8)
+#define USBCMD_CRS      (1<<9)
+#define USBCMD_EWE      (1<<10)
+#define USBCMD_EU3S     (1<<11)
+
+#define USBSTS_HCH      (1<<0)
+#define USBSTS_HSE      (1<<2)
+#define USBSTS_EINT     (1<<3)
+#define USBSTS_PCD      (1<<4)
+#define USBSTS_SSS      (1<<8)
+#define USBSTS_RSS      (1<<9)
+#define USBSTS_SRE      (1<<10)
+#define USBSTS_CNR      (1<<11)
+#define USBSTS_HCE      (1<<12)
+
+
+#define PORTSC_CCS          (1<<0)
+#define PORTSC_PED          (1<<1)
+#define PORTSC_OCA          (1<<3)
+#define PORTSC_PR           (1<<4)
+#define PORTSC_PLS_SHIFT        5
+#define PORTSC_PLS_MASK     0xf
+#define PORTSC_PP           (1<<9)
+#define PORTSC_SPEED_SHIFT      10
+#define PORTSC_SPEED_MASK   0xf
+#define PORTSC_SPEED_FULL   (1<<10)
+#define PORTSC_SPEED_LOW    (2<<10)
+#define PORTSC_SPEED_HIGH   (3<<10)
+#define PORTSC_SPEED_SUPER  (4<<10)
+#define PORTSC_PIC_SHIFT        14
+#define PORTSC_PIC_MASK     0x3
+#define PORTSC_LWS          (1<<16)
+#define PORTSC_CSC          (1<<17)
+#define PORTSC_PEC          (1<<18)
+#define PORTSC_WRC          (1<<19)
+#define PORTSC_OCC          (1<<20)
+#define PORTSC_PRC          (1<<21)
+#define PORTSC_PLC          (1<<22)
+#define PORTSC_CEC          (1<<23)
+#define PORTSC_CAS          (1<<24)
+#define PORTSC_WCE          (1<<25)
+#define PORTSC_WDE          (1<<26)
+#define PORTSC_WOE          (1<<27)
+#define PORTSC_DR           (1<<30)
+#define PORTSC_WPR          (1<<31)
+
+#define CRCR_RCS        (1<<0)
+#define CRCR_CS         (1<<1)
+#define CRCR_CA         (1<<2)
+#define CRCR_CRR        (1<<3)
+
+#define IMAN_IP         (1<<0)
+#define IMAN_IE         (1<<1)
+
+#define ERDP_EHB        (1<<3)
+
+#define TRB_SIZE 16
+
+enum {
+    PLS_U0              =  0,
+    PLS_U1              =  1,
+    PLS_U2              =  2,
+    PLS_U3              =  3,
+    PLS_DISABLED        =  4,
+    PLS_RX_DETECT       =  5,
+    PLS_INACTIVE        =  6,
+    PLS_POLLING         =  7,
+    PLS_RECOVERY        =  8,
+    PLS_HOT_RESET       =  9,
+    PLS_COMPILANCE_MODE = 10,
+    PLS_TEST_MODE       = 11,
+    PLS_RESUME          = 15,
+};
+
+#define CR_LINK TR_LINK
+
+#define TRB_C               (1<<0)
+#define TRB_TYPE_SHIFT          10
+#define TRB_TYPE_MASK       0x3f
+#define TRB_TYPE(t)         (((t).control >> TRB_TYPE_SHIFT) & TRB_TYPE_MASK)
+
+#define TRB_EV_ED           (1<<2)
+
+#define TRB_TR_ENT          (1<<1)
+#define TRB_TR_ISP          (1<<2)
+#define TRB_TR_NS           (1<<3)
+#define TRB_TR_CH           (1<<4)
+#define TRB_TR_IOC          (1<<5)
+#define TRB_TR_IDT          (1<<6)
+#define TRB_TR_TBC_SHIFT        7
+#define TRB_TR_TBC_MASK     0x3
+#define TRB_TR_BEI          (1<<9)
+#define TRB_TR_TLBPC_SHIFT      16
+#define TRB_TR_TLBPC_MASK   0xf
+#define TRB_TR_FRAMEID_SHIFT    20
+#define TRB_TR_FRAMEID_MASK 0x7ff
+#define TRB_TR_SIA          (1<<31)
+
+#define TRB_TR_DIR          (1<<16)
+
+#define TRB_CR_SLOTID_SHIFT     24
+#define TRB_CR_SLOTID_MASK  0xff
+#define TRB_CR_EPID_SHIFT       16
+#define TRB_CR_EPID_MASK    0x1f
+
+#define TRB_CR_BSR          (1<<9)
+#define TRB_CR_DC           (1<<9)
+
+#define TRB_LK_TC           (1<<1)
+
+#define TRB_INTR_SHIFT          22
+#define TRB_INTR_MASK       0x3ff
+#define TRB_INTR(t)         (((t).status >> TRB_INTR_SHIFT) & TRB_INTR_MASK)
+
+#define EP_TYPE_MASK        0x7
+#define EP_TYPE_SHIFT           3
+
+#define EP_STATE_MASK       0x7
+#define EP_DISABLED         (0<<0)
+#define EP_RUNNING          (1<<0)
+#define EP_HALTED           (2<<0)
+#define EP_STOPPED          (3<<0)
+#define EP_ERROR            (4<<0)
+
+#define SLOT_STATE_MASK     0x1f
+#define SLOT_STATE_SHIFT        27
+#define SLOT_STATE(s)       (((s)>>SLOT_STATE_SHIFT)&SLOT_STATE_MASK)
+#define SLOT_ENABLED        0
+#define SLOT_DEFAULT        1
+#define SLOT_ADDRESSED      2
+#define SLOT_CONFIGURED     3
+
+#define SLOT_CONTEXT_ENTRIES_MASK 0x1f
+#define SLOT_CONTEXT_ENTRIES_SHIFT 27
+
+typedef enum EPType {
+    ET_INVALID = 0,
+    ET_ISO_OUT,
+    ET_BULK_OUT,
+    ET_INTR_OUT,
+    ET_CONTROL,
+    ET_ISO_IN,
+    ET_BULK_IN,
+    ET_INTR_IN,
+} EPType;
+
+#endif /* TESTS_USB_HCD_XHCI_H */
diff --git a/tests/qtest/usb-hcd-xhci-test.c b/tests/qtest/usb-hcd-xhci-test.c
index XXXXXXX..XXXXXXX 100644
--- a/tests/qtest/usb-hcd-xhci-test.c
+++ b/tests/qtest/usb-hcd-xhci-test.c
@@ -XXX,XX +XXX,XX @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/bswap.h"
+#include "libqtest.h"
+#include "libqos/libqos-pc.h"
 #include "libqtest-single.h"
 #include "libqos/usb.h"
+#include "hw/pci/pci_ids.h"
+#include "hw/pci/pci_regs.h"
+#include "usb-hcd-xhci-test.h"
+
+/*** Test Setup & Teardown ***/
+typedef struct XHCIEvRingSeg {
+    uint32_t addr_low;
+    uint32_t addr_high;
+    uint32_t size;
+    uint32_t rsvd;
+} XHCIEvRingSeg;
+
+typedef struct XHCITRB {
+    uint64_t parameter;
+    uint32_t status;
+    uint32_t control;
+} XHCITRB;
+
+typedef struct XHCIQSlotState {
+    /* In-memory arrays */
+    uint64_t device_context;
+    uint64_t transfer_ring;
+
+    uint32_t tr_trb_entries;
+    uint32_t tr_trb_idx;
+    uint32_t tr_trb_c;
+} XHCIQSlotState;
+
+typedef struct XHCIQState {
+    /* QEMU PCI variables */
+    QOSState *parent;
+    QPCIDevice *dev;
+    QPCIBar bar;
+    uint64_t barsize;
+    uint32_t fingerprint;
+
+    /* In-memory arrays */
+    uint64_t dc_base_array;
+    uint64_t command_ring;
+    uint64_t event_ring_seg;
+    uint64_t event_ring;
+
+    uint32_t cr_trb_entries;
+    uint32_t cr_trb_idx;
+    uint32_t cr_trb_c;
+    uint32_t er_trb_entries;
+    uint32_t er_trb_idx;
+    uint32_t er_trb_c;
+
+    /* Host controller properties */
+    uint32_t rtoff, dboff;
+    uint32_t maxports, maxslots, maxintrs;
+
+    XHCIQSlotState slots[32];
+} XHCIQState;
+
+#define XHCI_NEC_ID (PCI_DEVICE_ID_NEC_UPD720200 << 16 | \
+                     PCI_VENDOR_ID_NEC)
+
+/**
+ * Locate, verify, and return a handle to the XHCI device.
+ */
+static QPCIDevice *get_xhci_device(QTestState *qts, uint32_t *fingerprint)
+{
+    QPCIDevice *xhci;
+    uint32_t xhci_fingerprint;
+    QPCIBus *pcibus;
+
+    pcibus = qpci_new_pc(qts, NULL);
+
+    /* Find the XHCI PCI device and verify it's the right one. */
+    xhci = qpci_device_find(pcibus, QPCI_DEVFN(0x1D, 0x0));
+    g_assert(xhci != NULL);
+
+    xhci_fingerprint = qpci_config_readl(xhci, PCI_VENDOR_ID);
+    switch (xhci_fingerprint) {
+    case XHCI_NEC_ID:
+        break;
+    default:
+        /* Unknown device. */
+        g_assert_not_reached();
+    }
+
+    if (fingerprint) {
+        *fingerprint = xhci_fingerprint;
+    }
+    return xhci;
+}
+
+static void free_xhci_device(QPCIDevice *dev)
+{
+    QPCIBus *pcibus = dev ? dev->bus : NULL;
+
+    /* libqos doesn't have a function for this, so free it manually */
+    g_free(dev);
+    qpci_free_pc(pcibus);
+}
+
+/**
+ * Start a Q35 machine and bookmark a handle to the XHCI device.
+ */
+G_GNUC_PRINTF(1, 0)
+static XHCIQState *xhci_vboot(const char *cli, va_list ap)
+{
+    XHCIQState *s;
+
+    s = g_new0(XHCIQState, 1);
+    s->parent = qtest_pc_vboot(cli, ap);
+    alloc_set_flags(&s->parent->alloc, ALLOC_LEAK_ASSERT);
+
+    /* Verify that we have an XHCI device present. */
+    s->dev = get_xhci_device(s->parent->qts, &s->fingerprint);
+    s->bar = qpci_iomap(s->dev, 0, &s->barsize);
+    /* turns on pci.cmd.iose, pci.cmd.mse and pci.cmd.bme */
+    qpci_device_enable(s->dev);
+
+    return s;
+}
+
+/**
+ * Start a Q35 machine and bookmark a handle to the XHCI device.
+ */
+G_GNUC_PRINTF(1, 2)
+static XHCIQState *xhci_boot(const char *cli, ...)
+{
+    XHCIQState *s;
+    va_list ap;
+
+    if (cli) {
+        va_start(ap, cli);
+        s = xhci_vboot(cli, ap);
+        va_end(ap);
+    } else {
+        s = xhci_boot("-M q35 "
+                      "-device nec-usb-xhci,id=xhci,bus=pcie.0,addr=1d.0 "
+                      "-drive id=drive0,if=none,file=null-co://,"
+                          "file.read-zeroes=on,format=raw");
+    }
+
+    return s;
+}
+
+/**
+ * Clean up the PCI device, then terminate the QEMU instance.
+ */
+static void xhci_shutdown(XHCIQState *xhci)
+{
+    QOSState *qs = xhci->parent;
+
+//    xhci_clean_mem(xhci);
+    free_xhci_device(xhci->dev);
+    g_free(xhci);
+    qtest_shutdown(qs);
+}
+
+/*** tests ***/
 
 static void test_xhci_hotplug(void)
 {
-    usb_test_hotplug(global_qtest, "xhci", "1", NULL);
+    XHCIQState *s;
+    QTestState *qts;
+
+    s = xhci_boot(NULL);
+    qts = s->parent->qts;
+
+    usb_test_hotplug(qts, "xhci", "1", NULL);
+
+    xhci_shutdown(s);
 }
 
 static void test_usb_uas_hotplug(void)
 {
-    QTestState *qts = global_qtest;
+    XHCIQState *s;
+    QTestState *qts;
+
+    s = xhci_boot(NULL);
+    qts = s->parent->qts;
 
     qtest_qmp_device_add(qts, "usb-uas", "uas", "{}");
     qtest_qmp_device_add(qts, "scsi-hd", "scsihd", "{'drive': 'drive0'}");
@@ -XXX,XX +XXX,XX @@ static void test_usb_uas_hotplug(void)
 
     qtest_qmp_device_del(qts, "scsihd");
     qtest_qmp_device_del(qts, "uas");
+
+    xhci_shutdown(s);
 }
 
 static void test_usb_ccid_hotplug(void)
 {
-    QTestState *qts = global_qtest;
+    XHCIQState *s;
+    QTestState *qts;
+
+    s = xhci_boot(NULL);
+    qts = s->parent->qts;
 
     qtest_qmp_device_add(qts, "usb-ccid", "ccid", "{}");
     qtest_qmp_device_del(qts, "ccid");
     /* check the device can be added again */
     qtest_qmp_device_add(qts, "usb-ccid", "ccid", "{}");
     qtest_qmp_device_del(qts, "ccid");
+
+    xhci_shutdown(s);
+}
+
+static uint64_t xhci_guest_zalloc(XHCIQState *s, uint64_t size)
+{
+    char mem[0x1000];
+    uint64_t ret;
+
+    g_assert(size <= 0x1000);
+
+    memset(mem, 0, size);
+
+    ret = guest_alloc(&s->parent->alloc, size);
+    qtest_memwrite(s->parent->qts, ret, mem, size);
+
+    return ret;
+}
+
+static uint32_t xhci_cap_readl(XHCIQState *s, uint64_t addr)
+{
+    return qpci_io_readl(s->dev, s->bar, addr);
+}
+
+static uint32_t xhci_op_readl(XHCIQState *s, uint64_t addr)
+{
+    return qpci_io_readl(s->dev, s->bar, 0x40 + addr);
+}
+
+static void xhci_op_writel(XHCIQState *s, uint64_t addr, uint32_t value)
+{
+    qpci_io_writel(s->dev, s->bar, 0x40 + addr, value);
+}
+
+static uint32_t xhci_port_readl(XHCIQState *s, uint32_t port, uint64_t addr)
+{
+    return xhci_op_readl(s, 0x400 + port * 0x10 + addr);
+}
+
+static uint32_t xhci_rt_readl(XHCIQState *s, uint64_t addr)
+{
+    return qpci_io_readl(s->dev, s->bar, s->rtoff + addr);
+}
+
+static void xhci_rt_writel(XHCIQState *s, uint64_t addr, uint32_t value)
+{
+    qpci_io_writel(s->dev, s->bar, s->rtoff + addr, value);
+}
+
+static void xhci_db_writel(XHCIQState *s, uint32_t db, uint32_t value)
+{
+    qpci_io_writel(s->dev, s->bar, s->dboff + db * 4, value);
+}
+
+static void wait_event_trb(XHCIQState *s, XHCITRB *trb)
+{
+    XHCITRB t;
+    uint64_t er_addr = s->event_ring + s->er_trb_idx * sizeof(*trb);
+    uint32_t value;
+    guint64 end_time = g_get_monotonic_time() + 5 * G_TIME_SPAN_SECOND;
+
+    /* Wait for event interrupt  */
+
+    do {
+        if (g_get_monotonic_time() >= end_time) {
+            g_error("Timeout expired");
+        }
+        qtest_clock_step(s->parent->qts, 10000);
+
+        value = xhci_op_readl(s, 0x4); /* USBSTS */
+    } while (!(value & USBSTS_EINT));
+
+    value = xhci_rt_readl(s, 0x20 + 0x0); /* IMAN */
+
+    /* With MSI-X enabled, IMAN IP is cleared after raising the interrupt */
+    g_assert(!(value & IMAN_IP));
+
+    /* Ensure MSI-X interrupt is pending */
+    assert(qpci_msix_test_clear_pending(s->dev, 0));
+    /* Then cleared */
+    assert(!qpci_msix_pending(s->dev, 0));
+
+    xhci_op_writel(s, 0x4, USBSTS_EINT); /* USBSTS clear EINT */
+
+    qtest_memread(s->parent->qts, er_addr, &t, sizeof(t));
+
+    trb->parameter = le64_to_cpu(t.parameter);
+    trb->status = le32_to_cpu(t.status);
+    trb->control = le32_to_cpu(t.control);
+
+    g_assert((trb->status >> 24) == CC_SUCCESS);
+    g_assert((trb->control & TRB_C) == s->er_trb_c); /* C bit has been set */
+
+    s->er_trb_idx++;
+    if (s->er_trb_idx == s->er_trb_entries) {
+        s->er_trb_idx = 0;
+        s->er_trb_c ^= 1;
+    }
+    /* Update ERDP to processed TRB addr and EHB bit, which clears EHB */
+    er_addr = s->event_ring + s->er_trb_idx * sizeof(*trb);
+    xhci_rt_writel(s, 0x38, (er_addr & 0xffffffff) | ERDP_EHB);
+}
+
+static void set_link_trb(XHCIQState *s, uint64_t ring, uint32_t c, uint32_t entries)
+{
+    XHCITRB trb;
+
+    g_assert(entries > 1);
+
+    memset(&trb, 0, sizeof(trb));
+    trb.parameter = cpu_to_le64(ring);
+    trb.control = cpu_to_le32(c | /* C */
+                              (TR_LINK << TRB_TYPE_SHIFT) |
+                              TRB_LK_TC);
+    qtest_memwrite(s->parent->qts, ring + sizeof(trb) * (entries - 1), &trb, sizeof(trb));
 }
 
+static void submit_cr_trb(XHCIQState *s, XHCITRB *trb)
+{
+    XHCITRB t;
+    uint64_t cr_addr = s->command_ring + s->cr_trb_idx * sizeof(*trb);
+
+    trb->control |= s->cr_trb_c; /* C */
+
+    t.parameter = cpu_to_le64(trb->parameter);
+    t.status = cpu_to_le32(trb->status);
+    t.control = cpu_to_le32(trb->control);
+
+    qtest_memwrite(s->parent->qts, cr_addr, &t, sizeof(t));
+    s->cr_trb_idx++;
+    /* Last entry contains the link, so wrap back */
+    if (s->cr_trb_idx == s->cr_trb_entries - 1) {
+        set_link_trb(s, s->command_ring, s->cr_trb_c, s->cr_trb_entries);
+        s->cr_trb_idx = 0;
+        s->cr_trb_c ^= 1;
+    }
+    xhci_db_writel(s, 0, 0); /* doorbell 0 */
+}
+
+static void pci_xhci_stress_rings(void)
+{
+    XHCIQState *s;
+    uint32_t value;
+    uint64_t input_context;
+    XHCIEvRingSeg ev_seg;
+    XHCITRB trb;
+    uint32_t hcsparams1;
+    uint32_t slotid;
+    void *mem;
+    int i;
+
+    mem = g_malloc(0x1000);
+    memset(mem, 0, 0x1000);
+
+    s = xhci_boot("-M q35 "
+            "-device nec-usb-xhci,id=xhci,bus=pcie.0,addr=1d.0 "
+            "-device usb-storage,bus=xhci.0,drive=drive0 "
+            "-drive id=drive0,if=none,file=null-co://,"
+                "file.read-zeroes=on,format=raw "
+            );
+//            "-d trace:*xhci*,trace:*usb*,trace:*msi*");
+
+    hcsparams1 = xhci_cap_readl(s, 0x4); /* HCSPARAMS1 */
+    s->maxports = (hcsparams1 >> 24) & 0xff;
+    s->maxintrs = (hcsparams1 >> 8) & 0x3ff;
+    s->maxslots = hcsparams1 & 0xff;
+
+    s->dboff = xhci_cap_readl(s, 0x14); /* DBOFF */
+    s->rtoff = xhci_cap_readl(s, 0x18); /* RTOFF */
+
+    s->dc_base_array = xhci_guest_zalloc(s, 0x800);
+    s->command_ring = xhci_guest_zalloc(s, 0x1000);
+    s->event_ring = xhci_guest_zalloc(s, 0x1000);
+    s->event_ring_seg = xhci_guest_zalloc(s, 0x100);
+
+    /* Arbitrary small sizes so we can make them wrap */
+    s->cr_trb_entries = 0x20;
+    s->cr_trb_c = 1;
+    s->er_trb_entries = 0x10;
+    s->er_trb_c = 1;
+
+    ev_seg.addr_low = cpu_to_le32(s->event_ring & 0xffffffff);
+    ev_seg.addr_high = cpu_to_le32(s->event_ring >> 32);
+    ev_seg.size = cpu_to_le32(0x10);
+    ev_seg.rsvd = 0;
+    qtest_memwrite(s->parent->qts, s->event_ring_seg, &ev_seg, sizeof(ev_seg));
+
+    xhci_op_writel(s, 0x0, USBCMD_HCRST); /* USBCMD */
+    do {
+        value = xhci_op_readl(s, 0x4); /* USBSTS */
+    } while (value & (1 << 11)); /* CNR */
+
+    xhci_op_writel(s, 0x38, s->maxslots); /* CONFIG */
+
+    /* DCBAAP */
+    xhci_op_writel(s, 0x30, s->dc_base_array & 0xffffffff);
+    xhci_op_writel(s, 0x34, s->dc_base_array >> 32);
+
+    /* CRCR */
+    xhci_op_writel(s, 0x18, (s->command_ring & 0xffffffff) | s->cr_trb_c);
+    xhci_op_writel(s, 0x1c, s->command_ring >> 32);
+
+    xhci_rt_writel(s, 0x28, 1); /* ERSTSZ */
+
+    /* ERSTBA */
+    xhci_rt_writel(s, 0x30, s->event_ring_seg & 0xffffffff);
+    xhci_rt_writel(s, 0x34, s->event_ring_seg >> 32);
+
+    /* ERDP */
+    xhci_rt_writel(s, 0x38, s->event_ring & 0xffffffff);
+    xhci_rt_writel(s, 0x3c, s->event_ring >> 32);
+
+    qpci_msix_enable(s->dev);
+    xhci_op_writel(s, 0x0, USBCMD_RS | USBCMD_INTE); /* RUN + INTE */
+
+    /* Enable interrupts on ER IMAN */
+    xhci_rt_writel(s, 0x20, IMAN_IE);
+
+    assert(!qpci_msix_pending(s->dev, 0));
+
+    /* Wrap the command and event rings with no-ops a few times */
+    for (i = 0; i < 100; i++) {
+        /* Issue a command ring no-op */
+        memset(&trb, 0, sizeof(trb));
+        trb.control |= CR_NOOP << TRB_TYPE_SHIFT;
+        trb.control |= TRB_TR_IOC;
+        submit_cr_trb(s, &trb);
+        wait_event_trb(s, &trb);
+    }
+
+    /* Query ports */
+    for (i = 0; i < s->maxports; i++) {
+        value = xhci_port_readl(s, i, 0); /* PORTSC */
+
+        /* Only first port should be attached and enabled */
+        if (i == 0) {
+            g_assert(value & PORTSC_CCS);
+            g_assert(value & PORTSC_PED);
+	    /* Port Speed must be identified */
+	    g_assert(((value >> PORTSC_SPEED_SHIFT) & PORTSC_SPEED_MASK) != 0);
+        } else {
+            g_assert(!(value & PORTSC_CCS));
+            g_assert(!(value & PORTSC_PED));
+            g_assert(((value >> PORTSC_PLS_SHIFT) & PORTSC_PLS_MASK) == 5);
+        }
+    }
+
+    /* Issue a command ring enable slot */
+    memset(&trb, 0, sizeof(trb));
+    trb.control |= CR_ENABLE_SLOT << TRB_TYPE_SHIFT;
+    trb.control |= TRB_TR_IOC;
+    submit_cr_trb(s, &trb);
+    wait_event_trb(s, &trb);
+    slotid = (trb.control >> TRB_CR_SLOTID_SHIFT) & 0xff;
+
+    s->slots[slotid].transfer_ring = xhci_guest_zalloc(s, 0x1000);
+    s->slots[slotid].tr_trb_entries = 0x10;
+    s->slots[slotid].tr_trb_c = 1;
+
+    /* 32-byte input context size, should check HCCPARAMS1 for 64-byte size */
+    input_context = xhci_guest_zalloc(s, 0x420);
+
+    /* Set input control context */
+    ((uint32_t *)mem)[1] = cpu_to_le32(0x3); /* Add device contexts 0 and 1 */
+    ((uint32_t *)mem)[8] = cpu_to_le32(1 << 27); /* 1 context entry */
+    ((uint32_t *)mem)[9] = cpu_to_le32(1 << 16); /* 1 port number */
+
+    /* Set endpoint 0 context */
+    ((uint32_t *)mem)[16] = 0;
+    ((uint32_t *)mem)[17] = cpu_to_le32((ET_CONTROL << EP_TYPE_SHIFT) |
+                                        (0x200 << 16)); /* max packet sz XXX? */
+    ((uint32_t *)mem)[18] = cpu_to_le32((s->slots[slotid].transfer_ring & 0xffffffff) | 1); /* DCS=1 */
+    ((uint32_t *)mem)[19] = cpu_to_le32(s->slots[slotid].transfer_ring >> 32);
+    ((uint32_t *)mem)[20] = cpu_to_le32(0x200); /* Average TRB length */
+    qtest_memwrite(s->parent->qts, input_context, mem, 0x420);
+
+    s->slots[slotid].device_context = xhci_guest_zalloc(s, 0x400);
+
+    ((uint64_t *)mem)[0] = cpu_to_le64(s->slots[slotid].device_context);
+    qtest_memwrite(s->parent->qts, s->dc_base_array + 8*slotid, mem, 8);
+
+    /* Issue a command ring address device */
+    memset(&trb, 0, sizeof(trb));
+    trb.parameter = input_context;
+    trb.control |= CR_ADDRESS_DEVICE << TRB_TYPE_SHIFT;
+    trb.control |= slotid << TRB_CR_SLOTID_SHIFT;
+    submit_cr_trb(s, &trb);
+    wait_event_trb(s, &trb);
+
+    /* XXX: Check EP state is running? */
+
+    /* Shut it down */
+    qpci_msix_disable(s->dev);
+
+    guest_free(&s->parent->alloc, s->slots[slotid].device_context);
+    guest_free(&s->parent->alloc, s->slots[slotid].transfer_ring);
+    guest_free(&s->parent->alloc, input_context);
+    guest_free(&s->parent->alloc, s->event_ring);
+    guest_free(&s->parent->alloc, s->event_ring_seg);
+    guest_free(&s->parent->alloc, s->command_ring);
+    guest_free(&s->parent->alloc, s->dc_base_array);
+
+    xhci_shutdown(s);
+}
+
+/* tests */
 int main(int argc, char **argv)
 {
     int ret;
+    const char *arch;
 
     g_test_init(&argc, &argv, NULL);
 
+    /* Check architecture */
+    arch = qtest_get_arch();
+    if (strcmp(arch, "i386") && strcmp(arch, "x86_64")) {
+        g_test_message("Skipping test for non-x86");
+        return 0;
+    }
+
+    if (!qtest_has_device("nec-usb-xhci")) {
+        return 0;
+    }
+
     qtest_add_func("/xhci/pci/hotplug", test_xhci_hotplug);
     if (qtest_has_device("usb-uas")) {
         qtest_add_func("/xhci/pci/hotplug/usb-uas", test_usb_uas_hotplug);
@@ -XXX,XX +XXX,XX @@ int main(int argc, char **argv)
     if (qtest_has_device("usb-ccid")) {
         qtest_add_func("/xhci/pci/hotplug/usb-ccid", test_usb_ccid_hotplug);
     }
+    if (qtest_has_device("usb-storage")) {
+        qtest_add_func("/xhci/pci/xhci-stress-rings", pci_xhci_stress_rings);
+    }
 
-    qtest_start("-device nec-usb-xhci,id=xhci"
-                " -drive id=drive0,if=none,file=null-co://,"
-                "file.read-zeroes=on,format=raw");
     ret = g_test_run();
+
     qtest_end();
 
     return ret;
-- 
2.45.2

Implement TR NOOP commands by setting up then immediately completing
the packet. Add a TR NOOP test to the xhci qtest.

The IBM AIX XHCI driver uses NOOP commands to check driver and
hardware health.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 hw/usb/hcd-xhci.c               | 28 ++++++++++++++++++++++++++-
 tests/qtest/usb-hcd-xhci-test.c | 34 +++++++++++++++++++++++++++++++++
 2 files changed, 61 insertions(+), 1 deletion(-)

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index XXXXXXX..XXXXXXX 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -XXX,XX +XXX,XX @@ static int xhci_fire_transfer(XHCIState *xhci, XHCITransfer *xfer, XHCIEPContext
     return xhci_submit(xhci, xfer, epctx);
 }
 
+static int xhci_noop_transfer(XHCIState *xhci, XHCITransfer *xfer)
+{
+    /*
+     * TR NOOP conceptually probably better not call into USB subsystem
+     * (usb_packet_setup() via xhci_setup_packet()). In practice it
+     * works and avoids code duplication.
+     */
+    if (xhci_setup_packet(xfer) < 0) {
+        return -1;
+    }
+    xhci_try_complete_packet(xfer);
+    return 0;
+}
+
 static void xhci_kick_ep(XHCIState *xhci, unsigned int slotid,
                          unsigned int epid, unsigned int streamid)
 {
@@ -XXX,XX +XXX,XX @@ static void xhci_kick_epctx(XHCIEPContext *epctx, unsigned int streamid)
 
     epctx->kick_active++;
     while (1) {
+        bool noop = false;
+
         length = xhci_ring_chain_length(xhci, ring);
         if (length <= 0) {
             if (epctx->type == ET_ISO_OUT || epctx->type == ET_ISO_IN) {
@@ -XXX,XX +XXX,XX @@ static void xhci_kick_epctx(XHCIEPContext *epctx, unsigned int streamid)
                 epctx->kick_active--;
                 return;
             }
+            if (type == TR_NOOP) {
+                noop = true;
+            }
         }
         xfer->streamid = streamid;
 
-        if (epctx->epid == 1) {
+        if (noop) {
+            if (length != 1) {
+                qemu_log_mask(LOG_GUEST_ERROR,
+                              "%s: NOOP TR TRB within TRB chain!\n", __func__);
+                /* Undefined behavior, we no-op the entire chain */
+            }
+            xhci_noop_transfer(xhci, xfer);
+        } else if (epctx->epid == 1) {
             xhci_fire_ctl_transfer(xhci, xfer);
         } else {
             xhci_fire_transfer(xhci, xfer, epctx);
diff --git a/tests/qtest/usb-hcd-xhci-test.c b/tests/qtest/usb-hcd-xhci-test.c
index XXXXXXX..XXXXXXX 100644
--- a/tests/qtest/usb-hcd-xhci-test.c
+++ b/tests/qtest/usb-hcd-xhci-test.c
@@ -XXX,XX +XXX,XX @@ static void submit_cr_trb(XHCIQState *s, XHCITRB *trb)
     xhci_db_writel(s, 0, 0); /* doorbell 0 */
 }
 
+static void submit_tr_trb(XHCIQState *s, int slot, XHCITRB *trb)
+{
+    XHCITRB t;
+    uint64_t tr_addr = s->slots[slot].transfer_ring + s->slots[slot].tr_trb_idx * sizeof(*trb);
+
+    trb->control |= s->slots[slot].tr_trb_c; /* C */
+
+    t.parameter = cpu_to_le64(trb->parameter);
+    t.status = cpu_to_le32(trb->status);
+    t.control = cpu_to_le32(trb->control);
+
+    qtest_memwrite(s->parent->qts, tr_addr, &t, sizeof(t));
+    s->slots[slot].tr_trb_idx++;
+    /* Last entry contains the link, so wrap back */
+    if (s->slots[slot].tr_trb_idx == s->slots[slot].tr_trb_entries - 1) {
+        set_link_trb(s, s->slots[slot].transfer_ring,
+                        s->slots[slot].tr_trb_c,
+                        s->slots[slot].tr_trb_entries);
+        s->slots[slot].tr_trb_idx = 0;
+        s->slots[slot].tr_trb_c ^= 1;
+    }
+    xhci_db_writel(s, slot, 1); /* doorbell slot, EP0 target */
+}
+
 static void pci_xhci_stress_rings(void)
 {
     XHCIQState *s;
@@ -XXX,XX +XXX,XX @@ static void pci_xhci_stress_rings(void)
 
     /* XXX: Check EP state is running? */
 
+    /* Wrap the transfer ring a few times */
+    for (i = 0; i < 100; i++) {
+        /* Issue a transfer ring slot 0 noop */
+        memset(&trb, 0, sizeof(trb));
+        trb.control |= TR_NOOP << TRB_TYPE_SHIFT;
+        trb.control |= TRB_TR_IOC;
+        submit_tr_trb(s, slotid, &trb);
+        wait_event_trb(s, &trb);
+    }
+
     /* Shut it down */
     qpci_msix_disable(s->dev);
 
-- 
2.45.2

Hi,

This series adds XHCI "TR NOOP" support, but adding qtests ran into
issues with PCI and MSIX, and fixing those irritated what looks like
a bug in the e1000e/igb tests.

Firstly, the XHCI NEC PCI host has a shared BAR for device registers
and MSIX space, which is not handled by qtest/pci. Keeping track of BAR
mappings in qtest and allowing MSIX to use an existing mapping seems to
work.

For the XHCI qtest, I added a MSIX check to ensure interrupts are working
correctly, similarly to e1000e it polls the PBA bit with the interrupt
masked. Doing this never actually clears the pending msix irq, because
that is not implemented in the hw/pci code. So a test that waits for an
interrupt to be triggered multiple times will fail.

Adding the PBA write operations to clear/trigger pending interrupts
breaks e1000e/igb tests that wait for multiple triggers, because they
are actually buggy and only triggering the irq once, but the missing
MSIX write support was saving them because it made it look like the
irq had triggered.

My XHCI qtest caught that issue because it has some code which
asserts that the interrupt is *not* pending.

Since RFC I fixed up some checkpatch issues as Fabiano pointed out,
and also dug into the e1000e/igb test failure a bit more and hopefully
fixed it properly.

Thanks,
Nick

Nicholas Piggin (8):
  qtest/pci: Enforce balanced iomap/unmap
  qtest/libqos/pci: Fix qpci_msix_enable sharing bar0
  pci/msix: Implement PBA writes
  tests/qtest/e1000e|igb: Fix e1000e and igb tests to re-trigger
    interrupts
  hw/usb/xhci: Move HCD constants to a header and add register constants
  qtest/xhci: Add controller and device setup and ring tests
  hw/usb/xhci: Support TR NOOP commands
  qtest/xhci: add a test for TR NOOP commands

-- 
2.45.2

Add assertions to ensure a BAR is not mapped twice, and only
previously mapped BARs are unmapped. This can help catch some
bugs.

Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
Reviewed-by: Fabiano Rosas <farosas@suse.de>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 tests/qtest/libqos/ahci.h       |  1 +
 tests/qtest/libqos/pci.h        |  2 ++
 tests/qtest/libqos/virtio-pci.h |  1 +
 tests/qtest/ahci-test.c         |  2 ++
 tests/qtest/libqos/ahci.c       |  6 ++++++
 tests/qtest/libqos/pci.c        | 32 +++++++++++++++++++++++++++++++-
 tests/qtest/libqos/virtio-pci.c |  6 +++++-
 7 files changed, 48 insertions(+), 2 deletions(-)

Use the BAR iomap tracking structure introduced in the previous change
to have qpci_misx_enable() use existing iomaps if msix bars are
already mapped.

diff --git a/tests/qtest/libqos/pci.h b/tests/qtest/libqos/pci.h
index XXXXXXX..XXXXXXX 100644
--- a/tests/qtest/libqos/pci.h
+++ b/tests/qtest/libqos/pci.h
@@ -XXX,XX +XXX,XX @@ struct QPCIDevice
     bool bars_mapped[6];
     QPCIBar bars[6];
     bool msix_enabled;
+    bool msix_table_bar_iomap, msix_pba_bar_iomap;
     QPCIBar msix_table_bar, msix_pba_bar;
     uint64_t msix_table_off, msix_pba_off;
 };
diff --git a/tests/qtest/libqos/pci.c b/tests/qtest/libqos/pci.c
index XXXXXXX..XXXXXXX 100644
--- a/tests/qtest/libqos/pci.c
+++ b/tests/qtest/libqos/pci.c
@@ -XXX,XX +XXX,XX @@ void qpci_msix_enable(QPCIDevice *dev)
 
     table = qpci_config_readl(dev, addr + PCI_MSIX_TABLE);
     bir_table = table & PCI_MSIX_FLAGS_BIRMASK;
-    dev->msix_table_bar = qpci_iomap(dev, bir_table, NULL);
+    if (dev->bars_mapped[bir_table]) {
+        dev->msix_table_bar = dev->bars[bir_table];
+    } else {
+        dev->msix_table_bar_iomap = true;
+        dev->msix_table_bar = qpci_iomap(dev, bir_table, NULL);
+    }
     dev->msix_table_off = table & ~PCI_MSIX_FLAGS_BIRMASK;
 
     table = qpci_config_readl(dev, addr + PCI_MSIX_PBA);
     bir_pba = table & PCI_MSIX_FLAGS_BIRMASK;
-    if (bir_pba != bir_table) {
-        dev->msix_pba_bar = qpci_iomap(dev, bir_pba, NULL);
+    if (dev->bars_mapped[bir_pba]) {
+        dev->msix_pba_bar = dev->bars[bir_pba];
     } else {
-        dev->msix_pba_bar = dev->msix_table_bar;
+        dev->msix_pba_bar_iomap = true;
+        dev->msix_pba_bar = qpci_iomap(dev, bir_pba, NULL);
     }
     dev->msix_pba_off = table & ~PCI_MSIX_FLAGS_BIRMASK;
 
@@ -XXX,XX +XXX,XX @@ void qpci_msix_disable(QPCIDevice *dev)
 {
     uint8_t addr;
     uint16_t val;
+    uint32_t table;
 
     g_assert(dev->msix_enabled);
     addr = qpci_find_capability(dev, PCI_CAP_ID_MSIX, 0);
@@ -XXX,XX +XXX,XX @@ void qpci_msix_disable(QPCIDevice *dev)
     qpci_config_writew(dev, addr + PCI_MSIX_FLAGS,
                                                 val & ~PCI_MSIX_FLAGS_ENABLE);
 
-    if (dev->msix_pba_bar.addr != dev->msix_table_bar.addr) {
+    if (dev->msix_pba_bar_iomap) {
+        dev->msix_pba_bar_iomap = false;
         qpci_iounmap(dev, dev->msix_pba_bar);
+    } else {
+        /*
+         * If we had reused an existing iomap, ensure it is still mapped
+         * otherwise it would be a bug if it were unmapped before msix is
+         * disabled. A refcounting iomap implementation could avoid this
+         * issue entirely, but let's wait until that's needed.
+         */
+        uint8_t bir_pba;
+        table = qpci_config_readl(dev, addr + PCI_MSIX_PBA);
+        bir_pba = table & PCI_MSIX_FLAGS_BIRMASK;
+        g_assert(dev->bars_mapped[bir_pba]);
+    }
+
+    if (dev->msix_table_bar_iomap) {
+        dev->msix_table_bar_iomap = false;
+        qpci_iounmap(dev, dev->msix_table_bar);
+    } else {
+        uint8_t bir_table;
+        table = qpci_config_readl(dev, addr + PCI_MSIX_TABLE);
+        bir_table = table & PCI_MSIX_FLAGS_BIRMASK;
+        g_assert(dev->bars_mapped[bir_table]);
     }
-    qpci_iounmap(dev, dev->msix_table_bar);
 
     dev->msix_enabled = 0;
     dev->msix_table_off = 0;
-- 
2.45.2

Implement MMIO PBA writes, 1 to trigger and 0 to clear.

This functionality is used by some qtests, which keep the msix irq
masked and test irq pending via the PBA bits, for simplicity. Some
tests expect to be able to clear the irq with a store, so a side-effect
of this is that qpci_msix_pending() would actually clear the pending
bit where it previously did not. This actually causes some [possibly
buggy] tests to fail. So to avoid breakage until tests are re-examined,
prior behavior of qpci_msix_pending() is kept by changing it to avoid
clearing PBA.

A new function qpci_msix_test_clear_pending() is added for tests that
do want the PBA clearing, and it will be used by XHCI and e1000e/igb
tests in subsequent changes.

Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
Cc: Dmitry Fleytman <dmitry.fleytman@gmail.com>
Cc: Akihiko Odaki <akihiko.odaki@daynix.com>
Cc: Sriram Yagnaraman <sriram.yagnaraman@ericsson.com>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 tests/qtest/libqos/pci.h |  1 +
 hw/pci/msix.c            | 16 ++++++++++++++++
 tests/qtest/libqos/pci.c | 20 +++++++++++++++++---
 3 files changed, 34 insertions(+), 3 deletions(-)

diff --git a/tests/qtest/libqos/pci.h b/tests/qtest/libqos/pci.h
index XXXXXXX..XXXXXXX 100644
--- a/tests/qtest/libqos/pci.h
+++ b/tests/qtest/libqos/pci.h
@@ -XXX,XX +XXX,XX @@ uint8_t qpci_find_capability(QPCIDevice *dev, uint8_t id, uint8_t start_addr);
 void qpci_msix_enable(QPCIDevice *dev);
 void qpci_msix_disable(QPCIDevice *dev);
 bool qpci_msix_pending(QPCIDevice *dev, uint16_t entry);
+bool qpci_msix_test_clear_pending(QPCIDevice *dev, uint16_t entry);
 bool qpci_msix_masked(QPCIDevice *dev, uint16_t entry);
 uint16_t qpci_msix_table_size(QPCIDevice *dev);
 
diff --git a/hw/pci/msix.c b/hw/pci/msix.c
index XXXXXXX..XXXXXXX 100644
--- a/hw/pci/msix.c
+++ b/hw/pci/msix.c
@@ -XXX,XX +XXX,XX @@ static uint64_t msix_pba_mmio_read(void *opaque, hwaddr addr,
 static void msix_pba_mmio_write(void *opaque, hwaddr addr,
                                 uint64_t val, unsigned size)
 {
+    PCIDevice *dev = opaque;
+    unsigned vector_start = addr * 8;
+    unsigned vector_end = MIN(addr + size * 8, dev->msix_entries_nr);
+    unsigned i;
+
+    for (i = vector_start; i < vector_end; i++) {
+        if ((val >> i) & 1) {
+            if (!msix_is_pending(dev, i)) {
+                msix_notify(dev, i);
+            }
+        } else {
+            if (msix_is_pending(dev, i)) {
+                msix_clr_pending(dev, i);
+            }
+        }
+    }
 }
 
 static const MemoryRegionOps msix_pba_mmio_ops = {
diff --git a/tests/qtest/libqos/pci.c b/tests/qtest/libqos/pci.c
index XXXXXXX..XXXXXXX 100644
--- a/tests/qtest/libqos/pci.c
+++ b/tests/qtest/libqos/pci.c
@@ -XXX,XX +XXX,XX @@ bool qpci_msix_pending(QPCIDevice *dev, uint16_t entry)
 
     g_assert(dev->msix_enabled);
     pba_entry = qpci_io_readl(dev, dev->msix_pba_bar, dev->msix_pba_off + off);
-    qpci_io_writel(dev, dev->msix_pba_bar, dev->msix_pba_off + off,
-                   pba_entry & ~(1 << bit_n));
-    return (pba_entry & (1 << bit_n)) != 0;
+    return pba_entry & (1 << bit_n);
+}
+
+bool qpci_msix_test_clear_pending(QPCIDevice *dev, uint16_t entry)
+{
+    uint32_t pba_entry;
+    uint8_t bit_n = entry % 32;
+    uint64_t  off = (entry / 32) * PCI_MSIX_ENTRY_SIZE / 4;
+
+    g_assert(dev->msix_enabled);
+    pba_entry = qpci_io_readl(dev, dev->msix_pba_bar, dev->msix_pba_off + off);
+    if (pba_entry & (1 << bit_n)) {
+        qpci_io_writel(dev, dev->msix_pba_bar, dev->msix_pba_off + off,
+                       pba_entry & ~(1 << bit_n));
+        return true;
+    }
+    return false;
 }
 
 bool qpci_msix_masked(QPCIDevice *dev, uint16_t entry)
-- 
2.45.2

The e1000e and igb tests don't clear the msix pending bit after waiting
for it sit is masked so the irq doesn't get delivered. Failing to clear
the pending interrupt means all subsequent waits for interrupt after the
first do not actually wait for an interrupt genreated by the device.

Explicitly clearing the msix pending bit results in the
multiple-transfers test hanging waiting for the second interrupt. This
happens because the e1000e and igb tests do not clear (or set
auto-clear) on queue interrupts, so the cause remains ste in ICR/EICR,
which inhibits triggering of a new interrupt.

Fix both these problems. Clear the msix pending bit explicitly after
waiting for it; and clear the ICR/EICR cause bits after seeing and
interrupt (also verify we saw the correct cause bit).

Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
Cc: Dmitry Fleytman <dmitry.fleytman@gmail.com>
Cc: Akihiko Odaki <akihiko.odaki@daynix.com>
Cc: Sriram Yagnaraman <sriram.yagnaraman@ericsson.com>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 tests/qtest/e1000e-test.c   | 8 ++++++--
 tests/qtest/igb-test.c      | 8 ++++++--
 tests/qtest/libqos/e1000e.c | 2 +-
 3 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/tests/qtest/e1000e-test.c b/tests/qtest/e1000e-test.c
index XXXXXXX..XXXXXXX 100644
--- a/tests/qtest/e1000e-test.c
+++ b/tests/qtest/e1000e-test.c
@@ -XXX,XX +XXX,XX @@ static void e1000e_send_verify(QE1000E *d, int *test_sockets, QGuestAllocator *a
     /* Put descriptor to the ring */
     e1000e_tx_ring_push(d, &descr);
 
-    /* Wait for TX WB interrupt */
+    /* Wait for TX WB interrupt (this clears the MSIX PBA) */
     e1000e_wait_isr(d, E1000E_TX0_MSG_ID);
+    /* Read ICR which clears it ready for next interrupt, assert TXQ0 cause */
+    g_assert(e1000e_macreg_read(d, E1000_ICR) & E1000_ICR_TXQ0);
 
     /* Check DD bit */
     g_assert_cmphex(le32_to_cpu(descr.upper.data) & E1000_TXD_STAT_DD, ==,
@@ -XXX,XX +XXX,XX @@ static void e1000e_receive_verify(QE1000E *d, int *test_sockets, QGuestAllocator
     /* Put descriptor to the ring */
     e1000e_rx_ring_push(d, &descr);
 
-    /* Wait for TX WB interrupt */
+    /* Wait for TX WB interrupt (this clears the MSIX PBA) */
     e1000e_wait_isr(d, E1000E_RX0_MSG_ID);
+    /* Read ICR which clears it ready for next interrupt, assert RXQ0 cause */
+    g_assert(e1000e_macreg_read(d, E1000_ICR) & E1000_ICR_RXQ0);
 
     /* Check DD bit */
     g_assert_cmphex(le32_to_cpu(descr.wb.upper.status_error) &
diff --git a/tests/qtest/igb-test.c b/tests/qtest/igb-test.c
index XXXXXXX..XXXXXXX 100644
--- a/tests/qtest/igb-test.c
+++ b/tests/qtest/igb-test.c
@@ -XXX,XX +XXX,XX @@ static void igb_send_verify(QE1000E *d, int *test_sockets, QGuestAllocator *allo
     /* Put descriptor to the ring */
     e1000e_tx_ring_push(d, &descr);
 
-    /* Wait for TX WB interrupt */
+    /* Wait for TX WB interrupt (this clears the MSIX PBA) */
     e1000e_wait_isr(d, E1000E_TX0_MSG_ID);
+    /* Read EICR which clears it ready for next interrupt, assert TXQ0 cause */
+    g_assert(e1000e_macreg_read(d, E1000_EICR) & (1 << E1000E_TX0_MSG_ID));
 
     /* Check DD bit */
     g_assert_cmphex(le32_to_cpu(descr.wb.status) & E1000_TXD_STAT_DD, ==,
@@ -XXX,XX +XXX,XX @@ static void igb_receive_verify(QE1000E *d, int *test_sockets, QGuestAllocator *a
     /* Put descriptor to the ring */
     e1000e_rx_ring_push(d, &descr);
 
-    /* Wait for TX WB interrupt */
+    /* Wait for TX WB interrupt (this clears the MSIX PBA) */
     e1000e_wait_isr(d, E1000E_RX0_MSG_ID);
+    /* Read EICR which clears it ready for next interrupt, assert RXQ0 cause */
+    g_assert(e1000e_macreg_read(d, E1000_EICR) & (1 << E1000E_RX0_MSG_ID));
 
     /* Check DD bit */
     g_assert_cmphex(le32_to_cpu(descr.wb.upper.status_error) &
diff --git a/tests/qtest/libqos/e1000e.c b/tests/qtest/libqos/e1000e.c
index XXXXXXX..XXXXXXX 100644
--- a/tests/qtest/libqos/e1000e.c
+++ b/tests/qtest/libqos/e1000e.c
@@ -XXX,XX +XXX,XX @@ void e1000e_wait_isr(QE1000E *d, uint16_t msg_id)
     guint64 end_time = g_get_monotonic_time() + 5 * G_TIME_SPAN_SECOND;
 
     do {
-        if (qpci_msix_pending(&d_pci->pci_dev, msg_id)) {
+        if (qpci_msix_test_clear_pending(&d_pci->pci_dev, msg_id)) {
             return;
         }
         qtest_clock_step(d_pci->pci_dev.bus->qts, 10000);
-- 
2.45.2

Prepare to use some of these constants in xhci qtest code.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 hw/usb/hcd-xhci.h | 190 +++++++++++++++++++++++++++++++
 hw/usb/hcd-xhci.c | 283 ++++++++++------------------------------------
 2 files changed, 250 insertions(+), 223 deletions(-)

diff --git a/hw/usb/hcd-xhci.h b/hw/usb/hcd-xhci.h
index XXXXXXX..XXXXXXX 100644
--- a/hw/usb/hcd-xhci.h
+++ b/hw/usb/hcd-xhci.h
@@ -XXX,XX +XXX,XX @@ typedef enum TRBCCode {
     CC_SPLIT_TRANSACTION_ERROR
 } TRBCCode;
 
+/* Register definitions */
+#define XHCI_HCCAP_CAPLENGTH     0x00
+#define XHCI_HCCAP_HCIVERSION    0x02
+#define XHCI_HCCAP_HCSPARAMS1    0x04
+#define XHCI_HCCAP_HCSPARAMS2    0x08
+#define XHCI_HCCAP_HCSPARAMS3    0x0C
+#define XHCI_HCCAP_HCCPARAMS1    0x10
+#define XHCI_HCCAP_DBOFF         0x14
+#define XHCI_HCCAP_RTSOFF        0x18
+#define XHCI_HCCAP_HCCPARAMS2    0x1C
+#define XHCI_HCCAP_EXTCAP_START  0x20 /* SW-defined */
+
+#define XHCI_PORT_PORTSC         0x00
+#define   PORTSC_CCS             (1 << 0)
+#define   PORTSC_PED             (1 << 1)
+#define   PORTSC_OCA             (1 << 3)
+#define   PORTSC_PR              (1 << 4)
+#define   PORTSC_PLS_SHIFT           5
+#define   PORTSC_PLS_MASK        0xf
+#define   PORTSC_PP              (1 << 9)
+#define   PORTSC_SPEED_SHIFT        10
+#define   PORTSC_SPEED_MASK      0xf
+#define   PORTSC_SPEED_FULL      (1 << 10)
+#define   PORTSC_SPEED_LOW       (2 << 10)
+#define   PORTSC_SPEED_HIGH      (3 << 10)
+#define   PORTSC_SPEED_SUPER     (4 << 10)
+#define   PORTSC_PIC_SHIFT          14
+#define   PORTSC_PIC_MASK        0x3
+#define   PORTSC_LWS             (1 << 16)
+#define   PORTSC_CSC             (1 << 17)
+#define   PORTSC_PEC             (1 << 18)
+#define   PORTSC_WRC             (1 << 19)
+#define   PORTSC_OCC             (1 << 20)
+#define   PORTSC_PRC             (1 << 21)
+#define   PORTSC_PLC             (1 << 22)
+#define   PORTSC_CEC             (1 << 23)
+#define   PORTSC_CAS             (1 << 24)
+#define   PORTSC_WCE             (1 << 25)
+#define   PORTSC_WDE             (1 << 26)
+#define   PORTSC_WOE             (1 << 27)
+#define   PORTSC_DR              (1 << 30)
+#define   PORTSC_WPR             (1 << 31)
+#define XHCI_PORT_PORTPMSC       0x04
+#define XHCI_PORT_PORTLI         0x08
+#define XHCI_PORT_PORTHLPMC      0x0C
+
+#define XHCI_OPER_USBCMD         0x00
+#define   USBCMD_RS              (1 << 0)
+#define   USBCMD_HCRST           (1 << 1)
+#define   USBCMD_INTE            (1 << 2)
+#define   USBCMD_HSEE            (1 << 3)
+#define   USBCMD_LHCRST          (1 << 7)
+#define   USBCMD_CSS             (1 << 8)
+#define   USBCMD_CRS             (1 << 9)
+#define   USBCMD_EWE             (1 << 10)
+#define   USBCMD_EU3S            (1 << 11)
+#define XHCI_OPER_USBSTS         0x04
+#define   USBSTS_HCH             (1 << 0)
+#define   USBSTS_HSE             (1 << 2)
+#define   USBSTS_EINT            (1 << 3)
+#define   USBSTS_PCD             (1 << 4)
+#define   USBSTS_SSS             (1 << 8)
+#define   USBSTS_RSS             (1 << 9)
+#define   USBSTS_SRE             (1 << 10)
+#define   USBSTS_CNR             (1 << 11)
+#define   USBSTS_HCE             (1 << 12)
+#define XHCI_OPER_PAGESIZE       0x08
+#define XHCI_OPER_DNCTRL         0x14
+#define XHCI_OPER_CRCR_LO        0x18
+#define   CRCR_RCS              (1 << 0)
+#define   CRCR_CS               (1 << 1)
+#define   CRCR_CA               (1 << 2)
+#define   CRCR_CRR              (1 << 3)
+#define XHCI_OPER_CRCR_HI        0x1C
+#define XHCI_OPER_DCBAAP_LO      0x30
+#define XHCI_OPER_DCBAAP_HI      0x34
+#define XHCI_OPER_CONFIG         0x38
+
+#define XHCI_OPER_MFINDEX        0x00
+#define XHCI_OPER_IR0            0x20
+#define XHCI_OPER_IR_SZ          0x20
+
+#define XHCI_INTR_IMAN           0x00
+#define   IMAN_IP                (1 << 0)
+#define   IMAN_IE                (1 << 1)
+#define XHCI_INTR_IMOD           0x04
+#define XHCI_INTR_ERSTSZ         0x08
+#define XHCI_INTR_ERSTBA_LO      0x10
+#define XHCI_INTR_ERSTBA_HI      0x14
+#define XHCI_INTR_ERDP_LO        0x18
+#define   ERDP_EHB               (1 << 3)
+#define XHCI_INTR_ERDP_HI        0x1C
+
+#define TRB_SIZE 16
+typedef struct XHCITRB {
+    uint64_t parameter;
+    uint32_t status;
+    uint32_t control;
+    dma_addr_t addr;
+    bool ccs;
+} XHCITRB;
+
+enum {
+    PLS_U0              =  0,
+    PLS_U1              =  1,
+    PLS_U2              =  2,
+    PLS_U3              =  3,
+    PLS_DISABLED        =  4,
+    PLS_RX_DETECT       =  5,
+    PLS_INACTIVE        =  6,
+    PLS_POLLING         =  7,
+    PLS_RECOVERY        =  8,
+    PLS_HOT_RESET       =  9,
+    PLS_COMPILANCE_MODE = 10,
+    PLS_TEST_MODE       = 11,
+    PLS_RESUME          = 15,
+};
+
+#define CR_LINK TR_LINK
+
+#define TRB_C               (1 << 0)
+#define TRB_TYPE_SHIFT          10
+#define TRB_TYPE_MASK       0x3f
+#define TRB_TYPE(t)         (((t).control >> TRB_TYPE_SHIFT) & TRB_TYPE_MASK)
+
+#define TRB_EV_ED           (1 << 2)
+
+#define TRB_TR_ENT          (1 << 1)
+#define TRB_TR_ISP          (1 << 2)
+#define TRB_TR_NS           (1 << 3)
+#define TRB_TR_CH           (1 << 4)
+#define TRB_TR_IOC          (1 << 5)
+#define TRB_TR_IDT          (1 << 6)
+#define TRB_TR_TBC_SHIFT        7
+#define TRB_TR_TBC_MASK     0x3
+#define TRB_TR_BEI          (1 << 9)
+#define TRB_TR_TLBPC_SHIFT      16
+#define TRB_TR_TLBPC_MASK   0xf
+#define TRB_TR_FRAMEID_SHIFT    20
+#define TRB_TR_FRAMEID_MASK 0x7ff
+#define TRB_TR_SIA          (1 << 31)
+
+#define TRB_TR_DIR          (1 << 16)
+
+#define TRB_CR_SLOTID_SHIFT     24
+#define TRB_CR_SLOTID_MASK  0xff
+#define TRB_CR_EPID_SHIFT       16
+#define TRB_CR_EPID_MASK    0x1f
+
+#define TRB_CR_BSR          (1 << 9)
+#define TRB_CR_DC           (1 << 9)
+
+#define TRB_LK_TC           (1 << 1)
+
+#define TRB_INTR_SHIFT          22
+#define TRB_INTR_MASK       0x3ff
+#define TRB_INTR(t)         (((t).status >> TRB_INTR_SHIFT) & TRB_INTR_MASK)
+
+#define EP_TYPE_MASK        0x7
+#define EP_TYPE_SHIFT           3
+
+#define EP_STATE_MASK       0x7
+#define EP_DISABLED         (0 << 0)
+#define EP_RUNNING          (1 << 0)
+#define EP_HALTED           (2 << 0)
+#define EP_STOPPED          (3 << 0)
+#define EP_ERROR            (4 << 0)
+
+#define SLOT_STATE_MASK     0x1f
+#define SLOT_STATE_SHIFT        27
+#define SLOT_STATE(s)       (((s) >> SLOT_STATE_SHIFT) & SLOT_STATE_MASK)
+#define SLOT_ENABLED        0
+#define SLOT_DEFAULT        1
+#define SLOT_ADDRESSED      2
+#define SLOT_CONFIGURED     3
+
+#define SLOT_CONTEXT_ENTRIES_MASK 0x1f
+#define SLOT_CONTEXT_ENTRIES_SHIFT 27
+
+typedef enum EPType {
+    ET_INVALID = 0,
+    ET_ISO_OUT,
+    ET_BULK_OUT,
+    ET_INTR_OUT,
+    ET_CONTROL,
+    ET_ISO_IN,
+    ET_BULK_IN,
+    ET_INTR_IN,
+} EPType;
+
 typedef struct XHCIRing {
     dma_addr_t dequeue;
     bool ccs;
diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index XXXXXXX..XXXXXXX 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -XXX,XX +XXX,XX @@
 # error Increase XHCI_LEN_REGS
 #endif
 
-/* bit definitions */
-#define USBCMD_RS       (1<<0)
-#define USBCMD_HCRST    (1<<1)
-#define USBCMD_INTE     (1<<2)
-#define USBCMD_HSEE     (1<<3)
-#define USBCMD_LHCRST   (1<<7)
-#define USBCMD_CSS      (1<<8)
-#define USBCMD_CRS      (1<<9)
-#define USBCMD_EWE      (1<<10)
-#define USBCMD_EU3S     (1<<11)
-
-#define USBSTS_HCH      (1<<0)
-#define USBSTS_HSE      (1<<2)
-#define USBSTS_EINT     (1<<3)
-#define USBSTS_PCD      (1<<4)
-#define USBSTS_SSS      (1<<8)
-#define USBSTS_RSS      (1<<9)
-#define USBSTS_SRE      (1<<10)
-#define USBSTS_CNR      (1<<11)
-#define USBSTS_HCE      (1<<12)
-
-
-#define PORTSC_CCS          (1<<0)
-#define PORTSC_PED          (1<<1)
-#define PORTSC_OCA          (1<<3)
-#define PORTSC_PR           (1<<4)
-#define PORTSC_PLS_SHIFT        5
-#define PORTSC_PLS_MASK     0xf
-#define PORTSC_PP           (1<<9)
-#define PORTSC_SPEED_SHIFT      10
-#define PORTSC_SPEED_MASK   0xf
-#define PORTSC_SPEED_FULL   (1<<10)
-#define PORTSC_SPEED_LOW    (2<<10)
-#define PORTSC_SPEED_HIGH   (3<<10)
-#define PORTSC_SPEED_SUPER  (4<<10)
-#define PORTSC_PIC_SHIFT        14
-#define PORTSC_PIC_MASK     0x3
-#define PORTSC_LWS          (1<<16)
-#define PORTSC_CSC          (1<<17)
-#define PORTSC_PEC          (1<<18)
-#define PORTSC_WRC          (1<<19)
-#define PORTSC_OCC          (1<<20)
-#define PORTSC_PRC          (1<<21)
-#define PORTSC_PLC          (1<<22)
-#define PORTSC_CEC          (1<<23)
-#define PORTSC_CAS          (1<<24)
-#define PORTSC_WCE          (1<<25)
-#define PORTSC_WDE          (1<<26)
-#define PORTSC_WOE          (1<<27)
-#define PORTSC_DR           (1<<30)
-#define PORTSC_WPR          (1<<31)
-
-#define CRCR_RCS        (1<<0)
-#define CRCR_CS         (1<<1)
-#define CRCR_CA         (1<<2)
-#define CRCR_CRR        (1<<3)
-
-#define IMAN_IP         (1<<0)
-#define IMAN_IE         (1<<1)
-
-#define ERDP_EHB        (1<<3)
-
-#define TRB_SIZE 16
-typedef struct XHCITRB {
-    uint64_t parameter;
-    uint32_t status;
-    uint32_t control;
-    dma_addr_t addr;
-    bool ccs;
-} XHCITRB;
-
-enum {
-    PLS_U0              =  0,
-    PLS_U1              =  1,
-    PLS_U2              =  2,
-    PLS_U3              =  3,
-    PLS_DISABLED        =  4,
-    PLS_RX_DETECT       =  5,
-    PLS_INACTIVE        =  6,
-    PLS_POLLING         =  7,
-    PLS_RECOVERY        =  8,
-    PLS_HOT_RESET       =  9,
-    PLS_COMPILANCE_MODE = 10,
-    PLS_TEST_MODE       = 11,
-    PLS_RESUME          = 15,
-};
-
-#define CR_LINK TR_LINK
-
-#define TRB_C               (1<<0)
-#define TRB_TYPE_SHIFT          10
-#define TRB_TYPE_MASK       0x3f
-#define TRB_TYPE(t)         (((t).control >> TRB_TYPE_SHIFT) & TRB_TYPE_MASK)
-
-#define TRB_EV_ED           (1<<2)
-
-#define TRB_TR_ENT          (1<<1)
-#define TRB_TR_ISP          (1<<2)
-#define TRB_TR_NS           (1<<3)
-#define TRB_TR_CH           (1<<4)
-#define TRB_TR_IOC          (1<<5)
-#define TRB_TR_IDT          (1<<6)
-#define TRB_TR_TBC_SHIFT        7
-#define TRB_TR_TBC_MASK     0x3
-#define TRB_TR_BEI          (1<<9)
-#define TRB_TR_TLBPC_SHIFT      16
-#define TRB_TR_TLBPC_MASK   0xf
-#define TRB_TR_FRAMEID_SHIFT    20
-#define TRB_TR_FRAMEID_MASK 0x7ff
-#define TRB_TR_SIA          (1<<31)
-
-#define TRB_TR_DIR          (1<<16)
-
-#define TRB_CR_SLOTID_SHIFT     24
-#define TRB_CR_SLOTID_MASK  0xff
-#define TRB_CR_EPID_SHIFT       16
-#define TRB_CR_EPID_MASK    0x1f
-
-#define TRB_CR_BSR          (1<<9)
-#define TRB_CR_DC           (1<<9)
-
-#define TRB_LK_TC           (1<<1)
-
-#define TRB_INTR_SHIFT          22
-#define TRB_INTR_MASK       0x3ff
-#define TRB_INTR(t)         (((t).status >> TRB_INTR_SHIFT) & TRB_INTR_MASK)
-
-#define EP_TYPE_MASK        0x7
-#define EP_TYPE_SHIFT           3
-
-#define EP_STATE_MASK       0x7
-#define EP_DISABLED         (0<<0)
-#define EP_RUNNING          (1<<0)
-#define EP_HALTED           (2<<0)
-#define EP_STOPPED          (3<<0)
-#define EP_ERROR            (4<<0)
-
-#define SLOT_STATE_MASK     0x1f
-#define SLOT_STATE_SHIFT        27
-#define SLOT_STATE(s)       (((s)>>SLOT_STATE_SHIFT)&SLOT_STATE_MASK)
-#define SLOT_ENABLED        0
-#define SLOT_DEFAULT        1
-#define SLOT_ADDRESSED      2
-#define SLOT_CONFIGURED     3
-
-#define SLOT_CONTEXT_ENTRIES_MASK 0x1f
-#define SLOT_CONTEXT_ENTRIES_SHIFT 27
-
 #define get_field(data, field)                  \
     (((data) >> field##_SHIFT) & field##_MASK)
 
@@ -XXX,XX +XXX,XX @@ enum {
         *data = val_;                                           \
     } while (0)
 
-typedef enum EPType {
-    ET_INVALID = 0,
-    ET_ISO_OUT,
-    ET_BULK_OUT,
-    ET_INTR_OUT,
-    ET_CONTROL,
-    ET_ISO_IN,
-    ET_BULK_IN,
-    ET_INTR_IN,
-} EPType;
-
 typedef struct XHCITransfer {
     XHCIEPContext *epctx;
     USBPacket packet;
@@ -XXX,XX +XXX,XX @@ static uint64_t xhci_cap_read(void *ptr, hwaddr reg, unsigned size)
     uint32_t ret;
 
     switch (reg) {
-    case 0x00: /* HCIVERSION, CAPLENGTH */
+    case XHCI_HCCAP_CAPLENGTH: /* Covers HCIVERSION and CAPLENGTH */
         ret = 0x01000000 | LEN_CAP;
         break;
-    case 0x04: /* HCSPARAMS 1 */
+    case XHCI_HCCAP_HCSPARAMS1:
         ret = ((xhci->numports_2+xhci->numports_3)<<24)
             | (xhci->numintrs<<8) | xhci->numslots;
         break;
-    case 0x08: /* HCSPARAMS 2 */
+    case XHCI_HCCAP_HCSPARAMS2:
         ret = 0x0000000f;
         break;
-    case 0x0c: /* HCSPARAMS 3 */
+    case XHCI_HCCAP_HCSPARAMS3:
         ret = 0x00000000;
         break;
-    case 0x10: /* HCCPARAMS */
-        if (sizeof(dma_addr_t) == 4) {
-            ret = 0x00080000 | (xhci->max_pstreams_mask << 12);
-        } else {
-            ret = 0x00080001 | (xhci->max_pstreams_mask << 12);
+    case XHCI_HCCAP_HCCPARAMS1:
+        ret = (XHCI_HCCAP_EXTCAP_START >> 2) | (xhci->max_pstreams_mask << 12);
+        if (sizeof(dma_addr_t) == 8) {
+            ret |= 0x00000001; /* AC64 */
         }
         break;
-    case 0x14: /* DBOFF */
+    case XHCI_HCCAP_DBOFF:
         ret = OFF_DOORBELL;
         break;
-    case 0x18: /* RTSOFF */
+    case XHCI_HCCAP_RTSOFF:
         ret = OFF_RUNTIME;
         break;
 
     /* extended capabilities */
-    case 0x20: /* Supported Protocol:00 */
+    case XHCI_HCCAP_EXTCAP_START + 0x00: /* Supported Protocol:00 */
         ret = 0x02000402; /* USB 2.0 */
         break;
-    case 0x24: /* Supported Protocol:04 */
+    case XHCI_HCCAP_EXTCAP_START + 0x04: /* Supported Protocol:04 */
         ret = 0x20425355; /* "USB " */
         break;
-    case 0x28: /* Supported Protocol:08 */
+    case XHCI_HCCAP_EXTCAP_START + 0x08: /* Supported Protocol:08 */
         ret = (xhci->numports_2 << 8) | (xhci->numports_3 + 1);
         break;
-    case 0x2c: /* Supported Protocol:0c */
+    case XHCI_HCCAP_EXTCAP_START + 0x0c: /* Supported Protocol:0c */
         ret = 0x00000000; /* reserved */
         break;
-    case 0x30: /* Supported Protocol:00 */
+    case XHCI_HCCAP_EXTCAP_START + 0x10: /* Supported Protocol:00 */
         ret = 0x03000002; /* USB 3.0 */
         break;
-    case 0x34: /* Supported Protocol:04 */
+    case XHCI_HCCAP_EXTCAP_START + 0x14: /* Supported Protocol:04 */
         ret = 0x20425355; /* "USB " */
         break;
-    case 0x38: /* Supported Protocol:08 */
+    case XHCI_HCCAP_EXTCAP_START + 0x18: /* Supported Protocol:08 */
         ret = (xhci->numports_3 << 8) | 1;
         break;
-    case 0x3c: /* Supported Protocol:0c */
+    case XHCI_HCCAP_EXTCAP_START + 0x1c: /* Supported Protocol:0c */
         ret = 0x00000000; /* reserved */
         break;
     default:
@@ -XXX,XX +XXX,XX @@ static uint64_t xhci_port_read(void *ptr, hwaddr reg, unsigned size)
     uint32_t ret;
 
     switch (reg) {
-    case 0x00: /* PORTSC */
+    case XHCI_PORT_PORTSC:
         ret = port->portsc;
         break;
-    case 0x04: /* PORTPMSC */
-    case 0x08: /* PORTLI */
+    case XHCI_PORT_PORTPMSC:
+    case XHCI_PORT_PORTLI:
         ret = 0;
         break;
-    case 0x0c: /* reserved */
     default:
         trace_usb_xhci_unimplemented("port read", reg);
         ret = 0;
@@ -XXX,XX +XXX,XX @@ static void xhci_port_write(void *ptr, hwaddr reg,
     trace_usb_xhci_port_write(port->portnr, reg, val);
 
     switch (reg) {
-    case 0x00: /* PORTSC */
+    case XHCI_PORT_PORTSC:
         /* write-1-to-start bits */
         if (val & PORTSC_WPR) {
             xhci_port_reset(port, true);
@@ -XXX,XX +XXX,XX @@ static void xhci_port_write(void *ptr, hwaddr reg,
             xhci_port_notify(port, notify);
         }
         break;
-    case 0x04: /* PORTPMSC */
-    case 0x08: /* PORTLI */
     default:
         trace_usb_xhci_unimplemented("port write", reg);
     }
@@ -XXX,XX +XXX,XX @@ static uint64_t xhci_oper_read(void *ptr, hwaddr reg, unsigned size)
     uint32_t ret;
 
     switch (reg) {
-    case 0x00: /* USBCMD */
+    case XHCI_OPER_USBCMD:
         ret = xhci->usbcmd;
         break;
-    case 0x04: /* USBSTS */
+    case XHCI_OPER_USBSTS:
         ret = xhci->usbsts;
         break;
-    case 0x08: /* PAGESIZE */
+    case XHCI_OPER_PAGESIZE:
         ret = 1; /* 4KiB */
         break;
-    case 0x14: /* DNCTRL */
+    case XHCI_OPER_DNCTRL:
         ret = xhci->dnctrl;
         break;
-    case 0x18: /* CRCR low */
+    case XHCI_OPER_CRCR_LO:
         ret = xhci->crcr_low & ~0xe;
         break;
-    case 0x1c: /* CRCR high */
+    case XHCI_OPER_CRCR_HI:
         ret = xhci->crcr_high;
         break;
-    case 0x30: /* DCBAAP low */
+    case XHCI_OPER_DCBAAP_LO:
         ret = xhci->dcbaap_low;
         break;
-    case 0x34: /* DCBAAP high */
+    case XHCI_OPER_DCBAAP_HI:
         ret = xhci->dcbaap_high;
         break;
-    case 0x38: /* CONFIG */
+    case XHCI_OPER_CONFIG:
         ret = xhci->config;
         break;
     default:
@@ -XXX,XX +XXX,XX @@ static void xhci_oper_write(void *ptr, hwaddr reg,
     trace_usb_xhci_oper_write(reg, val);
 
     switch (reg) {
-    case 0x00: /* USBCMD */
+    case XHCI_OPER_USBCMD:
         if ((val & USBCMD_RS) && !(xhci->usbcmd & USBCMD_RS)) {
             xhci_run(xhci);
         } else if (!(val & USBCMD_RS) && (xhci->usbcmd & USBCMD_RS)) {
@@ -XXX,XX +XXX,XX @@ static void xhci_oper_write(void *ptr, hwaddr reg,
         xhci_intr_update(xhci, 0);
         break;
 
-    case 0x04: /* USBSTS */
+    case XHCI_OPER_USBSTS:
         /* these bits are write-1-to-clear */
         xhci->usbsts &= ~(val & (USBSTS_HSE|USBSTS_EINT|USBSTS_PCD|USBSTS_SRE));
         xhci_intr_update(xhci, 0);
         break;
 
-    case 0x14: /* DNCTRL */
+    case XHCI_OPER_DNCTRL:
         xhci->dnctrl = val & 0xffff;
         break;
-    case 0x18: /* CRCR low */
+    case XHCI_OPER_CRCR_LO:
         xhci->crcr_low = (val & 0xffffffcf) | (xhci->crcr_low & CRCR_CRR);
         break;
-    case 0x1c: /* CRCR high */
+    case XHCI_OPER_CRCR_HI:
         xhci->crcr_high = val;
         if (xhci->crcr_low & (CRCR_CA|CRCR_CS) && (xhci->crcr_low & CRCR_CRR)) {
             XHCIEvent event = {ER_COMMAND_COMPLETE, CC_COMMAND_RING_STOPPED};
@@ -XXX,XX +XXX,XX @@ static void xhci_oper_write(void *ptr, hwaddr reg,
         }
         xhci->crcr_low &= ~(CRCR_CA | CRCR_CS);
         break;
-    case 0x30: /* DCBAAP low */
+    case XHCI_OPER_DCBAAP_LO:
         xhci->dcbaap_low = val & 0xffffffc0;
         break;
-    case 0x34: /* DCBAAP high */
+    case XHCI_OPER_DCBAAP_HI:
         xhci->dcbaap_high = val;
         break;
-    case 0x38: /* CONFIG */
+    case XHCI_OPER_CONFIG:
         xhci->config = val & 0xff;
         break;
     default:
@@ -XXX,XX +XXX,XX @@ static uint64_t xhci_runtime_read(void *ptr, hwaddr reg,
     XHCIState *xhci = ptr;
     uint32_t ret = 0;
 
-    if (reg < 0x20) {
+    if (reg < XHCI_OPER_IR0) {
         switch (reg) {
-        case 0x00: /* MFINDEX */
+        case XHCI_OPER_MFINDEX:
             ret = xhci_mfindex_get(xhci) & 0x3fff;
             break;
         default:
@@ -XXX,XX +XXX,XX @@ static uint64_t xhci_runtime_read(void *ptr, hwaddr reg,
             break;
         }
     } else {
-        int v = (reg - 0x20) / 0x20;
+        int v = (reg - XHCI_OPER_IR0) / XHCI_OPER_IR_SZ;
         XHCIInterrupter *intr = &xhci->intr[v];
-        switch (reg & 0x1f) {
-        case 0x00: /* IMAN */
+        switch (reg & (XHCI_OPER_IR_SZ - 1)) {
+        case XHCI_INTR_IMAN:
             ret = intr->iman;
             break;
-        case 0x04: /* IMOD */
+        case XHCI_INTR_IMOD:
             ret = intr->imod;
             break;
-        case 0x08: /* ERSTSZ */
+        case XHCI_INTR_ERSTSZ:
             ret = intr->erstsz;
             break;
-        case 0x10: /* ERSTBA low */
+        case XHCI_INTR_ERSTBA_LO:
             ret = intr->erstba_low;
             break;
-        case 0x14: /* ERSTBA high */
+        case XHCI_INTR_ERSTBA_HI:
             ret = intr->erstba_high;
             break;
-        case 0x18: /* ERDP low */
+        case XHCI_INTR_ERDP_LO:
             ret = intr->erdp_low;
             break;
-        case 0x1c: /* ERDP high */
+        case XHCI_INTR_ERDP_HI:
             ret = intr->erdp_high;
             break;
         }
@@ -XXX,XX +XXX,XX @@ static void xhci_runtime_write(void *ptr, hwaddr reg,
 
     trace_usb_xhci_runtime_write(reg, val);
 
-    if (reg < 0x20) {
+    if (reg < XHCI_OPER_IR0) {
         trace_usb_xhci_unimplemented("runtime write", reg);
         return;
     }
-    v = (reg - 0x20) / 0x20;
+    v = (reg - XHCI_OPER_IR0) / XHCI_OPER_IR_SZ;
     intr = &xhci->intr[v];
 
-    switch (reg & 0x1f) {
-    case 0x00: /* IMAN */
+    switch (reg & (XHCI_OPER_IR_SZ - 1)) {
+    case XHCI_INTR_IMAN:
         if (val & IMAN_IP) {
             intr->iman &= ~IMAN_IP;
         }
@@ -XXX,XX +XXX,XX @@ static void xhci_runtime_write(void *ptr, hwaddr reg,
         intr->iman |= val & IMAN_IE;
         xhci_intr_update(xhci, v);
         break;
-    case 0x04: /* IMOD */
+    case XHCI_INTR_IMOD:
         intr->imod = val;
         break;
-    case 0x08: /* ERSTSZ */
+    case XHCI_INTR_ERSTSZ:
         intr->erstsz = val & 0xffff;
         break;
-    case 0x10: /* ERSTBA low */
+    case XHCI_INTR_ERSTBA_LO:
         if (xhci->nec_quirks) {
             /* NEC driver bug: it doesn't align this to 64 bytes */
             intr->erstba_low = val & 0xfffffff0;
@@ -XXX,XX +XXX,XX @@ static void xhci_runtime_write(void *ptr, hwaddr reg,
             intr->erstba_low = val & 0xffffffc0;
         }
         break;
-    case 0x14: /* ERSTBA high */
+    case XHCI_INTR_ERSTBA_HI:
         intr->erstba_high = val;
         xhci_er_reset(xhci, v);
         break;
-    case 0x18: /* ERDP low */
+    case XHCI_INTR_ERDP_LO:
         if (val & ERDP_EHB) {
             intr->erdp_low &= ~ERDP_EHB;
         }
@@ -XXX,XX +XXX,XX @@ static void xhci_runtime_write(void *ptr, hwaddr reg,
             }
         }
         break;
-    case 0x1c: /* ERDP high */
+    case XHCI_INTR_ERDP_HI:
         intr->erdp_high = val;
         break;
     default:
-- 
2.45.2

Add tests which init the host controller registers to the point where
command and event rings, irqs are operational. Enumerate ports and set
up an attached device context that enables device transfer ring to be
set up and tested.

This test does a bunch of things at once and is not yet well librified,
but it allows testing basic mechanisms and gives a starting point for
further work.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 hw/usb/hcd-xhci.h               |   7 +
 hw/usb/hcd-xhci.c               |   7 -
 tests/qtest/usb-hcd-xhci-test.c | 498 +++++++++++++++++++++++++++++++-
 3 files changed, 499 insertions(+), 13 deletions(-)

diff --git a/hw/usb/hcd-xhci.h b/hw/usb/hcd-xhci.h
index XXXXXXX..XXXXXXX 100644
--- a/hw/usb/hcd-xhci.h
+++ b/hw/usb/hcd-xhci.h
@@ -XXX,XX +XXX,XX @@ typedef struct XHCIRing {
     bool ccs;
 } XHCIRing;
 
+typedef struct XHCIEvRingSeg {
+    uint32_t addr_low;
+    uint32_t addr_high;
+    uint32_t size;
+    uint32_t rsvd;
+} XHCIEvRingSeg;
+
 typedef struct XHCIPort {
     XHCIState *xhci;
     uint32_t portsc;
diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index XXXXXXX..XXXXXXX 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -XXX,XX +XXX,XX @@ struct XHCIEPContext {
     QEMUTimer *kick_timer;
 };
 
-typedef struct XHCIEvRingSeg {
-    uint32_t addr_low;
-    uint32_t addr_high;
-    uint32_t size;
-    uint32_t rsvd;
-} XHCIEvRingSeg;
-
 static void xhci_kick_ep(XHCIState *xhci, unsigned int slotid,
                          unsigned int epid, unsigned int streamid);
 static void xhci_kick_epctx(XHCIEPContext *epctx, unsigned int streamid);
diff --git a/tests/qtest/usb-hcd-xhci-test.c b/tests/qtest/usb-hcd-xhci-test.c
index XXXXXXX..XXXXXXX 100644
--- a/tests/qtest/usb-hcd-xhci-test.c
+++ b/tests/qtest/usb-hcd-xhci-test.c
@@ -XXX,XX +XXX,XX @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/bswap.h"
+#include "libqtest.h"
+#include "libqos/libqos-pc.h"
 #include "libqtest-single.h"
 #include "libqos/usb.h"
+#include "hw/pci/pci_ids.h"
+#include "hw/pci/pci_regs.h"
+#include "hw/usb/hcd-xhci.h"
+
+/*** Test Setup & Teardown ***/
+typedef struct XHCIQSlotState {
+    /* In-memory arrays */
+    uint64_t device_context;
+    uint64_t transfer_ring;
+
+    uint32_t tr_trb_entries;
+    uint32_t tr_trb_idx;
+    uint32_t tr_trb_c;
+} XHCIQSlotState;
+
+typedef struct XHCIQState {
+    /* QEMU PCI variables */
+    QOSState *parent;
+    QPCIDevice *dev;
+    QPCIBar bar;
+    uint64_t barsize;
+    uint32_t fingerprint;
+
+    /* In-memory arrays */
+    uint64_t dc_base_array;
+    uint64_t command_ring;
+    uint64_t event_ring_seg;
+    uint64_t event_ring;
+
+    uint32_t cr_trb_entries;
+    uint32_t cr_trb_idx;
+    uint32_t cr_trb_c;
+    uint32_t er_trb_entries;
+    uint32_t er_trb_idx;
+    uint32_t er_trb_c;
+
+    /* Host controller properties */
+    uint32_t rtoff, dboff;
+    uint32_t maxports, maxslots, maxintrs;
+
+    XHCIQSlotState slots[32];
+} XHCIQState;
+
+#define XHCI_NEC_ID (PCI_DEVICE_ID_NEC_UPD720200 << 16 | \
+                     PCI_VENDOR_ID_NEC)
+
+/**
+ * Locate, verify, and return a handle to the XHCI device.
+ */
+static QPCIDevice *get_xhci_device(QTestState *qts, uint32_t *fingerprint)
+{
+    QPCIDevice *xhci;
+    uint32_t xhci_fingerprint;
+    QPCIBus *pcibus;
+
+    pcibus = qpci_new_pc(qts, NULL);
+
+    /* Find the XHCI PCI device and verify it's the right one. */
+    xhci = qpci_device_find(pcibus, QPCI_DEVFN(0x1D, 0x0));
+    g_assert(xhci != NULL);
+
+    xhci_fingerprint = qpci_config_readl(xhci, PCI_VENDOR_ID);
+    switch (xhci_fingerprint) {
+    case XHCI_NEC_ID:
+        break;
+    default:
+        /* Unknown device. */
+        g_assert_not_reached();
+    }
+
+    if (fingerprint) {
+        *fingerprint = xhci_fingerprint;
+    }
+    return xhci;
+}
+
+static void free_xhci_device(QPCIDevice *dev)
+{
+    QPCIBus *pcibus = dev ? dev->bus : NULL;
+
+    /* libqos doesn't have a function for this, so free it manually */
+    g_free(dev);
+    qpci_free_pc(pcibus);
+}
+
+/**
+ * Start a Q35 machine and bookmark a handle to the XHCI device.
+ */
+G_GNUC_PRINTF(1, 0)
+static XHCIQState *xhci_vboot(const char *cli, va_list ap)
+{
+    XHCIQState *s;
+
+    s = g_new0(XHCIQState, 1);
+    s->parent = qtest_pc_vboot(cli, ap);
+    alloc_set_flags(&s->parent->alloc, ALLOC_LEAK_ASSERT);
+
+    /* Verify that we have an XHCI device present. */
+    s->dev = get_xhci_device(s->parent->qts, &s->fingerprint);
+    s->bar = qpci_iomap(s->dev, 0, &s->barsize);
+    /* turns on pci.cmd.iose, pci.cmd.mse and pci.cmd.bme */
+    qpci_device_enable(s->dev);
+
+    return s;
+}
+
+/**
+ * Start a Q35 machine and bookmark a handle to the XHCI device.
+ */
+G_GNUC_PRINTF(1, 2)
+static XHCIQState *xhci_boot(const char *cli, ...)
+{
+    XHCIQState *s;
+    va_list ap;
+
+    if (cli) {
+        va_start(ap, cli);
+        s = xhci_vboot(cli, ap);
+        va_end(ap);
+    } else {
+        s = xhci_boot("-M q35 "
+                      "-device nec-usb-xhci,id=xhci,bus=pcie.0,addr=1d.0 "
+                      "-drive id=drive0,if=none,file=null-co://,"
+                          "file.read-zeroes=on,format=raw");
+    }
+
+    return s;
+}
+
+/**
+ * Clean up the PCI device, then terminate the QEMU instance.
+ */
+static void xhci_shutdown(XHCIQState *xhci)
+{
+    QOSState *qs = xhci->parent;
+
+    free_xhci_device(xhci->dev);
+    g_free(xhci);
+    qtest_shutdown(qs);
+}
+
+/*** tests ***/
 
 static void test_xhci_hotplug(void)
 {
-    usb_test_hotplug(global_qtest, "xhci", "1", NULL);
+    XHCIQState *s;
+    QTestState *qts;
+
+    s = xhci_boot(NULL);
+    qts = s->parent->qts;
+
+    usb_test_hotplug(qts, "xhci", "1", NULL);
+
+    xhci_shutdown(s);
 }
 
 static void test_usb_uas_hotplug(void)
 {
-    QTestState *qts = global_qtest;
+    XHCIQState *s;
+    QTestState *qts;
+
+    s = xhci_boot(NULL);
+    qts = s->parent->qts;
 
     qtest_qmp_device_add(qts, "usb-uas", "uas", "{}");
     qtest_qmp_device_add(qts, "scsi-hd", "scsihd", "{'drive': 'drive0'}");
@@ -XXX,XX +XXX,XX @@ static void test_usb_uas_hotplug(void)
 
     qtest_qmp_device_del(qts, "scsihd");
     qtest_qmp_device_del(qts, "uas");
+
+    xhci_shutdown(s);
 }
 
 static void test_usb_ccid_hotplug(void)
 {
-    QTestState *qts = global_qtest;
+    XHCIQState *s;
+    QTestState *qts;
+
+    s = xhci_boot(NULL);
+    qts = s->parent->qts;
 
     qtest_qmp_device_add(qts, "usb-ccid", "ccid", "{}");
     qtest_qmp_device_del(qts, "ccid");
     /* check the device can be added again */
     qtest_qmp_device_add(qts, "usb-ccid", "ccid", "{}");
     qtest_qmp_device_del(qts, "ccid");
+
+    xhci_shutdown(s);
+}
+
+static uint64_t xhci_guest_zalloc(XHCIQState *s, uint64_t size)
+{
+    char mem[0x1000];
+    uint64_t ret;
+
+    g_assert(size <= 0x1000);
+
+    memset(mem, 0, size);
+
+    ret = guest_alloc(&s->parent->alloc, size);
+    qtest_memwrite(s->parent->qts, ret, mem, size);
+
+    return ret;
+}
+
+static uint32_t xhci_cap_readl(XHCIQState *s, uint64_t addr)
+{
+    return qpci_io_readl(s->dev, s->bar, addr);
+}
+
+static uint32_t xhci_op_readl(XHCIQState *s, uint64_t addr)
+{
+    return qpci_io_readl(s->dev, s->bar, 0x40 + addr);
+}
+
+static void xhci_op_writel(XHCIQState *s, uint64_t addr, uint32_t value)
+{
+    qpci_io_writel(s->dev, s->bar, 0x40 + addr, value);
+}
+
+static uint32_t xhci_port_readl(XHCIQState *s, uint32_t port, uint64_t addr)
+{
+    return xhci_op_readl(s, 0x400 + port * 0x10 + addr);
+}
+
+static uint32_t xhci_rt_readl(XHCIQState *s, uint64_t addr)
+{
+    return qpci_io_readl(s->dev, s->bar, s->rtoff + addr);
+}
+
+static void xhci_rt_writel(XHCIQState *s, uint64_t addr, uint32_t value)
+{
+    qpci_io_writel(s->dev, s->bar, s->rtoff + addr, value);
 }
 
+static void xhci_db_writel(XHCIQState *s, uint32_t db, uint32_t value)
+{
+    qpci_io_writel(s->dev, s->bar, s->dboff + db * 4, value);
+}
+
+static void wait_event_trb(XHCIQState *s, XHCITRB *trb)
+{
+    XHCITRB t;
+    uint64_t er_addr = s->event_ring + s->er_trb_idx * sizeof(*trb);
+    uint32_t value;
+    guint64 end_time = g_get_monotonic_time() + 5 * G_TIME_SPAN_SECOND;
+
+    /* Wait for event interrupt  */
+
+    do {
+        if (g_get_monotonic_time() >= end_time) {
+            g_error("Timeout expired");
+        }
+        qtest_clock_step(s->parent->qts, 10000);
+
+        value = xhci_op_readl(s, 0x4); /* USBSTS */
+    } while (!(value & USBSTS_EINT));
+
+    value = xhci_rt_readl(s, 0x20 + 0x0); /* IMAN */
+
+    /* With MSI-X enabled, IMAN IP is cleared after raising the interrupt */
+    g_assert(!(value & IMAN_IP));
+
+    /* Ensure MSI-X interrupt is pending */
+    assert(qpci_msix_test_clear_pending(s->dev, 0));
+    /* Then cleared */
+    assert(!qpci_msix_pending(s->dev, 0));
+
+    xhci_op_writel(s, 0x4, USBSTS_EINT); /* USBSTS clear EINT */
+
+    qtest_memread(s->parent->qts, er_addr, &t, sizeof(t));
+
+    trb->parameter = le64_to_cpu(t.parameter);
+    trb->status = le32_to_cpu(t.status);
+    trb->control = le32_to_cpu(t.control);
+
+    g_assert((trb->status >> 24) == CC_SUCCESS);
+    g_assert((trb->control & TRB_C) == s->er_trb_c); /* C bit has been set */
+
+    s->er_trb_idx++;
+    if (s->er_trb_idx == s->er_trb_entries) {
+        s->er_trb_idx = 0;
+        s->er_trb_c ^= 1;
+    }
+    /* Update ERDP to processed TRB addr and EHB bit, which clears EHB */
+    er_addr = s->event_ring + s->er_trb_idx * sizeof(*trb);
+    xhci_rt_writel(s, 0x38, (er_addr & 0xffffffff) | ERDP_EHB);
+}
+
+static void set_link_trb(XHCIQState *s, uint64_t ring, uint32_t c,
+                         uint32_t entries)
+{
+    XHCITRB trb;
+
+    g_assert(entries > 1);
+
+    memset(&trb, 0, sizeof(trb));
+    trb.parameter = cpu_to_le64(ring);
+    trb.control = cpu_to_le32(c | /* C */
+                              (TR_LINK << TRB_TYPE_SHIFT) |
+                              TRB_LK_TC);
+    qtest_memwrite(s->parent->qts, ring + sizeof(trb) * (entries - 1),
+                   &trb, sizeof(trb));
+}
+
+static void submit_cr_trb(XHCIQState *s, XHCITRB *trb)
+{
+    XHCITRB t;
+    uint64_t cr_addr = s->command_ring + s->cr_trb_idx * sizeof(*trb);
+
+    trb->control |= s->cr_trb_c; /* C */
+
+    t.parameter = cpu_to_le64(trb->parameter);
+    t.status = cpu_to_le32(trb->status);
+    t.control = cpu_to_le32(trb->control);
+
+    qtest_memwrite(s->parent->qts, cr_addr, &t, sizeof(t));
+    s->cr_trb_idx++;
+    /* Last entry contains the link, so wrap back */
+    if (s->cr_trb_idx == s->cr_trb_entries - 1) {
+        set_link_trb(s, s->command_ring, s->cr_trb_c, s->cr_trb_entries);
+        s->cr_trb_idx = 0;
+        s->cr_trb_c ^= 1;
+    }
+    xhci_db_writel(s, 0, 0); /* doorbell 0 */
+}
+
+/*
+ * This test brings up an endpoint and runs some noops through its command
+ * ring and gets responses back on the event ring.
+ *
+ * This could be librified in future (like AHCI0 to have a way to bring up
+ * an endpoint to test device protocols.
+ */
+static void pci_xhci_stress_rings(void)
+{
+    XHCIQState *s;
+    uint32_t value;
+    uint64_t input_context;
+    XHCIEvRingSeg ev_seg;
+    XHCITRB trb;
+    uint32_t hcsparams1;
+    uint32_t slotid;
+    g_autofree void *mem = g_malloc0(0x1000); /* buffer for writing to guest */
+    int i;
+
+    s = xhci_boot("-M q35 "
+            "-device nec-usb-xhci,id=xhci,bus=pcie.0,addr=1d.0 "
+            "-device usb-storage,bus=xhci.0,drive=drive0 "
+            "-drive id=drive0,if=none,file=null-co://,"
+                "file.read-zeroes=on,format=raw "
+            );
+
+    hcsparams1 = xhci_cap_readl(s, 0x4); /* HCSPARAMS1 */
+    s->maxports = (hcsparams1 >> 24) & 0xff;
+    s->maxintrs = (hcsparams1 >> 8) & 0x3ff;
+    s->maxslots = hcsparams1 & 0xff;
+
+    s->dboff = xhci_cap_readl(s, 0x14); /* DBOFF */
+    s->rtoff = xhci_cap_readl(s, 0x18); /* RTOFF */
+
+    s->dc_base_array = xhci_guest_zalloc(s, 0x800);
+    s->command_ring = xhci_guest_zalloc(s, 0x1000);
+    s->event_ring = xhci_guest_zalloc(s, 0x1000);
+    s->event_ring_seg = xhci_guest_zalloc(s, 0x100);
+
+    /* Arbitrary small sizes so we can make them wrap */
+    s->cr_trb_entries = 0x20;
+    s->cr_trb_c = 1;
+    s->er_trb_entries = 0x10;
+    s->er_trb_c = 1;
+
+    ev_seg.addr_low = cpu_to_le32(s->event_ring & 0xffffffff);
+    ev_seg.addr_high = cpu_to_le32(s->event_ring >> 32);
+    ev_seg.size = cpu_to_le32(0x10);
+    ev_seg.rsvd = 0;
+    qtest_memwrite(s->parent->qts, s->event_ring_seg, &ev_seg, sizeof(ev_seg));
+
+    xhci_op_writel(s, 0x0, USBCMD_HCRST); /* USBCMD */
+    do {
+        value = xhci_op_readl(s, 0x4); /* USBSTS */
+    } while (value & (1 << 11)); /* CNR */
+
+    xhci_op_writel(s, 0x38, s->maxslots); /* CONFIG */
+
+    /* DCBAAP */
+    xhci_op_writel(s, 0x30, s->dc_base_array & 0xffffffff);
+    xhci_op_writel(s, 0x34, s->dc_base_array >> 32);
+
+    /* CRCR */
+    xhci_op_writel(s, 0x18, (s->command_ring & 0xffffffff) | s->cr_trb_c);
+    xhci_op_writel(s, 0x1c, s->command_ring >> 32);
+
+    xhci_rt_writel(s, 0x28, 1); /* ERSTSZ */
+
+    /* ERSTBA */
+    xhci_rt_writel(s, 0x30, s->event_ring_seg & 0xffffffff);
+    xhci_rt_writel(s, 0x34, s->event_ring_seg >> 32);
+
+    /* ERDP */
+    xhci_rt_writel(s, 0x38, s->event_ring & 0xffffffff);
+    xhci_rt_writel(s, 0x3c, s->event_ring >> 32);
+
+    qpci_msix_enable(s->dev);
+    xhci_op_writel(s, 0x0, USBCMD_RS | USBCMD_INTE); /* RUN + INTE */
+
+    /* Enable interrupts on ER IMAN */
+    xhci_rt_writel(s, 0x20, IMAN_IE);
+
+    assert(!qpci_msix_pending(s->dev, 0));
+
+    /* Wrap the command and event rings with no-ops a few times */
+    for (i = 0; i < 100; i++) {
+        /* Issue a command ring no-op */
+        memset(&trb, 0, sizeof(trb));
+        trb.control |= CR_NOOP << TRB_TYPE_SHIFT;
+        trb.control |= TRB_TR_IOC;
+        submit_cr_trb(s, &trb);
+        wait_event_trb(s, &trb);
+    }
+
+    /* Query ports */
+    for (i = 0; i < s->maxports; i++) {
+        value = xhci_port_readl(s, i, 0); /* PORTSC */
+
+        /* Only first port should be attached and enabled */
+        if (i == 0) {
+            g_assert(value & PORTSC_CCS);
+            g_assert(value & PORTSC_PED);
+            /* Port Speed must be identified (non-zero) */
+            g_assert(((value >> PORTSC_SPEED_SHIFT) & PORTSC_SPEED_MASK) != 0);
+        } else {
+            g_assert(!(value & PORTSC_CCS));
+            g_assert(!(value & PORTSC_PED));
+            g_assert(((value >> PORTSC_PLS_SHIFT) & PORTSC_PLS_MASK) == 5);
+        }
+    }
+
+    /* Issue a command ring enable slot */
+    memset(&trb, 0, sizeof(trb));
+    trb.control |= CR_ENABLE_SLOT << TRB_TYPE_SHIFT;
+    trb.control |= TRB_TR_IOC;
+    submit_cr_trb(s, &trb);
+    wait_event_trb(s, &trb);
+    slotid = (trb.control >> TRB_CR_SLOTID_SHIFT) & 0xff;
+
+    s->slots[slotid].transfer_ring = xhci_guest_zalloc(s, 0x1000);
+    s->slots[slotid].tr_trb_entries = 0x10;
+    s->slots[slotid].tr_trb_c = 1;
+
+    /* 32-byte input context size, should check HCCPARAMS1 for 64-byte size */
+    input_context = xhci_guest_zalloc(s, 0x420);
+
+    /* Set input control context */
+    ((uint32_t *)mem)[1] = cpu_to_le32(0x3); /* Add device contexts 0 and 1 */
+    ((uint32_t *)mem)[8] = cpu_to_le32(1 << 27); /* 1 context entry */
+    ((uint32_t *)mem)[9] = cpu_to_le32(1 << 16); /* 1 port number */
+
+    /* Set endpoint 0 context */
+    ((uint32_t *)mem)[16] = 0;
+    ((uint32_t *)mem)[17] = cpu_to_le32((ET_CONTROL << EP_TYPE_SHIFT) |
+                                        (0x200 << 16)); /* max packet sz XXX? */
+    ((uint32_t *)mem)[18] = cpu_to_le32((s->slots[slotid].transfer_ring &
+                                         0xffffffff) | 1); /* DCS=1 */
+    ((uint32_t *)mem)[19] = cpu_to_le32(s->slots[slotid].transfer_ring >> 32);
+    ((uint32_t *)mem)[20] = cpu_to_le32(0x200); /* Average TRB length */
+    qtest_memwrite(s->parent->qts, input_context, mem, 0x420);
+
+    s->slots[slotid].device_context = xhci_guest_zalloc(s, 0x400);
+
+    ((uint64_t *)mem)[0] = cpu_to_le64(s->slots[slotid].device_context);
+    qtest_memwrite(s->parent->qts, s->dc_base_array + 8 * slotid, mem, 8);
+
+    /* Issue a command ring address device */
+    memset(&trb, 0, sizeof(trb));
+    trb.parameter = input_context;
+    trb.control |= CR_ADDRESS_DEVICE << TRB_TYPE_SHIFT;
+    trb.control |= slotid << TRB_CR_SLOTID_SHIFT;
+    submit_cr_trb(s, &trb);
+    wait_event_trb(s, &trb);
+
+    /* XXX: Could check EP state is running */
+
+    /* Shut it down */
+    qpci_msix_disable(s->dev);
+
+    guest_free(&s->parent->alloc, s->slots[slotid].device_context);
+    guest_free(&s->parent->alloc, s->slots[slotid].transfer_ring);
+    guest_free(&s->parent->alloc, input_context);
+    guest_free(&s->parent->alloc, s->event_ring);
+    guest_free(&s->parent->alloc, s->event_ring_seg);
+    guest_free(&s->parent->alloc, s->command_ring);
+    guest_free(&s->parent->alloc, s->dc_base_array);
+
+    xhci_shutdown(s);
+}
+
+/* tests */
 int main(int argc, char **argv)
 {
     int ret;
+    const char *arch;
 
     g_test_init(&argc, &argv, NULL);
 
+    /* Check architecture */
+    arch = qtest_get_arch();
+    if (strcmp(arch, "i386") && strcmp(arch, "x86_64")) {
+        g_test_message("Skipping test for non-x86");
+        return 0;
+    }
+
+    if (!qtest_has_device("nec-usb-xhci")) {
+        return 0;
+    }
+
     qtest_add_func("/xhci/pci/hotplug", test_xhci_hotplug);
     if (qtest_has_device("usb-uas")) {
         qtest_add_func("/xhci/pci/hotplug/usb-uas", test_usb_uas_hotplug);
@@ -XXX,XX +XXX,XX @@ int main(int argc, char **argv)
     if (qtest_has_device("usb-ccid")) {
         qtest_add_func("/xhci/pci/hotplug/usb-ccid", test_usb_ccid_hotplug);
     }
+    if (qtest_has_device("usb-storage")) {
+        qtest_add_func("/xhci/pci/xhci-stress-rings", pci_xhci_stress_rings);
+    }
 
-    qtest_start("-device nec-usb-xhci,id=xhci"
-                " -drive id=drive0,if=none,file=null-co://,"
-                "file.read-zeroes=on,format=raw");
     ret = g_test_run();
+
     qtest_end();
 
     return ret;
-- 
2.45.2

Implement XHCI TR NOOP commands by setting up then immediately
completing the packet.

The IBM AIX XHCI HCD driver uses NOOP commands to check driver and
hardware health, which works after this change.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 hw/usb/hcd-xhci.c | 28 +++++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

Run some TR NOOP commands through the transfer ring.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 tests/qtest/usb-hcd-xhci-test.c | 41 +++++++++++++++++++++++++++++++--
 1 file changed, 39 insertions(+), 2 deletions(-)

diff --git a/tests/qtest/usb-hcd-xhci-test.c b/tests/qtest/usb-hcd-xhci-test.c
index XXXXXXX..XXXXXXX 100644
--- a/tests/qtest/usb-hcd-xhci-test.c
+++ b/tests/qtest/usb-hcd-xhci-test.c
@@ -XXX,XX +XXX,XX @@ static void set_link_trb(XHCIQState *s, uint64_t ring, uint32_t c,
 static void submit_cr_trb(XHCIQState *s, XHCITRB *trb)
 {
     XHCITRB t;
-    uint64_t cr_addr = s->command_ring + s->cr_trb_idx * sizeof(*trb);
+    uint64_t cr_addr = s->command_ring +
+                       s->cr_trb_idx * sizeof(*trb);
 
     trb->control |= s->cr_trb_c; /* C */
 
@@ -XXX,XX +XXX,XX @@ static void submit_cr_trb(XHCIQState *s, XHCITRB *trb)
     xhci_db_writel(s, 0, 0); /* doorbell 0 */
 }
 
+static void submit_tr_trb(XHCIQState *s, int slot, XHCITRB *trb)
+{
+    XHCITRB t;
+    uint64_t tr_addr = s->slots[slot].transfer_ring +
+                       s->slots[slot].tr_trb_idx * sizeof(*trb);
+
+    trb->control |= s->slots[slot].tr_trb_c; /* C */
+
+    t.parameter = cpu_to_le64(trb->parameter);
+    t.status = cpu_to_le32(trb->status);
+    t.control = cpu_to_le32(trb->control);
+
+    qtest_memwrite(s->parent->qts, tr_addr, &t, sizeof(t));
+    s->slots[slot].tr_trb_idx++;
+    /* Last entry contains the link, so wrap back */
+    if (s->slots[slot].tr_trb_idx == s->slots[slot].tr_trb_entries - 1) {
+        set_link_trb(s, s->slots[slot].transfer_ring,
+                        s->slots[slot].tr_trb_c,
+                        s->slots[slot].tr_trb_entries);
+        s->slots[slot].tr_trb_idx = 0;
+        s->slots[slot].tr_trb_c ^= 1;
+    }
+    xhci_db_writel(s, slot, 1); /* doorbell slot, EP0 target */
+}
+
 /*
  * This test brings up an endpoint and runs some noops through its command
- * ring and gets responses back on the event ring.
+ * ring and gets responses back on the event ring, then brings up a device
+ * context and runs some noops through its transfer ring.
  *
  * This could be librified in future (like AHCI0 to have a way to bring up
  * an endpoint to test device protocols.
@@ -XXX,XX +XXX,XX @@ static void pci_xhci_stress_rings(void)
 
     /* XXX: Could check EP state is running */
 
+    /* Wrap the transfer ring a few times */
+    for (i = 0; i < 100; i++) {
+        /* Issue a transfer ring slot 0 noop */
+        memset(&trb, 0, sizeof(trb));
+        trb.control |= TR_NOOP << TRB_TYPE_SHIFT;
+        trb.control |= TRB_TR_IOC;
+        submit_tr_trb(s, slotid, &trb);
+        wait_event_trb(s, &trb);
+    }
+
     /* Shut it down */
     qpci_msix_disable(s->dev);
 
-- 
2.45.2