* [PATCH v4 01/22] hw/usb/xhci: Move HCD constants to a header and add register constants
2025-05-02 3:30 [PATCH v4 00/22] usb/xhci and usb/msd improvements and tests Nicholas Piggin
@ 2025-05-02 3:30 ` Nicholas Piggin
2025-05-12 12:25 ` Peter Maydell
2025-05-02 3:30 ` [PATCH v4 02/22] hw/usb/xhci: Rename and move HCD register region constants to header Nicholas Piggin
` (21 subsequent siblings)
22 siblings, 1 reply; 45+ messages in thread
From: Nicholas Piggin @ 2025-05-02 3:30 UTC (permalink / raw)
To: Gerd Hoffmann
Cc: Nicholas Piggin, qemu-devel, Kevin Wolf, Paolo Bonzini,
Michael S. Tsirkin, Marcel Apfelbaum, Fabiano Rosas,
Laurent Vivier, Phil Dennis-Jordan, Bernhard Beschow,
Philippe Mathieu-Daudé
Prepare to use some of these constants in xhci qtest code.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
hw/usb/hcd-xhci.h | 214 ++++++++++++++++++++++
hw/usb/hcd-xhci.c | 450 +++++++++++++++-------------------------------
2 files changed, 360 insertions(+), 304 deletions(-)
diff --git a/hw/usb/hcd-xhci.h b/hw/usb/hcd-xhci.h
index 9c3974f1489..ee364efd0ab 100644
--- a/hw/usb/hcd-xhci.h
+++ b/hw/usb/hcd-xhci.h
@@ -115,6 +115,220 @@ typedef enum TRBCCode {
CC_SPLIT_TRANSACTION_ERROR
} TRBCCode;
+/* Register definitions */
+#define XHCI_HCCAP_REG_CAPLENGTH 0x00
+#define XHCI_HCCAP_REG_HCIVERSION 0x02
+#define XHCI_HCCAP_REG_HCSPARAMS1 0x04
+#define XHCI_HCCAP_REG_HCSPARAMS2 0x08
+#define XHCI_HCCAP_REG_HCSPARAMS3 0x0C
+#define XHCI_HCCAP_REG_HCCPARAMS1 0x10
+#define XHCI_HCCPARAMS1_AC64 0x00000001
+#define XHCI_HCCPARAMS1_XECP_SHIFT 16
+#define XHCI_HCCPARAMS1_MAXPSASIZE_SHIFT 12
+#define XHCI_HCCAP_REG_DBOFF 0x14
+#define XHCI_HCCAP_REG_RTSOFF 0x18
+#define XHCI_HCCAP_REG_HCCPARAMS2 0x1C
+#define XHCI_HCCAP_EXTCAP_START 0x20 /* SW-defined */
+
+#define XHCI_PORT_PR_SZ 0x10
+#define XHCI_PORT_REG_PORTSC 0x00
+#define XHCI_PORTSC_CCS (1 << 0)
+#define XHCI_PORTSC_PED (1 << 1)
+#define XHCI_PORTSC_OCA (1 << 3)
+#define XHCI_PORTSC_PR (1 << 4)
+#define XHCI_PORTSC_PLS_SHIFT 5
+#define XHCI_PORTSC_PLS_MASK 0xf
+#define XHCI_PORTSC_PP (1 << 9)
+#define XHCI_PORTSC_SPEED_SHIFT 10
+#define XHCI_PORTSC_SPEED_MASK 0xf
+#define XHCI_PORTSC_SPEED_FULL (1 << 10)
+#define XHCI_PORTSC_SPEED_LOW (2 << 10)
+#define XHCI_PORTSC_SPEED_HIGH (3 << 10)
+#define XHCI_PORTSC_SPEED_SUPER (4 << 10)
+#define XHCI_PORTSC_PIC_SHIFT 14
+#define XHCI_PORTSC_PIC_MASK 0x3
+#define XHCI_PORTSC_LWS (1 << 16)
+#define XHCI_PORTSC_CSC (1 << 17)
+#define XHCI_PORTSC_PEC (1 << 18)
+#define XHCI_PORTSC_WRC (1 << 19)
+#define XHCI_PORTSC_OCC (1 << 20)
+#define XHCI_PORTSC_PRC (1 << 21)
+#define XHCI_PORTSC_PLC (1 << 22)
+#define XHCI_PORTSC_CEC (1 << 23)
+#define XHCI_PORTSC_CAS (1 << 24)
+#define XHCI_PORTSC_WCE (1 << 25)
+#define XHCI_PORTSC_WDE (1 << 26)
+#define XHCI_PORTSC_WOE (1 << 27)
+#define XHCI_PORTSC_DR (1 << 30)
+#define XHCI_PORTSC_WPR (1 << 31)
+/* read/write bits */
+#define XHCI_PORTSC_RW_MASK (XHCI_PORTSC_PP | \
+ XHCI_PORTSC_WCE | \
+ XHCI_PORTSC_WDE | \
+ XHCI_PORTSC_WOE)
+/* write-1-to-clear bits*/
+#define XHCI_PORTSC_W1C_MASK (XHCI_PORTSC_CSC | \
+ XHCI_PORTSC_PEC | \
+ XHCI_PORTSC_WRC | \
+ XHCI_PORTSC_OCC | \
+ XHCI_PORTSC_PRC | \
+ XHCI_PORTSC_PLC | \
+ XHCI_PORTSC_CEC)
+#define XHCI_PORT_REG_PORTPMSC 0x04
+#define XHCI_PORT_REG_PORTLI 0x08
+#define XHCI_PORT_REG_PORTHLPMC 0x0C
+
+#define XHCI_OPER_REG_USBCMD 0x00
+#define XHCI_USBCMD_RS (1 << 0)
+#define XHCI_USBCMD_HCRST (1 << 1)
+#define XHCI_USBCMD_INTE (1 << 2)
+#define XHCI_USBCMD_HSEE (1 << 3)
+#define XHCI_USBCMD_LHCRST (1 << 7)
+#define XHCI_USBCMD_CSS (1 << 8)
+#define XHCI_USBCMD_CRS (1 << 9)
+#define XHCI_USBCMD_EWE (1 << 10)
+#define XHCI_USBCMD_EU3S (1 << 11)
+#define XHCI_OPER_REG_USBSTS 0x04
+#define XHCI_USBSTS_HCH (1 << 0)
+#define XHCI_USBSTS_HSE (1 << 2)
+#define XHCI_USBSTS_EINT (1 << 3)
+#define XHCI_USBSTS_PCD (1 << 4)
+#define XHCI_USBSTS_SSS (1 << 8)
+#define XHCI_USBSTS_RSS (1 << 9)
+#define XHCI_USBSTS_SRE (1 << 10)
+#define XHCI_USBSTS_CNR (1 << 11)
+#define XHCI_USBSTS_HCE (1 << 12)
+/* these bits are write-1-to-clear */
+#define XHCI_USBSTS_W1C_MASK (XHCI_USBSTS_HSE | \
+ XHCI_USBSTS_EINT | \
+ XHCI_USBSTS_PCD | \
+ XHCI_USBSTS_SRE)
+#define XHCI_OPER_REG_PAGESIZE 0x08
+#define XHCI_OPER_REG_DNCTRL 0x14
+#define XHCI_OPER_REG_CRCR_LO 0x18
+#define XHCI_CRCR_RCS (1 << 0)
+#define XHCI_CRCR_CS (1 << 1)
+#define XHCI_CRCR_CA (1 << 2)
+#define XHCI_CRCR_CRR (1 << 3)
+#define XHCI_OPER_REG_CRCR_HI 0x1C
+#define XHCI_OPER_REG_DCBAAP_LO 0x30
+#define XHCI_OPER_REG_DCBAAP_HI 0x34
+#define XHCI_OPER_REG_CONFIG 0x38
+
+#define XHCI_DBELL_DB_SZ 0x4
+
+#define XHCI_INTR_REG_MFINDEX 0x00
+#define XHCI_INTR_REG_IR0 0x20
+#define XHCI_INTR_IR_SZ 0x20
+
+#define XHCI_INTR_REG_IMAN 0x00
+#define XHCI_IMAN_IP (1 << 0)
+#define XHCI_IMAN_IE (1 << 1)
+#define XHCI_INTR_REG_IMOD 0x04
+#define XHCI_INTR_REG_ERSTSZ 0x08
+#define XHCI_INTR_REG_ERSTBA_LO 0x10
+#define XHCI_INTR_REG_ERSTBA_HI 0x14
+#define XHCI_INTR_REG_ERDP_LO 0x18
+#define XHCI_ERDP_EHB (1 << 3)
+#define XHCI_INTR_REG_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 292c378bfc9..abd2002d2c0 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -47,8 +47,8 @@
#define TRANSFER_LIMIT 256
#define LEN_CAP 0x40
-#define LEN_OPER (0x400 + 0x10 * XHCI_MAXPORTS)
-#define LEN_RUNTIME ((XHCI_MAXINTRS + 1) * 0x20)
+#define LEN_OPER (0x400 + XHCI_PORT_PR_SZ * XHCI_MAXPORTS)
+#define LEN_RUNTIME ((XHCI_MAXINTRS + 1) * XHCI_INTR_IR_SZ)
#define LEN_DOORBELL ((XHCI_MAXSLOTS + 1) * 0x20)
#define OFF_OPER LEN_CAP
@@ -65,154 +65,6 @@
# 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)
@@ -223,17 +75,6 @@ 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;
@@ -440,7 +281,7 @@ static uint64_t xhci_mfindex_get(XHCIState *xhci)
static void xhci_mfwrap_update(XHCIState *xhci)
{
- const uint32_t bits = USBCMD_RS | USBCMD_EWE;
+ const uint32_t bits = XHCI_USBCMD_RS | XHCI_USBCMD_EWE;
uint32_t mfindex, left;
int64_t now;
@@ -465,7 +306,7 @@ static void xhci_mfwrap_timer(void *opaque)
static void xhci_die(XHCIState *xhci)
{
- xhci->usbsts |= USBSTS_HCE;
+ xhci->usbsts |= XHCI_USBSTS_HCE;
DPRINTF("xhci: asserted controller error\n");
}
@@ -557,51 +398,51 @@ static void xhci_intr_update(XHCIState *xhci, int v)
int level = 0;
if (v == 0) {
- if (xhci->intr[0].iman & IMAN_IP &&
- xhci->intr[0].iman & IMAN_IE &&
- xhci->usbcmd & USBCMD_INTE) {
+ if (xhci->intr[0].iman & XHCI_IMAN_IP &&
+ xhci->intr[0].iman & XHCI_IMAN_IE &&
+ xhci->usbcmd & XHCI_USBCMD_INTE) {
level = 1;
}
if (xhci->intr_raise) {
if (xhci->intr_raise(xhci, 0, level)) {
- xhci->intr[0].iman &= ~IMAN_IP;
+ xhci->intr[0].iman &= ~XHCI_IMAN_IP;
}
}
}
if (xhci->intr_update) {
xhci->intr_update(xhci, v,
- xhci->intr[v].iman & IMAN_IE);
+ xhci->intr[v].iman & XHCI_IMAN_IE);
}
}
static void xhci_intr_raise(XHCIState *xhci, int v)
{
- bool pending = (xhci->intr[v].erdp_low & ERDP_EHB);
+ bool pending = (xhci->intr[v].erdp_low & XHCI_ERDP_EHB);
- xhci->intr[v].erdp_low |= ERDP_EHB;
- xhci->intr[v].iman |= IMAN_IP;
- xhci->usbsts |= USBSTS_EINT;
+ xhci->intr[v].erdp_low |= XHCI_ERDP_EHB;
+ xhci->intr[v].iman |= XHCI_IMAN_IP;
+ xhci->usbsts |= XHCI_USBSTS_EINT;
if (pending) {
return;
}
- if (!(xhci->intr[v].iman & IMAN_IE)) {
+ if (!(xhci->intr[v].iman & XHCI_IMAN_IE)) {
return;
}
- if (!(xhci->usbcmd & USBCMD_INTE)) {
+ if (!(xhci->usbcmd & XHCI_USBCMD_INTE)) {
return;
}
if (xhci->intr_raise) {
if (xhci->intr_raise(xhci, v, true)) {
- xhci->intr[v].iman &= ~IMAN_IP;
+ xhci->intr[v].iman &= ~XHCI_IMAN_IP;
}
}
}
static inline int xhci_running(XHCIState *xhci)
{
- return !(xhci->usbsts & USBSTS_HCH);
+ return !(xhci->usbsts & XHCI_USBSTS_HCH);
}
static void xhci_write_event(XHCIState *xhci, XHCIEvent *event, int v)
@@ -846,15 +687,15 @@ static void xhci_er_reset(XHCIState *xhci, int v)
static void xhci_run(XHCIState *xhci)
{
trace_usb_xhci_run();
- xhci->usbsts &= ~USBSTS_HCH;
+ xhci->usbsts &= ~XHCI_USBSTS_HCH;
xhci->mfindex_start = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
}
static void xhci_stop(XHCIState *xhci)
{
trace_usb_xhci_stop();
- xhci->usbsts |= USBSTS_HCH;
- xhci->crcr_low &= ~CRCR_CRR;
+ xhci->usbsts |= XHCI_USBSTS_HCH;
+ xhci->crcr_low &= ~XHCI_CRCR_CRR;
}
static XHCIStreamContext *xhci_alloc_stream_contexts(unsigned count,
@@ -2481,7 +2322,7 @@ static void xhci_process_commands(XHCIState *xhci)
return;
}
- xhci->crcr_low |= CRCR_CRR;
+ xhci->crcr_low |= XHCI_CRCR_CRR;
while ((type = xhci_ring_fetch(xhci, &xhci->cmd_ring, &trb, &addr))) {
event.ptr = addr;
@@ -2633,32 +2474,32 @@ static void xhci_port_update(XHCIPort *port, int is_detach)
uint32_t pls = PLS_RX_DETECT;
assert(port);
- port->portsc = PORTSC_PP;
+ port->portsc = XHCI_PORTSC_PP;
if (!is_detach && xhci_port_have_device(port)) {
- port->portsc |= PORTSC_CCS;
+ port->portsc |= XHCI_PORTSC_CCS;
switch (port->uport->dev->speed) {
case USB_SPEED_LOW:
- port->portsc |= PORTSC_SPEED_LOW;
+ port->portsc |= XHCI_PORTSC_SPEED_LOW;
pls = PLS_POLLING;
break;
case USB_SPEED_FULL:
- port->portsc |= PORTSC_SPEED_FULL;
+ port->portsc |= XHCI_PORTSC_SPEED_FULL;
pls = PLS_POLLING;
break;
case USB_SPEED_HIGH:
- port->portsc |= PORTSC_SPEED_HIGH;
+ port->portsc |= XHCI_PORTSC_SPEED_HIGH;
pls = PLS_POLLING;
break;
case USB_SPEED_SUPER:
- port->portsc |= PORTSC_SPEED_SUPER;
- port->portsc |= PORTSC_PED;
+ port->portsc |= XHCI_PORTSC_SPEED_SUPER;
+ port->portsc |= XHCI_PORTSC_PED;
pls = PLS_U0;
break;
}
}
- set_field(&port->portsc, pls, PORTSC_PLS);
+ set_field(&port->portsc, pls, XHCI_PORTSC_PLS);
trace_usb_xhci_port_link(port->portnr, pls);
- xhci_port_notify(port, PORTSC_CSC);
+ xhci_port_notify(port, XHCI_PORTSC_CSC);
}
static void xhci_port_reset(XHCIPort *port, bool warm_reset)
@@ -2674,20 +2515,20 @@ static void xhci_port_reset(XHCIPort *port, bool warm_reset)
switch (port->uport->dev->speed) {
case USB_SPEED_SUPER:
if (warm_reset) {
- port->portsc |= PORTSC_WRC;
+ port->portsc |= XHCI_PORTSC_WRC;
}
/* fall through */
case USB_SPEED_LOW:
case USB_SPEED_FULL:
case USB_SPEED_HIGH:
- set_field(&port->portsc, PLS_U0, PORTSC_PLS);
+ set_field(&port->portsc, PLS_U0, XHCI_PORTSC_PLS);
trace_usb_xhci_port_link(port->portnr, PLS_U0);
- port->portsc |= PORTSC_PED;
+ port->portsc |= XHCI_PORTSC_PED;
break;
}
- port->portsc &= ~PORTSC_PR;
- xhci_port_notify(port, PORTSC_PRC);
+ port->portsc &= ~XHCI_PORTSC_PR;
+ xhci_port_notify(port, XHCI_PORTSC_PRC);
}
static void xhci_reset(DeviceState *dev)
@@ -2696,12 +2537,12 @@ static void xhci_reset(DeviceState *dev)
int i;
trace_usb_xhci_reset();
- if (!(xhci->usbsts & USBSTS_HCH)) {
+ if (!(xhci->usbsts & XHCI_USBSTS_HCH)) {
DPRINTF("xhci: reset while running!\n");
}
xhci->usbcmd = 0;
- xhci->usbsts = USBSTS_HCH;
+ xhci->usbsts = XHCI_USBSTS_HCH;
xhci->dnctrl = 0;
xhci->crcr_low = 0;
xhci->crcr_high = 0;
@@ -2742,56 +2583,56 @@ static uint64_t xhci_cap_read(void *ptr, hwaddr reg, unsigned size)
uint32_t ret;
switch (reg) {
- case 0x00: /* HCIVERSION, CAPLENGTH */
+ case XHCI_HCCAP_REG_CAPLENGTH: /* Covers HCIVERSION and CAPLENGTH */
ret = 0x01000000 | LEN_CAP;
break;
- case 0x04: /* HCSPARAMS 1 */
+ case XHCI_HCCAP_REG_HCSPARAMS1:
ret = ((xhci->numports_2+xhci->numports_3)<<24)
| (xhci->numintrs<<8) | xhci->numslots;
break;
- case 0x08: /* HCSPARAMS 2 */
+ case XHCI_HCCAP_REG_HCSPARAMS2:
ret = 0x0000000f;
break;
- case 0x0c: /* HCSPARAMS 3 */
+ case XHCI_HCCAP_REG_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_REG_HCCPARAMS1:
+ ret = (XHCI_HCCAP_EXTCAP_START / 4) << XHCI_HCCPARAMS1_XECP_SHIFT;
+ ret |= xhci->max_pstreams_mask << XHCI_HCCPARAMS1_MAXPSASIZE_SHIFT;
+ if (sizeof(dma_addr_t) == 8) {
+ ret |= XHCI_HCCPARAMS1_AC64;
}
break;
- case 0x14: /* DBOFF */
+ case XHCI_HCCAP_REG_DBOFF:
ret = OFF_DOORBELL;
break;
- case 0x18: /* RTSOFF */
+ case XHCI_HCCAP_REG_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:
@@ -2809,14 +2650,18 @@ static uint64_t xhci_port_read(void *ptr, hwaddr reg, unsigned size)
uint32_t ret;
switch (reg) {
- case 0x00: /* PORTSC */
+ case XHCI_PORT_REG_PORTSC:
ret = port->portsc;
break;
- case 0x04: /* PORTPMSC */
- case 0x08: /* PORTLI */
+ case XHCI_PORT_REG_PORTLI:
ret = 0;
break;
- case 0x0c: /* PORTHLPMC */
+ case XHCI_PORT_REG_PORTPMSC:
+ ret = 0;
+ qemu_log_mask(LOG_UNIMP, "%s: read from port register PORTHPMSC",
+ __func__);
+ break;
+ case XHCI_PORT_REG_PORTHLPMC:
ret = 0;
qemu_log_mask(LOG_UNIMP, "%s: read from port register PORTHLPMC",
__func__);
@@ -2841,37 +2686,35 @@ 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_REG_PORTSC:
/* write-1-to-start bits */
- if (val & PORTSC_WPR) {
+ if (val & XHCI_PORTSC_WPR) {
xhci_port_reset(port, true);
break;
}
- if (val & PORTSC_PR) {
+ if (val & XHCI_PORTSC_PR) {
xhci_port_reset(port, false);
break;
}
portsc = port->portsc;
notify = 0;
- /* write-1-to-clear bits*/
- portsc &= ~(val & (PORTSC_CSC|PORTSC_PEC|PORTSC_WRC|PORTSC_OCC|
- PORTSC_PRC|PORTSC_PLC|PORTSC_CEC));
- if (val & PORTSC_LWS) {
+ portsc &= ~(val & XHCI_PORTSC_W1C_MASK);
+ if (val & XHCI_PORTSC_LWS) {
/* overwrite PLS only when LWS=1 */
- uint32_t old_pls = get_field(port->portsc, PORTSC_PLS);
- uint32_t new_pls = get_field(val, PORTSC_PLS);
+ uint32_t old_pls = get_field(port->portsc, XHCI_PORTSC_PLS);
+ uint32_t new_pls = get_field(val, XHCI_PORTSC_PLS);
switch (new_pls) {
case PLS_U0:
if (old_pls != PLS_U0) {
- set_field(&portsc, new_pls, PORTSC_PLS);
+ set_field(&portsc, new_pls, XHCI_PORTSC_PLS);
trace_usb_xhci_port_link(port->portnr, new_pls);
- notify = PORTSC_PLC;
+ notify = XHCI_PORTSC_PLC;
}
break;
case PLS_U3:
if (old_pls < PLS_U3) {
- set_field(&portsc, new_pls, PORTSC_PLS);
+ set_field(&portsc, new_pls, XHCI_PORTSC_PLS);
trace_usb_xhci_port_link(port->portnr, new_pls);
}
break;
@@ -2884,22 +2727,21 @@ static void xhci_port_write(void *ptr, hwaddr reg,
break;
}
}
- /* read/write bits */
- portsc &= ~(PORTSC_PP|PORTSC_WCE|PORTSC_WDE|PORTSC_WOE);
- portsc |= (val & (PORTSC_PP|PORTSC_WCE|PORTSC_WDE|PORTSC_WOE));
+ portsc &= ~XHCI_PORTSC_RW_MASK;
+ portsc |= val & XHCI_PORTSC_RW_MASK;
port->portsc = portsc;
if (notify) {
xhci_port_notify(port, notify);
}
break;
- case 0x04: /* PORTPMSC */
- case 0x0c: /* PORTHLPMC */
+ case XHCI_PORT_REG_PORTPMSC:
+ case XHCI_PORT_REG_PORTHLPMC:
qemu_log_mask(LOG_UNIMP,
"%s: write 0x%" PRIx64
" (%u bytes) to port register at offset 0x%" HWADDR_PRIx,
__func__, val, size, reg);
break;
- case 0x08: /* PORTLI */
+ case XHCI_PORT_REG_PORTLI:
qemu_log_mask(LOG_GUEST_ERROR, "%s: Write to read-only PORTLI register",
__func__);
break;
@@ -2918,31 +2760,31 @@ static uint64_t xhci_oper_read(void *ptr, hwaddr reg, unsigned size)
uint32_t ret;
switch (reg) {
- case 0x00: /* USBCMD */
+ case XHCI_OPER_REG_USBCMD:
ret = xhci->usbcmd;
break;
- case 0x04: /* USBSTS */
+ case XHCI_OPER_REG_USBSTS:
ret = xhci->usbsts;
break;
- case 0x08: /* PAGESIZE */
+ case XHCI_OPER_REG_PAGESIZE:
ret = 1; /* 4KiB */
break;
- case 0x14: /* DNCTRL */
+ case XHCI_OPER_REG_DNCTRL:
ret = xhci->dnctrl;
break;
- case 0x18: /* CRCR low */
+ case XHCI_OPER_REG_CRCR_LO:
ret = xhci->crcr_low & ~0xe;
break;
- case 0x1c: /* CRCR high */
+ case XHCI_OPER_REG_CRCR_HI:
ret = xhci->crcr_high;
break;
- case 0x30: /* DCBAAP low */
+ case XHCI_OPER_REG_DCBAAP_LO:
ret = xhci->dcbaap_low;
break;
- case 0x34: /* DCBAAP high */
+ case XHCI_OPER_REG_DCBAAP_HI:
ret = xhci->dcbaap_high;
break;
- case 0x38: /* CONFIG */
+ case XHCI_OPER_REG_CONFIG:
ret = xhci->config;
break;
default:
@@ -2962,60 +2804,60 @@ static void xhci_oper_write(void *ptr, hwaddr reg,
trace_usb_xhci_oper_write(reg, val);
switch (reg) {
- case 0x00: /* USBCMD */
- if ((val & USBCMD_RS) && !(xhci->usbcmd & USBCMD_RS)) {
+ case XHCI_OPER_REG_USBCMD:
+ if ((val & XHCI_USBCMD_RS) && !(xhci->usbcmd & XHCI_USBCMD_RS)) {
xhci_run(xhci);
- } else if (!(val & USBCMD_RS) && (xhci->usbcmd & USBCMD_RS)) {
+ } else if (!(val & XHCI_USBCMD_RS) && (xhci->usbcmd & XHCI_USBCMD_RS)) {
xhci_stop(xhci);
}
- if (val & USBCMD_CSS) {
+ if (val & XHCI_USBCMD_CSS) {
/* save state */
- xhci->usbsts &= ~USBSTS_SRE;
+ xhci->usbsts &= ~XHCI_USBSTS_SRE;
}
- if (val & USBCMD_CRS) {
+ if (val & XHCI_USBCMD_CRS) {
/* restore state */
- xhci->usbsts |= USBSTS_SRE;
+ xhci->usbsts |= XHCI_USBSTS_SRE;
}
xhci->usbcmd = val & 0xc0f;
xhci_mfwrap_update(xhci);
- if (val & USBCMD_HCRST) {
+ if (val & XHCI_USBCMD_HCRST) {
xhci_reset(DEVICE(xhci));
}
xhci_intr_update(xhci, 0);
break;
- case 0x04: /* USBSTS */
- /* these bits are write-1-to-clear */
- xhci->usbsts &= ~(val & (USBSTS_HSE|USBSTS_EINT|USBSTS_PCD|USBSTS_SRE));
+ case XHCI_OPER_REG_USBSTS:
+ xhci->usbsts &= ~(val & XHCI_USBSTS_W1C_MASK);
xhci_intr_update(xhci, 0);
break;
- case 0x14: /* DNCTRL */
+ case XHCI_OPER_REG_DNCTRL:
xhci->dnctrl = val & 0xffff;
break;
- case 0x18: /* CRCR low */
- xhci->crcr_low = (val & 0xffffffcf) | (xhci->crcr_low & CRCR_CRR);
+ case XHCI_OPER_REG_CRCR_LO:
+ xhci->crcr_low = (val & 0xffffffcf) | (xhci->crcr_low & XHCI_CRCR_CRR);
break;
- case 0x1c: /* CRCR high */
+ case XHCI_OPER_REG_CRCR_HI:
xhci->crcr_high = val;
- if (xhci->crcr_low & (CRCR_CA|CRCR_CS) && (xhci->crcr_low & CRCR_CRR)) {
+ if (xhci->crcr_low & (XHCI_CRCR_CA | XHCI_CRCR_CS) &&
+ (xhci->crcr_low & XHCI_CRCR_CRR)) {
XHCIEvent event = {ER_COMMAND_COMPLETE, CC_COMMAND_RING_STOPPED};
- xhci->crcr_low &= ~CRCR_CRR;
+ xhci->crcr_low &= ~XHCI_CRCR_CRR;
xhci_event(xhci, &event, 0);
DPRINTF("xhci: command ring stopped (CRCR=%08x)\n", xhci->crcr_low);
} else {
dma_addr_t base = xhci_addr64(xhci->crcr_low & ~0x3f, val);
xhci_ring_init(xhci, &xhci->cmd_ring, base);
}
- xhci->crcr_low &= ~(CRCR_CA | CRCR_CS);
+ xhci->crcr_low &= ~(XHCI_CRCR_CA | XHCI_CRCR_CS);
break;
- case 0x30: /* DCBAAP low */
+ case XHCI_OPER_REG_DCBAAP_LO:
xhci->dcbaap_low = val & 0xffffffc0;
break;
- case 0x34: /* DCBAAP high */
+ case XHCI_OPER_REG_DCBAAP_HI:
xhci->dcbaap_high = val;
break;
- case 0x38: /* CONFIG */
+ case XHCI_OPER_REG_CONFIG:
xhci->config = val & 0xff;
break;
default:
@@ -3029,9 +2871,9 @@ static uint64_t xhci_runtime_read(void *ptr, hwaddr reg,
XHCIState *xhci = ptr;
uint32_t ret = 0;
- if (reg < 0x20) {
+ if (reg < XHCI_INTR_REG_IR0) {
switch (reg) {
- case 0x00: /* MFINDEX */
+ case XHCI_INTR_REG_MFINDEX:
ret = xhci_mfindex_get(xhci) & 0x3fff;
break;
default:
@@ -3039,28 +2881,28 @@ static uint64_t xhci_runtime_read(void *ptr, hwaddr reg,
break;
}
} else {
- int v = (reg - 0x20) / 0x20;
+ int v = (reg - XHCI_INTR_REG_IR0) / XHCI_INTR_IR_SZ;
XHCIInterrupter *intr = &xhci->intr[v];
- switch (reg & 0x1f) {
- case 0x00: /* IMAN */
+ switch (reg & (XHCI_INTR_IR_SZ - 1)) {
+ case XHCI_INTR_REG_IMAN:
ret = intr->iman;
break;
- case 0x04: /* IMOD */
+ case XHCI_INTR_REG_IMOD:
ret = intr->imod;
break;
- case 0x08: /* ERSTSZ */
+ case XHCI_INTR_REG_ERSTSZ:
ret = intr->erstsz;
break;
- case 0x10: /* ERSTBA low */
+ case XHCI_INTR_REG_ERSTBA_LO:
ret = intr->erstba_low;
break;
- case 0x14: /* ERSTBA high */
+ case XHCI_INTR_REG_ERSTBA_HI:
ret = intr->erstba_high;
break;
- case 0x18: /* ERDP low */
+ case XHCI_INTR_REG_ERDP_LO:
ret = intr->erdp_low;
break;
- case 0x1c: /* ERDP high */
+ case XHCI_INTR_REG_ERDP_HI:
ret = intr->erdp_high;
break;
}
@@ -3079,29 +2921,29 @@ static void xhci_runtime_write(void *ptr, hwaddr reg,
trace_usb_xhci_runtime_write(reg, val);
- if (reg < 0x20) {
+ if (reg < XHCI_INTR_REG_IR0) {
trace_usb_xhci_unimplemented("runtime write", reg);
return;
}
- v = (reg - 0x20) / 0x20;
+ v = (reg - XHCI_INTR_REG_IR0) / XHCI_INTR_IR_SZ;
intr = &xhci->intr[v];
- switch (reg & 0x1f) {
- case 0x00: /* IMAN */
- if (val & IMAN_IP) {
- intr->iman &= ~IMAN_IP;
+ switch (reg & (XHCI_INTR_IR_SZ - 1)) {
+ case XHCI_INTR_REG_IMAN:
+ if (val & XHCI_IMAN_IP) {
+ intr->iman &= ~XHCI_IMAN_IP;
}
- intr->iman &= ~IMAN_IE;
- intr->iman |= val & IMAN_IE;
+ intr->iman &= ~XHCI_IMAN_IE;
+ intr->iman |= val & XHCI_IMAN_IE;
xhci_intr_update(xhci, v);
break;
- case 0x04: /* IMOD */
+ case XHCI_INTR_REG_IMOD:
intr->imod = val;
break;
- case 0x08: /* ERSTSZ */
+ case XHCI_INTR_REG_ERSTSZ:
intr->erstsz = val & 0xffff;
break;
- case 0x10: /* ERSTBA low */
+ case XHCI_INTR_REG_ERSTBA_LO:
if (xhci->nec_quirks) {
/* NEC driver bug: it doesn't align this to 64 bytes */
intr->erstba_low = val & 0xfffffff0;
@@ -3109,16 +2951,17 @@ static void xhci_runtime_write(void *ptr, hwaddr reg,
intr->erstba_low = val & 0xffffffc0;
}
break;
- case 0x14: /* ERSTBA high */
+ case XHCI_INTR_REG_ERSTBA_HI:
intr->erstba_high = val;
xhci_er_reset(xhci, v);
break;
- case 0x18: /* ERDP low */
- if (val & ERDP_EHB) {
- intr->erdp_low &= ~ERDP_EHB;
+ case XHCI_INTR_REG_ERDP_LO:
+ if (val & XHCI_ERDP_EHB) {
+ intr->erdp_low &= ~XHCI_ERDP_EHB;
}
- intr->erdp_low = (val & ~ERDP_EHB) | (intr->erdp_low & ERDP_EHB);
- if (val & ERDP_EHB) {
+ intr->erdp_low = (val & ~XHCI_ERDP_EHB) |
+ (intr->erdp_low & XHCI_ERDP_EHB);
+ if (val & XHCI_ERDP_EHB) {
dma_addr_t erdp = xhci_addr64(intr->erdp_low, intr->erdp_high);
unsigned int dp_idx = (erdp - intr->er_start) / TRB_SIZE;
if (erdp >= intr->er_start &&
@@ -3128,7 +2971,7 @@ static void xhci_runtime_write(void *ptr, hwaddr reg,
}
}
break;
- case 0x1c: /* ERDP high */
+ case XHCI_INTR_REG_ERDP_HI:
intr->erdp_high = val;
break;
default:
@@ -3157,8 +3000,7 @@ static void xhci_doorbell_write(void *ptr, hwaddr reg,
return;
}
- reg >>= 2;
-
+ reg /= XHCI_DBELL_DB_SZ;
if (reg == 0) {
if (val == 0) {
xhci_process_commands(xhci);
@@ -3251,11 +3093,11 @@ static void xhci_wakeup(USBPort *usbport)
XHCIPort *port = xhci_lookup_port(xhci, usbport);
assert(port);
- if (get_field(port->portsc, PORTSC_PLS) != PLS_U3) {
+ if (get_field(port->portsc, XHCI_PORTSC_PLS) != PLS_U3) {
return;
}
- set_field(&port->portsc, PLS_RESUME, PORTSC_PLS);
- xhci_port_notify(port, PORTSC_PLC);
+ set_field(&port->portsc, PLS_RESUME, XHCI_PORTSC_PLS);
+ xhci_port_notify(port, XHCI_PORTSC_PLC);
}
static void xhci_complete(USBPort *port, USBPacket *packet)
@@ -3342,7 +3184,7 @@ static void usb_xhci_init(XHCIState *xhci)
XHCIPort *port;
unsigned int i, usbports, speedmask;
- xhci->usbsts = USBSTS_HCH;
+ xhci->usbsts = XHCI_USBSTS_HCH;
if (xhci->numports_2 > XHCI_MAXPORTS_2) {
xhci->numports_2 = XHCI_MAXPORTS_2;
@@ -3430,10 +3272,10 @@ static void usb_xhci_realize(DeviceState *dev, Error **errp)
for (i = 0; i < xhci->numports; i++) {
XHCIPort *port = &xhci->ports[i];
- uint32_t offset = OFF_OPER + 0x400 + 0x10 * i;
+ uint32_t offset = OFF_OPER + 0x400 + XHCI_PORT_PR_SZ * i;
port->xhci = xhci;
memory_region_init_io(&port->mem, OBJECT(dev), &xhci_port_ops, port,
- port->name, 0x10);
+ port->name, XHCI_PORT_PR_SZ);
memory_region_add_subregion(&xhci->mem, offset, &port->mem);
}
}
--
2.47.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v4 01/22] hw/usb/xhci: Move HCD constants to a header and add register constants
2025-05-02 3:30 ` [PATCH v4 01/22] hw/usb/xhci: Move HCD constants to a header and add register constants Nicholas Piggin
@ 2025-05-12 12:25 ` Peter Maydell
0 siblings, 0 replies; 45+ messages in thread
From: Peter Maydell @ 2025-05-12 12:25 UTC (permalink / raw)
To: Nicholas Piggin
Cc: Gerd Hoffmann, qemu-devel, Kevin Wolf, Paolo Bonzini,
Michael S. Tsirkin, Marcel Apfelbaum, Fabiano Rosas,
Laurent Vivier, Phil Dennis-Jordan, Bernhard Beschow,
Philippe Mathieu-Daudé
On Fri, 2 May 2025 at 04:31, Nicholas Piggin <npiggin@gmail.com> wrote:
>
> Prepare to use some of these constants in xhci qtest code.
This commit is doing more than it says in the commit message.
It also:
* renames some of the constants (mostly prepending XHCI_)
* adds new constants, e.g. for the register offsets and
for some shift amounts, where the original code used
hardcoded values
* makes at least one actual code logic change:
> - 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_REG_HCCPARAMS1:
> + ret = (XHCI_HCCAP_EXTCAP_START / 4) << XHCI_HCCPARAMS1_XECP_SHIFT;
> + ret |= xhci->max_pstreams_mask << XHCI_HCCPARAMS1_MAXPSASIZE_SHIFT;
> + if (sizeof(dma_addr_t) == 8) {
> + ret |= XHCI_HCCPARAMS1_AC64;
> }
> break;
It's really tricky to review when you have a single
big patch like this that does more than a simple
mechanical transformation. Could you split it up, please?
thanks
-- PMM
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v4 02/22] hw/usb/xhci: Rename and move HCD register region constants to header
2025-05-02 3:30 [PATCH v4 00/22] usb/xhci and usb/msd improvements and tests Nicholas Piggin
2025-05-02 3:30 ` [PATCH v4 01/22] hw/usb/xhci: Move HCD constants to a header and add register constants Nicholas Piggin
@ 2025-05-02 3:30 ` Nicholas Piggin
2025-05-12 12:29 ` Peter Maydell
2025-05-02 3:30 ` [PATCH v4 03/22] tests/qtest/xhci: test the qemu-xhci device Nicholas Piggin
` (20 subsequent siblings)
22 siblings, 1 reply; 45+ messages in thread
From: Nicholas Piggin @ 2025-05-02 3:30 UTC (permalink / raw)
To: Gerd Hoffmann
Cc: Nicholas Piggin, qemu-devel, Kevin Wolf, Paolo Bonzini,
Michael S. Tsirkin, Marcel Apfelbaum, Fabiano Rosas,
Laurent Vivier, Phil Dennis-Jordan, Bernhard Beschow,
Philippe Mathieu-Daudé
This also adds some missing constants rather than open-coding
offsets and sizes.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
hw/usb/hcd-xhci.h | 16 ++++++++++++++++
hw/usb/hcd-xhci.c | 48 ++++++++++++++++++++++-------------------------
2 files changed, 38 insertions(+), 26 deletions(-)
diff --git a/hw/usb/hcd-xhci.h b/hw/usb/hcd-xhci.h
index ee364efd0ab..20059fcf66c 100644
--- a/hw/usb/hcd-xhci.h
+++ b/hw/usb/hcd-xhci.h
@@ -115,6 +115,22 @@ typedef enum TRBCCode {
CC_SPLIT_TRANSACTION_ERROR
} TRBCCode;
+/* Register regions */
+#define XHCI_REGS_LENGTH_CAP 0x40
+#define XHCI_REGS_LENGTH_OPER 0x400
+#define XHCI_REGS_LENGTH_PORT (XHCI_PORT_PR_SZ * XHCI_MAXPORTS)
+#define XHCI_REGS_LENGTH_RUNTIME ((XHCI_MAXINTRS + 1) * XHCI_INTR_IR_SZ)
+/* XXX: Should doorbell length be *4 rather than *32? */
+#define XHCI_REGS_LENGTH_DOORBELL ((XHCI_MAXSLOTS + 1) * 0x20)
+
+#define XHCI_REGS_OFFSET_CAP 0
+#define XHCI_REGS_OFFSET_OPER (XHCI_REGS_OFFSET_CAP + \
+ XHCI_REGS_LENGTH_CAP)
+#define XHCI_REGS_OFFSET_PORT (XHCI_REGS_OFFSET_OPER + \
+ XHCI_REGS_LENGTH_OPER)
+#define XHCI_REGS_OFFSET_RUNTIME 0x1000
+#define XHCI_REGS_OFFSET_DOORBELL 0x2000
+
/* Register definitions */
#define XHCI_HCCAP_REG_CAPLENGTH 0x00
#define XHCI_HCCAP_REG_HCIVERSION 0x02
diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index abd2002d2c0..c12b72cb9d8 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -46,22 +46,14 @@
#define COMMAND_LIMIT 256
#define TRANSFER_LIMIT 256
-#define LEN_CAP 0x40
-#define LEN_OPER (0x400 + XHCI_PORT_PR_SZ * XHCI_MAXPORTS)
-#define LEN_RUNTIME ((XHCI_MAXINTRS + 1) * XHCI_INTR_IR_SZ)
-#define LEN_DOORBELL ((XHCI_MAXSLOTS + 1) * 0x20)
-
-#define OFF_OPER LEN_CAP
-#define OFF_RUNTIME 0x1000
-#define OFF_DOORBELL 0x2000
-
-#if (OFF_OPER + LEN_OPER) > OFF_RUNTIME
-#error Increase OFF_RUNTIME
+#if (XHCI_REGS_OFFSET_PORT + XHCI_REGS_LENGTH_PORT) > XHCI_REGS_OFFSET_RUNTIME
+#error Increase XHCI_REGS_OFFSET_RUNTIME
#endif
-#if (OFF_RUNTIME + LEN_RUNTIME) > OFF_DOORBELL
-#error Increase OFF_DOORBELL
+#if (XHCI_REGS_OFFSET_RUNTIME + XHCI_REGS_LENGTH_RUNTIME) > \
+ XHCI_REGS_OFFSET_DOORBELL
+#error Increase XHCI_REGS_OFFSET_DOORBELL
#endif
-#if (OFF_DOORBELL + LEN_DOORBELL) > XHCI_LEN_REGS
+#if (XHCI_REGS_OFFSET_DOORBELL + XHCI_REGS_LENGTH_DOORBELL) > XHCI_LEN_REGS
# error Increase XHCI_LEN_REGS
#endif
@@ -2584,7 +2576,7 @@ static uint64_t xhci_cap_read(void *ptr, hwaddr reg, unsigned size)
switch (reg) {
case XHCI_HCCAP_REG_CAPLENGTH: /* Covers HCIVERSION and CAPLENGTH */
- ret = 0x01000000 | LEN_CAP;
+ ret = 0x01000000 | XHCI_REGS_LENGTH_CAP;
break;
case XHCI_HCCAP_REG_HCSPARAMS1:
ret = ((xhci->numports_2+xhci->numports_3)<<24)
@@ -2604,10 +2596,10 @@ static uint64_t xhci_cap_read(void *ptr, hwaddr reg, unsigned size)
}
break;
case XHCI_HCCAP_REG_DBOFF:
- ret = OFF_DOORBELL;
+ ret = XHCI_REGS_OFFSET_DOORBELL;
break;
case XHCI_HCCAP_REG_RTSOFF:
- ret = OFF_RUNTIME;
+ ret = XHCI_REGS_OFFSET_RUNTIME;
break;
/* extended capabilities */
@@ -3257,22 +3249,26 @@ static void usb_xhci_realize(DeviceState *dev, Error **errp)
memory_region_init(&xhci->mem, OBJECT(dev), "xhci", XHCI_LEN_REGS);
memory_region_init_io(&xhci->mem_cap, OBJECT(dev), &xhci_cap_ops, xhci,
- "capabilities", LEN_CAP);
+ "capabilities", XHCI_REGS_LENGTH_CAP);
memory_region_init_io(&xhci->mem_oper, OBJECT(dev), &xhci_oper_ops, xhci,
- "operational", 0x400);
+ "operational", XHCI_REGS_LENGTH_OPER);
memory_region_init_io(&xhci->mem_runtime, OBJECT(dev), &xhci_runtime_ops,
- xhci, "runtime", LEN_RUNTIME);
+ xhci, "runtime", XHCI_REGS_LENGTH_RUNTIME);
memory_region_init_io(&xhci->mem_doorbell, OBJECT(dev), &xhci_doorbell_ops,
- xhci, "doorbell", LEN_DOORBELL);
+ xhci, "doorbell", XHCI_REGS_LENGTH_DOORBELL);
- memory_region_add_subregion(&xhci->mem, 0, &xhci->mem_cap);
- memory_region_add_subregion(&xhci->mem, OFF_OPER, &xhci->mem_oper);
- memory_region_add_subregion(&xhci->mem, OFF_RUNTIME, &xhci->mem_runtime);
- memory_region_add_subregion(&xhci->mem, OFF_DOORBELL, &xhci->mem_doorbell);
+ memory_region_add_subregion(&xhci->mem, XHCI_REGS_OFFSET_CAP,
+ &xhci->mem_cap);
+ memory_region_add_subregion(&xhci->mem, XHCI_REGS_OFFSET_OPER,
+ &xhci->mem_oper);
+ memory_region_add_subregion(&xhci->mem, XHCI_REGS_OFFSET_RUNTIME,
+ &xhci->mem_runtime);
+ memory_region_add_subregion(&xhci->mem, XHCI_REGS_OFFSET_DOORBELL,
+ &xhci->mem_doorbell);
for (i = 0; i < xhci->numports; i++) {
XHCIPort *port = &xhci->ports[i];
- uint32_t offset = OFF_OPER + 0x400 + XHCI_PORT_PR_SZ * i;
+ uint32_t offset = XHCI_REGS_OFFSET_PORT + XHCI_PORT_PR_SZ * i;
port->xhci = xhci;
memory_region_init_io(&port->mem, OBJECT(dev), &xhci_port_ops, port,
port->name, XHCI_PORT_PR_SZ);
--
2.47.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v4 02/22] hw/usb/xhci: Rename and move HCD register region constants to header
2025-05-02 3:30 ` [PATCH v4 02/22] hw/usb/xhci: Rename and move HCD register region constants to header Nicholas Piggin
@ 2025-05-12 12:29 ` Peter Maydell
0 siblings, 0 replies; 45+ messages in thread
From: Peter Maydell @ 2025-05-12 12:29 UTC (permalink / raw)
To: Nicholas Piggin
Cc: Gerd Hoffmann, qemu-devel, Kevin Wolf, Paolo Bonzini,
Michael S. Tsirkin, Marcel Apfelbaum, Fabiano Rosas,
Laurent Vivier, Phil Dennis-Jordan, Bernhard Beschow,
Philippe Mathieu-Daudé
On Fri, 2 May 2025 at 04:34, Nicholas Piggin <npiggin@gmail.com> wrote:
>
> This also adds some missing constants rather than open-coding
> offsets and sizes.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> hw/usb/hcd-xhci.h | 16 ++++++++++++++++
> hw/usb/hcd-xhci.c | 48 ++++++++++++++++++++++-------------------------
> 2 files changed, 38 insertions(+), 26 deletions(-)
>
> diff --git a/hw/usb/hcd-xhci.h b/hw/usb/hcd-xhci.h
> index ee364efd0ab..20059fcf66c 100644
> --- a/hw/usb/hcd-xhci.h
> +++ b/hw/usb/hcd-xhci.h
> @@ -115,6 +115,22 @@ typedef enum TRBCCode {
> CC_SPLIT_TRANSACTION_ERROR
> } TRBCCode;
>
> +/* Register regions */
> +#define XHCI_REGS_LENGTH_CAP 0x40
> +#define XHCI_REGS_LENGTH_OPER 0x400
Old code defines LEN_OPER as
(0x400 + XHCI_PORT_PR_SZ * XHCI_MAXPORTS), not just 0x400.
If this is fixing a bug, please keep the bug fixes in
their own commits, not mixed in with renaming or moving
constant definitions.
> diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
> index abd2002d2c0..c12b72cb9d8 100644
> --- a/hw/usb/hcd-xhci.c
> +++ b/hw/usb/hcd-xhci.c
> @@ -46,22 +46,14 @@
> #define COMMAND_LIMIT 256
> #define TRANSFER_LIMIT 256
>
> -#define LEN_CAP 0x40
> -#define LEN_OPER (0x400 + XHCI_PORT_PR_SZ * XHCI_MAXPORTS)
> -#define LEN_RUNTIME ((XHCI_MAXINTRS + 1) * XHCI_INTR_IR_SZ)
> -#define LEN_DOORBELL ((XHCI_MAXSLOTS + 1) * 0x20)
> for (i = 0; i < xhci->numports; i++) {
> XHCIPort *port = &xhci->ports[i];
> - uint32_t offset = OFF_OPER + 0x400 + XHCI_PORT_PR_SZ * i;
> + uint32_t offset = XHCI_REGS_OFFSET_PORT + XHCI_PORT_PR_SZ * i;
Old code uses OPER offset, new code is using PORT offset.
> port->xhci = xhci;
> memory_region_init_io(&port->mem, OBJECT(dev), &xhci_port_ops, port,
> port->name, XHCI_PORT_PR_SZ);
> --
> 2.47.1
thanks
-- PMM
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v4 03/22] tests/qtest/xhci: test the qemu-xhci device
2025-05-02 3:30 [PATCH v4 00/22] usb/xhci and usb/msd improvements and tests Nicholas Piggin
2025-05-02 3:30 ` [PATCH v4 01/22] hw/usb/xhci: Move HCD constants to a header and add register constants Nicholas Piggin
2025-05-02 3:30 ` [PATCH v4 02/22] hw/usb/xhci: Rename and move HCD register region constants to header Nicholas Piggin
@ 2025-05-02 3:30 ` Nicholas Piggin
2025-05-19 13:54 ` Fabiano Rosas
2025-05-02 3:30 ` [PATCH v4 04/22] tests/qtest/xhci: Add controller and device setup and ring tests Nicholas Piggin
` (19 subsequent siblings)
22 siblings, 1 reply; 45+ messages in thread
From: Nicholas Piggin @ 2025-05-02 3:30 UTC (permalink / raw)
To: Gerd Hoffmann
Cc: Nicholas Piggin, qemu-devel, Kevin Wolf, Paolo Bonzini,
Michael S. Tsirkin, Marcel Apfelbaum, Fabiano Rosas,
Laurent Vivier, Phil Dennis-Jordan, Bernhard Beschow,
Philippe Mathieu-Daudé
Add support in the test code for running multiple drivers, and add
tests for the qemu-xhci device.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
tests/qtest/usb-hcd-xhci-test.c | 190 +++++++++++++++++++++++++++++---
1 file changed, 176 insertions(+), 14 deletions(-)
diff --git a/tests/qtest/usb-hcd-xhci-test.c b/tests/qtest/usb-hcd-xhci-test.c
index 0cccfd85a64..abdd52c444c 100644
--- a/tests/qtest/usb-hcd-xhci-test.c
+++ b/tests/qtest/usb-hcd-xhci-test.c
@@ -8,17 +8,147 @@
*/
#include "qemu/osdep.h"
+#include "libqtest.h"
#include "libqtest-single.h"
+#include "libqos/libqos.h"
+#include "libqos/libqos-pc.h"
#include "libqos/usb.h"
+#include "hw/pci/pci.h"
+#include "hw/pci/pci_ids.h"
-static void test_xhci_hotplug(void)
+typedef struct TestData {
+ const char *device;
+ uint32_t fingerprint;
+} TestData;
+
+/*** Test Setup & Teardown ***/
+typedef struct XHCIQState {
+ /* QEMU PCI variables */
+ QOSState *parent;
+ QPCIDevice *dev;
+ QPCIBar bar;
+ uint64_t barsize;
+ uint32_t fingerprint;
+} XHCIQState;
+
+#define XHCI_QEMU_ID (PCI_DEVICE_ID_REDHAT_XHCI << 16 | \
+ PCI_VENDOR_ID_REDHAT)
+#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)
+{
+ QPCIDevice *xhci;
+ 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);
+
+ 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 = qpci_config_readl(s->dev, PCI_VENDOR_ID);
+ 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;
+
+ va_start(ap, cli);
+ s = xhci_vboot(cli, ap);
+ va_end(ap);
+
+ return s;
+}
+
+static XHCIQState *xhci_boot_dev(const char *device, uint32_t fingerprint)
+{
+ XHCIQState *s;
+
+ s = xhci_boot("-M q35 "
+ "-device %s,id=xhci,bus=pcie.0,addr=1d.0 "
+ "-drive id=drive0,if=none,file=null-co://,"
+ "file.read-zeroes=on,format=raw", device);
+ g_assert_cmphex(s->fingerprint, ==, fingerprint);
+
+ 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(const void *arg)
{
- usb_test_hotplug(global_qtest, "xhci", "1", NULL);
+ const TestData *td = arg;
+ XHCIQState *s;
+ QTestState *qts;
+
+ s = xhci_boot_dev(td->device, td->fingerprint);
+ qts = s->parent->qts;
+
+ usb_test_hotplug(qts, "xhci", "1", NULL);
+
+ xhci_shutdown(s);
}
-static void test_usb_uas_hotplug(void)
+static void test_usb_uas_hotplug(const void *arg)
{
- QTestState *qts = global_qtest;
+ const TestData *td = arg;
+ XHCIQState *s;
+ QTestState *qts;
+
+ s = xhci_boot_dev(td->device, td->fingerprint);
+ qts = s->parent->qts;
qtest_qmp_device_add(qts, "usb-uas", "uas", "{}");
qtest_qmp_device_add(qts, "scsi-hd", "scsihd", "{'drive': 'drive0'}");
@@ -32,9 +162,14 @@ static void test_usb_uas_hotplug(void)
qtest_qmp_device_del(qts, "uas");
}
-static void test_usb_ccid_hotplug(void)
+static void test_usb_ccid_hotplug(const void *arg)
{
- QTestState *qts = global_qtest;
+ const TestData *td = arg;
+ XHCIQState *s;
+ QTestState *qts;
+
+ s = xhci_boot_dev(td->device, td->fingerprint);
+ qts = s->parent->qts;
qtest_qmp_device_add(qts, "usb-ccid", "ccid", "{}");
qtest_qmp_device_del(qts, "ccid");
@@ -43,23 +178,50 @@ static void test_usb_ccid_hotplug(void)
qtest_qmp_device_del(qts, "ccid");
}
+static void add_test(const char *name, TestData *td, void (*fn)(const void *))
+{
+ g_autofree char *full_name = g_strdup_printf(
+ "/xhci/pci/%s/%s", td->device, name);
+ qtest_add_data_func(full_name, td, fn);
+}
+
+static void add_tests(TestData *td)
+{
+ add_test("hotplug", td, test_xhci_hotplug);
+ if (qtest_has_device("usb-uas")) {
+ add_test("usb-uas", td, test_usb_uas_hotplug);
+ }
+ if (qtest_has_device("usb-ccid")) {
+ add_test("usb-ccid", td, test_usb_ccid_hotplug);
+ }
+}
+
+/* tests */
int main(int argc, char **argv)
{
int ret;
+ const char *arch;
+ int i;
+ TestData td[] = {
+ { .device = "qemu-xhci", .fingerprint = XHCI_QEMU_ID, },
+ { .device = "nec-usb-xhci", .fingerprint = XHCI_NEC_ID, },
+ };
g_test_init(&argc, &argv, NULL);
- 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);
+ /* 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("usb-ccid")) {
- qtest_add_func("/xhci/pci/hotplug/usb-ccid", test_usb_ccid_hotplug);
+
+ for (i = 0; i < ARRAY_SIZE(td); i++) {
+ if (qtest_has_device(td[i].device)) {
+ add_tests(&td[i]);
+ }
}
- 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();
--
2.47.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v4 03/22] tests/qtest/xhci: test the qemu-xhci device
2025-05-02 3:30 ` [PATCH v4 03/22] tests/qtest/xhci: test the qemu-xhci device Nicholas Piggin
@ 2025-05-19 13:54 ` Fabiano Rosas
0 siblings, 0 replies; 45+ messages in thread
From: Fabiano Rosas @ 2025-05-19 13:54 UTC (permalink / raw)
To: Nicholas Piggin, Gerd Hoffmann
Cc: Nicholas Piggin, qemu-devel, Kevin Wolf, Paolo Bonzini,
Michael S. Tsirkin, Marcel Apfelbaum, Laurent Vivier,
Phil Dennis-Jordan, Bernhard Beschow, Philippe Mathieu-Daudé
Nicholas Piggin <npiggin@gmail.com> writes:
> Add support in the test code for running multiple drivers, and add
> tests for the qemu-xhci device.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
Reviewed-by: Fabiano Rosas <farosas@suse.de>
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v4 04/22] tests/qtest/xhci: Add controller and device setup and ring tests
2025-05-02 3:30 [PATCH v4 00/22] usb/xhci and usb/msd improvements and tests Nicholas Piggin
` (2 preceding siblings ...)
2025-05-02 3:30 ` [PATCH v4 03/22] tests/qtest/xhci: test the qemu-xhci device Nicholas Piggin
@ 2025-05-02 3:30 ` Nicholas Piggin
2025-05-19 14:03 ` Fabiano Rosas
2025-05-02 3:30 ` [PATCH v4 05/22] tests/qtest/xhci: Add basic USB Mass Storage tests Nicholas Piggin
` (18 subsequent siblings)
22 siblings, 1 reply; 45+ messages in thread
From: Nicholas Piggin @ 2025-05-02 3:30 UTC (permalink / raw)
To: Gerd Hoffmann
Cc: Nicholas Piggin, qemu-devel, Kevin Wolf, Paolo Bonzini,
Michael S. Tsirkin, Marcel Apfelbaum, Fabiano Rosas,
Laurent Vivier, Phil Dennis-Jordan, Bernhard Beschow,
Philippe Mathieu-Daudé
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.
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 | 336 ++++++++++++++++++++++++++++++++
3 files changed, 343 insertions(+), 7 deletions(-)
diff --git a/hw/usb/hcd-xhci.h b/hw/usb/hcd-xhci.h
index 20059fcf66c..02a005ce78d 100644
--- a/hw/usb/hcd-xhci.h
+++ b/hw/usb/hcd-xhci.h
@@ -350,6 +350,13 @@ 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 c12b72cb9d8..ef9f2a7db41 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -128,13 +128,6 @@ 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 abdd52c444c..291d1dfc36e 100644
--- a/tests/qtest/usb-hcd-xhci-test.c
+++ b/tests/qtest/usb-hcd-xhci-test.c
@@ -8,6 +8,7 @@
*/
#include "qemu/osdep.h"
+#include "qemu/bswap.h"
#include "libqtest.h"
#include "libqtest-single.h"
#include "libqos/libqos.h"
@@ -15,6 +16,8 @@
#include "libqos/usb.h"
#include "hw/pci/pci.h"
#include "hw/pci/pci_ids.h"
+#include "hw/pci/pci_regs.h"
+#include "hw/usb/hcd-xhci.h"
typedef struct TestData {
const char *device;
@@ -22,6 +25,22 @@ typedef struct TestData {
} TestData;
/*** Test Setup & Teardown ***/
+
+/* Transfer-Ring state */
+typedef struct XHCIQTRState {
+ uint64_t addr; /* In-memory ring */
+
+ uint32_t trb_entries;
+ uint32_t trb_idx;
+ uint32_t trb_c;
+} XHCIQTRState;
+
+typedef struct XHCIQSlotState {
+ /* In-memory device context array */
+ uint64_t device_context;
+ XHCIQTRState transfer_ring[31]; /* 1 for each EP */
+} XHCIQSlotState;
+
typedef struct XHCIQState {
/* QEMU PCI variables */
QOSState *parent;
@@ -29,6 +48,21 @@ typedef struct XHCIQState {
QPCIBar bar;
uint64_t barsize;
uint32_t fingerprint;
+
+ /* In-memory arrays */
+ uint64_t dc_base_array;
+ uint64_t event_ring_seg;
+ XHCIQTRState command_ring;
+ XHCIQTRState event_ring;
+
+ /* Host controller properties */
+ uint32_t rtoff, dboff;
+ uint32_t maxports, maxslots, maxintrs;
+
+ /* Current properties */
+ uint32_t slotid; /* enabled slot id (only enable one) */
+
+ XHCIQSlotState slots[32];
} XHCIQState;
#define XHCI_QEMU_ID (PCI_DEVICE_ID_REDHAT_XHCI << 16 | \
@@ -160,6 +194,8 @@ static void test_usb_uas_hotplug(const void *arg)
qtest_qmp_device_del(qts, "scsihd");
qtest_qmp_device_del(qts, "uas");
+
+ xhci_shutdown(s);
}
static void test_usb_ccid_hotplug(const void *arg)
@@ -176,6 +212,305 @@ static void test_usb_ccid_hotplug(const void *arg)
/* 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)
+{
+ uint64_t ret;
+
+ ret = guest_alloc(&s->parent->alloc, size);
+ g_assert(ret);
+ qtest_memset(s->parent->qts, ret, 0, size);
+
+ return ret;
+}
+
+static uint32_t xhci_cap_readl(XHCIQState *s, uint64_t addr)
+{
+ return qpci_io_readl(s->dev, s->bar, XHCI_REGS_OFFSET_CAP + addr);
+}
+
+static uint32_t xhci_op_readl(XHCIQState *s, uint64_t addr)
+{
+ return qpci_io_readl(s->dev, s->bar, XHCI_REGS_OFFSET_OPER + addr);
+}
+
+static void xhci_op_writel(XHCIQState *s, uint64_t addr, uint32_t value)
+{
+ qpci_io_writel(s->dev, s->bar, XHCI_REGS_OFFSET_OPER + addr, value);
+}
+
+static uint32_t xhci_port_readl(XHCIQState *s, uint32_t port, uint64_t addr)
+{
+ return qpci_io_readl(s->dev, s->bar,
+ XHCI_REGS_OFFSET_PORT + port * XHCI_PORT_PR_SZ + 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 uint32_t xhci_intr_readl(XHCIQState *s, uint32_t intr, uint64_t addr)
+{
+ return xhci_rt_readl(s, XHCI_INTR_REG_IR0 +
+ intr * XHCI_INTR_IR_SZ + addr);
+}
+
+
+static void xhci_intr_writel(XHCIQState *s, uint32_t intr, uint64_t addr,
+ uint32_t value)
+{
+ xhci_rt_writel(s, XHCI_INTR_REG_IR0 +
+ intr * XHCI_INTR_IR_SZ + 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 * XHCI_DBELL_DB_SZ, value);
+}
+
+static bool xhci_test_isr(XHCIQState *s)
+{
+ return xhci_op_readl(s, XHCI_OPER_REG_USBSTS) & XHCI_USBSTS_EINT;
+}
+
+static void wait_event_trb(XHCIQState *s, XHCITRB *trb)
+{
+ XHCITRB t;
+ XHCIQTRState *tr = &s->event_ring;
+ uint64_t er_addr = tr->addr + tr->trb_idx * TRB_SIZE;
+ uint32_t value;
+ guint64 end_time = g_get_monotonic_time() + 5 * G_TIME_SPAN_SECOND;
+
+ /* Wait for event interrupt */
+ while (!xhci_test_isr(s)) {
+ if (g_get_monotonic_time() >= end_time) {
+ g_error("Timeout expired");
+ }
+ qtest_clock_step(s->parent->qts, 10000);
+ }
+
+ /* With MSI-X enabled, IMAN IP is cleared after raising the interrupt */
+ value = xhci_intr_readl(s, 0, XHCI_INTR_REG_IMAN);
+ g_assert(!(value & XHCI_IMAN_IP));
+
+ xhci_op_writel(s, XHCI_OPER_REG_USBSTS, XHCI_USBSTS_EINT); /* clear EINT */
+
+ qtest_memread(s->parent->qts, er_addr, &t, TRB_SIZE);
+
+ 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) == tr->trb_c); /* C bit has been set */
+
+ tr->trb_idx++;
+ if (tr->trb_idx == tr->trb_entries) {
+ tr->trb_idx = 0;
+ tr->trb_c ^= 1;
+ }
+ /* Update ERDP to processed TRB addr and EHB bit, which clears EHB */
+ er_addr = tr->addr + tr->trb_idx * TRB_SIZE;
+ xhci_intr_writel(s, 0, XHCI_INTR_REG_ERDP_LO,
+ (er_addr & 0xffffffff) | XHCI_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, TRB_SIZE);
+ trb.parameter = ring;
+ trb.control = cpu_to_le32(c | /* C */
+ (TR_LINK << TRB_TYPE_SHIFT) |
+ TRB_LK_TC);
+ qtest_memwrite(s->parent->qts, ring + TRB_SIZE * (entries - 1),
+ &trb, TRB_SIZE);
+}
+
+static uint64_t queue_trb(XHCIQState *s, XHCIQTRState *tr, const XHCITRB *trb)
+{
+ uint64_t tr_addr = tr->addr + tr->trb_idx * TRB_SIZE;
+ XHCITRB t;
+
+ t.parameter = cpu_to_le64(trb->parameter);
+ t.status = cpu_to_le32(trb->status);
+ t.control = cpu_to_le32(trb->control | tr->trb_c);
+
+ qtest_memwrite(s->parent->qts, tr_addr, &t, TRB_SIZE);
+ tr->trb_idx++;
+ /* Last entry contains the link, so wrap back */
+ if (tr->trb_idx == tr->trb_entries - 1) {
+ set_link_trb(s, tr->addr, tr->trb_c, tr->trb_entries);
+ tr->trb_idx = 0;
+ tr->trb_c ^= 1;
+ }
+
+ return tr_addr;
+}
+
+static uint64_t submit_cr_trb(XHCIQState *s, const XHCITRB *trb)
+{
+ XHCIQTRState *tr = &s->command_ring;
+ uint64_t ret;
+
+ ret = queue_trb(s, tr, trb);
+
+ xhci_db_writel(s, 0, 0); /* doorbell host, doorbell 0 (command) */
+
+ return ret;
+}
+
+static void xhci_enable_device(XHCIQState *s)
+{
+ XHCIQTRState *tr;
+ XHCIEvRingSeg ev_seg;
+ uint32_t hcsparams1;
+ uint32_t value;
+ int i;
+
+ qpci_msix_enable(s->dev);
+
+ hcsparams1 = xhci_cap_readl(s, XHCI_HCCAP_REG_HCSPARAMS1);
+ s->maxports = (hcsparams1 >> 24) & 0xff;
+ s->maxintrs = (hcsparams1 >> 8) & 0x3ff;
+ s->maxslots = hcsparams1 & 0xff;
+
+ s->dboff = xhci_cap_readl(s, XHCI_HCCAP_REG_DBOFF);
+ s->rtoff = xhci_cap_readl(s, XHCI_HCCAP_REG_RTSOFF);
+
+ s->dc_base_array = xhci_guest_zalloc(s, 0x800);
+ s->event_ring_seg = xhci_guest_zalloc(s, 0x100);
+
+ /* Arbitrary small sizes so we can make them wrap */
+ tr = &s->command_ring;
+ tr->addr = xhci_guest_zalloc(s, 0x1000);
+ tr->trb_entries = 0x20;
+ tr->trb_c = 1;
+
+ tr = &s->event_ring;
+ tr->addr = xhci_guest_zalloc(s, 0x1000);
+ tr->trb_entries = 0x10;
+ tr->trb_c = 1;
+
+ tr = &s->event_ring;
+ ev_seg.addr_low = cpu_to_le32(tr->addr & 0xffffffff);
+ ev_seg.addr_high = cpu_to_le32(tr->addr >> 32);
+ ev_seg.size = cpu_to_le32(tr->trb_entries);
+ ev_seg.rsvd = 0;
+ qtest_memwrite(s->parent->qts, s->event_ring_seg, &ev_seg, sizeof(ev_seg));
+
+ xhci_op_writel(s, XHCI_OPER_REG_USBCMD, XHCI_USBCMD_HCRST);
+ do {
+ value = xhci_op_readl(s, XHCI_OPER_REG_USBSTS);
+ } while (value & XHCI_USBSTS_CNR);
+
+ xhci_op_writel(s, XHCI_OPER_REG_CONFIG, s->maxslots);
+
+ xhci_op_writel(s, XHCI_OPER_REG_DCBAAP_LO, s->dc_base_array & 0xffffffff);
+ xhci_op_writel(s, XHCI_OPER_REG_DCBAAP_HI, s->dc_base_array >> 32);
+
+ tr = &s->command_ring;
+ xhci_op_writel(s, XHCI_OPER_REG_CRCR_LO,
+ (tr->addr & 0xffffffff) | tr->trb_c);
+ xhci_op_writel(s, XHCI_OPER_REG_CRCR_HI, tr->addr >> 32);
+
+ xhci_intr_writel(s, 0, XHCI_INTR_REG_ERSTSZ, 1);
+
+ xhci_intr_writel(s, 0, XHCI_INTR_REG_ERSTBA_LO,
+ s->event_ring_seg & 0xffffffff);
+ xhci_intr_writel(s, 0, XHCI_INTR_REG_ERSTBA_HI,
+ s->event_ring_seg >> 32);
+
+ /* ERDP */
+ tr = &s->event_ring;
+ xhci_intr_writel(s, 0, XHCI_INTR_REG_ERDP_LO, tr->addr & 0xffffffff);
+ xhci_intr_writel(s, 0, XHCI_INTR_REG_ERDP_HI, tr->addr >> 32);
+
+ xhci_op_writel(s, XHCI_OPER_REG_USBCMD, XHCI_USBCMD_RS | XHCI_USBCMD_INTE);
+
+ /* Enable interrupts on ER IMAN */
+ xhci_intr_writel(s, 0, XHCI_INTR_REG_IMAN, XHCI_IMAN_IE);
+
+ /* Ensure there is no interrupt pending */
+ g_assert(!xhci_test_isr(s));
+
+ /* Query ports */
+ for (i = 0; i < s->maxports; i++) {
+ value = xhci_port_readl(s, i, 0); /* PORTSC */
+
+ /* All ports should be disabled */
+ g_assert(!(value & XHCI_PORTSC_CCS));
+ g_assert(!(value & XHCI_PORTSC_PED));
+ g_assert(((value >> XHCI_PORTSC_PLS_SHIFT) &
+ XHCI_PORTSC_PLS_MASK) == 5);
+ }
+}
+
+static void xhci_disable_device(XHCIQState *s)
+{
+ int i;
+
+ /* Shut it down */
+ qpci_msix_disable(s->dev);
+
+ guest_free(&s->parent->alloc, s->slots[s->slotid].device_context);
+ for (i = 0; i < 31; i++) {
+ guest_free(&s->parent->alloc,
+ s->slots[s->slotid].transfer_ring[i].addr);
+ }
+ guest_free(&s->parent->alloc, s->event_ring.addr);
+ guest_free(&s->parent->alloc, s->command_ring.addr);
+ guest_free(&s->parent->alloc, s->event_ring_seg);
+ guest_free(&s->parent->alloc, s->dc_base_array);
+}
+
+/*
+ * This test brings up an endpoint and runs some noops through its command
+ * ring and gets responses back on the event ring, then brings up a device
+ * context and runs some noops through its transfer ring (if available).
+ */
+static void test_xhci_stress_rings(const void *arg)
+{
+ const TestData *td = arg;
+ XHCIQState *s;
+ XHCITRB trb;
+ uint64_t tag;
+ int i;
+
+ s = xhci_boot("-M q35 "
+ "-device %s,id=xhci,bus=pcie.0,addr=1d.0 ",
+ td->device);
+ g_assert_cmphex(s->fingerprint, ==, td->fingerprint);
+
+ xhci_enable_device(s);
+
+ /* 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, TRB_SIZE);
+ trb.control |= CR_NOOP << TRB_TYPE_SHIFT;
+ trb.control |= TRB_TR_IOC;
+ tag = submit_cr_trb(s, &trb);
+ wait_event_trb(s, &trb);
+ g_assert_cmphex(trb.parameter , ==, tag);
+ g_assert_cmpint(TRB_TYPE(trb), ==, ER_COMMAND_COMPLETE);
+ }
+
+ xhci_disable_device(s);
+ xhci_shutdown(s);
}
static void add_test(const char *name, TestData *td, void (*fn)(const void *))
@@ -194,6 +529,7 @@ static void add_tests(TestData *td)
if (qtest_has_device("usb-ccid")) {
add_test("usb-ccid", td, test_usb_ccid_hotplug);
}
+ add_test("xhci-stress-rings", td, test_xhci_stress_rings);
}
/* tests */
--
2.47.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v4 04/22] tests/qtest/xhci: Add controller and device setup and ring tests
2025-05-02 3:30 ` [PATCH v4 04/22] tests/qtest/xhci: Add controller and device setup and ring tests Nicholas Piggin
@ 2025-05-19 14:03 ` Fabiano Rosas
0 siblings, 0 replies; 45+ messages in thread
From: Fabiano Rosas @ 2025-05-19 14:03 UTC (permalink / raw)
To: Nicholas Piggin, Gerd Hoffmann
Cc: Nicholas Piggin, qemu-devel, Kevin Wolf, Paolo Bonzini,
Michael S. Tsirkin, Marcel Apfelbaum, Laurent Vivier,
Phil Dennis-Jordan, Bernhard Beschow, Philippe Mathieu-Daudé
Nicholas Piggin <npiggin@gmail.com> writes:
> 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.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
Reviewed-by: Fabiano Rosas <farosas@suse.de>
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v4 05/22] tests/qtest/xhci: Add basic USB Mass Storage tests
2025-05-02 3:30 [PATCH v4 00/22] usb/xhci and usb/msd improvements and tests Nicholas Piggin
` (3 preceding siblings ...)
2025-05-02 3:30 ` [PATCH v4 04/22] tests/qtest/xhci: Add controller and device setup and ring tests Nicholas Piggin
@ 2025-05-02 3:30 ` Nicholas Piggin
2025-05-19 14:44 ` Fabiano Rosas
2025-05-02 3:30 ` [PATCH v4 06/22] hw/usb/xhci: Support TR NOOP commands Nicholas Piggin
` (17 subsequent siblings)
22 siblings, 1 reply; 45+ messages in thread
From: Nicholas Piggin @ 2025-05-02 3:30 UTC (permalink / raw)
To: Gerd Hoffmann
Cc: Nicholas Piggin, qemu-devel, Kevin Wolf, Paolo Bonzini,
Michael S. Tsirkin, Marcel Apfelbaum, Fabiano Rosas,
Laurent Vivier, Phil Dennis-Jordan, Bernhard Beschow,
Philippe Mathieu-Daudé
Add a usb-storage device to xhci tests, enable USB Mass Storage Bulk
endpoints, and run some MSD commands through it.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
tests/qtest/usb-hcd-xhci-test.c | 359 +++++++++++++++++++++++++++++++-
1 file changed, 351 insertions(+), 8 deletions(-)
diff --git a/tests/qtest/usb-hcd-xhci-test.c b/tests/qtest/usb-hcd-xhci-test.c
index 291d1dfc36e..39c5c36e940 100644
--- a/tests/qtest/usb-hcd-xhci-test.c
+++ b/tests/qtest/usb-hcd-xhci-test.c
@@ -373,6 +373,20 @@ static uint64_t submit_cr_trb(XHCIQState *s, const XHCITRB *trb)
return ret;
}
+static uint64_t submit_tr_trb(XHCIQState *s, int slot, int ep,
+ const XHCITRB *trb)
+{
+ XHCIQSlotState *sl = &s->slots[slot];
+ XHCIQTRState *tr = &sl->transfer_ring[ep];
+ uint64_t ret;
+
+ ret = queue_trb(s, tr, trb);
+
+ xhci_db_writel(s, slot, 1 + ep); /* doorbell slot, EP<ep> target */
+
+ return ret;
+}
+
static void xhci_enable_device(XHCIQState *s)
{
XHCIQTRState *tr;
@@ -451,14 +465,165 @@ static void xhci_enable_device(XHCIQState *s)
for (i = 0; i < s->maxports; i++) {
value = xhci_port_readl(s, i, 0); /* PORTSC */
- /* All ports should be disabled */
- g_assert(!(value & XHCI_PORTSC_CCS));
- g_assert(!(value & XHCI_PORTSC_PED));
- g_assert(((value >> XHCI_PORTSC_PLS_SHIFT) &
- XHCI_PORTSC_PLS_MASK) == 5);
+ /* First port should be attached and enabled if we have usb-storage */
+ if (qtest_has_device("usb-storage") && i == 0) {
+ g_assert(value & XHCI_PORTSC_CCS);
+ g_assert(value & XHCI_PORTSC_PED);
+ /* Port Speed must be identified (non-zero) */
+ g_assert(((value >> XHCI_PORTSC_SPEED_SHIFT) &
+ XHCI_PORTSC_SPEED_MASK) != 0);
+ } else {
+ g_assert(!(value & XHCI_PORTSC_CCS));
+ g_assert(!(value & XHCI_PORTSC_PED));
+ g_assert(((value >> XHCI_PORTSC_PLS_SHIFT) &
+ XHCI_PORTSC_PLS_MASK) == 5);
+ }
}
}
+/* XXX: what should these values be? */
+#define TRB_MAX_PACKET_SIZE 0x200
+#define TRB_AVERAGE_LENGTH 0x200
+
+static void xhci_enable_slot(XHCIQState *s)
+{
+ XHCIQTRState *tr;
+ uint64_t input_context;
+ XHCITRB trb;
+ uint64_t tag;
+ g_autofree void *mem = g_malloc0(0x1000); /* buffer for writing to guest */
+ uint32_t *dc; /* device context */
+
+ /* Issue a command ring enable slot */
+ memset(&trb, 0, TRB_SIZE);
+ trb.control |= CR_ENABLE_SLOT << TRB_TYPE_SHIFT;
+ trb.control |= TRB_TR_IOC;
+ tag = submit_cr_trb(s, &trb);
+ wait_event_trb(s, &trb);
+ g_assert_cmphex(trb.parameter , ==, tag);
+ g_assert_cmpint(TRB_TYPE(trb), ==, ER_COMMAND_COMPLETE);
+ s->slotid = (trb.control >> TRB_CR_SLOTID_SHIFT) & 0xff;
+
+ /* 32-byte input context size, should check HCCPARAMS1 for 64-byte size */
+ input_context = xhci_guest_zalloc(s, 0x420);
+
+ /* Set input control context */
+ memset(mem, 0, 0x420);
+ ((uint32_t *)mem)[1] = cpu_to_le32(0x3); /* Add device contexts 0 and 1 */
+
+ /* Slot context */
+ dc = mem + 1 * 0x20;
+ dc[0] = cpu_to_le32(1 << 27); /* 1 context entry */
+ dc[1] = cpu_to_le32(1 << 16); /* 1 port number */
+
+ /* Endpoint 0 context */
+ tr = &s->slots[s->slotid].transfer_ring[0];
+ tr->addr = xhci_guest_zalloc(s, 0x1000);
+ tr->trb_entries = 0x10;
+ tr->trb_c = 1;
+
+ dc = mem + 2 * 0x20;
+ dc[0] = 0;
+ dc[1] = cpu_to_le32((ET_CONTROL << EP_TYPE_SHIFT) |
+ (TRB_MAX_PACKET_SIZE << 16));
+ dc[2] = cpu_to_le32((tr->addr & 0xffffffff) | 1); /* DCS=1 */
+ dc[3] = cpu_to_le32(tr->addr >> 32);
+ dc[4] = cpu_to_le32(TRB_AVERAGE_LENGTH);
+ qtest_memwrite(s->parent->qts, input_context, mem, 0x420);
+
+ s->slots[s->slotid].device_context = xhci_guest_zalloc(s, 0x400);
+
+ ((uint64_t *)mem)[0] = cpu_to_le64(s->slots[s->slotid].device_context);
+ qtest_memwrite(s->parent->qts, s->dc_base_array + 8 * s->slotid, mem, 8);
+
+ /* Issue a command ring address device */
+ memset(&trb, 0, TRB_SIZE);
+ trb.parameter = input_context;
+ trb.control |= CR_ADDRESS_DEVICE << TRB_TYPE_SHIFT;
+ trb.control |= s->slotid << TRB_CR_SLOTID_SHIFT;
+ tag = submit_cr_trb(s, &trb);
+ wait_event_trb(s, &trb);
+ g_assert_cmphex(trb.parameter , ==, tag);
+ g_assert_cmpint(TRB_TYPE(trb), ==, ER_COMMAND_COMPLETE);
+
+ guest_free(&s->parent->alloc, input_context);
+
+ /* Check EP0 is running */
+ qtest_memread(s->parent->qts, s->slots[s->slotid].device_context, mem, 0x400);
+ g_assert((((uint32_t *)mem)[8] & 0x3) == EP_RUNNING);
+}
+
+static void xhci_enable_msd_bulk_endpoints(XHCIQState *s)
+{
+ XHCIQTRState *tr;
+ uint64_t input_context;
+ XHCITRB trb;
+ uint64_t tag;
+ g_autofree void *mem = g_malloc0(0x1000); /* buffer for writing to guest */
+ uint32_t *dc; /* device context */
+
+ /* Configure 2 more endpoints */
+
+ /* 32-byte input context size, should check HCCPARAMS1 for 64-byte size */
+ input_context = xhci_guest_zalloc(s, 0x420);
+
+ /* Set input control context */
+ memset(mem, 0, 0x420);
+ ((uint32_t *)mem)[1] = cpu_to_le32(0x19); /* Add device contexts 0, 3, 4 */
+
+ /* Slot context */
+ dc = mem + 1 * 0x20;
+ dc[0] = cpu_to_le32(1 << 27); /* 1 context entry */
+ dc[1] = cpu_to_le32(1 << 16); /* 1 port number */
+
+ /* Endpoint 1 (IN) context */
+ tr = &s->slots[s->slotid].transfer_ring[2];
+ tr->addr = xhci_guest_zalloc(s, 0x1000);
+ tr->trb_entries = 0x10;
+ tr->trb_c = 1;
+
+ dc = mem + 4 * 0x20;
+ dc[0] = 0;
+ dc[1] = cpu_to_le32((ET_BULK_IN << EP_TYPE_SHIFT) |
+ (TRB_MAX_PACKET_SIZE << 16));
+ dc[2] = cpu_to_le32((tr->addr & 0xffffffff) | 1); /* DCS=1 */
+ dc[3] = cpu_to_le32(tr->addr >> 32);
+ dc[4] = cpu_to_le32(TRB_AVERAGE_LENGTH);
+
+ /* Endpoint 2 (OUT) context */
+ tr = &s->slots[s->slotid].transfer_ring[3];
+ tr->addr = xhci_guest_zalloc(s, 0x1000);
+ tr->trb_entries = 0x10;
+ tr->trb_c = 1;
+
+ dc = mem + 5 * 0x20;
+ dc[0] = 0;
+ dc[1] = cpu_to_le32((ET_BULK_OUT << EP_TYPE_SHIFT) |
+ (TRB_MAX_PACKET_SIZE << 16));
+ dc[2] = cpu_to_le32((tr->addr & 0xffffffff) | 1); /* DCS=1 */
+ dc[3] = cpu_to_le32(tr->addr >> 32);
+ dc[4] = cpu_to_le32(TRB_AVERAGE_LENGTH);
+ qtest_memwrite(s->parent->qts, input_context, mem, 0x420);
+
+ /* Issue a command ring configure endpoint */
+ memset(&trb, 0, TRB_SIZE);
+ trb.parameter = input_context;
+ trb.control |= CR_CONFIGURE_ENDPOINT << TRB_TYPE_SHIFT;
+ trb.control |= s->slotid << TRB_CR_SLOTID_SHIFT;
+ tag = submit_cr_trb(s, &trb);
+ wait_event_trb(s, &trb);
+ g_assert_cmphex(trb.parameter , ==, tag);
+ g_assert_cmpint(TRB_TYPE(trb), ==, ER_COMMAND_COMPLETE);
+
+ guest_free(&s->parent->alloc, input_context);
+
+ /* Check EPs are running */
+ qtest_memread(s->parent->qts, s->slots[s->slotid].device_context, mem, 0x400);
+ g_assert((((uint32_t *)mem)[1*8] & 0x3) == EP_RUNNING);
+ g_assert((((uint32_t *)mem)[3*8] & 0x3) == EP_RUNNING);
+ g_assert((((uint32_t *)mem)[4*8] & 0x3) == EP_RUNNING);
+}
+
static void xhci_disable_device(XHCIQState *s)
{
int i;
@@ -477,6 +642,144 @@ static void xhci_disable_device(XHCIQState *s)
guest_free(&s->parent->alloc, s->dc_base_array);
}
+struct QEMU_PACKED usb_msd_cbw {
+ uint32_t sig;
+ uint32_t tag;
+ uint32_t data_len;
+ uint8_t flags;
+ uint8_t lun;
+ uint8_t cmd_len;
+ uint8_t cmd[16];
+};
+
+struct QEMU_PACKED usb_msd_csw {
+ uint32_t sig;
+ uint32_t tag;
+ uint32_t residue;
+ uint8_t status;
+};
+
+static ssize_t xhci_submit_scsi_cmd(XHCIQState *s,
+ const uint8_t *cmd, uint8_t cmd_len,
+ void *data, uint32_t data_len,
+ bool data_in)
+{
+ struct usb_msd_cbw cbw;
+ struct usb_msd_csw csw;
+ uint64_t trb_data;
+ XHCITRB trb;
+ uint64_t tag;
+
+ /* TRB data payload */
+ trb_data = xhci_guest_zalloc(s, data_len > sizeof(cbw) ? data_len : sizeof(cbw));
+
+ memset(&cbw, 0, sizeof(cbw));
+ cbw.sig = cpu_to_le32(0x43425355);
+ cbw.tag = cpu_to_le32(0);
+ cbw.data_len = cpu_to_le32(data_len);
+ cbw.flags = data_in ? 0x80 : 0x00;
+ cbw.lun = 0;
+ cbw.cmd_len = cmd_len; /* cmd len */
+ memcpy(cbw.cmd, cmd, cmd_len);
+ qtest_memwrite(s->parent->qts, trb_data, &cbw, sizeof(cbw));
+
+ /* Issue a transfer ring ep 3 data (out) */
+ memset(&trb, 0, TRB_SIZE);
+ trb.parameter = trb_data;
+ trb.status = sizeof(cbw);
+ trb.control |= TR_NORMAL << TRB_TYPE_SHIFT;
+ trb.control |= TRB_TR_IOC;
+ tag = submit_tr_trb(s, s->slotid, 3, &trb);
+ wait_event_trb(s, &trb);
+ g_assert_cmphex(trb.parameter, ==, tag);
+ g_assert_cmpint(TRB_TYPE(trb), ==, ER_TRANSFER);
+
+ if (data_in) {
+ g_assert(data_len);
+
+ /* Issue a transfer ring ep 2 data (in) */
+ memset(&trb, 0, TRB_SIZE);
+ trb.parameter = trb_data;
+ trb.status = data_len; /* data_len bytes, no more packets */
+ trb.control |= TR_NORMAL << TRB_TYPE_SHIFT;
+ trb.control |= TRB_TR_IOC;
+ tag = submit_tr_trb(s, s->slotid, 2, &trb);
+ wait_event_trb(s, &trb);
+ g_assert_cmphex(trb.parameter, ==, tag);
+ g_assert_cmpint(TRB_TYPE(trb), ==, ER_TRANSFER);
+
+ qtest_memread(s->parent->qts, trb_data, data, data_len);
+ } else if (data_len) {
+ qtest_memwrite(s->parent->qts, trb_data, data, data_len);
+
+ /* Issue a transfer ring ep 3 data (out) */
+ memset(&trb, 0, TRB_SIZE);
+ trb.parameter = trb_data;
+ trb.status = data_len; /* data_len bytes, no more packets */
+ trb.control |= TR_NORMAL << TRB_TYPE_SHIFT;
+ trb.control |= TRB_TR_IOC;
+ tag = submit_tr_trb(s, s->slotid, 3, &trb);
+ wait_event_trb(s, &trb);
+ g_assert_cmphex(trb.parameter, ==, tag);
+ g_assert_cmpint(TRB_TYPE(trb), ==, ER_TRANSFER);
+ } else {
+ /* No data */
+ }
+
+ /* Issue a transfer ring ep 2 data (in) */
+ memset(&trb, 0, TRB_SIZE);
+ trb.parameter = trb_data;
+ trb.status = sizeof(csw);
+ trb.control |= TR_NORMAL << TRB_TYPE_SHIFT;
+ trb.control |= TRB_TR_IOC;
+ tag = submit_tr_trb(s, s->slotid, 2, &trb);
+ wait_event_trb(s, &trb);
+ g_assert_cmphex(trb.parameter, ==, tag);
+ g_assert_cmpint(TRB_TYPE(trb), ==, ER_TRANSFER);
+
+ qtest_memread(s->parent->qts, trb_data, &csw, sizeof(csw));
+
+ guest_free(&s->parent->alloc, trb_data);
+
+ g_assert(csw.sig == cpu_to_le32(0x53425355));
+ g_assert(csw.tag == cpu_to_le32(0));
+ if (csw.status) {
+ return -1;
+ }
+ return data_len - le32_to_cpu(csw.residue); /* bytes copied */
+}
+
+#include "scsi/constants.h"
+
+static void xhci_test_msd(XHCIQState *s)
+{
+ uint8_t scsi_cmd[16];
+ g_autofree void *mem = g_malloc0(0x1000); /* buffer for writing to guest */
+
+ /* Clear SENSE data */
+ memset(scsi_cmd, 0, sizeof(scsi_cmd));
+ scsi_cmd[0] = INQUIRY;
+ if (xhci_submit_scsi_cmd(s, scsi_cmd, 6, mem, 0, false) < 0) {
+ g_assert_not_reached();
+ }
+
+ /* Report LUNs */
+ memset(scsi_cmd, 0, sizeof(scsi_cmd));
+ scsi_cmd[0] = REPORT_LUNS;
+ /* length in big-endian */
+ scsi_cmd[6] = 0x00;
+ scsi_cmd[7] = 0x00;
+ scsi_cmd[8] = 0x01;
+ scsi_cmd[9] = 0x00;
+
+ if (xhci_submit_scsi_cmd(s, scsi_cmd, 16, mem, 0x100, true) < 0) {
+ g_assert_not_reached();
+ }
+
+ /* Check REPORT_LUNS data found 1 LUN */
+ g_assert(((uint32_t *)mem)[0] == cpu_to_be32(8)); /* LUN List Length */
+}
+
/*
* This test brings up an endpoint and runs some noops through its command
* ring and gets responses back on the event ring, then brings up a device
@@ -490,9 +793,18 @@ static void test_xhci_stress_rings(const void *arg)
uint64_t tag;
int i;
- s = xhci_boot("-M q35 "
- "-device %s,id=xhci,bus=pcie.0,addr=1d.0 ",
- td->device);
+ if (qtest_has_device("usb-storage")) {
+ s = xhci_boot("-M q35 "
+ "-device %s,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 ",
+ td->device);
+ } else {
+ s = xhci_boot("-M q35 "
+ "-device %s,id=xhci,bus=pcie.0,addr=1d.0 ",
+ td->device);
+ }
g_assert_cmphex(s->fingerprint, ==, td->fingerprint);
xhci_enable_device(s);
@@ -513,6 +825,34 @@ static void test_xhci_stress_rings(const void *arg)
xhci_shutdown(s);
}
+/*
+ * This test brings up a USB MSD endpoint and runs MSD (SCSI) commands.
+ */
+static void test_usb_msd(const void *arg)
+{
+ const TestData *td = arg;
+ XHCIQState *s;
+
+ s = xhci_boot("-M q35 "
+ "-device %s,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 ",
+ td->device);
+ g_assert_cmphex(s->fingerprint, ==, td->fingerprint);
+
+ xhci_enable_device(s);
+
+ xhci_enable_slot(s);
+
+ xhci_enable_msd_bulk_endpoints(s);
+
+ xhci_test_msd(s);
+
+ xhci_disable_device(s);
+ xhci_shutdown(s);
+}
+
static void add_test(const char *name, TestData *td, void (*fn)(const void *))
{
g_autofree char *full_name = g_strdup_printf(
@@ -530,6 +870,9 @@ static void add_tests(TestData *td)
add_test("usb-ccid", td, test_usb_ccid_hotplug);
}
add_test("xhci-stress-rings", td, test_xhci_stress_rings);
+ if (qtest_has_device("usb-storage")) {
+ add_test("usb-msd", td, test_usb_msd);
+ }
}
/* tests */
--
2.47.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v4 05/22] tests/qtest/xhci: Add basic USB Mass Storage tests
2025-05-02 3:30 ` [PATCH v4 05/22] tests/qtest/xhci: Add basic USB Mass Storage tests Nicholas Piggin
@ 2025-05-19 14:44 ` Fabiano Rosas
0 siblings, 0 replies; 45+ messages in thread
From: Fabiano Rosas @ 2025-05-19 14:44 UTC (permalink / raw)
To: Nicholas Piggin, Gerd Hoffmann
Cc: Nicholas Piggin, qemu-devel, Kevin Wolf, Paolo Bonzini,
Michael S. Tsirkin, Marcel Apfelbaum, Laurent Vivier,
Phil Dennis-Jordan, Bernhard Beschow, Philippe Mathieu-Daudé
Nicholas Piggin <npiggin@gmail.com> writes:
> Add a usb-storage device to xhci tests, enable USB Mass Storage Bulk
> endpoints, and run some MSD commands through it.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
Checkpatch doesn't like it.
Reviewed-by: Fabiano Rosas <farosas@suse.de>
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v4 06/22] hw/usb/xhci: Support TR NOOP commands
2025-05-02 3:30 [PATCH v4 00/22] usb/xhci and usb/msd improvements and tests Nicholas Piggin
` (4 preceding siblings ...)
2025-05-02 3:30 ` [PATCH v4 05/22] tests/qtest/xhci: Add basic USB Mass Storage tests Nicholas Piggin
@ 2025-05-02 3:30 ` Nicholas Piggin
2025-05-12 13:06 ` Peter Maydell
2025-05-02 3:30 ` [PATCH v4 07/22] tests/qtest/xhci: add a test for " Nicholas Piggin
` (16 subsequent siblings)
22 siblings, 1 reply; 45+ messages in thread
From: Nicholas Piggin @ 2025-05-02 3:30 UTC (permalink / raw)
To: Gerd Hoffmann
Cc: Nicholas Piggin, qemu-devel, Kevin Wolf, Paolo Bonzini,
Michael S. Tsirkin, Marcel Apfelbaum, Fabiano Rosas,
Laurent Vivier, Phil Dennis-Jordan, Bernhard Beschow,
Philippe Mathieu-Daudé
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(-)
diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index ef9f2a7db41..6a490a5febf 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -1664,6 +1664,20 @@ 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)
{
@@ -1786,6 +1800,8 @@ 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) {
@@ -1814,10 +1830,20 @@ 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);
--
2.47.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v4 06/22] hw/usb/xhci: Support TR NOOP commands
2025-05-02 3:30 ` [PATCH v4 06/22] hw/usb/xhci: Support TR NOOP commands Nicholas Piggin
@ 2025-05-12 13:06 ` Peter Maydell
0 siblings, 0 replies; 45+ messages in thread
From: Peter Maydell @ 2025-05-12 13:06 UTC (permalink / raw)
To: Nicholas Piggin
Cc: Gerd Hoffmann, qemu-devel, Kevin Wolf, Paolo Bonzini,
Michael S. Tsirkin, Marcel Apfelbaum, Fabiano Rosas,
Laurent Vivier, Phil Dennis-Jordan, Bernhard Beschow,
Philippe Mathieu-Daudé
On Fri, 2 May 2025 at 04:33, Nicholas Piggin <npiggin@gmail.com> wrote:
>
> 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(-)
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
thanks
-- PMM
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v4 07/22] tests/qtest/xhci: add a test for TR NOOP commands
2025-05-02 3:30 [PATCH v4 00/22] usb/xhci and usb/msd improvements and tests Nicholas Piggin
` (5 preceding siblings ...)
2025-05-02 3:30 ` [PATCH v4 06/22] hw/usb/xhci: Support TR NOOP commands Nicholas Piggin
@ 2025-05-02 3:30 ` Nicholas Piggin
2025-05-19 14:54 ` Fabiano Rosas
2025-05-02 3:30 ` [PATCH v4 08/22] tests/qtest/usb-hcd-xhci: Deliver msix interrupts Nicholas Piggin
` (15 subsequent siblings)
22 siblings, 1 reply; 45+ messages in thread
From: Nicholas Piggin @ 2025-05-02 3:30 UTC (permalink / raw)
To: Gerd Hoffmann
Cc: Nicholas Piggin, qemu-devel, Kevin Wolf, Paolo Bonzini,
Michael S. Tsirkin, Marcel Apfelbaum, Fabiano Rosas,
Laurent Vivier, Phil Dennis-Jordan, Bernhard Beschow,
Philippe Mathieu-Daudé
Run some TR NOOP commands through the transfer ring.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
tests/qtest/usb-hcd-xhci-test.c | 36 +++++++++++++++++++++++++++++++++
1 file changed, 36 insertions(+)
diff --git a/tests/qtest/usb-hcd-xhci-test.c b/tests/qtest/usb-hcd-xhci-test.c
index 39c5c36e940..7f801f8f1a0 100644
--- a/tests/qtest/usb-hcd-xhci-test.c
+++ b/tests/qtest/usb-hcd-xhci-test.c
@@ -753,9 +753,29 @@ static ssize_t xhci_submit_scsi_cmd(XHCIQState *s,
static void xhci_test_msd(XHCIQState *s)
{
+ XHCITRB trb;
+ uint64_t tag;
uint8_t scsi_cmd[16];
g_autofree void *mem = g_malloc0(0x1000); /* buffer for writing to guest */
+ /* Issue a transfer ring ep 2 noop */
+ memset(&trb, 0, TRB_SIZE);
+ trb.control |= TR_NOOP << TRB_TYPE_SHIFT;
+ trb.control |= TRB_TR_IOC;
+ tag = submit_tr_trb(s, s->slotid, 2, &trb);
+ wait_event_trb(s, &trb);
+ g_assert_cmphex(trb.parameter, ==, tag);
+ g_assert_cmpint(TRB_TYPE(trb), ==, ER_TRANSFER);
+
+ /* Issue a transfer ring ep 3 noop */
+ memset(&trb, 0, TRB_SIZE);
+ trb.control |= TR_NOOP << TRB_TYPE_SHIFT;
+ trb.control |= TRB_TR_IOC;
+ tag = submit_tr_trb(s, s->slotid, 3, &trb);
+ wait_event_trb(s, &trb);
+ g_assert_cmphex(trb.parameter, ==, tag);
+ g_assert_cmpint(TRB_TYPE(trb), ==, ER_TRANSFER);
+
/* Clear SENSE data */
memset(scsi_cmd, 0, sizeof(scsi_cmd));
scsi_cmd[0] = INQUIRY;
@@ -821,6 +841,22 @@ static void test_xhci_stress_rings(const void *arg)
g_assert_cmpint(TRB_TYPE(trb), ==, ER_COMMAND_COMPLETE);
}
+ if (qtest_has_device("usb-storage")) {
+ xhci_enable_slot(s);
+
+ /* Wrap the transfer ring a few times */
+ for (i = 0; i < 100; i++) {
+ /* Issue a transfer ring ep 0 noop */
+ memset(&trb, 0, TRB_SIZE);
+ trb.control |= TR_NOOP << TRB_TYPE_SHIFT;
+ trb.control |= TRB_TR_IOC;
+ tag = submit_tr_trb(s, s->slotid, 0, &trb);
+ wait_event_trb(s, &trb);
+ g_assert_cmphex(trb.parameter, ==, tag);
+ g_assert_cmpint(TRB_TYPE(trb), ==, ER_TRANSFER);
+ }
+ }
+
xhci_disable_device(s);
xhci_shutdown(s);
}
--
2.47.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v4 07/22] tests/qtest/xhci: add a test for TR NOOP commands
2025-05-02 3:30 ` [PATCH v4 07/22] tests/qtest/xhci: add a test for " Nicholas Piggin
@ 2025-05-19 14:54 ` Fabiano Rosas
0 siblings, 0 replies; 45+ messages in thread
From: Fabiano Rosas @ 2025-05-19 14:54 UTC (permalink / raw)
To: Nicholas Piggin, Gerd Hoffmann
Cc: Nicholas Piggin, qemu-devel, Kevin Wolf, Paolo Bonzini,
Michael S. Tsirkin, Marcel Apfelbaum, Laurent Vivier,
Phil Dennis-Jordan, Bernhard Beschow, Philippe Mathieu-Daudé
Nicholas Piggin <npiggin@gmail.com> writes:
> Run some TR NOOP commands through the transfer ring.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
Reviewed-by: Fabiano Rosas <farosas@suse.de>
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v4 08/22] tests/qtest/usb-hcd-xhci: Deliver msix interrupts
2025-05-02 3:30 [PATCH v4 00/22] usb/xhci and usb/msd improvements and tests Nicholas Piggin
` (6 preceding siblings ...)
2025-05-02 3:30 ` [PATCH v4 07/22] tests/qtest/xhci: add a test for " Nicholas Piggin
@ 2025-05-02 3:30 ` Nicholas Piggin
2025-05-02 8:24 ` Philippe Mathieu-Daudé
2025-05-02 3:30 ` [PATCH v4 09/22] hw/usb/hcd-xhci-pci: Make PCI device more configurable Nicholas Piggin
` (14 subsequent siblings)
22 siblings, 1 reply; 45+ messages in thread
From: Nicholas Piggin @ 2025-05-02 3:30 UTC (permalink / raw)
To: Gerd Hoffmann
Cc: Nicholas Piggin, qemu-devel, Kevin Wolf, Paolo Bonzini,
Michael S. Tsirkin, Marcel Apfelbaum, Fabiano Rosas,
Laurent Vivier, Phil Dennis-Jordan, Bernhard Beschow,
Philippe Mathieu-Daudé
msix
---
tests/qtest/usb-hcd-xhci-test.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/tests/qtest/usb-hcd-xhci-test.c b/tests/qtest/usb-hcd-xhci-test.c
index 7f801f8f1a0..2eecc8d9f26 100644
--- a/tests/qtest/usb-hcd-xhci-test.c
+++ b/tests/qtest/usb-hcd-xhci-test.c
@@ -48,6 +48,8 @@ typedef struct XHCIQState {
QPCIBar bar;
uint64_t barsize;
uint32_t fingerprint;
+ uint64_t guest_msix_addr;
+ uint32_t msix_data;
/* In-memory arrays */
uint64_t dc_base_array;
@@ -279,7 +281,8 @@ static void xhci_db_writel(XHCIQState *s, uint32_t db, uint32_t value)
static bool xhci_test_isr(XHCIQState *s)
{
- return xhci_op_readl(s, XHCI_OPER_REG_USBSTS) & XHCI_USBSTS_EINT;
+ return qpci_msix_test_interrupt(s->dev, 0,
+ s->guest_msix_addr, s->msix_data);
}
static void wait_event_trb(XHCIQState *s, XHCITRB *trb)
@@ -298,6 +301,9 @@ static void wait_event_trb(XHCIQState *s, XHCITRB *trb)
qtest_clock_step(s->parent->qts, 10000);
}
+ value = xhci_op_readl(s, XHCI_OPER_REG_USBSTS);
+ g_assert(value & XHCI_USBSTS_EINT);
+
/* With MSI-X enabled, IMAN IP is cleared after raising the interrupt */
value = xhci_intr_readl(s, 0, XHCI_INTR_REG_IMAN);
g_assert(!(value & XHCI_IMAN_IP));
@@ -395,7 +401,12 @@ static void xhci_enable_device(XHCIQState *s)
uint32_t value;
int i;
+ s->guest_msix_addr = xhci_guest_zalloc(s, 4);
+ s->msix_data = 0x1234abcd;
+
qpci_msix_enable(s->dev);
+ qpci_msix_set_entry(s->dev, 0, s->guest_msix_addr, s->msix_data);
+ qpci_msix_set_masked(s->dev, 0, false);
hcsparams1 = xhci_cap_readl(s, XHCI_HCCAP_REG_HCSPARAMS1);
s->maxports = (hcsparams1 >> 24) & 0xff;
@@ -640,6 +651,7 @@ static void xhci_disable_device(XHCIQState *s)
guest_free(&s->parent->alloc, s->command_ring.addr);
guest_free(&s->parent->alloc, s->event_ring_seg);
guest_free(&s->parent->alloc, s->dc_base_array);
+ guest_free(&s->parent->alloc, s->guest_msix_addr);
}
struct QEMU_PACKED usb_msd_cbw {
--
2.47.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v4 08/22] tests/qtest/usb-hcd-xhci: Deliver msix interrupts
2025-05-02 3:30 ` [PATCH v4 08/22] tests/qtest/usb-hcd-xhci: Deliver msix interrupts Nicholas Piggin
@ 2025-05-02 8:24 ` Philippe Mathieu-Daudé
2025-05-05 1:05 ` Nicholas Piggin
0 siblings, 1 reply; 45+ messages in thread
From: Philippe Mathieu-Daudé @ 2025-05-02 8:24 UTC (permalink / raw)
To: Nicholas Piggin, Gerd Hoffmann
Cc: qemu-devel, Kevin Wolf, Paolo Bonzini, Michael S. Tsirkin,
Marcel Apfelbaum, Fabiano Rosas, Laurent Vivier,
Phil Dennis-Jordan, Bernhard Beschow
On 2/5/25 05:30, Nicholas Piggin wrote:
> msix
Hmm? :)
> ---
> tests/qtest/usb-hcd-xhci-test.c | 14 +++++++++++++-
> 1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/tests/qtest/usb-hcd-xhci-test.c b/tests/qtest/usb-hcd-xhci-test.c
> index 7f801f8f1a0..2eecc8d9f26 100644
> --- a/tests/qtest/usb-hcd-xhci-test.c
> +++ b/tests/qtest/usb-hcd-xhci-test.c
> @@ -48,6 +48,8 @@ typedef struct XHCIQState {
> QPCIBar bar;
> uint64_t barsize;
> uint32_t fingerprint;
> + uint64_t guest_msix_addr;
> + uint32_t msix_data;
>
> /* In-memory arrays */
> uint64_t dc_base_array;
> @@ -279,7 +281,8 @@ static void xhci_db_writel(XHCIQState *s, uint32_t db, uint32_t value)
>
> static bool xhci_test_isr(XHCIQState *s)
> {
> - return xhci_op_readl(s, XHCI_OPER_REG_USBSTS) & XHCI_USBSTS_EINT;
> + return qpci_msix_test_interrupt(s->dev, 0,
> + s->guest_msix_addr, s->msix_data);
> }
>
> static void wait_event_trb(XHCIQState *s, XHCITRB *trb)
> @@ -298,6 +301,9 @@ static void wait_event_trb(XHCIQState *s, XHCITRB *trb)
> qtest_clock_step(s->parent->qts, 10000);
> }
>
> + value = xhci_op_readl(s, XHCI_OPER_REG_USBSTS);
> + g_assert(value & XHCI_USBSTS_EINT);
> +
> /* With MSI-X enabled, IMAN IP is cleared after raising the interrupt */
> value = xhci_intr_readl(s, 0, XHCI_INTR_REG_IMAN);
> g_assert(!(value & XHCI_IMAN_IP));
> @@ -395,7 +401,12 @@ static void xhci_enable_device(XHCIQState *s)
> uint32_t value;
> int i;
>
> + s->guest_msix_addr = xhci_guest_zalloc(s, 4);
> + s->msix_data = 0x1234abcd;
> +
> qpci_msix_enable(s->dev);
> + qpci_msix_set_entry(s->dev, 0, s->guest_msix_addr, s->msix_data);
> + qpci_msix_set_masked(s->dev, 0, false);
>
> hcsparams1 = xhci_cap_readl(s, XHCI_HCCAP_REG_HCSPARAMS1);
> s->maxports = (hcsparams1 >> 24) & 0xff;
> @@ -640,6 +651,7 @@ static void xhci_disable_device(XHCIQState *s)
> guest_free(&s->parent->alloc, s->command_ring.addr);
> guest_free(&s->parent->alloc, s->event_ring_seg);
> guest_free(&s->parent->alloc, s->dc_base_array);
> + guest_free(&s->parent->alloc, s->guest_msix_addr);
> }
>
> struct QEMU_PACKED usb_msd_cbw {
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v4 08/22] tests/qtest/usb-hcd-xhci: Deliver msix interrupts
2025-05-02 8:24 ` Philippe Mathieu-Daudé
@ 2025-05-05 1:05 ` Nicholas Piggin
0 siblings, 0 replies; 45+ messages in thread
From: Nicholas Piggin @ 2025-05-05 1:05 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, Gerd Hoffmann
Cc: qemu-devel, Kevin Wolf, Paolo Bonzini, Michael S. Tsirkin,
Marcel Apfelbaum, Fabiano Rosas, Laurent Vivier,
Phil Dennis-Jordan, Bernhard Beschow
On Fri May 2, 2025 at 6:24 PM AEST, Philippe Mathieu-Daudé wrote:
> On 2/5/25 05:30, Nicholas Piggin wrote:
>> msix
>
> Hmm? :)
Oops, thanks. Too much juggling around of the different patch series.
This depends on some of the libqos pci changes so I split it out (and
it's probably nicer to be separate anyway).
Thanks,
Nick
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v4 09/22] hw/usb/hcd-xhci-pci: Make PCI device more configurable
2025-05-02 3:30 [PATCH v4 00/22] usb/xhci and usb/msd improvements and tests Nicholas Piggin
` (7 preceding siblings ...)
2025-05-02 3:30 ` [PATCH v4 08/22] tests/qtest/usb-hcd-xhci: Deliver msix interrupts Nicholas Piggin
@ 2025-05-02 3:30 ` Nicholas Piggin
2025-05-12 13:12 ` Peter Maydell
2025-05-02 3:30 ` [PATCH v4 10/22] hw/usb/hcd-xhci-pci: Add TI TUSB73X0 XHCI controller model Nicholas Piggin
` (13 subsequent siblings)
22 siblings, 1 reply; 45+ messages in thread
From: Nicholas Piggin @ 2025-05-02 3:30 UTC (permalink / raw)
To: Gerd Hoffmann
Cc: Nicholas Piggin, qemu-devel, Kevin Wolf, Paolo Bonzini,
Michael S. Tsirkin, Marcel Apfelbaum, Fabiano Rosas,
Laurent Vivier, Phil Dennis-Jordan, Bernhard Beschow,
Philippe Mathieu-Daudé
To prepare to support another USB PCI Host Controller, make some PCI
configuration dynamic.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
hw/usb/hcd-xhci-pci.h | 9 ++++
hw/usb/hcd-xhci-pci.c | 118 +++++++++++++++++++++++++++++++++---------
2 files changed, 103 insertions(+), 24 deletions(-)
diff --git a/hw/usb/hcd-xhci-pci.h b/hw/usb/hcd-xhci-pci.h
index 5b61ae84555..09aabae6e01 100644
--- a/hw/usb/hcd-xhci-pci.h
+++ b/hw/usb/hcd-xhci-pci.h
@@ -41,6 +41,15 @@ typedef struct XHCIPciState {
OnOffAuto msi;
OnOffAuto msix;
bool conditional_intr_mapping;
+ uint8_t cache_line_size;
+ uint8_t pm_cap_off;
+ uint8_t pcie_cap_off;
+ uint8_t msi_cap_off;
+ uint8_t msix_cap_off;
+ int msix_bar_nr;
+ uint64_t msix_bar_size;
+ uint32_t msix_table_off;
+ uint32_t msix_pba_off;
} XHCIPciState;
#endif
diff --git a/hw/usb/hcd-xhci-pci.c b/hw/usb/hcd-xhci-pci.c
index b93c80b09d8..911daf7e51f 100644
--- a/hw/usb/hcd-xhci-pci.c
+++ b/hw/usb/hcd-xhci-pci.c
@@ -32,9 +32,6 @@
#include "trace.h"
#include "qapi/error.h"
-#define OFF_MSIX_TABLE 0x3000
-#define OFF_MSIX_PBA 0x3800
-
static void xhci_pci_intr_update(XHCIState *xhci, int n, bool enable)
{
XHCIPciState *s = container_of(xhci, XHCIPciState, xhci);
@@ -120,6 +117,31 @@ static int xhci_pci_vmstate_post_load(void *opaque, int version_id)
return 0;
}
+static int xhci_pci_add_pm_capability(PCIDevice *pci_dev, uint8_t offset,
+ Error **errp)
+{
+ int err;
+
+ err = pci_add_capability(pci_dev, PCI_CAP_ID_PM, offset,
+ PCI_PM_SIZEOF, errp);
+ if (err < 0) {
+ return err;
+ }
+
+ pci_set_word(pci_dev->config + offset + PCI_PM_PMC,
+ PCI_PM_CAP_VER_1_2 |
+ PCI_PM_CAP_D1 | PCI_PM_CAP_D2 |
+ PCI_PM_CAP_PME_D0 | PCI_PM_CAP_PME_D1 |
+ PCI_PM_CAP_PME_D2 | PCI_PM_CAP_PME_D3hot);
+ pci_set_word(pci_dev->wmask + offset + PCI_PM_PMC, 0);
+ pci_set_word(pci_dev->config + offset + PCI_PM_CTRL,
+ PCI_PM_CTRL_NO_SOFT_RESET);
+ pci_set_word(pci_dev->wmask + offset + PCI_PM_CTRL,
+ PCI_PM_CTRL_STATE_MASK);
+
+ return 0;
+}
+
static void usb_xhci_pci_realize(struct PCIDevice *dev, Error **errp)
{
int ret;
@@ -128,7 +150,7 @@ static void usb_xhci_pci_realize(struct PCIDevice *dev, Error **errp)
dev->config[PCI_CLASS_PROG] = 0x30; /* xHCI */
dev->config[PCI_INTERRUPT_PIN] = 0x01; /* interrupt pin 1 */
- dev->config[PCI_CACHE_LINE_SIZE] = 0x10;
+ dev->config[PCI_CACHE_LINE_SIZE] = s->cache_line_size;
dev->config[0x60] = 0x30; /* release number */
object_property_set_link(OBJECT(&s->xhci), "host", OBJECT(s), NULL);
@@ -144,40 +166,78 @@ static void usb_xhci_pci_realize(struct PCIDevice *dev, Error **errp)
s->xhci.nec_quirks = true;
}
- if (s->msi != ON_OFF_AUTO_OFF) {
- ret = msi_init(dev, 0x70, s->xhci.numintrs, true, false, &err);
- /*
- * Any error other than -ENOTSUP(board's MSI support is broken)
- * is a programming error
- */
- assert(!ret || ret == -ENOTSUP);
- if (ret && s->msi == ON_OFF_AUTO_ON) {
- /* Can't satisfy user's explicit msi=on request, fail */
- error_append_hint(&err, "You have to use msi=auto (default) or "
- "msi=off with this machine type.\n");
+ if (s->pm_cap_off) {
+ if (xhci_pci_add_pm_capability(dev, s->pm_cap_off, &err)) {
error_propagate(errp, err);
return;
}
- assert(!err || s->msi == ON_OFF_AUTO_AUTO);
- /* With msi=auto, we fall back to MSI off silently */
- error_free(err);
}
+
+ if (s->msi != ON_OFF_AUTO_OFF) {
+ ret = msi_init(dev, s->msi_cap_off, s->xhci.numintrs,
+ true, false, &err);
+ if (ret) {
+ if (ret != -ENOTSUP) {
+ /* Programming error */
+ error_propagate(errp, err);
+ return;
+ }
+ if (s->msi == ON_OFF_AUTO_ON) {
+ /* Can't satisfy user's explicit msi=on request, fail */
+ error_append_hint(&err, "You have to use msi=auto (default) "
+ "or msi=off with this machine type.\n");
+ error_propagate(errp, err);
+ return;
+ }
+ error_free(err);
+ err = NULL; /* With msi=auto, we fall back to MSI off silently */
+ }
+ }
+
pci_register_bar(dev, 0,
PCI_BASE_ADDRESS_SPACE_MEMORY |
PCI_BASE_ADDRESS_MEM_TYPE_64,
&s->xhci.mem);
if (pci_bus_is_express(pci_get_bus(dev))) {
- ret = pcie_endpoint_cap_init(dev, 0xa0);
+ ret = pcie_endpoint_cap_init(dev, s->pcie_cap_off);
assert(ret > 0);
}
if (s->msix != ON_OFF_AUTO_OFF) {
- /* TODO check for errors, and should fail when msix=on */
- msix_init(dev, s->xhci.numintrs,
- &s->xhci.mem, 0, OFF_MSIX_TABLE,
- &s->xhci.mem, 0, OFF_MSIX_PBA,
- 0x90, NULL);
+ MemoryRegion *msix_bar = &s->xhci.mem;
+
+ if (s->msix_bar_nr != 0) {
+ memory_region_init(&dev->msix_exclusive_bar, OBJECT(dev),
+ "xhci-msix", s->msix_bar_size);
+ msix_bar = &dev->msix_exclusive_bar;
+ pci_register_bar(dev, s->msix_bar_nr,
+ PCI_BASE_ADDRESS_SPACE_MEMORY |
+ PCI_BASE_ADDRESS_MEM_TYPE_64,
+ msix_bar);
+ }
+
+ ret = msix_init(dev, s->xhci.numintrs,
+ msix_bar, s->msix_bar_nr, s->msix_table_off,
+ msix_bar, s->msix_bar_nr, s->msix_pba_off,
+ s->msix_cap_off, &err);
+ if (ret) {
+ if (ret != -ENOTSUP) {
+ /* Programming error */
+ error_propagate(errp, err);
+ return;
+ }
+ if (s->msix == ON_OFF_AUTO_ON) {
+ /* Can't satisfy user's explicit msix=on request, fail */
+ error_append_hint(&err, "You have to use msix=auto (default) "
+ "or msix=off with this machine type.\n");
+ error_propagate(errp, err);
+ return;
+ }
+ error_free(err);
+ err = NULL; /* With msix=auto, we fall back to MSI-X off silently */
+ /* Should we unregister BAR and memory region here? */
+ }
}
s->xhci.as = pci_get_address_space(dev);
}
@@ -214,6 +274,16 @@ static void xhci_instance_init(Object *obj)
PCI_DEVICE(obj)->cap_present |= QEMU_PCI_CAP_EXPRESS;
object_initialize_child(obj, "xhci-core", &s->xhci, TYPE_XHCI);
qdev_alias_all_properties(DEVICE(&s->xhci), obj);
+
+ s->cache_line_size = 0x10;
+ s->pm_cap_off = 0;
+ s->pcie_cap_off = 0xa0;
+ s->msi_cap_off = 0x70;
+ s->msix_cap_off = 0x90;
+ s->msix_bar_nr = 0;
+ s->msix_bar_size = 0;
+ s->msix_table_off = 0x3000;
+ s->msix_pba_off = 0x3800;
}
static const Property xhci_pci_properties[] = {
--
2.47.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v4 09/22] hw/usb/hcd-xhci-pci: Make PCI device more configurable
2025-05-02 3:30 ` [PATCH v4 09/22] hw/usb/hcd-xhci-pci: Make PCI device more configurable Nicholas Piggin
@ 2025-05-12 13:12 ` Peter Maydell
0 siblings, 0 replies; 45+ messages in thread
From: Peter Maydell @ 2025-05-12 13:12 UTC (permalink / raw)
To: Nicholas Piggin
Cc: Gerd Hoffmann, qemu-devel, Kevin Wolf, Paolo Bonzini,
Michael S. Tsirkin, Marcel Apfelbaum, Fabiano Rosas,
Laurent Vivier, Phil Dennis-Jordan, Bernhard Beschow,
Philippe Mathieu-Daudé
On Fri, 2 May 2025 at 04:33, Nicholas Piggin <npiggin@gmail.com> wrote:
>
> To prepare to support another USB PCI Host Controller, make some PCI
> configuration dynamic.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
I think this patch is trying to do too many things at once.
It's OK to bundle the small "turn a constant into a state
struct field" changes together, but the ones that are
more extensive code logic changes should be split out.
In particular "add support for pm capability" should be
its own patch as that's a new feature, and it looks like
there's also a change in here that's fixing a TODO comment?
thanks
-- PMM
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v4 10/22] hw/usb/hcd-xhci-pci: Add TI TUSB73X0 XHCI controller model
2025-05-02 3:30 [PATCH v4 00/22] usb/xhci and usb/msd improvements and tests Nicholas Piggin
` (8 preceding siblings ...)
2025-05-02 3:30 ` [PATCH v4 09/22] hw/usb/hcd-xhci-pci: Make PCI device more configurable Nicholas Piggin
@ 2025-05-02 3:30 ` Nicholas Piggin
2025-05-12 13:15 ` Peter Maydell
2025-05-02 3:30 ` [PATCH v4 11/22] usb/msd: Split in and out packet handling Nicholas Piggin
` (12 subsequent siblings)
22 siblings, 1 reply; 45+ messages in thread
From: Nicholas Piggin @ 2025-05-02 3:30 UTC (permalink / raw)
To: Gerd Hoffmann
Cc: Nicholas Piggin, qemu-devel, Kevin Wolf, Paolo Bonzini,
Michael S. Tsirkin, Marcel Apfelbaum, Fabiano Rosas,
Laurent Vivier, Phil Dennis-Jordan, Bernhard Beschow,
Philippe Mathieu-Daudé
The TI TUSB73X0 controller has some interesting differences from NEC,
notably a separate BAR for MSIX, and PM capabilities. The spec is freely
available without sign-up.
This controller is accepted by IBM Power proprietary firmware and
software (when the subsystem IDs are set to Power servers, which is not
done here). IBM code is picky about device support, so the NEC device
can not be used.
xhci qtests are enabled for this device.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
include/hw/pci/pci_ids.h | 1 +
include/hw/usb/xhci.h | 1 +
hw/usb/hcd-xhci-ti.c | 77 +++++++++++++++++++++++++++++++++
tests/qtest/usb-hcd-xhci-test.c | 3 ++
hw/usb/Kconfig | 5 +++
hw/usb/meson.build | 1 +
6 files changed, 88 insertions(+)
create mode 100644 hw/usb/hcd-xhci-ti.c
diff --git a/include/hw/pci/pci_ids.h b/include/hw/pci/pci_ids.h
index 33e2898be95..99fe751703f 100644
--- a/include/hw/pci/pci_ids.h
+++ b/include/hw/pci/pci_ids.h
@@ -182,6 +182,7 @@
#define PCI_VENDOR_ID_HP 0x103c
#define PCI_VENDOR_ID_TI 0x104c
+#define PCI_DEVICE_ID_TI_TUSB73X0 0x8241
#define PCI_VENDOR_ID_MOTOROLA 0x1057
#define PCI_DEVICE_ID_MOTOROLA_MPC106 0x0002
diff --git a/include/hw/usb/xhci.h b/include/hw/usb/xhci.h
index 5c90e1373e5..203ec1fca32 100644
--- a/include/hw/usb/xhci.h
+++ b/include/hw/usb/xhci.h
@@ -3,6 +3,7 @@
#define TYPE_XHCI "base-xhci"
#define TYPE_NEC_XHCI "nec-usb-xhci"
+#define TYPE_TI_XHCI "ti-usb-xhci"
#define TYPE_QEMU_XHCI "qemu-xhci"
#define TYPE_XHCI_SYSBUS "sysbus-xhci"
diff --git a/hw/usb/hcd-xhci-ti.c b/hw/usb/hcd-xhci-ti.c
new file mode 100644
index 00000000000..b7bb71c62e8
--- /dev/null
+++ b/hw/usb/hcd-xhci-ti.c
@@ -0,0 +1,77 @@
+/*
+ * USB xHCI TI TUSB73X0 controller emulation
+ * Datasheet https://www.ti.com/product/TUSB7340
+ *
+ * Copyright (c) 2025 IBM Corporation
+ * Derived from hcd-xhci-nec.c, copyright accordingly.
+ *
+ * SPDX-License-Identifier: LGPL-2.1-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "hw/usb.h"
+#include "qemu/module.h"
+#include "hw/pci/pci.h"
+#include "hw/qdev-properties.h"
+
+#include "hcd-xhci-pci.h"
+
+OBJECT_DECLARE_SIMPLE_TYPE(XHCITiState, TI_XHCI)
+
+struct XHCITiState {
+ XHCIPciState parent_obj;
+
+ uint32_t intrs;
+ uint32_t slots;
+};
+
+static const Property ti_xhci_properties[] = {
+ DEFINE_PROP_UINT32("intrs", XHCITiState, intrs, 8),
+ DEFINE_PROP_UINT32("slots", XHCITiState, slots, XHCI_MAXSLOTS),
+};
+
+static void ti_xhci_instance_init(Object *obj)
+{
+ XHCIPciState *pci = XHCI_PCI(obj);
+ XHCITiState *ti = TI_XHCI(obj);
+
+ pci->xhci.numintrs = ti->intrs;
+ pci->xhci.numslots = ti->slots;
+
+ /* Taken from datasheet */
+ pci->cache_line_size = 0x0;
+ pci->pm_cap_off = 0x40;
+ pci->pcie_cap_off = 0x70;
+ pci->msi_cap_off = 0x48;
+ pci->msix_cap_off = 0xc0;
+ pci->msix_bar_nr = 0x2;
+ pci->msix_bar_size = 0x800000;
+ pci->msix_table_off = 0x0;
+ pci->msix_pba_off = 0x1000;
+}
+
+static void ti_xhci_class_init(ObjectClass *klass, const void *data)
+{
+ PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
+ DeviceClass *dc = DEVICE_CLASS(klass);
+
+ device_class_set_props(dc, ti_xhci_properties);
+ k->vendor_id = PCI_VENDOR_ID_TI;
+ k->device_id = PCI_DEVICE_ID_TI_TUSB73X0;
+ k->revision = 0x02;
+}
+
+static const TypeInfo ti_xhci_info = {
+ .name = TYPE_TI_XHCI,
+ .parent = TYPE_XHCI_PCI,
+ .instance_size = sizeof(XHCITiState),
+ .instance_init = ti_xhci_instance_init,
+ .class_init = ti_xhci_class_init,
+};
+
+static void ti_xhci_register_types(void)
+{
+ type_register_static(&ti_xhci_info);
+}
+
+type_init(ti_xhci_register_types)
diff --git a/tests/qtest/usb-hcd-xhci-test.c b/tests/qtest/usb-hcd-xhci-test.c
index 2eecc8d9f26..428200d9e41 100644
--- a/tests/qtest/usb-hcd-xhci-test.c
+++ b/tests/qtest/usb-hcd-xhci-test.c
@@ -71,6 +71,8 @@ typedef struct XHCIQState {
PCI_VENDOR_ID_REDHAT)
#define XHCI_NEC_ID (PCI_DEVICE_ID_NEC_UPD720200 << 16 | \
PCI_VENDOR_ID_NEC)
+#define XHCI_TI_ID (PCI_DEVICE_ID_TI_TUSB73X0 << 16 | \
+ PCI_VENDOR_ID_TI)
/**
* Locate, verify, and return a handle to the XHCI device.
@@ -932,6 +934,7 @@ int main(int argc, char **argv)
TestData td[] = {
{ .device = "qemu-xhci", .fingerprint = XHCI_QEMU_ID, },
{ .device = "nec-usb-xhci", .fingerprint = XHCI_NEC_ID, },
+ { .device = "ti-usb-xhci", .fingerprint = XHCI_TI_ID, },
};
g_test_init(&argc, &argv, NULL);
diff --git a/hw/usb/Kconfig b/hw/usb/Kconfig
index 69c663be52f..00d82a97211 100644
--- a/hw/usb/Kconfig
+++ b/hw/usb/Kconfig
@@ -49,6 +49,11 @@ config USB_XHCI_NEC
default y if PCI_DEVICES
select USB_XHCI_PCI
+config USB_XHCI_TI
+ bool
+ default y if PCI_DEVICES
+ select USB_XHCI_PCI
+
config USB_XHCI_SYSBUS
bool
select USB_XHCI
diff --git a/hw/usb/meson.build b/hw/usb/meson.build
index 17360a5b5a4..375fa420be6 100644
--- a/hw/usb/meson.build
+++ b/hw/usb/meson.build
@@ -23,6 +23,7 @@ system_ss.add(when: 'CONFIG_USB_XHCI', if_true: files('hcd-xhci.c'))
system_ss.add(when: 'CONFIG_USB_XHCI_PCI', if_true: files('hcd-xhci-pci.c'))
system_ss.add(when: 'CONFIG_USB_XHCI_SYSBUS', if_true: files('hcd-xhci-sysbus.c'))
system_ss.add(when: 'CONFIG_USB_XHCI_NEC', if_true: files('hcd-xhci-nec.c'))
+system_ss.add(when: 'CONFIG_USB_XHCI_TI', if_true: files('hcd-xhci-ti.c'))
system_ss.add(when: 'CONFIG_USB_DWC2', if_true: files('hcd-dwc2.c'))
system_ss.add(when: 'CONFIG_USB_DWC3', if_true: files('hcd-dwc3.c'))
system_ss.add(when: 'CONFIG_USB_CHIPIDEA', if_true: files('chipidea.c'))
--
2.47.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v4 10/22] hw/usb/hcd-xhci-pci: Add TI TUSB73X0 XHCI controller model
2025-05-02 3:30 ` [PATCH v4 10/22] hw/usb/hcd-xhci-pci: Add TI TUSB73X0 XHCI controller model Nicholas Piggin
@ 2025-05-12 13:15 ` Peter Maydell
0 siblings, 0 replies; 45+ messages in thread
From: Peter Maydell @ 2025-05-12 13:15 UTC (permalink / raw)
To: Nicholas Piggin
Cc: Gerd Hoffmann, qemu-devel, Kevin Wolf, Paolo Bonzini,
Michael S. Tsirkin, Marcel Apfelbaum, Fabiano Rosas,
Laurent Vivier, Phil Dennis-Jordan, Bernhard Beschow,
Philippe Mathieu-Daudé
On Fri, 2 May 2025 at 04:32, Nicholas Piggin <npiggin@gmail.com> wrote:
>
> The TI TUSB73X0 controller has some interesting differences from NEC,
> notably a separate BAR for MSIX, and PM capabilities. The spec is freely
> available without sign-up.
>
> This controller is accepted by IBM Power proprietary firmware and
> software (when the subsystem IDs are set to Power servers, which is not
> done here). IBM code is picky about device support, so the NEC device
> can not be used.
>
> xhci qtests are enabled for this device.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
thanks
-- PMM
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v4 11/22] usb/msd: Split in and out packet handling
2025-05-02 3:30 [PATCH v4 00/22] usb/xhci and usb/msd improvements and tests Nicholas Piggin
` (9 preceding siblings ...)
2025-05-02 3:30 ` [PATCH v4 10/22] hw/usb/hcd-xhci-pci: Add TI TUSB73X0 XHCI controller model Nicholas Piggin
@ 2025-05-02 3:30 ` Nicholas Piggin
2025-05-05 9:22 ` Kevin Wolf
2025-05-02 3:30 ` [PATCH v4 12/22] usb/msd: Ensure packet structure layout is correct Nicholas Piggin
` (11 subsequent siblings)
22 siblings, 1 reply; 45+ messages in thread
From: Nicholas Piggin @ 2025-05-02 3:30 UTC (permalink / raw)
To: Gerd Hoffmann
Cc: Nicholas Piggin, qemu-devel, Kevin Wolf, Paolo Bonzini,
Michael S. Tsirkin, Marcel Apfelbaum, Fabiano Rosas,
Laurent Vivier, Phil Dennis-Jordan, Bernhard Beschow,
Philippe Mathieu-Daudé
Split in and out packet handling int otheir own functions, to make
them a bit more managable.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
hw/usb/dev-storage.c | 266 +++++++++++++++++++++++--------------------
1 file changed, 145 insertions(+), 121 deletions(-)
diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
index b13fe345c45..394fb8e1ec0 100644
--- a/hw/usb/dev-storage.c
+++ b/hw/usb/dev-storage.c
@@ -395,158 +395,182 @@ static void usb_msd_cancel_io(USBDevice *dev, USBPacket *p)
}
}
-static void usb_msd_handle_data(USBDevice *dev, USBPacket *p)
+static void usb_msd_handle_data_out(USBDevice *dev, USBPacket *p)
{
MSDState *s = (MSDState *)dev;
uint32_t tag;
struct usb_msd_cbw cbw;
- uint8_t devep = p->ep->nr;
SCSIDevice *scsi_dev;
int len;
- if (s->needs_reset) {
- p->status = USB_RET_STALL;
- return;
- }
+ switch (s->mode) {
+ case USB_MSDM_CBW:
+ if (p->iov.size != 31) {
+ error_report("usb-msd: Bad CBW size");
+ goto fail;
+ }
+ usb_packet_copy(p, &cbw, 31);
+ if (le32_to_cpu(cbw.sig) != 0x43425355) {
+ error_report("usb-msd: Bad signature %08x",
+ le32_to_cpu(cbw.sig));
+ goto fail;
+ }
+ scsi_dev = scsi_device_find(&s->bus, 0, 0, cbw.lun);
+ if (scsi_dev == NULL) {
+ error_report("usb-msd: Bad LUN %d", cbw.lun);
+ goto fail;
+ }
+ tag = le32_to_cpu(cbw.tag);
+ s->data_len = le32_to_cpu(cbw.data_len);
+ if (s->data_len == 0) {
+ s->mode = USB_MSDM_CSW;
+ } else if (cbw.flags & 0x80) {
+ s->mode = USB_MSDM_DATAIN;
+ } else {
+ s->mode = USB_MSDM_DATAOUT;
+ }
+ trace_usb_msd_cmd_submit(cbw.lun, tag, cbw.flags,
+ cbw.cmd_len, s->data_len);
+ assert(le32_to_cpu(s->csw.residue) == 0);
+ s->scsi_len = 0;
+ s->req = scsi_req_new(scsi_dev, tag, cbw.lun,
+ cbw.cmd, cbw.cmd_len, NULL);
+ if (s->commandlog) {
+ scsi_req_print(s->req);
+ }
+ len = scsi_req_enqueue(s->req);
+ if (len) {
+ scsi_req_continue(s->req);
+ }
+ break;
- switch (p->pid) {
- case USB_TOKEN_OUT:
- if (devep != 2)
+ case USB_MSDM_DATAOUT:
+ trace_usb_msd_data_out(p->iov.size, s->data_len);
+ if (p->iov.size > s->data_len) {
goto fail;
+ }
- switch (s->mode) {
- case USB_MSDM_CBW:
- if (p->iov.size != 31) {
- error_report("usb-msd: Bad CBW size");
- goto fail;
- }
- usb_packet_copy(p, &cbw, 31);
- if (le32_to_cpu(cbw.sig) != 0x43425355) {
- error_report("usb-msd: Bad signature %08x",
- le32_to_cpu(cbw.sig));
- goto fail;
- }
- scsi_dev = scsi_device_find(&s->bus, 0, 0, cbw.lun);
- if (scsi_dev == NULL) {
- error_report("usb-msd: Bad LUN %d", cbw.lun);
- goto fail;
- }
- tag = le32_to_cpu(cbw.tag);
- s->data_len = le32_to_cpu(cbw.data_len);
- if (s->data_len == 0) {
- s->mode = USB_MSDM_CSW;
- } else if (cbw.flags & 0x80) {
- s->mode = USB_MSDM_DATAIN;
- } else {
- s->mode = USB_MSDM_DATAOUT;
- }
- trace_usb_msd_cmd_submit(cbw.lun, tag, cbw.flags,
- cbw.cmd_len, s->data_len);
- assert(le32_to_cpu(s->csw.residue) == 0);
- s->scsi_len = 0;
- s->req = scsi_req_new(scsi_dev, tag, cbw.lun, cbw.cmd, cbw.cmd_len, NULL);
- if (s->commandlog) {
- scsi_req_print(s->req);
- }
- len = scsi_req_enqueue(s->req);
+ if (s->scsi_len) {
+ usb_msd_copy_data(s, p);
+ }
+ if (le32_to_cpu(s->csw.residue)) {
+ len = p->iov.size - p->actual_length;
if (len) {
- scsi_req_continue(s->req);
+ usb_packet_skip(p, len);
+ if (len > s->data_len) {
+ len = s->data_len;
+ }
+ s->data_len -= len;
+ if (s->data_len == 0) {
+ s->mode = USB_MSDM_CSW;
+ }
}
- break;
+ }
+ if (p->actual_length < p->iov.size) {
+ trace_usb_msd_packet_async();
+ s->packet = p;
+ p->status = USB_RET_ASYNC;
+ }
+ break;
- case USB_MSDM_DATAOUT:
- trace_usb_msd_data_out(p->iov.size, s->data_len);
- if (p->iov.size > s->data_len) {
- goto fail;
- }
+ default:
+ goto fail;
+ }
+ return;
- if (s->scsi_len) {
- usb_msd_copy_data(s, p);
- }
- if (le32_to_cpu(s->csw.residue)) {
- len = p->iov.size - p->actual_length;
- if (len) {
- usb_packet_skip(p, len);
- if (len > s->data_len) {
- len = s->data_len;
- }
- s->data_len -= len;
- if (s->data_len == 0) {
- s->mode = USB_MSDM_CSW;
- }
- }
- }
- if (p->actual_length < p->iov.size) {
- trace_usb_msd_packet_async();
- s->packet = p;
- p->status = USB_RET_ASYNC;
- }
- break;
+fail:
+ p->status = USB_RET_STALL;
+}
- default:
+static void usb_msd_handle_data_in(USBDevice *dev, USBPacket *p)
+{
+ MSDState *s = (MSDState *)dev;
+ int len;
+
+ switch (s->mode) {
+ case USB_MSDM_DATAOUT:
+ if (s->data_len != 0 || p->iov.size < 13) {
goto fail;
}
+ /* Waiting for SCSI write to complete. */
+ trace_usb_msd_packet_async();
+ s->packet = p;
+ p->status = USB_RET_ASYNC;
break;
- case USB_TOKEN_IN:
- if (devep != 1)
+ case USB_MSDM_CSW:
+ if (p->iov.size < 13) {
goto fail;
+ }
- switch (s->mode) {
- case USB_MSDM_DATAOUT:
- if (s->data_len != 0 || p->iov.size < 13) {
- goto fail;
- }
- /* Waiting for SCSI write to complete. */
+ if (s->req) {
+ /* still in flight */
trace_usb_msd_packet_async();
s->packet = p;
p->status = USB_RET_ASYNC;
- break;
+ } else {
+ usb_msd_send_status(s, p);
+ s->mode = USB_MSDM_CBW;
+ }
+ break;
- case USB_MSDM_CSW:
- if (p->iov.size < 13) {
- goto fail;
+ case USB_MSDM_DATAIN:
+ trace_usb_msd_data_in(p->iov.size, s->data_len, s->scsi_len);
+ if (s->scsi_len) {
+ usb_msd_copy_data(s, p);
+ }
+ if (le32_to_cpu(s->csw.residue)) {
+ len = p->iov.size - p->actual_length;
+ if (len) {
+ usb_packet_skip(p, len);
+ if (len > s->data_len) {
+ len = s->data_len;
+ }
+ s->data_len -= len;
+ if (s->data_len == 0) {
+ s->mode = USB_MSDM_CSW;
+ }
}
+ }
+ if (p->actual_length < p->iov.size && s->mode == USB_MSDM_DATAIN) {
+ trace_usb_msd_packet_async();
+ s->packet = p;
+ p->status = USB_RET_ASYNC;
+ }
+ break;
- if (s->req) {
- /* still in flight */
- trace_usb_msd_packet_async();
- s->packet = p;
- p->status = USB_RET_ASYNC;
- } else {
- usb_msd_send_status(s, p);
- s->mode = USB_MSDM_CBW;
- }
- break;
+ default:
+ goto fail;
+ }
+ return;
- case USB_MSDM_DATAIN:
- trace_usb_msd_data_in(p->iov.size, s->data_len, s->scsi_len);
- if (s->scsi_len) {
- usb_msd_copy_data(s, p);
- }
- if (le32_to_cpu(s->csw.residue)) {
- len = p->iov.size - p->actual_length;
- if (len) {
- usb_packet_skip(p, len);
- if (len > s->data_len) {
- len = s->data_len;
- }
- s->data_len -= len;
- if (s->data_len == 0) {
- s->mode = USB_MSDM_CSW;
- }
- }
- }
- if (p->actual_length < p->iov.size && s->mode == USB_MSDM_DATAIN) {
- trace_usb_msd_packet_async();
- s->packet = p;
- p->status = USB_RET_ASYNC;
- }
- break;
+fail:
+ p->status = USB_RET_STALL;
+}
+
+static void usb_msd_handle_data(USBDevice *dev, USBPacket *p)
+{
+ MSDState *s = (MSDState *)dev;
+ uint8_t devep = p->ep->nr;
- default:
+ if (s->needs_reset) {
+ p->status = USB_RET_STALL;
+ return;
+ }
+
+ switch (p->pid) {
+ case USB_TOKEN_OUT:
+ if (devep != 2) {
+ goto fail;
+ }
+ usb_msd_handle_data_out(dev, p);
+ break;
+
+ case USB_TOKEN_IN:
+ if (devep != 1) {
goto fail;
}
+ usb_msd_handle_data_in(dev, p);
break;
default:
--
2.47.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v4 11/22] usb/msd: Split in and out packet handling
2025-05-02 3:30 ` [PATCH v4 11/22] usb/msd: Split in and out packet handling Nicholas Piggin
@ 2025-05-05 9:22 ` Kevin Wolf
0 siblings, 0 replies; 45+ messages in thread
From: Kevin Wolf @ 2025-05-05 9:22 UTC (permalink / raw)
To: Nicholas Piggin
Cc: Gerd Hoffmann, qemu-devel, Paolo Bonzini, Michael S. Tsirkin,
Marcel Apfelbaum, Fabiano Rosas, Laurent Vivier,
Phil Dennis-Jordan, Bernhard Beschow, Philippe Mathieu-Daudé
Am 02.05.2025 um 05:30 hat Nicholas Piggin geschrieben:
> Split in and out packet handling int otheir own functions, to make
> them a bit more managable.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v4 12/22] usb/msd: Ensure packet structure layout is correct
2025-05-02 3:30 [PATCH v4 00/22] usb/xhci and usb/msd improvements and tests Nicholas Piggin
` (10 preceding siblings ...)
2025-05-02 3:30 ` [PATCH v4 11/22] usb/msd: Split in and out packet handling Nicholas Piggin
@ 2025-05-02 3:30 ` Nicholas Piggin
2025-05-05 9:30 ` Kevin Wolf
2025-05-02 3:30 ` [PATCH v4 13/22] usb/msd: Improved handling of mass storage reset Nicholas Piggin
` (10 subsequent siblings)
22 siblings, 1 reply; 45+ messages in thread
From: Nicholas Piggin @ 2025-05-02 3:30 UTC (permalink / raw)
To: Gerd Hoffmann
Cc: Nicholas Piggin, qemu-devel, Kevin Wolf, Paolo Bonzini,
Michael S. Tsirkin, Marcel Apfelbaum, Fabiano Rosas,
Laurent Vivier, Phil Dennis-Jordan, Bernhard Beschow,
Philippe Mathieu-Daudé
These structures are hardware interfaces, ensure the layout is
correct. Add defines for the data sizes throughout the code.
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
hw/usb/dev-storage.c | 21 ++++++++++++++++-----
1 file changed, 16 insertions(+), 5 deletions(-)
diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
index 394fb8e1ec0..41924b9320e 100644
--- a/hw/usb/dev-storage.c
+++ b/hw/usb/dev-storage.c
@@ -27,7 +27,14 @@
#define MassStorageReset 0xff
#define GetMaxLun 0xfe
-struct usb_msd_cbw {
+/*
+ * CBW and CSW packets have a minimum size, enough to contain the
+ * respective data structure.
+ */
+#define CBW_SIZE sizeof(struct usb_msd_cbw)
+#define CSW_SIZE sizeof(struct usb_msd_csw)
+
+struct QEMU_PACKED usb_msd_cbw {
uint32_t sig;
uint32_t tag;
uint32_t data_len;
@@ -405,11 +412,11 @@ static void usb_msd_handle_data_out(USBDevice *dev, USBPacket *p)
switch (s->mode) {
case USB_MSDM_CBW:
- if (p->iov.size != 31) {
+ if (p->iov.size != CBW_SIZE) {
error_report("usb-msd: Bad CBW size");
goto fail;
}
- usb_packet_copy(p, &cbw, 31);
+ usb_packet_copy(p, &cbw, CBW_SIZE);
if (le32_to_cpu(cbw.sig) != 0x43425355) {
error_report("usb-msd: Bad signature %08x",
le32_to_cpu(cbw.sig));
@@ -489,7 +496,7 @@ static void usb_msd_handle_data_in(USBDevice *dev, USBPacket *p)
switch (s->mode) {
case USB_MSDM_DATAOUT:
- if (s->data_len != 0 || p->iov.size < 13) {
+ if (s->data_len != 0 || p->iov.size < CSW_SIZE) {
goto fail;
}
/* Waiting for SCSI write to complete. */
@@ -499,7 +506,7 @@ static void usb_msd_handle_data_in(USBDevice *dev, USBPacket *p)
break;
case USB_MSDM_CSW:
- if (p->iov.size < 13) {
+ if (p->iov.size < CSW_SIZE) {
goto fail;
}
@@ -636,6 +643,10 @@ static const TypeInfo usb_storage_dev_type_info = {
static void usb_msd_register_types(void)
{
+ /* Ensure the header structures are the right size */
+ qemu_build_assert(CBW_SIZE == 31);
+ qemu_build_assert(CSW_SIZE == 13);
+
type_register_static(&usb_storage_dev_type_info);
}
--
2.47.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v4 12/22] usb/msd: Ensure packet structure layout is correct
2025-05-02 3:30 ` [PATCH v4 12/22] usb/msd: Ensure packet structure layout is correct Nicholas Piggin
@ 2025-05-05 9:30 ` Kevin Wolf
0 siblings, 0 replies; 45+ messages in thread
From: Kevin Wolf @ 2025-05-05 9:30 UTC (permalink / raw)
To: Nicholas Piggin
Cc: Gerd Hoffmann, qemu-devel, Paolo Bonzini, Michael S. Tsirkin,
Marcel Apfelbaum, Fabiano Rosas, Laurent Vivier,
Phil Dennis-Jordan, Bernhard Beschow, Philippe Mathieu-Daudé
Am 02.05.2025 um 05:30 hat Nicholas Piggin geschrieben:
> These structures are hardware interfaces, ensure the layout is
> correct. Add defines for the data sizes throughout the code.
>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> @@ -636,6 +643,10 @@ static const TypeInfo usb_storage_dev_type_info = {
>
> static void usb_msd_register_types(void)
> {
> + /* Ensure the header structures are the right size */
> + qemu_build_assert(CBW_SIZE == 31);
> + qemu_build_assert(CSW_SIZE == 13);
> +
> type_register_static(&usb_storage_dev_type_info);
> }
There is no real reason to have this assertion inside of a function at
the end of the file. I'd prefer QEMU_BUILD_BUG_ON() next to the struct
declarations, but obviously it's correct either way, so with or without
that changed:
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v4 13/22] usb/msd: Improved handling of mass storage reset
2025-05-02 3:30 [PATCH v4 00/22] usb/xhci and usb/msd improvements and tests Nicholas Piggin
` (11 preceding siblings ...)
2025-05-02 3:30 ` [PATCH v4 12/22] usb/msd: Ensure packet structure layout is correct Nicholas Piggin
@ 2025-05-02 3:30 ` Nicholas Piggin
2025-05-02 3:30 ` [PATCH v4 14/22] usb/msd: Improve packet validation error logging Nicholas Piggin
` (9 subsequent siblings)
22 siblings, 0 replies; 45+ messages in thread
From: Nicholas Piggin @ 2025-05-02 3:30 UTC (permalink / raw)
To: Gerd Hoffmann
Cc: Nicholas Piggin, qemu-devel, Kevin Wolf, Paolo Bonzini,
Michael S. Tsirkin, Marcel Apfelbaum, Fabiano Rosas,
Laurent Vivier, Phil Dennis-Jordan, Bernhard Beschow,
Philippe Mathieu-Daudé
The mass storage reset request handling does not reset in-flight
SCSI requests or USB MSD packets. Implement this by calling the
device reset handler which should take care of everything.
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
hw/usb/dev-storage.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
index 41924b9320e..fda14271eae 100644
--- a/hw/usb/dev-storage.c
+++ b/hw/usb/dev-storage.c
@@ -366,7 +366,7 @@ static void usb_msd_handle_control(USBDevice *dev, USBPacket *p,
/* Class specific requests. */
case ClassInterfaceOutRequest | MassStorageReset:
/* Reset state ready for the next CBW. */
- s->mode = USB_MSDM_CBW;
+ usb_msd_handle_reset(dev);
break;
case ClassInterfaceRequest | GetMaxLun:
maxlun = 0;
--
2.47.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* [PATCH v4 14/22] usb/msd: Improve packet validation error logging
2025-05-02 3:30 [PATCH v4 00/22] usb/xhci and usb/msd improvements and tests Nicholas Piggin
` (12 preceding siblings ...)
2025-05-02 3:30 ` [PATCH v4 13/22] usb/msd: Improved handling of mass storage reset Nicholas Piggin
@ 2025-05-02 3:30 ` Nicholas Piggin
2025-05-05 10:26 ` Kevin Wolf
2025-05-02 3:30 ` [PATCH v4 15/22] usb/msd: Allow CBW packet size greater than 31 Nicholas Piggin
` (8 subsequent siblings)
22 siblings, 1 reply; 45+ messages in thread
From: Nicholas Piggin @ 2025-05-02 3:30 UTC (permalink / raw)
To: Gerd Hoffmann
Cc: Nicholas Piggin, qemu-devel, Kevin Wolf, Paolo Bonzini,
Michael S. Tsirkin, Marcel Apfelbaum, Fabiano Rosas,
Laurent Vivier, Phil Dennis-Jordan, Bernhard Beschow,
Philippe Mathieu-Daudé
Errors in incoming USB MSD packet format or context would typically
be guest software errors. Log these under guest errors.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
hw/usb/dev-storage.c | 53 +++++++++++++++++++++++++++++++++++---------
1 file changed, 42 insertions(+), 11 deletions(-)
diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
index fda14271eae..7bc2f7664b2 100644
--- a/hw/usb/dev-storage.c
+++ b/hw/usb/dev-storage.c
@@ -10,6 +10,7 @@
#include "qemu/osdep.h"
#include "qapi/error.h"
#include "qemu/error-report.h"
+#include "qemu/log.h"
#include "qemu/module.h"
#include "qemu/option.h"
#include "qemu/config-file.h"
@@ -402,6 +403,36 @@ static void usb_msd_cancel_io(USBDevice *dev, USBPacket *p)
}
}
+static bool try_get_valid_cbw(USBPacket *p, struct usb_msd_cbw *cbw)
+{
+ uint32_t sig;
+
+ if (p->iov.size != CBW_SIZE) {
+ qemu_log_mask(LOG_GUEST_ERROR, "usb-msd: Bad CBW size %zu\n",
+ p->iov.size);
+ return false;
+ }
+ usb_packet_copy(p, cbw, CBW_SIZE);
+ sig = le32_to_cpu(cbw->sig);
+ if (sig != 0x43425355) {
+ qemu_log_mask(LOG_GUEST_ERROR, "usb-msd: Bad CBW signature 0x%08x\n",
+ sig);
+ return false;
+ }
+
+ return true;
+}
+
+static bool check_valid_csw(USBPacket *p)
+{
+ if (p->iov.size < CSW_SIZE) {
+ qemu_log_mask(LOG_GUEST_ERROR, "usb-msd: Bad CSW size %zu\n",
+ p->iov.size);
+ return false;
+ }
+ return true;
+}
+
static void usb_msd_handle_data_out(USBDevice *dev, USBPacket *p)
{
MSDState *s = (MSDState *)dev;
@@ -412,19 +443,13 @@ static void usb_msd_handle_data_out(USBDevice *dev, USBPacket *p)
switch (s->mode) {
case USB_MSDM_CBW:
- if (p->iov.size != CBW_SIZE) {
- error_report("usb-msd: Bad CBW size");
- goto fail;
- }
- usb_packet_copy(p, &cbw, CBW_SIZE);
- if (le32_to_cpu(cbw.sig) != 0x43425355) {
- error_report("usb-msd: Bad signature %08x",
- le32_to_cpu(cbw.sig));
+ if (!try_get_valid_cbw(p, &cbw)) {
goto fail;
}
scsi_dev = scsi_device_find(&s->bus, 0, 0, cbw.lun);
if (scsi_dev == NULL) {
- error_report("usb-msd: Bad LUN %d", cbw.lun);
+ qemu_log_mask(LOG_GUEST_ERROR, "usb-msd: Bad CBW LUN %d\n",
+ cbw.lun);
goto fail;
}
tag = le32_to_cpu(cbw.tag);
@@ -496,9 +521,15 @@ static void usb_msd_handle_data_in(USBDevice *dev, USBPacket *p)
switch (s->mode) {
case USB_MSDM_DATAOUT:
- if (s->data_len != 0 || p->iov.size < CSW_SIZE) {
+ if (!check_valid_csw(p)) {
+ goto fail;
+ }
+ if (s->data_len != 0) {
+ qemu_log_mask(LOG_GUEST_ERROR, "usb-msd: CSW received before "
+ "all data was sent\n");
goto fail;
}
+
/* Waiting for SCSI write to complete. */
trace_usb_msd_packet_async();
s->packet = p;
@@ -506,7 +537,7 @@ static void usb_msd_handle_data_in(USBDevice *dev, USBPacket *p)
break;
case USB_MSDM_CSW:
- if (p->iov.size < CSW_SIZE) {
+ if (!check_valid_csw(p)) {
goto fail;
}
--
2.47.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v4 14/22] usb/msd: Improve packet validation error logging
2025-05-02 3:30 ` [PATCH v4 14/22] usb/msd: Improve packet validation error logging Nicholas Piggin
@ 2025-05-05 10:26 ` Kevin Wolf
0 siblings, 0 replies; 45+ messages in thread
From: Kevin Wolf @ 2025-05-05 10:26 UTC (permalink / raw)
To: Nicholas Piggin
Cc: Gerd Hoffmann, qemu-devel, Paolo Bonzini, Michael S. Tsirkin,
Marcel Apfelbaum, Fabiano Rosas, Laurent Vivier,
Phil Dennis-Jordan, Bernhard Beschow, Philippe Mathieu-Daudé
Am 02.05.2025 um 05:30 hat Nicholas Piggin geschrieben:
> Errors in incoming USB MSD packet format or context would typically
> be guest software errors. Log these under guest errors.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> +static bool check_valid_csw(USBPacket *p)
> +{
> + if (p->iov.size < CSW_SIZE) {
> + qemu_log_mask(LOG_GUEST_ERROR, "usb-msd: Bad CSW size %zu\n",
> + p->iov.size);
> + return false;
> + }
> + return true;
> +}
I feel this name might be a bit misleading. The spec says a CSW is valid
if its size is exactly 13 bytes, the signature is correct and the tag
matches the CBW tag. Of course, this is something that the host would
check after the device completes the request, not the device when it
receives the CSW.
Maybe just validate_csw_size() or something?
The logic looks good to me, though.
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v4 15/22] usb/msd: Allow CBW packet size greater than 31
2025-05-02 3:30 [PATCH v4 00/22] usb/xhci and usb/msd improvements and tests Nicholas Piggin
` (13 preceding siblings ...)
2025-05-02 3:30 ` [PATCH v4 14/22] usb/msd: Improve packet validation error logging Nicholas Piggin
@ 2025-05-02 3:30 ` Nicholas Piggin
2025-05-05 10:50 ` Kevin Wolf
2025-05-02 3:30 ` [PATCH v4 16/22] usb/msd: Split async packet tracking into data and csw Nicholas Piggin
` (7 subsequent siblings)
22 siblings, 1 reply; 45+ messages in thread
From: Nicholas Piggin @ 2025-05-02 3:30 UTC (permalink / raw)
To: Gerd Hoffmann
Cc: Nicholas Piggin, qemu-devel, Kevin Wolf, Paolo Bonzini,
Michael S. Tsirkin, Marcel Apfelbaum, Fabiano Rosas,
Laurent Vivier, Phil Dennis-Jordan, Bernhard Beschow,
Philippe Mathieu-Daudé
The CBW structure is 31 bytes, so CBW DATAOUT packets must be at least
31 bytes. QEMU enforces exactly 31 bytes, but this is inconsistent with
how it handles CSW packets (where it allows greater than or equal to 13
bytes) despite wording in the spec[*] being similar for both packet
types: "shall end as a short packet with exactly 31 bytes transferred".
[*] USB MSD Bulk-Only Transport 1.0
For consistency, and on the principle of being tolerant in accepting
input, relax the CBW size check.
Alternatively, both checks could be tightened to exact. Or a message
could be printed warning of possible guest error if size is not exact,
but still accept the packets.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
hw/usb/dev-storage.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
index 7bc2f7664b2..fe8955bf212 100644
--- a/hw/usb/dev-storage.c
+++ b/hw/usb/dev-storage.c
@@ -407,7 +407,7 @@ static bool try_get_valid_cbw(USBPacket *p, struct usb_msd_cbw *cbw)
{
uint32_t sig;
- if (p->iov.size != CBW_SIZE) {
+ if (p->iov.size < CBW_SIZE) {
qemu_log_mask(LOG_GUEST_ERROR, "usb-msd: Bad CBW size %zu\n",
p->iov.size);
return false;
--
2.47.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v4 15/22] usb/msd: Allow CBW packet size greater than 31
2025-05-02 3:30 ` [PATCH v4 15/22] usb/msd: Allow CBW packet size greater than 31 Nicholas Piggin
@ 2025-05-05 10:50 ` Kevin Wolf
0 siblings, 0 replies; 45+ messages in thread
From: Kevin Wolf @ 2025-05-05 10:50 UTC (permalink / raw)
To: Nicholas Piggin
Cc: Gerd Hoffmann, qemu-devel, Paolo Bonzini, Michael S. Tsirkin,
Marcel Apfelbaum, Fabiano Rosas, Laurent Vivier,
Phil Dennis-Jordan, Bernhard Beschow, Philippe Mathieu-Daudé
Am 02.05.2025 um 05:30 hat Nicholas Piggin geschrieben:
> The CBW structure is 31 bytes, so CBW DATAOUT packets must be at least
> 31 bytes. QEMU enforces exactly 31 bytes, but this is inconsistent with
> how it handles CSW packets (where it allows greater than or equal to 13
> bytes) despite wording in the spec[*] being similar for both packet
> types: "shall end as a short packet with exactly 31 bytes transferred".
>
> [*] USB MSD Bulk-Only Transport 1.0
>
> For consistency, and on the principle of being tolerant in accepting
> input, relax the CBW size check.
>
> Alternatively, both checks could be tightened to exact. Or a message
> could be printed warning of possible guest error if size is not exact,
> but still accept the packets.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
This doesn't look right to me.
CBW is a message from the host to the device. The device must fully
validate the data in it (see "6.2 Valid and Meaningful CBW"). My
understanding is that a wrong CBW size is an error.
CSW is a message from the device to the host, i.e. the iovec doesn't
really have any content when we get it. It's essentially just a buffer
in which usb-storage has to construct a valid CSW (of the exact size
13). If the buffer is larger than it has to be, that's a different case
than receiving a CBW of the wrong size. I'm not entirely sure what the
mechanism is to send exactly 13 bytes, but I assume it's related to
p->actual_length, which is updated in usb_packet_copy().
Actually, if we reject too small buffers, why do we even need the MIN()
in usb_msd_send_status()? Shouldn't len be an unconditional CSW_SIZE?
Kevin
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v4 16/22] usb/msd: Split async packet tracking into data and csw
2025-05-02 3:30 [PATCH v4 00/22] usb/xhci and usb/msd improvements and tests Nicholas Piggin
` (14 preceding siblings ...)
2025-05-02 3:30 ` [PATCH v4 15/22] usb/msd: Allow CBW packet size greater than 31 Nicholas Piggin
@ 2025-05-02 3:30 ` Nicholas Piggin
2025-05-05 13:05 ` Kevin Wolf
2025-05-02 3:30 ` [PATCH v4 17/22] usb/msd: Add some additional assertions Nicholas Piggin
` (6 subsequent siblings)
22 siblings, 1 reply; 45+ messages in thread
From: Nicholas Piggin @ 2025-05-02 3:30 UTC (permalink / raw)
To: Gerd Hoffmann
Cc: Nicholas Piggin, qemu-devel, Kevin Wolf, Paolo Bonzini,
Michael S. Tsirkin, Marcel Apfelbaum, Fabiano Rosas,
Laurent Vivier, Phil Dennis-Jordan, Bernhard Beschow,
Philippe Mathieu-Daudé
The async packet handling logic has places that infer whether the
async packet is data or CSW, based on context. This is not wrong,
it just makes the logic easier to follow if they are categorised
when they are accepted.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
include/hw/usb/msd.h | 5 +-
hw/usb/dev-storage.c | 121 +++++++++++++++++++++++++++----------------
2 files changed, 79 insertions(+), 47 deletions(-)
diff --git a/include/hw/usb/msd.h b/include/hw/usb/msd.h
index f9fd862b529..a40d15f5def 100644
--- a/include/hw/usb/msd.h
+++ b/include/hw/usb/msd.h
@@ -33,8 +33,11 @@ struct MSDState {
struct usb_msd_csw csw;
SCSIRequest *req;
SCSIBus bus;
+
/* For async completion. */
- USBPacket *packet;
+ USBPacket *data_packet;
+ USBPacket *csw_in_packet;
+
/* usb-storage only */
BlockConf conf;
bool removable;
diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
index fe8955bf212..66fffda3713 100644
--- a/hw/usb/dev-storage.c
+++ b/hw/usb/dev-storage.c
@@ -185,18 +185,33 @@ static const USBDesc desc = {
.str = desc_strings,
};
-static void usb_msd_packet_complete(MSDState *s, int status)
+static void usb_msd_data_packet_complete(MSDState *s, int status)
{
- USBPacket *p = s->packet;
+ USBPacket *p = s->data_packet;
/*
- * Set s->packet to NULL before calling usb_packet_complete
- * because another request may be issued before
- * usb_packet_complete returns.
+ * Set s->data_packet to NULL before calling usb_packet_complete
+ * because another request may be issued before usb_packet_complete
+ * returns.
*/
trace_usb_msd_packet_complete();
+ s->data_packet = NULL;
+ p->status = status;
+ usb_packet_complete(&s->dev, p);
+}
+
+static void usb_msd_csw_packet_complete(MSDState *s, int status)
+{
+ USBPacket *p = s->csw_in_packet;
+
+ /*
+ * Set s->csw_in_packet to NULL before calling usb_packet_complete
+ * because another request may be issued before usb_packet_complete
+ * returns.
+ */
+ trace_usb_msd_packet_complete();
+ s->csw_in_packet = NULL;
p->status = status;
- s->packet = NULL;
usb_packet_complete(&s->dev, p);
}
@@ -204,8 +219,12 @@ static void usb_msd_fatal_error(MSDState *s)
{
trace_usb_msd_fatal_error();
- if (s->packet) {
- usb_msd_packet_complete(s, USB_RET_STALL);
+ if (s->data_packet) {
+ usb_msd_data_packet_complete(s, USB_RET_STALL);
+ }
+
+ if (s->csw_in_packet) {
+ usb_msd_csw_packet_complete(s, USB_RET_STALL);
}
/*
@@ -250,7 +269,7 @@ static void usb_msd_send_status(MSDState *s, USBPacket *p)
void usb_msd_transfer_data(SCSIRequest *req, uint32_t len)
{
MSDState *s = DO_UPCAST(MSDState, dev.qdev, req->bus->qbus.parent);
- USBPacket *p = s->packet;
+ USBPacket *p = s->data_packet;
if ((s->mode == USB_MSDM_DATAOUT) != (req->cmd.mode == SCSI_XFER_TO_DEV)) {
usb_msd_fatal_error(s);
@@ -261,10 +280,10 @@ void usb_msd_transfer_data(SCSIRequest *req, uint32_t len)
s->scsi_off = 0;
if (p) {
usb_msd_copy_data(s, p);
- p = s->packet;
+ p = s->data_packet;
if (p && p->actual_length == p->iov.size) {
/* USB_RET_SUCCESS status clears previous ASYNC status */
- usb_msd_packet_complete(s, USB_RET_SUCCESS);
+ usb_msd_data_packet_complete(s, USB_RET_SUCCESS);
}
}
}
@@ -272,7 +291,7 @@ void usb_msd_transfer_data(SCSIRequest *req, uint32_t len)
void usb_msd_command_complete(SCSIRequest *req, size_t resid)
{
MSDState *s = DO_UPCAST(MSDState, dev.qdev, req->bus->qbus.parent);
- USBPacket *p = s->packet;
+ USBPacket *p = s->data_packet;
trace_usb_msd_cmd_complete(req->status, req->tag);
@@ -281,35 +300,37 @@ void usb_msd_command_complete(SCSIRequest *req, size_t resid)
s->csw.residue = cpu_to_le32(s->data_len);
s->csw.status = req->status != 0;
- if (s->packet) {
- if (s->data_len == 0 && s->mode == USB_MSDM_DATAOUT) {
- /* A deferred packet with no write data remaining must be
- the status read packet. */
- usb_msd_send_status(s, p);
- s->mode = USB_MSDM_CBW;
- } else if (s->mode == USB_MSDM_CSW) {
- usb_msd_send_status(s, p);
- s->mode = USB_MSDM_CBW;
- } else {
- if (s->data_len) {
- int len = (p->iov.size - p->actual_length);
- usb_packet_skip(p, len);
- if (len > s->data_len) {
- len = s->data_len;
- }
- s->data_len -= len;
- }
- if (s->data_len == 0) {
- s->mode = USB_MSDM_CSW;
+ scsi_req_unref(req);
+ s->req = NULL;
+
+ if (p) {
+ g_assert(s->mode == USB_MSDM_DATAIN || s->mode == USB_MSDM_DATAOUT);
+ if (s->data_len) {
+ int len = (p->iov.size - p->actual_length);
+ usb_packet_skip(p, len);
+ if (len > s->data_len) {
+ len = s->data_len;
}
+ s->data_len -= len;
+ }
+ if (s->data_len == 0) {
+ s->mode = USB_MSDM_CSW;
}
/* USB_RET_SUCCESS status clears previous ASYNC status */
- usb_msd_packet_complete(s, USB_RET_SUCCESS);
+ usb_msd_data_packet_complete(s, USB_RET_SUCCESS);
} else if (s->data_len == 0) {
s->mode = USB_MSDM_CSW;
}
- scsi_req_unref(req);
- s->req = NULL;
+
+ if (s->mode == USB_MSDM_CSW) {
+ p = s->csw_in_packet;
+ if (p) {
+ usb_msd_send_status(s, p);
+ s->mode = USB_MSDM_CBW;
+ /* USB_RET_SUCCESS status clears previous ASYNC status */
+ usb_msd_csw_packet_complete(s, USB_RET_SUCCESS);
+ }
+ }
}
void usb_msd_request_cancelled(SCSIRequest *req)
@@ -339,8 +360,12 @@ void usb_msd_handle_reset(USBDevice *dev)
}
assert(s->req == NULL);
- if (s->packet) {
- usb_msd_packet_complete(s, USB_RET_STALL);
+ if (s->data_packet) {
+ usb_msd_data_packet_complete(s, USB_RET_STALL);
+ }
+
+ if (s->csw_in_packet) {
+ usb_msd_csw_packet_complete(s, USB_RET_STALL);
}
memset(&s->csw, 0, sizeof(s->csw));
@@ -395,11 +420,15 @@ static void usb_msd_cancel_io(USBDevice *dev, USBPacket *p)
{
MSDState *s = USB_STORAGE_DEV(dev);
- assert(s->packet == p);
- s->packet = NULL;
-
- if (s->req) {
- scsi_req_cancel(s->req);
+ if (p == s->data_packet) {
+ s->data_packet = NULL;
+ if (s->req) {
+ scsi_req_cancel(s->req);
+ }
+ } else if (p == s->csw_in_packet) {
+ s->csw_in_packet = NULL;
+ } else {
+ g_assert_not_reached();
}
}
@@ -500,7 +529,7 @@ static void usb_msd_handle_data_out(USBDevice *dev, USBPacket *p)
}
if (p->actual_length < p->iov.size) {
trace_usb_msd_packet_async();
- s->packet = p;
+ s->data_packet = p;
p->status = USB_RET_ASYNC;
}
break;
@@ -532,7 +561,7 @@ static void usb_msd_handle_data_in(USBDevice *dev, USBPacket *p)
/* Waiting for SCSI write to complete. */
trace_usb_msd_packet_async();
- s->packet = p;
+ s->csw_in_packet = p;
p->status = USB_RET_ASYNC;
break;
@@ -544,7 +573,7 @@ static void usb_msd_handle_data_in(USBDevice *dev, USBPacket *p)
if (s->req) {
/* still in flight */
trace_usb_msd_packet_async();
- s->packet = p;
+ s->csw_in_packet = p;
p->status = USB_RET_ASYNC;
} else {
usb_msd_send_status(s, p);
@@ -572,7 +601,7 @@ static void usb_msd_handle_data_in(USBDevice *dev, USBPacket *p)
}
if (p->actual_length < p->iov.size && s->mode == USB_MSDM_DATAIN) {
trace_usb_msd_packet_async();
- s->packet = p;
+ s->data_packet = p;
p->status = USB_RET_ASYNC;
}
break;
--
2.47.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v4 16/22] usb/msd: Split async packet tracking into data and csw
2025-05-02 3:30 ` [PATCH v4 16/22] usb/msd: Split async packet tracking into data and csw Nicholas Piggin
@ 2025-05-05 13:05 ` Kevin Wolf
2025-05-05 14:04 ` Kevin Wolf
0 siblings, 1 reply; 45+ messages in thread
From: Kevin Wolf @ 2025-05-05 13:05 UTC (permalink / raw)
To: Nicholas Piggin
Cc: Gerd Hoffmann, qemu-devel, Paolo Bonzini, Michael S. Tsirkin,
Marcel Apfelbaum, Fabiano Rosas, Laurent Vivier,
Phil Dennis-Jordan, Bernhard Beschow, Philippe Mathieu-Daudé
Am 02.05.2025 um 05:30 hat Nicholas Piggin geschrieben:
> The async packet handling logic has places that infer whether the
> async packet is data or CSW, based on context. This is not wrong,
> it just makes the logic easier to follow if they are categorised
> when they are accepted.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> include/hw/usb/msd.h | 5 +-
> hw/usb/dev-storage.c | 121 +++++++++++++++++++++++++++----------------
> 2 files changed, 79 insertions(+), 47 deletions(-)
>
> diff --git a/include/hw/usb/msd.h b/include/hw/usb/msd.h
> index f9fd862b529..a40d15f5def 100644
> --- a/include/hw/usb/msd.h
> +++ b/include/hw/usb/msd.h
> @@ -33,8 +33,11 @@ struct MSDState {
> struct usb_msd_csw csw;
> SCSIRequest *req;
> SCSIBus bus;
> +
> /* For async completion. */
> - USBPacket *packet;
> + USBPacket *data_packet;
> + USBPacket *csw_in_packet;
This makes the state more complex, because there is a rule here that
isn't explicit in the code: At most one of data_packet or csw_in_packet
can be set at the same time.
Both are quite similar, so most of the patch just duplicates things that
are currently done for s->packet.
Wouldn't it make more sense to have one USBPacket pointer, but some
state that explicitly tells us what we're waiting for, data or the
status? I was thinking of a new bool at first, but on second thoughts,
s->mode looks quite similar to what we need here.
What if we just introduce a new state in the s->mode state machine for
"CSW read in progress" as opposed to USB_MSDM_CSW meaning "expecting the
host to read the CSW next"? Then the cases for which you currently set
s->csw_in_packet would instead transition to this new state, and
usb_msd_command_complete() (which has the only real change in this
patch) could just directly rely on s->mode.
> @@ -395,11 +420,15 @@ static void usb_msd_cancel_io(USBDevice *dev, USBPacket *p)
> {
> MSDState *s = USB_STORAGE_DEV(dev);
>
> - assert(s->packet == p);
> - s->packet = NULL;
> -
> - if (s->req) {
> - scsi_req_cancel(s->req);
> + if (p == s->data_packet) {
> + s->data_packet = NULL;
> + if (s->req) {
> + scsi_req_cancel(s->req);
> + }
> + } else if (p == s->csw_in_packet) {
> + s->csw_in_packet = NULL;
> + } else {
> + g_assert_not_reached();
> }
> }
I think scsi_req_cancel() is required even in csw_in_packet case.
Whether someone already asked for the result doesn't change the state of
the in-flight SCSI request, so we shouldn't try to cancel it in one
case, but not in the other.
Kevin
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v4 16/22] usb/msd: Split async packet tracking into data and csw
2025-05-05 13:05 ` Kevin Wolf
@ 2025-05-05 14:04 ` Kevin Wolf
0 siblings, 0 replies; 45+ messages in thread
From: Kevin Wolf @ 2025-05-05 14:04 UTC (permalink / raw)
To: Nicholas Piggin
Cc: Gerd Hoffmann, qemu-devel, Paolo Bonzini, Michael S. Tsirkin,
Marcel Apfelbaum, Fabiano Rosas, Laurent Vivier,
Phil Dennis-Jordan, Bernhard Beschow, Philippe Mathieu-Daudé
Am 05.05.2025 um 15:05 hat Kevin Wolf geschrieben:
> Am 02.05.2025 um 05:30 hat Nicholas Piggin geschrieben:
> > The async packet handling logic has places that infer whether the
> > async packet is data or CSW, based on context. This is not wrong,
> > it just makes the logic easier to follow if they are categorised
> > when they are accepted.
> >
> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > ---
> > include/hw/usb/msd.h | 5 +-
> > hw/usb/dev-storage.c | 121 +++++++++++++++++++++++++++----------------
> > 2 files changed, 79 insertions(+), 47 deletions(-)
> >
> > diff --git a/include/hw/usb/msd.h b/include/hw/usb/msd.h
> > index f9fd862b529..a40d15f5def 100644
> > --- a/include/hw/usb/msd.h
> > +++ b/include/hw/usb/msd.h
> > @@ -33,8 +33,11 @@ struct MSDState {
> > struct usb_msd_csw csw;
> > SCSIRequest *req;
> > SCSIBus bus;
> > +
> > /* For async completion. */
> > - USBPacket *packet;
> > + USBPacket *data_packet;
> > + USBPacket *csw_in_packet;
>
> This makes the state more complex, because there is a rule here that
> isn't explicit in the code: At most one of data_packet or csw_in_packet
> can be set at the same time.
>
> Both are quite similar, so most of the patch just duplicates things that
> are currently done for s->packet.
>
> Wouldn't it make more sense to have one USBPacket pointer, but some
> state that explicitly tells us what we're waiting for, data or the
> status? I was thinking of a new bool at first, but on second thoughts,
> s->mode looks quite similar to what we need here.
>
> What if we just introduce a new state in the s->mode state machine for
> "CSW read in progress" as opposed to USB_MSDM_CSW meaning "expecting the
> host to read the CSW next"? Then the cases for which you currently set
> s->csw_in_packet would instead transition to this new state, and
> usb_msd_command_complete() (which has the only real change in this
> patch) could just directly rely on s->mode.
After looking at the rest of the series, this is probably not what we
want either.
I think a big part of the problem why this device is hard to understand
is that it's too much centered around what the host does rather than
what the core logic of the device is. Which is really simple:
while (!unrealizing) {
cbw = read_in_packet();
req = start_scsi_req();
ret = wait_for_scsi_req(req);
write_out_packet(status_to_csw(ret));
}
This flow (which is really Figure 1 in Chapter 5 in the spec) is nowhere
to be seen in the code. Can we change things to look more like this?
Could be almost literally this if we make it coroutine based, or just
the callback based equivalent of it.
And then when we receive a packet, we just queue it as the next in or
out packet, and leave it to the main state machine/coroutine to make use
of the packet or wait for one if it's missing.
Kevin
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v4 17/22] usb/msd: Add some additional assertions
2025-05-02 3:30 [PATCH v4 00/22] usb/xhci and usb/msd improvements and tests Nicholas Piggin
` (15 preceding siblings ...)
2025-05-02 3:30 ` [PATCH v4 16/22] usb/msd: Split async packet tracking into data and csw Nicholas Piggin
@ 2025-05-02 3:30 ` Nicholas Piggin
2025-05-02 3:30 ` [PATCH v4 18/22] usb/msd: Rename mode to cbw_state, and tweak names Nicholas Piggin
` (5 subsequent siblings)
22 siblings, 0 replies; 45+ messages in thread
From: Nicholas Piggin @ 2025-05-02 3:30 UTC (permalink / raw)
To: Gerd Hoffmann
Cc: Nicholas Piggin, qemu-devel, Kevin Wolf, Paolo Bonzini,
Michael S. Tsirkin, Marcel Apfelbaum, Fabiano Rosas,
Laurent Vivier, Phil Dennis-Jordan, Bernhard Beschow,
Philippe Mathieu-Daudé
Add more assertions to help verify internal logic.
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
hw/usb/dev-storage.c | 23 +++++++++++++++++++----
1 file changed, 19 insertions(+), 4 deletions(-)
diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
index 66fffda3713..81bfdf08992 100644
--- a/hw/usb/dev-storage.c
+++ b/hw/usb/dev-storage.c
@@ -271,13 +271,24 @@ void usb_msd_transfer_data(SCSIRequest *req, uint32_t len)
MSDState *s = DO_UPCAST(MSDState, dev.qdev, req->bus->qbus.parent);
USBPacket *p = s->data_packet;
- if ((s->mode == USB_MSDM_DATAOUT) != (req->cmd.mode == SCSI_XFER_TO_DEV)) {
- usb_msd_fatal_error(s);
- return;
+ if (s->mode == USB_MSDM_DATAIN) {
+ if (req->cmd.mode == SCSI_XFER_TO_DEV) {
+ usb_msd_fatal_error(s);
+ return;
+ }
+ } else if (s->mode == USB_MSDM_DATAOUT) {
+ if (req->cmd.mode != SCSI_XFER_TO_DEV) {
+ usb_msd_fatal_error(s);
+ return;
+ }
+ } else {
+ g_assert_not_reached();
}
+ assert(s->scsi_len == 0);
s->scsi_len = len;
s->scsi_off = 0;
+
if (p) {
usb_msd_copy_data(s, p);
p = s->data_packet;
@@ -295,6 +306,10 @@ void usb_msd_command_complete(SCSIRequest *req, size_t resid)
trace_usb_msd_cmd_complete(req->status, req->tag);
+ g_assert(s->req);
+ /* The CBW is what starts the SCSI request */
+ g_assert(s->mode != USB_MSDM_CBW);
+
s->csw.sig = cpu_to_le32(0x53425355);
s->csw.tag = cpu_to_le32(req->tag);
s->csw.residue = cpu_to_le32(s->data_len);
@@ -493,7 +508,7 @@ static void usb_msd_handle_data_out(USBDevice *dev, USBPacket *p)
trace_usb_msd_cmd_submit(cbw.lun, tag, cbw.flags,
cbw.cmd_len, s->data_len);
assert(le32_to_cpu(s->csw.residue) == 0);
- s->scsi_len = 0;
+ assert(s->scsi_len == 0);
s->req = scsi_req_new(scsi_dev, tag, cbw.lun,
cbw.cmd, cbw.cmd_len, NULL);
if (s->commandlog) {
--
2.47.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* [PATCH v4 18/22] usb/msd: Rename mode to cbw_state, and tweak names
2025-05-02 3:30 [PATCH v4 00/22] usb/xhci and usb/msd improvements and tests Nicholas Piggin
` (16 preceding siblings ...)
2025-05-02 3:30 ` [PATCH v4 17/22] usb/msd: Add some additional assertions Nicholas Piggin
@ 2025-05-02 3:30 ` Nicholas Piggin
2025-05-02 3:30 ` [PATCH v4 19/22] usb/msd: Add NODATA CBW state Nicholas Piggin
` (4 subsequent siblings)
22 siblings, 0 replies; 45+ messages in thread
From: Nicholas Piggin @ 2025-05-02 3:30 UTC (permalink / raw)
To: Gerd Hoffmann
Cc: Nicholas Piggin, qemu-devel, Kevin Wolf, Paolo Bonzini,
Michael S. Tsirkin, Marcel Apfelbaum, Fabiano Rosas,
Laurent Vivier, Phil Dennis-Jordan, Bernhard Beschow,
Philippe Mathieu-Daudé
This reflects a little better what it does, particularly with a
subsequent change to relax the order packets are seen in. This
field is not the general state of the MSD state machine, rather
it follows packets that are completed as part of a CBW command.
The difference is a bit subtle, so for a concrete example, the
next change will permit the host to send a CSW packet before it
sends the associated CBW packet. In that case the CSW packet
will be tracked and the MSD state machine will move, but this
mode / cbw_state field would remain unchanged (in the "expecting
CBW" state), until the CBW packet arrives.
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
include/hw/usb/msd.h | 14 ++++++-------
hw/usb/dev-storage.c | 50 +++++++++++++++++++++++---------------------
2 files changed, 33 insertions(+), 31 deletions(-)
diff --git a/include/hw/usb/msd.h b/include/hw/usb/msd.h
index a40d15f5def..af12a16c35f 100644
--- a/include/hw/usb/msd.h
+++ b/include/hw/usb/msd.h
@@ -10,12 +10,12 @@
#include "hw/usb.h"
#include "hw/scsi/scsi.h"
-enum USBMSDMode {
- USB_MSDM_CBW, /* Command Block. */
- USB_MSDM_DATAOUT, /* Transfer data to device. */
- USB_MSDM_DATAIN, /* Transfer data from device. */
- USB_MSDM_CSW /* Command Status. */
-};
+typedef enum USBMSDCBWState {
+ USB_MSD_CBW_NONE, /* Ready, waiting for CBW packet. */
+ USB_MSD_CBW_DATAOUT, /* Expecting DATA-OUT (to device) packet */
+ USB_MSD_CBW_DATAIN, /* Expecting DATA-IN (from device) packet */
+ USB_MSD_CBW_CSW /* No more data, expecting CSW packet. */
+} USBMSDCBWState;
struct QEMU_PACKED usb_msd_csw {
uint32_t sig;
@@ -26,7 +26,7 @@ struct QEMU_PACKED usb_msd_csw {
struct MSDState {
USBDevice dev;
- enum USBMSDMode mode;
+ USBMSDCBWState cbw_state;
uint32_t scsi_off;
uint32_t scsi_len;
uint32_t data_len;
diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
index 81bfdf08992..5b773a22e60 100644
--- a/hw/usb/dev-storage.c
+++ b/hw/usb/dev-storage.c
@@ -271,12 +271,12 @@ void usb_msd_transfer_data(SCSIRequest *req, uint32_t len)
MSDState *s = DO_UPCAST(MSDState, dev.qdev, req->bus->qbus.parent);
USBPacket *p = s->data_packet;
- if (s->mode == USB_MSDM_DATAIN) {
+ if (s->cbw_state == USB_MSD_CBW_DATAIN) {
if (req->cmd.mode == SCSI_XFER_TO_DEV) {
usb_msd_fatal_error(s);
return;
}
- } else if (s->mode == USB_MSDM_DATAOUT) {
+ } else if (s->cbw_state == USB_MSD_CBW_DATAOUT) {
if (req->cmd.mode != SCSI_XFER_TO_DEV) {
usb_msd_fatal_error(s);
return;
@@ -308,7 +308,7 @@ void usb_msd_command_complete(SCSIRequest *req, size_t resid)
g_assert(s->req);
/* The CBW is what starts the SCSI request */
- g_assert(s->mode != USB_MSDM_CBW);
+ g_assert(s->cbw_state != USB_MSD_CBW_NONE);
s->csw.sig = cpu_to_le32(0x53425355);
s->csw.tag = cpu_to_le32(req->tag);
@@ -319,7 +319,8 @@ void usb_msd_command_complete(SCSIRequest *req, size_t resid)
s->req = NULL;
if (p) {
- g_assert(s->mode == USB_MSDM_DATAIN || s->mode == USB_MSDM_DATAOUT);
+ g_assert(s->cbw_state == USB_MSD_CBW_DATAIN ||
+ s->cbw_state == USB_MSD_CBW_DATAOUT);
if (s->data_len) {
int len = (p->iov.size - p->actual_length);
usb_packet_skip(p, len);
@@ -329,19 +330,19 @@ void usb_msd_command_complete(SCSIRequest *req, size_t resid)
s->data_len -= len;
}
if (s->data_len == 0) {
- s->mode = USB_MSDM_CSW;
+ s->cbw_state = USB_MSD_CBW_CSW;
}
/* USB_RET_SUCCESS status clears previous ASYNC status */
usb_msd_data_packet_complete(s, USB_RET_SUCCESS);
} else if (s->data_len == 0) {
- s->mode = USB_MSDM_CSW;
+ s->cbw_state = USB_MSD_CBW_CSW;
}
- if (s->mode == USB_MSDM_CSW) {
+ if (s->cbw_state == USB_MSD_CBW_CSW) {
p = s->csw_in_packet;
if (p) {
usb_msd_send_status(s, p);
- s->mode = USB_MSDM_CBW;
+ s->cbw_state = USB_MSD_CBW_NONE;
/* USB_RET_SUCCESS status clears previous ASYNC status */
usb_msd_csw_packet_complete(s, USB_RET_SUCCESS);
}
@@ -384,7 +385,7 @@ void usb_msd_handle_reset(USBDevice *dev)
}
memset(&s->csw, 0, sizeof(s->csw));
- s->mode = USB_MSDM_CBW;
+ s->cbw_state = USB_MSD_CBW_NONE;
s->needs_reset = false;
}
@@ -485,8 +486,8 @@ static void usb_msd_handle_data_out(USBDevice *dev, USBPacket *p)
SCSIDevice *scsi_dev;
int len;
- switch (s->mode) {
- case USB_MSDM_CBW:
+ switch (s->cbw_state) {
+ case USB_MSD_CBW_NONE:
if (!try_get_valid_cbw(p, &cbw)) {
goto fail;
}
@@ -499,11 +500,11 @@ static void usb_msd_handle_data_out(USBDevice *dev, USBPacket *p)
tag = le32_to_cpu(cbw.tag);
s->data_len = le32_to_cpu(cbw.data_len);
if (s->data_len == 0) {
- s->mode = USB_MSDM_CSW;
+ s->cbw_state = USB_MSD_CBW_CSW;
} else if (cbw.flags & 0x80) {
- s->mode = USB_MSDM_DATAIN;
+ s->cbw_state = USB_MSD_CBW_DATAIN;
} else {
- s->mode = USB_MSDM_DATAOUT;
+ s->cbw_state = USB_MSD_CBW_DATAOUT;
}
trace_usb_msd_cmd_submit(cbw.lun, tag, cbw.flags,
cbw.cmd_len, s->data_len);
@@ -520,7 +521,7 @@ static void usb_msd_handle_data_out(USBDevice *dev, USBPacket *p)
}
break;
- case USB_MSDM_DATAOUT:
+ case USB_MSD_CBW_DATAOUT:
trace_usb_msd_data_out(p->iov.size, s->data_len);
if (p->iov.size > s->data_len) {
goto fail;
@@ -538,7 +539,7 @@ static void usb_msd_handle_data_out(USBDevice *dev, USBPacket *p)
}
s->data_len -= len;
if (s->data_len == 0) {
- s->mode = USB_MSDM_CSW;
+ s->cbw_state = USB_MSD_CBW_CSW;
}
}
}
@@ -563,8 +564,8 @@ static void usb_msd_handle_data_in(USBDevice *dev, USBPacket *p)
MSDState *s = (MSDState *)dev;
int len;
- switch (s->mode) {
- case USB_MSDM_DATAOUT:
+ switch (s->cbw_state) {
+ case USB_MSD_CBW_DATAOUT:
if (!check_valid_csw(p)) {
goto fail;
}
@@ -580,7 +581,7 @@ static void usb_msd_handle_data_in(USBDevice *dev, USBPacket *p)
p->status = USB_RET_ASYNC;
break;
- case USB_MSDM_CSW:
+ case USB_MSD_CBW_CSW:
if (!check_valid_csw(p)) {
goto fail;
}
@@ -592,11 +593,11 @@ static void usb_msd_handle_data_in(USBDevice *dev, USBPacket *p)
p->status = USB_RET_ASYNC;
} else {
usb_msd_send_status(s, p);
- s->mode = USB_MSDM_CBW;
+ s->cbw_state = USB_MSD_CBW_NONE;
}
break;
- case USB_MSDM_DATAIN:
+ case USB_MSD_CBW_DATAIN:
trace_usb_msd_data_in(p->iov.size, s->data_len, s->scsi_len);
if (s->scsi_len) {
usb_msd_copy_data(s, p);
@@ -610,11 +611,12 @@ static void usb_msd_handle_data_in(USBDevice *dev, USBPacket *p)
}
s->data_len -= len;
if (s->data_len == 0) {
- s->mode = USB_MSDM_CSW;
+ s->cbw_state = USB_MSD_CBW_CSW;
}
}
}
- if (p->actual_length < p->iov.size && s->mode == USB_MSDM_DATAIN) {
+ if (p->actual_length < p->iov.size &&
+ s->cbw_state == USB_MSD_CBW_DATAIN) {
trace_usb_msd_packet_async();
s->data_packet = p;
p->status = USB_RET_ASYNC;
@@ -679,7 +681,7 @@ static const VMStateDescription vmstate_usb_msd = {
.minimum_version_id = 1,
.fields = (const VMStateField[]) {
VMSTATE_USB_DEVICE(dev, MSDState),
- VMSTATE_UINT32(mode, MSDState),
+ VMSTATE_UINT32(cbw_state, MSDState),
VMSTATE_UINT32(scsi_len, MSDState),
VMSTATE_UINT32(scsi_off, MSDState),
VMSTATE_UINT32(data_len, MSDState),
--
2.47.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* [PATCH v4 19/22] usb/msd: Add NODATA CBW state
2025-05-02 3:30 [PATCH v4 00/22] usb/xhci and usb/msd improvements and tests Nicholas Piggin
` (17 preceding siblings ...)
2025-05-02 3:30 ` [PATCH v4 18/22] usb/msd: Rename mode to cbw_state, and tweak names Nicholas Piggin
@ 2025-05-02 3:30 ` Nicholas Piggin
2025-05-02 3:30 ` [PATCH v4 20/22] usb/msd: Permit a DATA-IN or CSW packet before CBW packet Nicholas Piggin
` (3 subsequent siblings)
22 siblings, 0 replies; 45+ messages in thread
From: Nicholas Piggin @ 2025-05-02 3:30 UTC (permalink / raw)
To: Gerd Hoffmann
Cc: Nicholas Piggin, qemu-devel, Kevin Wolf, Paolo Bonzini,
Michael S. Tsirkin, Marcel Apfelbaum, Fabiano Rosas,
Laurent Vivier, Phil Dennis-Jordan, Bernhard Beschow,
Philippe Mathieu-Daudé
This is not really required for the state machine but it improves
the symmetry of zero-data packets with data packets, and helps with
assertions and reasoning about traces.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
include/hw/usb/msd.h | 1 +
hw/usb/dev-storage.c | 10 +++++++---
2 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/include/hw/usb/msd.h b/include/hw/usb/msd.h
index af12a16c35f..6d741e44160 100644
--- a/include/hw/usb/msd.h
+++ b/include/hw/usb/msd.h
@@ -14,6 +14,7 @@ typedef enum USBMSDCBWState {
USB_MSD_CBW_NONE, /* Ready, waiting for CBW packet. */
USB_MSD_CBW_DATAOUT, /* Expecting DATA-OUT (to device) packet */
USB_MSD_CBW_DATAIN, /* Expecting DATA-IN (from device) packet */
+ USB_MSD_CBW_NODATA, /* No data, CSW but also a SCSI completion */
USB_MSD_CBW_CSW /* No more data, expecting CSW packet. */
} USBMSDCBWState;
diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
index 5b773a22e60..a2544d2659f 100644
--- a/hw/usb/dev-storage.c
+++ b/hw/usb/dev-storage.c
@@ -318,9 +318,12 @@ void usb_msd_command_complete(SCSIRequest *req, size_t resid)
scsi_req_unref(req);
s->req = NULL;
+ g_assert(s->cbw_state == USB_MSD_CBW_DATAIN ||
+ s->cbw_state == USB_MSD_CBW_DATAOUT ||
+ s->cbw_state == USB_MSD_CBW_NODATA);
+
if (p) {
- g_assert(s->cbw_state == USB_MSD_CBW_DATAIN ||
- s->cbw_state == USB_MSD_CBW_DATAOUT);
+ g_assert(s->cbw_state != USB_MSD_CBW_NODATA);
if (s->data_len) {
int len = (p->iov.size - p->actual_length);
usb_packet_skip(p, len);
@@ -500,7 +503,7 @@ static void usb_msd_handle_data_out(USBDevice *dev, USBPacket *p)
tag = le32_to_cpu(cbw.tag);
s->data_len = le32_to_cpu(cbw.data_len);
if (s->data_len == 0) {
- s->cbw_state = USB_MSD_CBW_CSW;
+ s->cbw_state = USB_MSD_CBW_NODATA;
} else if (cbw.flags & 0x80) {
s->cbw_state = USB_MSD_CBW_DATAIN;
} else {
@@ -565,6 +568,7 @@ static void usb_msd_handle_data_in(USBDevice *dev, USBPacket *p)
int len;
switch (s->cbw_state) {
+ case USB_MSD_CBW_NODATA:
case USB_MSD_CBW_DATAOUT:
if (!check_valid_csw(p)) {
goto fail;
--
2.47.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* [PATCH v4 20/22] usb/msd: Permit a DATA-IN or CSW packet before CBW packet
2025-05-02 3:30 [PATCH v4 00/22] usb/xhci and usb/msd improvements and tests Nicholas Piggin
` (18 preceding siblings ...)
2025-05-02 3:30 ` [PATCH v4 19/22] usb/msd: Add NODATA CBW state Nicholas Piggin
@ 2025-05-02 3:30 ` Nicholas Piggin
2025-05-02 3:30 ` [PATCH v4 21/22] tests/qtest/xhci: Test USB Mass Storage relaxed CSW order Nicholas Piggin
` (2 subsequent siblings)
22 siblings, 0 replies; 45+ messages in thread
From: Nicholas Piggin @ 2025-05-02 3:30 UTC (permalink / raw)
To: Gerd Hoffmann
Cc: Nicholas Piggin, qemu-devel, Kevin Wolf, Paolo Bonzini,
Michael S. Tsirkin, Marcel Apfelbaum, Fabiano Rosas,
Laurent Vivier, Phil Dennis-Jordan, Bernhard Beschow,
Philippe Mathieu-Daudé
The USB MSD protocol has 3 packets that make up a command, and only one
command may be active at any time.
- CBW to start a command (that contains a SCSI request).
- DATA (IN or OUT) to request data transfer between host and SCSI layer.
- CSW to return status and complete the command.
DATA is omitted if the request has no data.
The QEMU MSD model requires these packets to arrive in this order, CBW,
DATA, CSW. This is the way the state machine is generally described in
the MSD spec, and this must be how most USB stacks operate. Except AIX.
Universal Serial Bus Mass Storage Class Bulk-Only Transport 1.0 contains
one word in one sentence that permits the relaxed ordering:
3.3 Host/Device Packet Transfer Order
The host shall send the CBW before the associated Data-Out, and the
device shall send Data-In after the associated CBW and before the
associated CSW. The host may request Data-In or CSW before sending the
associated CBW.
Complicating matters, DATA-IN and CSW are both input packets that arrive
in the same manner, so before a CBW it is impossible to determine if an
IN packet is for data or CSW.
So permit "unusually-ordered" packets by tracking them as an "unknown"
packet until the CBW arrives, then they are categorized into a DATA or
CSW packet.
It is not clear whether the spec permits multiple such packets before
the CBW. This implementation permits only one, which seems to be enough
for AIX.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
include/hw/usb/msd.h | 1 +
hw/usb/dev-storage.c | 43 +++++++++++++++++++++++++++++++++++++++++++
2 files changed, 44 insertions(+)
diff --git a/include/hw/usb/msd.h b/include/hw/usb/msd.h
index 6d741e44160..fb430f87afc 100644
--- a/include/hw/usb/msd.h
+++ b/include/hw/usb/msd.h
@@ -38,6 +38,7 @@ struct MSDState {
/* For async completion. */
USBPacket *data_packet;
USBPacket *csw_in_packet;
+ USBPacket *unknown_in_packet;
/* usb-storage only */
BlockConf conf;
diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
index a2544d2659f..f421bd1f8dd 100644
--- a/hw/usb/dev-storage.c
+++ b/hw/usb/dev-storage.c
@@ -446,6 +446,8 @@ static void usb_msd_cancel_io(USBDevice *dev, USBPacket *p)
}
} else if (p == s->csw_in_packet) {
s->csw_in_packet = NULL;
+ } else if (p == s->unknown_in_packet) {
+ s->unknown_in_packet = NULL;
} else {
g_assert_not_reached();
}
@@ -509,6 +511,19 @@ static void usb_msd_handle_data_out(USBDevice *dev, USBPacket *p)
} else {
s->cbw_state = USB_MSD_CBW_DATAOUT;
}
+ if (s->unknown_in_packet) {
+ if (s->cbw_state == USB_MSD_CBW_DATAIN) {
+ /* Must be a DATAIN packet */
+ s->data_packet = s->unknown_in_packet;
+ } else {
+ /* Must be the CSW packet */
+ if (!check_valid_csw(s->unknown_in_packet)) {
+ goto fail;
+ }
+ s->csw_in_packet = s->unknown_in_packet;
+ }
+ s->unknown_in_packet = NULL;
+ }
trace_usb_msd_cmd_submit(cbw.lun, tag, cbw.flags,
cbw.cmd_len, s->data_len);
assert(le32_to_cpu(s->csw.residue) == 0);
@@ -526,6 +541,11 @@ static void usb_msd_handle_data_out(USBDevice *dev, USBPacket *p)
case USB_MSD_CBW_DATAOUT:
trace_usb_msd_data_out(p->iov.size, s->data_len);
+ if (s->unknown_in_packet) {
+ error_report("usb-msd: unknown_in_packet in DATAOUT state");
+ goto fail;
+ }
+
if (p->iov.size > s->data_len) {
goto fail;
}
@@ -568,8 +588,23 @@ static void usb_msd_handle_data_in(USBDevice *dev, USBPacket *p)
int len;
switch (s->cbw_state) {
+ case USB_MSD_CBW_NONE:
+ if (s->unknown_in_packet) {
+ qemu_log_mask(LOG_GUEST_ERROR, "usb-msd: second IN packet was"
+ "received before CBW\n");
+ goto fail;
+ }
+ trace_usb_msd_packet_async();
+ s->unknown_in_packet = p;
+ p->status = USB_RET_ASYNC;
+ break;
+
case USB_MSD_CBW_NODATA:
case USB_MSD_CBW_DATAOUT:
+ if (s->unknown_in_packet) {
+ error_report("usb-msd: unknown_in_packet in DATAOUT state");
+ goto fail;
+ }
if (!check_valid_csw(p)) {
goto fail;
}
@@ -586,6 +621,10 @@ static void usb_msd_handle_data_in(USBDevice *dev, USBPacket *p)
break;
case USB_MSD_CBW_CSW:
+ if (s->unknown_in_packet) {
+ error_report("usb-msd: unknown_in_packet in CSW state");
+ goto fail;
+ }
if (!check_valid_csw(p)) {
goto fail;
}
@@ -603,6 +642,10 @@ static void usb_msd_handle_data_in(USBDevice *dev, USBPacket *p)
case USB_MSD_CBW_DATAIN:
trace_usb_msd_data_in(p->iov.size, s->data_len, s->scsi_len);
+ if (s->unknown_in_packet) {
+ error_report("usb-msd: unknown_in_packet in DATAIN state");
+ goto fail;
+ }
if (s->scsi_len) {
usb_msd_copy_data(s, p);
}
--
2.47.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* [PATCH v4 21/22] tests/qtest/xhci: Test USB Mass Storage relaxed CSW order
2025-05-02 3:30 [PATCH v4 00/22] usb/xhci and usb/msd improvements and tests Nicholas Piggin
` (19 preceding siblings ...)
2025-05-02 3:30 ` [PATCH v4 20/22] usb/msd: Permit a DATA-IN or CSW packet before CBW packet Nicholas Piggin
@ 2025-05-02 3:30 ` Nicholas Piggin
2025-05-19 15:03 ` Fabiano Rosas
2025-05-02 3:30 ` [PATCH v4 22/22] usb/msd: Add more tracing Nicholas Piggin
2025-05-05 2:03 ` [PATCH v4 00/22] usb/xhci and usb/msd improvements and tests Nicholas Piggin
22 siblings, 1 reply; 45+ messages in thread
From: Nicholas Piggin @ 2025-05-02 3:30 UTC (permalink / raw)
To: Gerd Hoffmann
Cc: Nicholas Piggin, qemu-devel, Kevin Wolf, Paolo Bonzini,
Michael S. Tsirkin, Marcel Apfelbaum, Fabiano Rosas,
Laurent Vivier, Phil Dennis-Jordan, Bernhard Beschow,
Philippe Mathieu-Daudé
This adds a qtest for the improvement to the MSD protocol that
allows an IN packet before the CBW packet. Send a CSW packet
before a zero-length CBW command packet is sent. This test would
fail with the MSD change reverted.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
tests/qtest/usb-hcd-xhci-test.c | 180 ++++++++++++++++++++++++++++----
1 file changed, 158 insertions(+), 22 deletions(-)
diff --git a/tests/qtest/usb-hcd-xhci-test.c b/tests/qtest/usb-hcd-xhci-test.c
index 428200d9e41..740e9cd3815 100644
--- a/tests/qtest/usb-hcd-xhci-test.c
+++ b/tests/qtest/usb-hcd-xhci-test.c
@@ -287,11 +287,48 @@ static bool xhci_test_isr(XHCIQState *s)
s->guest_msix_addr, s->msix_data);
}
-static void wait_event_trb(XHCIQState *s, XHCITRB *trb)
+static bool check_event_trb(XHCIQState *s, XHCITRB *trb)
{
+ XHCIQTRState *tr = &s->event_ring;
+ uint64_t er_addr = tr->addr + tr->trb_idx * TRB_SIZE;
XHCITRB t;
+
+ qtest_memread(s->parent->qts, er_addr, &t, TRB_SIZE);
+ trb->parameter = le64_to_cpu(t.parameter);
+ trb->status = le32_to_cpu(t.status);
+ trb->control = le32_to_cpu(t.control);
+
+ return ((trb->control & TRB_C) == tr->trb_c);
+}
+
+static void consume_event(XHCIQState *s)
+{
XHCIQTRState *tr = &s->event_ring;
uint64_t er_addr = tr->addr + tr->trb_idx * TRB_SIZE;
+
+ tr->trb_idx++;
+ if (tr->trb_idx == tr->trb_entries) {
+ tr->trb_idx = 0;
+ tr->trb_c ^= 1;
+ }
+ /* Update ERDP to processed TRB addr and EHB bit, which clears EHB */
+ er_addr = tr->addr + tr->trb_idx * TRB_SIZE;
+ xhci_intr_writel(s, 0, XHCI_INTR_REG_ERDP_LO,
+ (er_addr & 0xffffffff) | XHCI_ERDP_EHB);
+}
+
+static bool try_get_event_trb(XHCIQState *s, XHCITRB *trb)
+{
+ if (check_event_trb(s, trb)) {
+ consume_event(s);
+ return true;
+ }
+ return false;
+}
+
+static void wait_event_trb(XHCIQState *s, XHCITRB *trb)
+{
+ XHCIQTRState *tr = &s->event_ring;
uint32_t value;
guint64 end_time = g_get_monotonic_time() + 5 * G_TIME_SPAN_SECOND;
@@ -306,30 +343,24 @@ static void wait_event_trb(XHCIQState *s, XHCITRB *trb)
value = xhci_op_readl(s, XHCI_OPER_REG_USBSTS);
g_assert(value & XHCI_USBSTS_EINT);
- /* With MSI-X enabled, IMAN IP is cleared after raising the interrupt */
- value = xhci_intr_readl(s, 0, XHCI_INTR_REG_IMAN);
- g_assert(!(value & XHCI_IMAN_IP));
-
- xhci_op_writel(s, XHCI_OPER_REG_USBSTS, XHCI_USBSTS_EINT); /* clear EINT */
-
- qtest_memread(s->parent->qts, er_addr, &t, TRB_SIZE);
-
- trb->parameter = le64_to_cpu(t.parameter);
- trb->status = le32_to_cpu(t.status);
- trb->control = le32_to_cpu(t.control);
+ if (0) {
+ /*
+ * With MSI-X enabled, IMAN IP is cleared after raising the interrupt,
+ * but if concurrent events may be occurring, it could be set again.
+ */
+ value = xhci_intr_readl(s, 0, XHCI_INTR_REG_IMAN);
+ g_assert(!(value & XHCI_IMAN_IP));
+ }
- g_assert((trb->status >> 24) == CC_SUCCESS);
+ if (!check_event_trb(s, trb)) {
+ g_assert_not_reached();
+ }
+ g_assert_cmpint((trb->status >> 24), ==, CC_SUCCESS);
g_assert((trb->control & TRB_C) == tr->trb_c); /* C bit has been set */
- tr->trb_idx++;
- if (tr->trb_idx == tr->trb_entries) {
- tr->trb_idx = 0;
- tr->trb_c ^= 1;
- }
- /* Update ERDP to processed TRB addr and EHB bit, which clears EHB */
- er_addr = tr->addr + tr->trb_idx * TRB_SIZE;
- xhci_intr_writel(s, 0, XHCI_INTR_REG_ERDP_LO,
- (er_addr & 0xffffffff) | XHCI_ERDP_EHB);
+ xhci_op_writel(s, XHCI_OPER_REG_USBSTS, XHCI_USBSTS_EINT); /* clear EINT */
+
+ consume_event(s);
}
static void set_link_trb(XHCIQState *s, uint64_t ring, uint32_t c,
@@ -763,6 +794,106 @@ static ssize_t xhci_submit_scsi_cmd(XHCIQState *s,
return data_len - le32_to_cpu(csw.residue); /* bytes copied */
}
+/*
+ * Submit command with CSW sent ahead of CBW.
+ * Can only be no-data or data-out commands (because a data-in command
+ * would interpret the CSW as a data-in).
+ */
+static ssize_t xhci_submit_out_of_order_scsi_cmd(XHCIQState *s,
+ const uint8_t *cmd, uint8_t cmd_len,
+ void *data, uint32_t data_len)
+{
+ struct usb_msd_cbw cbw;
+ struct usb_msd_csw csw;
+ uint64_t trb_data, csw_data;
+ XHCITRB trb, csw_trb;
+ uint64_t tag, csw_tag;
+ bool got_csw = false;
+
+ /* TRB data payload */
+ trb_data = xhci_guest_zalloc(s, data_len > sizeof(cbw) ? data_len : sizeof(cbw));
+ csw_data = xhci_guest_zalloc(s, sizeof(csw));
+
+ /* Issue a transfer ring ep 2 data (in) */
+ memset(&csw_trb, 0, TRB_SIZE);
+ csw_trb.parameter = csw_data;
+ csw_trb.status = sizeof(csw);
+ csw_trb.control |= TR_NORMAL << TRB_TYPE_SHIFT;
+ csw_trb.control |= TRB_TR_IOC;
+ csw_tag = submit_tr_trb(s, s->slotid, 2, &csw_trb);
+
+ memset(&cbw, 0, sizeof(cbw));
+ cbw.sig = cpu_to_le32(0x43425355);
+ cbw.tag = cpu_to_le32(0);
+ cbw.data_len = cpu_to_le32(data_len);
+ cbw.flags = 0x00;
+ cbw.lun = 0;
+ cbw.cmd_len = cmd_len; /* cmd len */
+ memcpy(cbw.cmd, cmd, cmd_len);
+ qtest_memwrite(s->parent->qts, trb_data, &cbw, sizeof(cbw));
+
+ /* Issue a transfer ring ep 3 data (out) */
+ memset(&trb, 0, TRB_SIZE);
+ trb.parameter = trb_data;
+ trb.status = sizeof(cbw);
+ trb.control |= TR_NORMAL << TRB_TYPE_SHIFT;
+ trb.control |= TRB_TR_IOC;
+ tag = submit_tr_trb(s, s->slotid, 3, &trb);
+
+ wait_event_trb(s, &trb);
+ if (trb.parameter == csw_tag) {
+ g_assert_cmpint(TRB_TYPE(trb), ==, ER_TRANSFER);
+ got_csw = true;
+ if (!try_get_event_trb(s, &trb)) {
+ wait_event_trb(s, &trb);
+ }
+ }
+ g_assert_cmphex(trb.parameter, ==, tag);
+ g_assert_cmpint(TRB_TYPE(trb), ==, ER_TRANSFER);
+
+ if (data_len) {
+ qtest_memwrite(s->parent->qts, trb_data, data, data_len);
+
+ /* Issue a transfer ring ep 3 data (out) */
+ memset(&trb, 0, TRB_SIZE);
+ trb.parameter = trb_data;
+ trb.status = data_len; /* data_len bytes, no more packets */
+ trb.control |= TR_NORMAL << TRB_TYPE_SHIFT;
+ trb.control |= TRB_TR_IOC;
+ tag = submit_tr_trb(s, s->slotid, 3, &trb);
+ wait_event_trb(s, &trb);
+ if (trb.parameter == csw_tag) {
+ g_assert_cmpint(TRB_TYPE(trb), ==, ER_TRANSFER);
+ got_csw = true;
+ if (!try_get_event_trb(s, &trb)) {
+ wait_event_trb(s, &trb);
+ }
+ }
+ g_assert_cmphex(trb.parameter, ==, tag);
+ g_assert_cmpint(TRB_TYPE(trb), ==, ER_TRANSFER);
+ } else {
+ /* No data */
+ }
+
+ if (!got_csw) {
+ wait_event_trb(s, &csw_trb);
+ g_assert_cmphex(csw_trb.parameter, ==, csw_tag);
+ g_assert_cmpint(TRB_TYPE(csw_trb), ==, ER_TRANSFER);
+ }
+
+ qtest_memread(s->parent->qts, csw_data, &csw, sizeof(csw));
+
+ guest_free(&s->parent->alloc, trb_data);
+ guest_free(&s->parent->alloc, csw_data);
+
+ g_assert(csw.sig == cpu_to_le32(0x53425355));
+ g_assert(csw.tag == cpu_to_le32(0));
+ if (csw.status) {
+ return -1;
+ }
+ return data_len - le32_to_cpu(csw.residue); /* bytes copied */
+}
+
#include "scsi/constants.h"
static void xhci_test_msd(XHCIQState *s)
@@ -797,6 +928,11 @@ static void xhci_test_msd(XHCIQState *s)
g_assert_not_reached();
}
+ /* Try an "out of order" command */
+ if (xhci_submit_out_of_order_scsi_cmd(s, scsi_cmd, 6, mem, 0) < 0) {
+ g_assert_not_reached();
+ }
+
/* Report LUNs */
memset(scsi_cmd, 0, sizeof(scsi_cmd));
scsi_cmd[0] = REPORT_LUNS;
--
2.47.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v4 21/22] tests/qtest/xhci: Test USB Mass Storage relaxed CSW order
2025-05-02 3:30 ` [PATCH v4 21/22] tests/qtest/xhci: Test USB Mass Storage relaxed CSW order Nicholas Piggin
@ 2025-05-19 15:03 ` Fabiano Rosas
0 siblings, 0 replies; 45+ messages in thread
From: Fabiano Rosas @ 2025-05-19 15:03 UTC (permalink / raw)
To: Nicholas Piggin, Gerd Hoffmann
Cc: Nicholas Piggin, qemu-devel, Kevin Wolf, Paolo Bonzini,
Michael S. Tsirkin, Marcel Apfelbaum, Laurent Vivier,
Phil Dennis-Jordan, Bernhard Beschow, Philippe Mathieu-Daudé
Nicholas Piggin <npiggin@gmail.com> writes:
> This adds a qtest for the improvement to the MSD protocol that
> allows an IN packet before the CBW packet. Send a CSW packet
> before a zero-length CBW command packet is sent. This test would
> fail with the MSD change reverted.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
#define CBW_SIGNATURE 0x43425355
#define CSW_SIGNATURE 0x53425355
would be nice
Reviewed-by: Fabiano Rosas <farosas@suse.de>
#define CBW_SIGNATURE 0x43425355
#define CSW_SIGNATURE 0x53425355
would be nice
Reviewed-by: Fabiano Rosas <farosas@suse.de>
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v4 22/22] usb/msd: Add more tracing
2025-05-02 3:30 [PATCH v4 00/22] usb/xhci and usb/msd improvements and tests Nicholas Piggin
` (20 preceding siblings ...)
2025-05-02 3:30 ` [PATCH v4 21/22] tests/qtest/xhci: Test USB Mass Storage relaxed CSW order Nicholas Piggin
@ 2025-05-02 3:30 ` Nicholas Piggin
2025-05-05 2:03 ` [PATCH v4 00/22] usb/xhci and usb/msd improvements and tests Nicholas Piggin
22 siblings, 0 replies; 45+ messages in thread
From: Nicholas Piggin @ 2025-05-02 3:30 UTC (permalink / raw)
To: Gerd Hoffmann
Cc: Nicholas Piggin, qemu-devel, Kevin Wolf, Paolo Bonzini,
Michael S. Tsirkin, Marcel Apfelbaum, Fabiano Rosas,
Laurent Vivier, Phil Dennis-Jordan, Bernhard Beschow,
Philippe Mathieu-Daudé
Add tracing for more received packet types, cbw_state changes, and
some more SCSI callbacks. These were useful in debugging relaxed
packet ordering support.
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
hw/usb/dev-storage.c | 61 +++++++++++++++++++++++++++++++++-----------
hw/usb/trace-events | 11 +++++++-
2 files changed, 56 insertions(+), 16 deletions(-)
diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
index f421bd1f8dd..79f857de599 100644
--- a/hw/usb/dev-storage.c
+++ b/hw/usb/dev-storage.c
@@ -185,6 +185,21 @@ static const USBDesc desc = {
.str = desc_strings,
};
+
+static void usb_msd_change_cbw_state(MSDState *s, USBMSDCBWState cbw_state)
+{
+ g_assert(s->cbw_state != cbw_state);
+ s->cbw_state = cbw_state;
+ trace_usb_msd_cbw_state(s->cbw_state);
+}
+
+static void usb_msd_set_cbw_state(MSDState *s, USBMSDCBWState cbw_state)
+{
+ if (s->cbw_state != cbw_state) {
+ usb_msd_change_cbw_state(s, cbw_state);
+ }
+}
+
static void usb_msd_data_packet_complete(MSDState *s, int status)
{
USBPacket *p = s->data_packet;
@@ -194,7 +209,7 @@ static void usb_msd_data_packet_complete(MSDState *s, int status)
* because another request may be issued before usb_packet_complete
* returns.
*/
- trace_usb_msd_packet_complete();
+ trace_usb_msd_data_packet_complete();
s->data_packet = NULL;
p->status = status;
usb_packet_complete(&s->dev, p);
@@ -209,7 +224,7 @@ static void usb_msd_csw_packet_complete(MSDState *s, int status)
* because another request may be issued before usb_packet_complete
* returns.
*/
- trace_usb_msd_packet_complete();
+ trace_usb_msd_csw_packet_complete();
s->csw_in_packet = NULL;
p->status = status;
usb_packet_complete(&s->dev, p);
@@ -238,7 +253,11 @@ static void usb_msd_fatal_error(MSDState *s)
static void usb_msd_copy_data(MSDState *s, USBPacket *p)
{
uint32_t len;
+
len = p->iov.size - p->actual_length;
+
+ trace_usb_msd_copy_data(s->req->tag, len);
+
if (len > s->scsi_len)
len = s->scsi_len;
usb_packet_copy(p, scsi_req_get_buf(s->req) + s->scsi_off, len);
@@ -271,6 +290,8 @@ void usb_msd_transfer_data(SCSIRequest *req, uint32_t len)
MSDState *s = DO_UPCAST(MSDState, dev.qdev, req->bus->qbus.parent);
USBPacket *p = s->data_packet;
+ trace_usb_msd_transfer_data(req->tag, len);
+
if (s->cbw_state == USB_MSD_CBW_DATAIN) {
if (req->cmd.mode == SCSI_XFER_TO_DEV) {
usb_msd_fatal_error(s);
@@ -333,19 +354,19 @@ void usb_msd_command_complete(SCSIRequest *req, size_t resid)
s->data_len -= len;
}
if (s->data_len == 0) {
- s->cbw_state = USB_MSD_CBW_CSW;
+ usb_msd_change_cbw_state(s, USB_MSD_CBW_CSW);
}
/* USB_RET_SUCCESS status clears previous ASYNC status */
usb_msd_data_packet_complete(s, USB_RET_SUCCESS);
} else if (s->data_len == 0) {
- s->cbw_state = USB_MSD_CBW_CSW;
+ usb_msd_change_cbw_state(s, USB_MSD_CBW_CSW);
}
if (s->cbw_state == USB_MSD_CBW_CSW) {
p = s->csw_in_packet;
if (p) {
usb_msd_send_status(s, p);
- s->cbw_state = USB_MSD_CBW_NONE;
+ usb_msd_change_cbw_state(s, USB_MSD_CBW_NONE);
/* USB_RET_SUCCESS status clears previous ASYNC status */
usb_msd_csw_packet_complete(s, USB_RET_SUCCESS);
}
@@ -388,7 +409,7 @@ void usb_msd_handle_reset(USBDevice *dev)
}
memset(&s->csw, 0, sizeof(s->csw));
- s->cbw_state = USB_MSD_CBW_NONE;
+ usb_msd_set_cbw_state(s, USB_MSD_CBW_NONE);
s->needs_reset = false;
}
@@ -439,6 +460,8 @@ static void usb_msd_cancel_io(USBDevice *dev, USBPacket *p)
{
MSDState *s = USB_STORAGE_DEV(dev);
+ trace_usb_msd_cancel_io();
+
if (p == s->data_packet) {
s->data_packet = NULL;
if (s->req) {
@@ -491,11 +514,14 @@ static void usb_msd_handle_data_out(USBDevice *dev, USBPacket *p)
SCSIDevice *scsi_dev;
int len;
+ trace_usb_msd_data_out(p->iov.size, s->data_len);
+
switch (s->cbw_state) {
case USB_MSD_CBW_NONE:
if (!try_get_valid_cbw(p, &cbw)) {
goto fail;
}
+ trace_usb_msd_cbw_out();
scsi_dev = scsi_device_find(&s->bus, 0, 0, cbw.lun);
if (scsi_dev == NULL) {
qemu_log_mask(LOG_GUEST_ERROR, "usb-msd: Bad CBW LUN %d\n",
@@ -505,11 +531,11 @@ static void usb_msd_handle_data_out(USBDevice *dev, USBPacket *p)
tag = le32_to_cpu(cbw.tag);
s->data_len = le32_to_cpu(cbw.data_len);
if (s->data_len == 0) {
- s->cbw_state = USB_MSD_CBW_NODATA;
+ usb_msd_change_cbw_state(s, USB_MSD_CBW_NODATA);
} else if (cbw.flags & 0x80) {
- s->cbw_state = USB_MSD_CBW_DATAIN;
+ usb_msd_change_cbw_state(s, USB_MSD_CBW_DATAIN);
} else {
- s->cbw_state = USB_MSD_CBW_DATAOUT;
+ usb_msd_change_cbw_state(s, USB_MSD_CBW_DATAOUT);
}
if (s->unknown_in_packet) {
if (s->cbw_state == USB_MSD_CBW_DATAIN) {
@@ -540,7 +566,6 @@ static void usb_msd_handle_data_out(USBDevice *dev, USBPacket *p)
break;
case USB_MSD_CBW_DATAOUT:
- trace_usb_msd_data_out(p->iov.size, s->data_len);
if (s->unknown_in_packet) {
error_report("usb-msd: unknown_in_packet in DATAOUT state");
goto fail;
@@ -562,7 +587,7 @@ static void usb_msd_handle_data_out(USBDevice *dev, USBPacket *p)
}
s->data_len -= len;
if (s->data_len == 0) {
- s->cbw_state = USB_MSD_CBW_CSW;
+ usb_msd_change_cbw_state(s, USB_MSD_CBW_CSW);
}
}
}
@@ -579,6 +604,7 @@ static void usb_msd_handle_data_out(USBDevice *dev, USBPacket *p)
return;
fail:
+ trace_usb_msd_bad_packet();
p->status = USB_RET_STALL;
}
@@ -587,8 +613,11 @@ static void usb_msd_handle_data_in(USBDevice *dev, USBPacket *p)
MSDState *s = (MSDState *)dev;
int len;
+ trace_usb_msd_data_in(p->iov.size, s->data_len, s->scsi_len);
+
switch (s->cbw_state) {
case USB_MSD_CBW_NONE:
+ trace_usb_msd_unknown_in(p->iov.size);
if (s->unknown_in_packet) {
qemu_log_mask(LOG_GUEST_ERROR, "usb-msd: second IN packet was"
"received before CBW\n");
@@ -602,12 +631,13 @@ static void usb_msd_handle_data_in(USBDevice *dev, USBPacket *p)
case USB_MSD_CBW_NODATA:
case USB_MSD_CBW_DATAOUT:
if (s->unknown_in_packet) {
- error_report("usb-msd: unknown_in_packet in DATAOUT state");
+ error_report("usb-msd: unknown_in_packet in DATAOUT/NODATA state");
goto fail;
}
if (!check_valid_csw(p)) {
goto fail;
}
+ trace_usb_msd_csw_in();
if (s->data_len != 0) {
qemu_log_mask(LOG_GUEST_ERROR, "usb-msd: CSW received before "
"all data was sent\n");
@@ -621,6 +651,7 @@ static void usb_msd_handle_data_in(USBDevice *dev, USBPacket *p)
break;
case USB_MSD_CBW_CSW:
+ trace_usb_msd_csw_in();
if (s->unknown_in_packet) {
error_report("usb-msd: unknown_in_packet in CSW state");
goto fail;
@@ -636,12 +667,11 @@ static void usb_msd_handle_data_in(USBDevice *dev, USBPacket *p)
p->status = USB_RET_ASYNC;
} else {
usb_msd_send_status(s, p);
- s->cbw_state = USB_MSD_CBW_NONE;
+ usb_msd_change_cbw_state(s, USB_MSD_CBW_NONE);
}
break;
case USB_MSD_CBW_DATAIN:
- trace_usb_msd_data_in(p->iov.size, s->data_len, s->scsi_len);
if (s->unknown_in_packet) {
error_report("usb-msd: unknown_in_packet in DATAIN state");
goto fail;
@@ -658,7 +688,7 @@ static void usb_msd_handle_data_in(USBDevice *dev, USBPacket *p)
}
s->data_len -= len;
if (s->data_len == 0) {
- s->cbw_state = USB_MSD_CBW_CSW;
+ usb_msd_change_cbw_state(s, USB_MSD_CBW_CSW);
}
}
}
@@ -676,6 +706,7 @@ static void usb_msd_handle_data_in(USBDevice *dev, USBPacket *p)
return;
fail:
+ trace_usb_msd_bad_packet();
p->status = USB_RET_STALL;
}
diff --git a/hw/usb/trace-events b/hw/usb/trace-events
index dd04f14add1..851ba9986c3 100644
--- a/hw/usb/trace-events
+++ b/hw/usb/trace-events
@@ -264,12 +264,21 @@ usb_msd_maxlun(unsigned maxlun) "%d"
usb_msd_send_status(unsigned status, unsigned tag, size_t size) "status %d, tag 0x%x, len %zd"
usb_msd_data_in(unsigned packet, unsigned remaining, unsigned total) "%d/%d (scsi %d)"
usb_msd_data_out(unsigned packet, unsigned remaining) "%d/%d"
+usb_msd_unknown_in(unsigned packet) "%d"
+usb_msd_cbw_out(void) ""
+usb_msd_csw_in(void) ""
usb_msd_packet_async(void) ""
-usb_msd_packet_complete(void) ""
+usb_msd_data_packet_complete(void) ""
+usb_msd_csw_packet_complete(void) ""
+usb_msd_bad_packet(void) ""
usb_msd_cmd_submit(unsigned lun, unsigned tag, unsigned flags, unsigned len, unsigned data_len) "lun %u, tag 0x%x, flags 0x%08x, len %d, data-len %d"
usb_msd_cmd_complete(unsigned status, unsigned tag) "status %d, tag 0x%x"
+usb_msd_copy_data(unsigned tag, unsigned len) "tag 0x%x len %d"
+usb_msd_transfer_data(unsigned tag, unsigned len) "tag 0x%x len %d"
usb_msd_cmd_cancel(unsigned tag) "tag 0x%x"
+usb_msd_cancel_io(void) ""
usb_msd_fatal_error(void) ""
+usb_msd_cbw_state(unsigned cbw_state) "cbw-state %d"
# dev-uas.c
usb_uas_reset(int addr) "dev %d"
--
2.47.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v4 00/22] usb/xhci and usb/msd improvements and tests
2025-05-02 3:30 [PATCH v4 00/22] usb/xhci and usb/msd improvements and tests Nicholas Piggin
` (21 preceding siblings ...)
2025-05-02 3:30 ` [PATCH v4 22/22] usb/msd: Add more tracing Nicholas Piggin
@ 2025-05-05 2:03 ` Nicholas Piggin
2025-05-05 9:02 ` Kevin Wolf
22 siblings, 1 reply; 45+ messages in thread
From: Nicholas Piggin @ 2025-05-05 2:03 UTC (permalink / raw)
To: Nicholas Piggin, Gerd Hoffmann
Cc: qemu-devel, Kevin Wolf, Paolo Bonzini, Michael S. Tsirkin,
Marcel Apfelbaum, Fabiano Rosas, Laurent Vivier,
Phil Dennis-Jordan, Bernhard Beschow, Philippe Mathieu-Daudé
On Fri May 2, 2025 at 1:30 PM AEST, Nicholas Piggin wrote:
> This is merged from two series now because code especially the test
> cases have started to depend on one another.
Question for the list, hw/usb/* is marked orphan. I don't have the
bandwidth to take it on. There's one or two other little things that
need to be taken, e.g.,
https://lore.kernel.org/qemu-devel/20250405140002.3537411-1-linux@roeck-us.net/
Bernhard and Phil Dennis-Jordan have been doing some good work and
reviews on host controllers and Kevin on usb-storage. Any interest
to maintain it or do odd fixes? I suppose most are in the same boat
as me.
I would like to get this series merged, but I realize the mass storage
change to relax packet ordering of a command particularly is quite
complicated and under-reviewed.
Would there be objection if I made a pull request for Guenter's
patches, the hcd stuff, the qtests, and some of the easier / reviewed
bits of msd?
Thanks,
Nick
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v4 00/22] usb/xhci and usb/msd improvements and tests
2025-05-05 2:03 ` [PATCH v4 00/22] usb/xhci and usb/msd improvements and tests Nicholas Piggin
@ 2025-05-05 9:02 ` Kevin Wolf
2025-05-12 13:20 ` Peter Maydell
0 siblings, 1 reply; 45+ messages in thread
From: Kevin Wolf @ 2025-05-05 9:02 UTC (permalink / raw)
To: Nicholas Piggin
Cc: Gerd Hoffmann, qemu-devel, Paolo Bonzini, Michael S. Tsirkin,
Marcel Apfelbaum, Fabiano Rosas, Laurent Vivier,
Phil Dennis-Jordan, Bernhard Beschow, Philippe Mathieu-Daudé
Am 05.05.2025 um 04:03 hat Nicholas Piggin geschrieben:
> On Fri May 2, 2025 at 1:30 PM AEST, Nicholas Piggin wrote:
> > This is merged from two series now because code especially the test
> > cases have started to depend on one another.
>
> Question for the list, hw/usb/* is marked orphan. I don't have the
> bandwidth to take it on. There's one or two other little things that
> need to be taken, e.g.,
>
> https://lore.kernel.org/qemu-devel/20250405140002.3537411-1-linux@roeck-us.net/
>
> Bernhard and Phil Dennis-Jordan have been doing some good work and
> reviews on host controllers and Kevin on usb-storage. Any interest
> to maintain it or do odd fixes? I suppose most are in the same boat
> as me.
The changes I made (or merged) were mostly for the external interfaces
of the device. Initialising the qdev device, setting properties, passing
them to the SCSI device etc. It's been a while since I last looked at
some actual USB stuff (and not in the context of QEMU), so I'm not
necessarily the natural reviewer/maintainer for this part of it. (On the
other hand, who is if Gerd doesn't have the time for it any more?)
> I would like to get this series merged, but I realize the mass storage
> change to relax packet ordering of a command particularly is quite
> complicated and under-reviewed.
I can try to find the time to have a look at the series, but given that
I'll have to familiarise myself with the specs again, it might take a
while.
> Would there be objection if I made a pull request for Guenter's
> patches, the hcd stuff, the qtests, and some of the easier / reviewed
> bits of msd?
That makes sense to me. I suppose I can also give a quick review for the
initial part of the msd patches, at lot of which seems to be more or
less just refactoring.
Kevin
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v4 00/22] usb/xhci and usb/msd improvements and tests
2025-05-05 9:02 ` Kevin Wolf
@ 2025-05-12 13:20 ` Peter Maydell
2025-05-12 15:33 ` Fabiano Rosas
0 siblings, 1 reply; 45+ messages in thread
From: Peter Maydell @ 2025-05-12 13:20 UTC (permalink / raw)
To: Kevin Wolf
Cc: Nicholas Piggin, Gerd Hoffmann, qemu-devel, Paolo Bonzini,
Michael S. Tsirkin, Marcel Apfelbaum, Fabiano Rosas,
Laurent Vivier, Phil Dennis-Jordan, Bernhard Beschow,
Philippe Mathieu-Daudé
On Mon, 5 May 2025 at 10:03, Kevin Wolf <kwolf@redhat.com> wrote:
>
> Am 05.05.2025 um 04:03 hat Nicholas Piggin geschrieben:
> > I would like to get this series merged, but I realize the mass storage
> > change to relax packet ordering of a command particularly is quite
> > complicated and under-reviewed.
>
> I can try to find the time to have a look at the series, but given that
> I'll have to familiarise myself with the specs again, it might take a
> while.
>
> > Would there be objection if I made a pull request for Guenter's
> > patches, the hcd stuff, the qtests, and some of the easier / reviewed
> > bits of msd?
>
> That makes sense to me. I suppose I can also give a quick review for the
> initial part of the msd patches, at lot of which seems to be more or
> less just refactoring.
I've now reviewed the hw/usb/xhci patches, so I think you
now have reviews for everything here except the tests/qtest/
patches (3, 4, 5, 7, 8, 21).
-- PMM
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v4 00/22] usb/xhci and usb/msd improvements and tests
2025-05-12 13:20 ` Peter Maydell
@ 2025-05-12 15:33 ` Fabiano Rosas
0 siblings, 0 replies; 45+ messages in thread
From: Fabiano Rosas @ 2025-05-12 15:33 UTC (permalink / raw)
To: Peter Maydell, Kevin Wolf
Cc: Nicholas Piggin, Gerd Hoffmann, qemu-devel, Paolo Bonzini,
Michael S. Tsirkin, Marcel Apfelbaum, Laurent Vivier,
Phil Dennis-Jordan, Bernhard Beschow, Philippe Mathieu-Daudé
Peter Maydell <peter.maydell@linaro.org> writes:
> On Mon, 5 May 2025 at 10:03, Kevin Wolf <kwolf@redhat.com> wrote:
>>
>> Am 05.05.2025 um 04:03 hat Nicholas Piggin geschrieben:
>> > I would like to get this series merged, but I realize the mass storage
>> > change to relax packet ordering of a command particularly is quite
>> > complicated and under-reviewed.
>>
>> I can try to find the time to have a look at the series, but given that
>> I'll have to familiarise myself with the specs again, it might take a
>> while.
>>
>> > Would there be objection if I made a pull request for Guenter's
>> > patches, the hcd stuff, the qtests, and some of the easier / reviewed
>> > bits of msd?
>>
>> That makes sense to me. I suppose I can also give a quick review for the
>> initial part of the msd patches, at lot of which seems to be more or
>> less just refactoring.
>
> I've now reviewed the hw/usb/xhci patches, so I think you
> now have reviews for everything here except the tests/qtest/
> patches (3, 4, 5, 7, 8, 21).
>
I'll get to them this week. It'd be nice to see the new version of the
pending dependency series that's mentioned in the cover letter. For
these subsystems I'm not familiar with, most of my qtest review will
rely on putting the series through asan and tests on a loaded
machine. If there's latent bugs in qtest, it gets super confusing.
^ permalink raw reply [flat|nested] 45+ messages in thread