[PATCH 0/2] hw/usb/hcd-xhci: check the DCBAA read in Address Device

[PATCH 0/2] hw/usb/hcd-xhci: check the DCBAA read in Address Device

[Junjie Cao]

Address Device discards the result of the DCBAA entry read. With DCBAAP
pointing at unassigned memory the command still completes with Success
and the slot and ep0 contexts are written to guest address 0:
https://gitlab.com/qemu-project/qemu/-/issues/4706

The series keeps the existing HCE-only error signalling; halting the
controller on a failed host access, as xHCI 4.10.2.6 asks, is a
separate change. The post_load path has no test.

Tested on x86_64 with the reporter's reproducer (before: Success and
address 0 overwritten; after: TRB Error, HCE set, address 0 intact) and
the full usb-hcd-xhci-test.

Patch 2 touches the same test file as the #4658 series
(20261030165807.79423-1-tcalifano@redhat.com); the two apply in either
order with a 3-way merge.

Junjie Cao (2):
  hw/usb/hcd-xhci: fail Address Device when the DCBAA entry is
    unreadable
  tests/qtest/usb-hcd-xhci: test Address Device with an unreadable DCBAA

 hw/usb/hcd-xhci.c               | 41 ++++++++++------
 tests/qtest/usb-hcd-xhci-test.c | 87 ++++++++++++++++++++++++++++-----
 2 files changed, 102 insertions(+), 26 deletions(-)


base-commit: 9703b6cf418d2ce72fae8d00e511135ce0bfda43
-- 
2.43.0

[PATCH 1/2] hw/usb/hcd-xhci: fail Address Device when the DCBAA entry is unreadable

[Junjie Cao]

xhci_address_slot() ignores the result of reading the slot's DCBAA
entry. When that read fails the output context pointer is whatever the
failed read left behind, zero for unassigned memory, so the slot and
ep0 contexts land at guest address 0 and the command completes with
Success.

Log the failed read and raise HCE, as the context DMA helpers do, and
return TRB Error, as Address Device already does for an unreadable
input control context. xhci_die() only flags HCE and leaves the
controller running; the HSE and halt of xHCI 1.2 section 4.10.2.6 are
not added here.

usb_xhci_post_load() reads the same entry; a failure there now disables
the slot, as an unresolvable slot context already does.

Fixes: 62c6ae04cf43 ("xhci: Initial xHCI implementation")
Fixes: 37352df30fbc ("xhci: add live migration support")
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/4706
Cc: qemu-stable@nongnu.org
Signed-off-by: Junjie Cao <junjie.cao@linux.dev>
---
 hw/usb/hcd-xhci.c | 41 +++++++++++++++++++++++++++--------------
 1 file changed, 27 insertions(+), 14 deletions(-)

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 5d7a77b8bc..b5cc1ad2ff 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -534,6 +534,23 @@ static inline void xhci_dma_write_u32s(XHCIState *xhci, dma_addr_t addr,
     }
 }
 
