[PATCH 0/4] hw/usb/hcd-xhci: two isoch fixes with qtests

[PATCH 0/4] hw/usb/hcd-xhci: two isoch fixes with qtests

[Junjie Cao]

Two xHCI bugs from the issue tracker, each with a qtest.

The interval mask is 32 bits wide, so once MFINDEX passes 2^32 isoch TDs
with SIA are no longer held until the next interval boundary. This
matches #3973: onset after 6.2 days of uptime, cleared by re-binding the
controller in the guest.

A guest can declare a non-isoch endpoint as isoch and hit an assertion
when the device NAKs (#3886). #544 is the same assertion, reached from
the kick timer as in patch 4. Its reproducer from 2021 no longer
triggers on master, so it is not tagged.

Both tests fail on master (c1c18d1e64). Tested on x86_64 with an
ASan/UBSan build: usb-hcd-xhci-test, usb-hcd-ehci-test and
usb-hcd-uhci-test pass. The new tests skip on ppc64.

Junjie Cao (4):
  hw/usb/hcd-xhci: fix interval alignment after MFINDEX passes 2^32
  hw/usb/hcd-xhci: don't assert on NAK when retrying an isoch transfer
  tests/qtest/usb-hcd-xhci: test isoch pacing with MFINDEX above 2^32
  tests/qtest/usb-hcd-xhci: test isoch endpoint type mismatch

 hw/usb/hcd-xhci.c               |  33 +---
 tests/qtest/usb-hcd-xhci-test.c | 318 ++++++++++++++++++++++++++++++++
 2 files changed, 328 insertions(+), 23 deletions(-)


base-commit: c1c18d1e640b64292859ce9f30f3c344edfb0294
-- 
2.43.0

[PATCH 1/4] hw/usb/hcd-xhci: fix interval alignment after MFINDEX passes 2^32

[Junjie Cao]

epctx->interval is an unsigned int, so ~(epctx->interval - 1) is a
32-bit mask that is zero-extended when and-ed with the 64-bit microframe
index. Once mfindex no longer fits in 32 bits (2^32 * 125us, about 6.2
days after the controller was started), asap loses its upper half and
always compares below mfindex. Isoch TDs with SIA are then run at once
instead of at the next interval boundary. xhci_calc_intr_kick() has the
same expression.

Use ROUND_UP(), which builds the mask in the type of mfindex. The
interval is always a power of two.

The reporter of #3973 also saw the symptom with UHCI. This change does
not explain that.

Fixes: 3d1396842d ("xhci: iso xfer support")
Fixes: 4d7a81c06f ("xhci: emulate intr endpoint intervals correctly")
Link: https://gitlab.com/qemu-project/qemu/-/issues/3973
Cc: qemu-stable@nongnu.org
Signed-off-by: Junjie Cao <junjie.cao@intel.com>
---
 hw/usb/hcd-xhci.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index d342aa2739..ce042e74bd 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -1742,8 +1742,7 @@ static int xhci_fire_ctl_transfer(XHCIState *xhci, XHCITransfer *xfer)
 static void xhci_calc_intr_kick(XHCIState *xhci, XHCITransfer *xfer,
                                 XHCIEPContext *epctx, uint64_t mfindex)
 {
-    uint64_t asap = ((mfindex + epctx->interval - 1) &
-                     ~(epctx->interval-1));
+    uint64_t asap = ROUND_UP(mfindex, epctx->interval);
     uint64_t kick = epctx->mfindex_last + epctx->interval;
 
     assert(epctx->interval != 0);
@@ -1754,8 +1753,7 @@ static void xhci_calc_iso_kick(XHCIState *xhci, XHCITransfer *xfer,
                                XHCIEPContext *epctx, uint64_t mfindex)
 {
     if (xfer->trbs[0].control & TRB_TR_SIA) {
-        uint64_t asap = ((mfindex + epctx->interval - 1) &
-                         ~(epctx->interval-1));
+        uint64_t asap = ROUND_UP(mfindex, epctx->interval);
         if (asap >= epctx->mfindex_last &&
             asap <= epctx->mfindex_last + epctx->interval * 4) {
             xfer->mfindex_kick = epctx->mfindex_last + epctx->interval;
-- 
2.43.0

[PATCH 2/4] hw/usb/hcd-xhci: don't assert on NAK when retrying an isoch transfer

[Junjie Cao]

The endpoint type in the xHCI endpoint context comes from the guest and
is not checked against the device. A guest can configure the interrupt
IN endpoint of usb-kbd as Isoch IN. The idle HID endpoint NAKs, and as
soon as the transfer goes through the retry path in xhci_kick_epctx()
it hits

  assert(xfer->packet.status != USB_RET_NAK);

No device model NAKs on an isoch endpoint, so this only triggers with a
mismatched endpoint type.

The two retry branches differ only in what they do on NAK: the isoch one
asserts, the other keeps the transfer pending. Merge them.

Fixes: 3d1396842d ("xhci: iso xfer support")
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/3886
Reported-by: Feifan Qian <bea1e@proton.me>
Cc: qemu-stable@nongnu.org
Signed-off-by: Junjie Cao <junjie.cao@intel.com>
---
 hw/usb/hcd-xhci.c | 27 ++++++++-------------------
 1 file changed, 8 insertions(+), 19 deletions(-)

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index ce042e74bd..6cd5ac87b8 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -1912,26 +1912,15 @@ static void xhci_kick_epctx(XHCIEPContext *epctx, unsigned int streamid)
             xfer->timed_xfer = 0;
             xfer->running_retry = 1;
         }
-        if (xfer->iso_xfer) {
-            /* retry iso transfer */
-            if (xhci_setup_packet(xfer) < 0) {
-                return;
-            }
-            usb_handle_packet(xfer->packet.ep->dev, &xfer->packet);
-            assert(xfer->packet.status != USB_RET_NAK);
-            xhci_try_complete_packet(xfer);
-        } else {
-            /* retry nak'ed transfer */
-            if (xhci_setup_packet(xfer) < 0) {
-                return;
-            }
-            usb_handle_packet(xfer->packet.ep->dev, &xfer->packet);
-            if (xfer->packet.status == USB_RET_NAK) {
-                xhci_xfer_unmap(xfer);
-                return;
-            }
-            xhci_try_complete_packet(xfer);
+        if (xhci_setup_packet(xfer) < 0) {
+            return;
+        }
+        usb_handle_packet(xfer->packet.ep->dev, &xfer->packet);
+        if (xfer->packet.status == USB_RET_NAK) {
+            xhci_xfer_unmap(xfer);
+            return;
         }
+        xhci_try_complete_packet(xfer);
         assert(!xfer->running_retry);
         if (xfer->complete) {
             /* update ring dequeue ptr */
-- 
2.43.0

[PATCH 3/4] tests/qtest/usb-hcd-xhci: test isoch pacing with MFINDEX above 2^32

[Junjie Cao]

Add the minimum needed to drive a transfer ring from the test. Use it to
queue an isoch TD with SIA on usb-audio after stepping the clock past
2^32 microframes, and check that the TD waits for the next interval
boundary. Before the ROUND_UP() change it completed as soon as the
doorbell was rung.

The machine is started with pit=off. With the i8254 present the clock
step takes 26s in an ASan build instead of well under a second.

Signed-off-by: Junjie Cao <junjie.cao@intel.com>
---

Notes:
    Nick Piggin's v4 series has a fuller xhci ring harness for qtest:
    https://lore.kernel.org/qemu-devel/20250502033047.102465-1-npiggin@gmail.com/
    The helpers here are self-contained so that the fixes do not depend on
    it. I can rebase onto that harness if it is revived.

 tests/qtest/usb-hcd-xhci-test.c | 283 ++++++++++++++++++++++++++++++++
 1 file changed, 283 insertions(+)

diff --git a/tests/qtest/usb-hcd-xhci-test.c b/tests/qtest/usb-hcd-xhci-test.c
index b58fa1e2da..61eca37b15 100644
--- a/tests/qtest/usb-hcd-xhci-test.c
+++ b/tests/qtest/usb-hcd-xhci-test.c
@@ -10,8 +10,68 @@
 #include "qemu/osdep.h"
 #include "libqtest-single.h"
 #include "libqos/usb.h"
+#include "libqos/malloc-pc.h"
 #include "qobject/qdict.h"
 
+/* capability registers */
+#define XHCI_CAPLENGTH          0x00
+#define XHCI_HCSPARAMS1         0x04
+#define XHCI_DBOFF              0x14
+#define XHCI_RTSOFF             0x18
+/* operational registers */
+#define XHCI_USBCMD             0x00
+#define XHCI_USBSTS             0x04
+#define XHCI_CRCR               0x18
+#define XHCI_DCBAAP             0x30
+#define XHCI_CONFIG             0x38
+#define XHCI_PORTSC(n)          (0x400 + 0x10 * (n))
+/* interrupter 0, relative to the runtime registers */
+#define XHCI_ERSTSZ             0x28
+#define XHCI_ERSTBA             0x30
+#define XHCI_ERDP               0x38
+
+#define USBCMD_RS               (1 << 0)
+#define USBCMD_HCRST            (1 << 1)
+#define USBSTS_HCH              (1 << 0)
+#define PORTSC_CCS              (1 << 0)
+#define PORTSC_PR               (1 << 4)
+#define PORTSC_PP               (1 << 9)
+#define CRCR_RCS                (1 << 0)
+#define ERDP_EHB                (1 << 3)
+
+#define TRB_C                   (1 << 0)
+#define TRB_TR_IOC              (1 << 5)
+#define TRB_TR_SIA              (1U << 31)
+#define TRB_TYPE(t)             ((t) << 10)
+#define TRB_GET_TYPE(control)   (((control) >> 10) & 0x3f)
+#define TRB_GET_CCODE(status)   ((status) >> 24)
+#define TRB_GET_SLOT(control)   ((control) >> 24)
+
+#define TR_ISOCH                5
+#define CR_ENABLE_SLOT          9
+#define CR_ADDRESS_DEVICE       11
+#define CR_CONFIGURE_ENDPOINT   12
+#define ER_TRANSFER             32
+#define ER_COMMAND_COMPLETE     33
+#define CC_SUCCESS              1
+
+#define EP_TYPE_ISOCH_OUT       1
+#define EP_TYPE_CONTROL         4
+
+#define XHCI_RING_TRBS          64
+#define XHCI_MICROFRAME_NS      125000
+
+typedef struct XHCITest {
+    QTestState *qts;
+    QGuestAllocator alloc;
+    QPCIBus *bus;
+    struct qhc hc;
+    uint32_t oper, runtime, doorbell;
+    uint64_t cmd_ring, event_ring, input_ctx;
+    unsigned int cmd_idx, event_idx;
+    unsigned int port, slot;
+} XHCITest;
+
 static void wait_device_deleted_event(QTestState *qtest, const char *id)
 {
     QDict *resp, *data;
@@ -109,6 +169,227 @@ static void test_usb_ccid_hotplug(void)
     qtest_qmp_device_del(qts, "ccid");
 }
 
+static uint32_t xhci_readl(XHCITest *x, uint32_t off)
+{
+    return qpci_io_readl(x->hc.dev, x->hc.bar, off);
+}
+
+static void xhci_writel(XHCITest *x, uint32_t off, uint32_t val)
+{
+    qpci_io_writel(x->hc.dev, x->hc.bar, off, val);
+}
+
+static void xhci_writeq(XHCITest *x, uint32_t off, uint64_t val)
+{
+    xhci_writel(x, off, val);
+    xhci_writel(x, off + 4, val >> 32);
+}
+
+static uint64_t xhci_alloc_page(XHCITest *x)
+{
+    uint64_t addr = guest_alloc(&x->alloc, 0x1000);
+
+    qtest_memset(x->qts, addr, 0, 0x1000);
+    return addr;
+}
+
+static void xhci_write_trb(XHCITest *x, uint64_t addr, uint64_t parameter,
+                           uint32_t status, uint32_t control)
+{
+    qtest_writeq(x->qts, addr, parameter);
+    qtest_writel(x->qts, addr + 8, status);
+    qtest_writel(x->qts, addr + 12, control);
+}
+
+/* Fetch the next event if there is one. Does not advance the clock. */
+static bool xhci_next_event(XHCITest *x, uint32_t *status, uint32_t *control)
+{
+    uint64_t addr = x->event_ring + 16 * x->event_idx;
+    uint32_t c = qtest_readl(x->qts, addr + 12);
+
+    if (!(c & TRB_C)) {
+        return false;
+    }
+    if (status) {
+        *status = qtest_readl(x->qts, addr + 8);
+    }
+    if (control) {
+        *control = c;
+    }
+    x->event_idx++;
+    g_assert_cmpuint(x->event_idx, <, XHCI_RING_TRBS);
+    xhci_writeq(x, x->runtime + XHCI_ERDP, (addr + 16) | ERDP_EHB);
+    return true;
+}
+
+static unsigned int xhci_command(XHCITest *x, uint64_t parameter,
+                                 uint32_t control)
+{
+    uint32_t status;
+
+    g_assert_cmpuint(x->cmd_idx, <, XHCI_RING_TRBS);
+    xhci_write_trb(x, x->cmd_ring + 16 * x->cmd_idx++, parameter, 0,
+                   control | TRB_C);
+    xhci_writel(x, x->doorbell, 0);
+
+    g_assert_true(xhci_next_event(x, &status, &control));
+    g_assert_cmpuint(TRB_GET_TYPE(control), ==, ER_COMMAND_COMPLETE);
+    g_assert_cmpuint(TRB_GET_CCODE(status), ==, CC_SUCCESS);
+    return TRB_GET_SLOT(control);
+}
+
+/*
+ * Start qemu-xhci with one USB device, run the controller and bring the
+ * device to the Addressed state.
+ */
+static void xhci_test_start(XHCITest *x, const char *usb_device)
+{
+    uint64_t dcbaa, erst, ep0_ring;
+    unsigned int maxports;
+
+    memset(x, 0, sizeof(*x));
+    /* pit=off: a long clock step would run the i8254 timer all the way */
+    x->qts = qtest_initf("-machine pc,pit=off -nodefaults "
+                         "-device qemu-xhci,id=xhci,addr=04.0 %s", usb_device);
+    pc_alloc_init(&x->alloc, x->qts, ALLOC_NO_FLAGS);
+    x->bus = qpci_new_pc(x->qts, NULL);
+    qusb_pci_init_one(x->bus, &x->hc, QPCI_DEVFN(4, 0), 0);
+
+    x->oper = qpci_io_readb(x->hc.dev, x->hc.bar, XHCI_CAPLENGTH);
+    x->runtime = xhci_readl(x, XHCI_RTSOFF) & ~0x1f;
+    x->doorbell = xhci_readl(x, XHCI_DBOFF) & ~0x3;
+    maxports = xhci_readl(x, XHCI_HCSPARAMS1) >> 24;
+
+    xhci_writel(x, x->oper + XHCI_USBCMD, USBCMD_HCRST);
+    g_assert_false(xhci_readl(x, x->oper + XHCI_USBCMD) & USBCMD_HCRST);
+
+    dcbaa = xhci_alloc_page(x);
+    erst = xhci_alloc_page(x);
+    x->cmd_ring = xhci_alloc_page(x);
+    x->event_ring = xhci_alloc_page(x);
+    x->input_ctx = xhci_alloc_page(x);
+
+    xhci_writel(x, x->oper + XHCI_CONFIG, 1);
+    xhci_writeq(x, x->oper + XHCI_DCBAAP, dcbaa);
+    qtest_writeq(x->qts, erst, x->event_ring);
+    qtest_writel(x->qts, erst + 8, XHCI_RING_TRBS);
+    xhci_writel(x, x->runtime + XHCI_ERSTSZ, 1);
+    xhci_writeq(x, x->runtime + XHCI_ERSTBA, erst);
+    xhci_writeq(x, x->runtime + XHCI_ERDP, x->event_ring | ERDP_EHB);
+    xhci_writeq(x, x->oper + XHCI_CRCR, x->cmd_ring | CRCR_RCS);
+    xhci_writel(x, x->oper + XHCI_USBCMD, USBCMD_RS);
+    g_assert_false(xhci_readl(x, x->oper + XHCI_USBSTS) & USBSTS_HCH);
+
+    for (x->port = 0; x->port < maxports; x->port++) {
+        if (xhci_readl(x, x->oper + XHCI_PORTSC(x->port)) & PORTSC_CCS) {
+            break;
+        }
+    }
+    g_assert_cmpuint(x->port, <, maxports);
+    xhci_writel(x, x->oper + XHCI_PORTSC(x->port), PORTSC_PP | PORTSC_PR);
+    while (xhci_next_event(x, NULL, NULL)) {
+        /* drop the port status change events */
+    }
+
+    x->slot = xhci_command(x, 0, TRB_TYPE(CR_ENABLE_SLOT));
+    qtest_writeq(x->qts, dcbaa + 8 * x->slot, xhci_alloc_page(x));
+
+    /* input control context: add slot and ep0 */
+    qtest_writel(x->qts, x->input_ctx + 0x04, 0x3);
+    /* slot context: one context entry, root hub port */
+    qtest_writel(x->qts, x->input_ctx + 0x20, 1 << 27);
+    qtest_writel(x->qts, x->input_ctx + 0x24, (x->port + 1) << 16);
+    /* ep0 context */
+    ep0_ring = xhci_alloc_page(x);
+    qtest_writel(x->qts, x->input_ctx + 0x44,
+                 (64 << 16) | (EP_TYPE_CONTROL << 3));
+    qtest_writeq(x->qts, x->input_ctx + 0x48, ep0_ring | 1);
+    xhci_command(x, x->input_ctx,
+                 TRB_TYPE(CR_ADDRESS_DEVICE) | (x->slot << 24));
+}
+
+/* Returns the address of the transfer ring. */
+static uint64_t xhci_configure_ep(XHCITest *x, unsigned int epid,
+                                  unsigned int type, unsigned int interval,
+                                  unsigned int max_packet)
+{
+    uint64_t ring = xhci_alloc_page(x);
+    uint64_t epctx = x->input_ctx + 0x20 * (epid + 1);
+
+    qtest_memset(x->qts, x->input_ctx, 0, 0x1000);
+    qtest_writel(x->qts, x->input_ctx + 0x04, (1 << epid) | 1);
+    qtest_writel(x->qts, x->input_ctx + 0x20, epid << 27);
+    qtest_writel(x->qts, x->input_ctx + 0x24, (x->port + 1) << 16);
+    qtest_writel(x->qts, epctx + 0x00, interval << 16);
+    qtest_writel(x->qts, epctx + 0x04, (max_packet << 16) | (type << 3));
+    qtest_writeq(x->qts, epctx + 0x08, ring | 1);
+    xhci_command(x, x->input_ctx,
+                 TRB_TYPE(CR_CONFIGURE_ENDPOINT) | (x->slot << 24));
+    return ring;
+}
+
+static void xhci_test_end(XHCITest *x)
+{
+    g_free(x->hc.dev);
+    qpci_free_pc(x->bus);
+    alloc_destroy(&x->alloc);
+    qtest_quit(x->qts);
+}
+
+static bool xhci_test_supported(const char *usb_device)
+{
+    const char *arch = qtest_get_arch();
+
+    if (strcmp(arch, "i386") != 0 && strcmp(arch, "x86_64") != 0) {
+        g_test_skip("Test only runs on x86 (pc machine)");
+        return false;
+    }
+    if (!qtest_has_device("qemu-xhci") || !qtest_has_device(usb_device)) {
+        g_test_skip("Devices not available");
+        return false;
+    }
+    return true;
+}
+
+/*
+ * An isoch TD with SIA set is run at the next interval boundary. That has to
+ * hold once the microframe index no longer fits in 32 bits as well.
+ */
+static void test_xhci_isoch_mfindex_32bit(void)
+{
+    const unsigned int interval = 6;
+    uint32_t control;
+    uint64_t ring;
+    XHCITest x;
+
+    if (!xhci_test_supported("usb-audio")) {
+        return;
+    }
+
+    xhci_test_start(&x, "-audiodev none,id=snd0 "
+                        "-device usb-audio,audiodev=snd0");
+    ring = xhci_configure_ep(&x, 2, EP_TYPE_ISOCH_OUT, interval, 64);
+
+    /* Go past 2^32 microframes and stop off an interval boundary. */
+    qtest_clock_step(x.qts, (1ULL << 32) * XHCI_MICROFRAME_NS);
+    qtest_clock_step(x.qts, 5 * XHCI_MICROFRAME_NS);
+
+    xhci_write_trb(&x, ring, xhci_alloc_page(&x), 64,
+                   TRB_TYPE(TR_ISOCH) | TRB_TR_SIA | TRB_TR_IOC | TRB_C);
+    xhci_writel(&x, x.doorbell + 4 * x.slot, 2);
+    g_assert_false(xhci_next_event(&x, NULL, NULL));
+
+    /*
+     * The streaming interface has not been enabled, so usb-audio stalls the
+     * TD. What matters is when that happens.
+     */
+    qtest_clock_step(x.qts, XHCI_MICROFRAME_NS << interval);
+    g_assert_true(xhci_next_event(&x, NULL, &control));
+    g_assert_cmpuint(TRB_GET_TYPE(control), ==, ER_TRANSFER);
+
+    xhci_test_end(&x);
+}
+
 int main(int argc, char **argv)
 {
     int ret;
@@ -123,6 +404,8 @@ int main(int argc, char **argv)
     if (qtest_has_device("usb-ccid")) {
         qtest_add_func("/xhci/pci/hotplug/usb-ccid", test_usb_ccid_hotplug);
     }
+    qtest_add_func("/xhci/pci/isoch/mfindex-32bit",
+                   test_xhci_isoch_mfindex_32bit);
 
     qtest_start("-device nec-usb-xhci,id=xhci"
                 " -drive id=drive0,if=none,file=null-co://,"
-- 
2.43.0

[PATCH 4/4] tests/qtest/usb-hcd-xhci: test isoch endpoint type mismatch

[Junjie Cao]

Configure the interrupt IN endpoint of usb-kbd as Isoch IN and queue one
TD. The TD is deferred by a microframe, the kick timer retries it,
usb-kbd NAKs, and a second doorbell retries it again. The TD has to stay
pending both times and the controller has to keep running.

Without the fix QEMU aborts in the timer retry. The endpoint setup is
taken from the reproducer attached to #3886.

Signed-off-by: Junjie Cao <junjie.cao@intel.com>
---
 tests/qtest/usb-hcd-xhci-test.c | 35 +++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/tests/qtest/usb-hcd-xhci-test.c b/tests/qtest/usb-hcd-xhci-test.c
index 61eca37b15..2e173af750 100644
--- a/tests/qtest/usb-hcd-xhci-test.c
+++ b/tests/qtest/usb-hcd-xhci-test.c
@@ -33,6 +33,7 @@
 #define USBCMD_RS               (1 << 0)
 #define USBCMD_HCRST            (1 << 1)
 #define USBSTS_HCH              (1 << 0)
+#define USBSTS_HCE              (1 << 12)
 #define PORTSC_CCS              (1 << 0)
 #define PORTSC_PR               (1 << 4)
 #define PORTSC_PP               (1 << 9)
@@ -57,6 +58,7 @@
 
 #define EP_TYPE_ISOCH_OUT       1
 #define EP_TYPE_CONTROL         4
+#define EP_TYPE_ISOCH_IN        5
 
 #define XHCI_RING_TRBS          64
 #define XHCI_MICROFRAME_NS      125000
@@ -390,6 +392,37 @@ static void test_xhci_isoch_mfindex_32bit(void)
     xhci_test_end(&x);
 }
 
+/*
+ * The endpoint type in the endpoint context is whatever the guest says. Tell
+ * the controller that the interrupt endpoint of usb-kbd is isoch. The idle
+ * keyboard NAKs, and the TD has to stay pending when first the kick timer and
+ * then a doorbell retry it.
+ */
+static void test_xhci_isoch_ep_type_mismatch(void)
+{
+    uint64_t ring;
+    XHCITest x;
+
+    if (!xhci_test_supported("usb-kbd")) {
+        return;
+    }
+
+    xhci_test_start(&x, "-device usb-kbd");
+    ring = xhci_configure_ep(&x, 3, EP_TYPE_ISOCH_IN, 0, 8);
+
+    xhci_write_trb(&x, ring, xhci_alloc_page(&x), 8,
+                   TRB_TYPE(TR_ISOCH) | TRB_TR_SIA | TRB_TR_IOC | TRB_C);
+    xhci_writel(&x, x.doorbell + 4 * x.slot, 3);
+    qtest_clock_step(x.qts, 2 * XHCI_MICROFRAME_NS);
+    xhci_writel(&x, x.doorbell + 4 * x.slot, 3);
+
+    g_assert_false(xhci_next_event(&x, NULL, NULL));
+    g_assert_cmphex(xhci_readl(&x, x.oper + XHCI_USBSTS) &
+                    (USBSTS_HCH | USBSTS_HCE), ==, 0);
+
+    xhci_test_end(&x);
+}
+
 int main(int argc, char **argv)
 {
     int ret;
@@ -406,6 +439,8 @@ int main(int argc, char **argv)
     }
     qtest_add_func("/xhci/pci/isoch/mfindex-32bit",
                    test_xhci_isoch_mfindex_32bit);
+    qtest_add_func("/xhci/pci/isoch/ep-type-mismatch",
+                   test_xhci_isoch_ep_type_mismatch);
 
     qtest_start("-device nec-usb-xhci,id=xhci"
                 " -drive id=drive0,if=none,file=null-co://,"
-- 
2.43.0

Re: [PATCH 1/4] hw/usb/hcd-xhci: fix interval alignment after MFINDEX passes 2^32

[Philippe Mathieu-Daudé]

On 21/9/26 07:46, Junjie Cao wrote:
> epctx->interval is an unsigned int, so ~(epctx->interval - 1) is a
> 32-bit mask that is zero-extended when and-ed with the 64-bit microframe
> index. Once mfindex no longer fits in 32 bits (2^32 * 125us, about 6.2
> days after the controller was started), asap loses its upper half and
> always compares below mfindex. Isoch TDs with SIA are then run at once
> instead of at the next interval boundary. xhci_calc_intr_kick() has the
> same expression.
> 
> Use ROUND_UP(), which builds the mask in the type of mfindex. The
> interval is always a power of two.
> 
> The reporter of #3973 also saw the symptom with UHCI. This change does
> not explain that.

Nice.

Reviewed-by: Philippe Mathieu-Daudé <philmd@oss.qualcomm.com>

> 
> Fixes: 3d1396842d ("xhci: iso xfer support")
> Fixes: 4d7a81c06f ("xhci: emulate intr endpoint intervals correctly")
> Link: https://gitlab.com/qemu-project/qemu/-/issues/3973
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Junjie Cao <junjie.cao@intel.com>
> ---
>   hw/usb/hcd-xhci.c | 6 ++----
>   1 file changed, 2 insertions(+), 4 deletions(-)

Re: [PATCH 3/4] tests/qtest/usb-hcd-xhci: test isoch pacing with MFINDEX above 2^32

[Philippe Mathieu-Daudé]

On 21/9/26 07:46, Junjie Cao wrote:
> Add the minimum needed to drive a transfer ring from the test. Use it to
> queue an isoch TD with SIA on usb-audio after stepping the clock past
> 2^32 microframes, and check that the TD waits for the next interval
> boundary. Before the ROUND_UP() change it completed as soon as the
> doorbell was rung.
> 
> The machine is started with pit=off. With the i8254 present the clock
> step takes 26s in an ASan build instead of well under a second.
> 
> Signed-off-by: Junjie Cao <junjie.cao@intel.com>
> ---
> 
> Notes:
>      Nick Piggin's v4 series has a fuller xhci ring harness for qtest:
>      https://lore.kernel.org/qemu-devel/20250502033047.102465-1-npiggin@gmail.com/
>      The helpers here are self-contained so that the fixes do not depend on
>      it. I can rebase onto that harness if it is revived.
> 
>   tests/qtest/usb-hcd-xhci-test.c | 283 ++++++++++++++++++++++++++++++++
>   1 file changed, 283 insertions(+)

Acked-by: Philippe Mathieu-Daudé <philmd@oss.qualcomm.com>

Re: [PATCH 2/4] hw/usb/hcd-xhci: don't assert on NAK when retrying an isoch transfer

[Thomas Huth]

On 21/09/2026 07.46, Junjie Cao wrote:
> The endpoint type in the xHCI endpoint context comes from the guest and
> is not checked against the device. A guest can configure the interrupt
> IN endpoint of usb-kbd as Isoch IN. The idle HID endpoint NAKs, and as
> soon as the transfer goes through the retry path in xhci_kick_epctx()
> it hits
> 
>    assert(xfer->packet.status != USB_RET_NAK);
> 
> No device model NAKs on an isoch endpoint, so this only triggers with a
> mismatched endpoint type.
> 
> The two retry branches differ only in what they do on NAK: the isoch one
> asserts, the other keeps the transfer pending. Merge them.
> 
> Fixes: 3d1396842d ("xhci: iso xfer support")
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/3886
> Reported-by: Feifan Qian <bea1e@proton.me>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Junjie Cao <junjie.cao@intel.com>
> ---
>   hw/usb/hcd-xhci.c | 27 ++++++++-------------------
>   1 file changed, 8 insertions(+), 19 deletions(-)
Reviewed-by: Thomas Huth <thuth@redhat.com>