+static bool xhci_read_dcbaa(XHCIState *xhci, unsigned int slotid,
+                            dma_addr_t *octx)
+{
+    dma_addr_t dcbaap = xhci_addr64(xhci->dcbaap_low, xhci->dcbaap_high);
+    uint64_t poctx;
+
+    if (ldq_le_dma(xhci->as, dcbaap + 8 * slotid, &poctx,
+                   MEMTXATTRS_UNSPECIFIED) != MEMTX_OK) {
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: DMA memory access failed!\n",
+                      __func__);
+        xhci_die(xhci);
+        return false;
+    }
+    *octx = xhci_mask64(poctx);
+    return true;
+}
+
 static XHCIPort *xhci_lookup_port(XHCIState *xhci, struct USBPort *uport)
 {
     int index;
@@ -2103,8 +2120,7 @@ static TRBCCode xhci_address_slot(XHCIState *xhci, unsigned int slotid,
     XHCISlot *slot;
     USBPort *uport;
     USBDevice *dev;
-    dma_addr_t ictx, octx, dcbaap;
-    uint64_t poctx;
+    dma_addr_t ictx, octx;
     uint32_t ictl_ctx[2];
     uint32_t slot_ctx[4];
     uint32_t ep0_ctx[5];
@@ -2113,10 +2129,10 @@ static TRBCCode xhci_address_slot(XHCIState *xhci, unsigned int slotid,
 
     assert(slotid >= 1 && slotid <= xhci->numslots);
 
-    dcbaap = xhci_addr64(xhci->dcbaap_low, xhci->dcbaap_high);
-    ldq_le_dma(xhci->as, dcbaap + 8 * slotid, &poctx, MEMTXATTRS_UNSPECIFIED);
+    if (!xhci_read_dcbaa(xhci, slotid, &octx)) {
+        return CC_TRB_ERROR;
+    }
     ictx = xhci_mask64(pictx);
-    octx = xhci_mask64(poctx);
 
     DPRINTF("xhci: input context at "DMA_ADDR_FMT"\n", ictx);
     DPRINTF("xhci: output context at "DMA_ADDR_FMT"\n", octx);
@@ -3522,24 +3538,21 @@ static int usb_xhci_post_load(void *opaque, int version_id)
     XHCIState *xhci = opaque;
     XHCISlot *slot;
     XHCIEPContext *epctx;
-    dma_addr_t dcbaap, pctx;
+    dma_addr_t pctx;
     uint32_t slot_ctx[4];
     uint32_t ep_ctx[5];
     int slotid, epid, state;
-    uint64_t addr;
-
-    dcbaap = xhci_addr64(xhci->dcbaap_low, xhci->dcbaap_high);
 
     for (slotid = 1; slotid <= xhci->numslots; slotid++) {
         slot = &xhci->slots[slotid-1];
         if (!slot->addressed) {
             continue;
         }
-        ldq_le_dma(xhci->as, dcbaap + 8 * slotid, &addr, MEMTXATTRS_UNSPECIFIED);
-        slot->ctx = xhci_mask64(addr);
-
-        xhci_dma_read_u32s(xhci, slot->ctx, slot_ctx, sizeof(slot_ctx));
-        slot->uport = xhci_lookup_uport(xhci, slot_ctx);
+        slot->uport = NULL;
+        if (xhci_read_dcbaa(xhci, slotid, &slot->ctx)) {
+            xhci_dma_read_u32s(xhci, slot->ctx, slot_ctx, sizeof(slot_ctx));
+            slot->uport = xhci_lookup_uport(xhci, slot_ctx);
+        }
         if (!slot->uport) {
             /* should not happen, but may trigger on guest bugs */
             slot->enabled = 0;
-- 
2.43.0

[PATCH 2/2] tests/qtest/usb-hcd-xhci: test Address Device with an unreadable DCBAA

[Junjie Cao]

Point DCBAAP at unassigned memory before Address Device. The fixed
controller completes it with TRB Error and raises HCE; check that, and
that the 64 bytes at guest address 0, where the contexts used to be
written, stay intact. With DCBAAP restored the command then succeeds
on retry.

Signed-off-by: Junjie Cao <junjie.cao@linux.dev>
---
 tests/qtest/usb-hcd-xhci-test.c | 87 ++++++++++++++++++++++++++++-----
 1 file changed, 75 insertions(+), 12 deletions(-)

diff --git a/tests/qtest/usb-hcd-xhci-test.c b/tests/qtest/usb-hcd-xhci-test.c
index add2290bff..be09b838c5 100644
--- a/tests/qtest/usb-hcd-xhci-test.c
+++ b/tests/qtest/usb-hcd-xhci-test.c
@@ -56,6 +56,7 @@
 #define ER_TRANSFER             32
 #define ER_COMMAND_COMPLETE     33
 #define CC_SUCCESS              1
+#define CC_TRB_ERROR            5
 
 #define EP_TYPE_ISOCH_OUT       1
 #define EP_TYPE_CONTROL         4
@@ -64,6 +65,8 @@
 
 #define XHCI_RING_TRBS          64
 #define XHCI_MICROFRAME_NS      125000
+/* in the PCI hole of the pc machine, nothing is mapped there */
+#define XHCI_UNASSIGNED_ADDR    0xf0000000
 
 typedef struct XHCITest {
     QTestState *qts;
@@ -71,7 +74,7 @@ typedef struct XHCITest {
     QPCIBus *bus;
     struct qhc hc;
     uint32_t oper, runtime, doorbell;
-    uint64_t cmd_ring, event_ring, input_ctx;
+    uint64_t dcbaa, cmd_ring, event_ring, input_ctx;
     unsigned int cmd_idx, event_idx;
     unsigned int port, slot;
 } XHCITest;
@@ -226,8 +229,9 @@ static bool xhci_next_event(XHCITest *x, uint32_t *status, uint32_t *control)
     return true;
 }
 
-static unsigned int xhci_command(XHCITest *x, uint64_t parameter,
-                                 uint32_t control)
+/* Submit a command and return its completion code. */
+static unsigned int xhci_submit_command(XHCITest *x, uint64_t parameter,
+                                        uint32_t control, unsigned int *slot)
 {
     uint32_t status;
 
@@ -238,17 +242,30 @@ static unsigned int xhci_command(XHCITest *x, uint64_t parameter,
 
     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);
+    if (slot) {
+        *slot = TRB_GET_SLOT(control);
+    }
+    return TRB_GET_CCODE(status);
+}
+
+/* Submit a command that has to succeed and return the slot ID. */
+static unsigned int xhci_command(XHCITest *x, uint64_t parameter,
+                                 uint32_t control)
+{
+    unsigned int slot;
+
+    g_assert_cmpuint(xhci_submit_command(x, parameter, control, &slot), ==,
+                     CC_SUCCESS);
+    return slot;
 }
 
 /*
- * Start qemu-xhci with one USB device, run the controller and bring the
- * device to the Addressed state.
+ * Start qemu-xhci with one USB device, run the controller, enable a slot
+ * for the device and fill in the input context for Address Device.
  */
-static void xhci_test_start(XHCITest *x, const char *usb_device)
+static void xhci_test_init(XHCITest *x, const char *usb_device)
 {
-    uint64_t dcbaa, erst, ep0_ring;
+    uint64_t erst, ep0_ring;
     unsigned int maxports;
 
     memset(x, 0, sizeof(*x));
@@ -267,14 +284,14 @@ static void xhci_test_start(XHCITest *x, const char *usb_device)
     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);
+    x->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);
+    xhci_writeq(x, x->oper + XHCI_DCBAAP, x->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);
@@ -296,7 +313,7 @@ static void xhci_test_start(XHCITest *x, const char *usb_device)
     }
 
     x->slot = xhci_command(x, 0, TRB_TYPE(CR_ENABLE_SLOT));
-    qtest_writeq(x->qts, dcbaa + 8 * x->slot, xhci_alloc_page(x));
+    qtest_writeq(x->qts, x->dcbaa + 8 * x->slot, xhci_alloc_page(x));
 
     /* input control context: add slot and ep0 */
     qtest_writel(x->qts, x->input_ctx + 0x04, 0x3);
@@ -308,6 +325,12 @@ static void xhci_test_start(XHCITest *x, const char *usb_device)
     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_test_init(), then bring the device to the Addressed state. */
+static void xhci_test_start(XHCITest *x, const char *usb_device)
+{
+    xhci_test_init(x, usb_device);
     xhci_command(x, x->input_ctx,
                  TRB_TYPE(CR_ADDRESS_DEVICE) | (x->slot << 24));
 }
@@ -355,6 +378,44 @@ static bool xhci_test_supported(const char *usb_device)
     return true;
 }
 
+/*
+ * Address Device reads the output context pointer from the DCBAA. Point
+ * DCBAAP at unassigned memory: the command has to fail instead of carrying
+ * on with the zero the failed read leaves behind, which used to put the
+ * contexts at guest address 0 and still report Success.
+ */
+static void test_xhci_address_device_dcbaa_read_error(void)
+{
+    uint8_t fill[64], low_mem[sizeof(fill)];
+    unsigned int ccode;
+    XHCITest x;
+
+    if (!xhci_test_supported("usb-kbd")) {
+        return;
+    }
+
+    xhci_test_init(&x, "-device usb-kbd");
+    memset(fill, 0xaa, sizeof(fill));
+    qtest_memwrite(x.qts, 0, fill, sizeof(fill));
+    xhci_writeq(&x, x.oper + XHCI_DCBAAP, XHCI_UNASSIGNED_ADDR);
+
+    ccode = xhci_submit_command(&x, x.input_ctx,
+                                TRB_TYPE(CR_ADDRESS_DEVICE) | (x.slot << 24),
+                                NULL);
+    g_assert_cmpuint(ccode, ==, CC_TRB_ERROR);
+    g_assert_cmphex(xhci_readl(&x, x.oper + XHCI_USBSTS) & USBSTS_HCE, ==,
+                    USBSTS_HCE);
+    qtest_memread(x.qts, 0, low_mem, sizeof(low_mem));
+    g_assert_cmpmem(low_mem, sizeof(low_mem), fill, sizeof(fill));
+
+    /* The failed command left the slot usable. */
+    xhci_writeq(&x, x.oper + XHCI_DCBAAP, x.dcbaa);
+    xhci_command(&x, x.input_ctx,
+                 TRB_TYPE(CR_ADDRESS_DEVICE) | (x.slot << 24));
+
+    xhci_test_end(&x);
+}
+
 /*
  * 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.
@@ -481,6 +542,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/address-device/dcbaa-read-error",
+                   test_xhci_address_device_dcbaa_read_error);
     qtest_add_func("/xhci/pci/isoch/mfindex-32bit",
                    test_xhci_isoch_mfindex_32bit);
     qtest_add_func("/xhci/pci/intr/mfindex-32bit",
-- 
2.43.0