* [Qemu-devel] [PATCH 1/4] xhci: drop ER_FULL_HACK workaround
2017-02-06 11:55 [Qemu-devel] [PATCH 0/4] xhci: cleanups, add qemu-xhci device Gerd Hoffmann
@ 2017-02-06 11:55 ` Gerd Hoffmann
2017-02-06 11:55 ` [Qemu-devel] [PATCH 2/4] xhci: add qemu xhci controller Gerd Hoffmann
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Gerd Hoffmann @ 2017-02-06 11:55 UTC (permalink / raw)
To: qemu-devel; +Cc: Gerd Hoffmann
The nec/renesas driver problems have finally been debugged and root
caused, see commit "7da76e1 xhci: fix event queue IRQ handling".
It's pretty clear now that
(a) The whole "driver can't handle ring full" story is most likely
wrong.
(b) The ER_FULL_HACK workaround based on the false assumtion doesn't
much. It avoids the driver crashing (without commit 7da76e1), but
it doesn't make usb work.
(c) With 7da76e1 applied it doesn't trigger any more.
So, lets kill it. Or, to be exact, lets almost kill it. Some data
fields are kept unused in the state struct, for live migration backward
compatibility.
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
hw/usb/hcd-xhci.c | 117 +++++-------------------------------------------------
1 file changed, 11 insertions(+), 106 deletions(-)
diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 54b3901..74184ac 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -49,9 +49,6 @@
/* Very pessimistic, let's hope it's enough for all cases */
#define EV_QUEUE (((3 * 24) + 16) * MAXSLOTS)
-/* Do not deliver ER Full events. NEC's driver does some things not bound
- * to the specs when it gets them */
-#define ER_FULL_HACK
#define TRB_LINK_LIMIT 4
@@ -431,12 +428,14 @@ typedef struct XHCIInterrupter {
uint32_t erdp_low;
uint32_t erdp_high;
- bool msix_used, er_pcs, er_full;
+ bool msix_used, er_pcs;
dma_addr_t er_start;
uint32_t er_size;
unsigned int er_ep_idx;
+ /* kept for live migration compat only */
+ bool er_full_unused;
XHCIEvent ev_buffer[EV_QUEUE];
unsigned int ev_buffer_put;
unsigned int ev_buffer_get;
@@ -826,7 +825,7 @@ static void xhci_intr_raise(XHCIState *xhci, int v)
static inline int xhci_running(XHCIState *xhci)
{
- return !(xhci->usbsts & USBSTS_HCH) && !xhci->intr[0].er_full;
+ return !(xhci->usbsts & USBSTS_HCH);
}
static void xhci_die(XHCIState *xhci)
@@ -865,74 +864,6 @@ static void xhci_write_event(XHCIState *xhci, XHCIEvent *event, int v)
}
}
-static void xhci_events_update(XHCIState *xhci, int v)
-{
- XHCIInterrupter *intr = &xhci->intr[v];
- dma_addr_t erdp;
- unsigned int dp_idx;
- bool do_irq = 0;
-
- if (xhci->usbsts & USBSTS_HCH) {
- return;
- }
-
- erdp = xhci_addr64(intr->erdp_low, intr->erdp_high);
- if (erdp < intr->er_start ||
- erdp >= (intr->er_start + TRB_SIZE*intr->er_size)) {
- DPRINTF("xhci: ERDP out of bounds: "DMA_ADDR_FMT"\n", erdp);
- DPRINTF("xhci: ER[%d] at "DMA_ADDR_FMT" len %d\n",
- v, intr->er_start, intr->er_size);
- xhci_die(xhci);
- return;
- }
- dp_idx = (erdp - intr->er_start) / TRB_SIZE;
- assert(dp_idx < intr->er_size);
-
- /* NEC didn't read section 4.9.4 of the spec (v1.0 p139 top Note) and thus
- * deadlocks when the ER is full. Hack it by holding off events until
- * the driver decides to free at least half of the ring */
- if (intr->er_full) {
- int er_free = dp_idx - intr->er_ep_idx;
- if (er_free <= 0) {
- er_free += intr->er_size;
- }
- if (er_free < (intr->er_size/2)) {
- DPRINTF("xhci_events_update(): event ring still "
- "more than half full (hack)\n");
- return;
- }
- }
-
- while (intr->ev_buffer_put != intr->ev_buffer_get) {
- assert(intr->er_full);
- if (((intr->er_ep_idx+1) % intr->er_size) == dp_idx) {
- DPRINTF("xhci_events_update(): event ring full again\n");
-#ifndef ER_FULL_HACK
- XHCIEvent full = {ER_HOST_CONTROLLER, CC_EVENT_RING_FULL_ERROR};
- xhci_write_event(xhci, &full, v);
-#endif
- do_irq = 1;
- break;
- }
- XHCIEvent *event = &intr->ev_buffer[intr->ev_buffer_get];
- xhci_write_event(xhci, event, v);
- intr->ev_buffer_get++;
- do_irq = 1;
- if (intr->ev_buffer_get == EV_QUEUE) {
- intr->ev_buffer_get = 0;
- }
- }
-
- if (do_irq) {
- xhci_intr_raise(xhci, v);
- }
-
- if (intr->er_full && intr->ev_buffer_put == intr->ev_buffer_get) {
- DPRINTF("xhci_events_update(): event ring no longer full\n");
- intr->er_full = 0;
- }
-}
-
static void xhci_event(XHCIState *xhci, XHCIEvent *event, int v)
{
XHCIInterrupter *intr;
@@ -945,19 +876,6 @@ static void xhci_event(XHCIState *xhci, XHCIEvent *event, int v)
}
intr = &xhci->intr[v];
- if (intr->er_full) {
- DPRINTF("xhci_event(): ER full, queueing\n");
- if (((intr->ev_buffer_put+1) % EV_QUEUE) == intr->ev_buffer_get) {
- DPRINTF("xhci: event queue full, dropping event!\n");
- return;
- }
- intr->ev_buffer[intr->ev_buffer_put++] = *event;
- if (intr->ev_buffer_put == EV_QUEUE) {
- intr->ev_buffer_put = 0;
- }
- return;
- }
-
erdp = xhci_addr64(intr->erdp_low, intr->erdp_high);
if (erdp < intr->er_start ||
erdp >= (intr->er_start + TRB_SIZE*intr->er_size)) {
@@ -971,21 +889,12 @@ static void xhci_event(XHCIState *xhci, XHCIEvent *event, int v)
dp_idx = (erdp - intr->er_start) / TRB_SIZE;
assert(dp_idx < intr->er_size);
- if ((intr->er_ep_idx+1) % intr->er_size == dp_idx) {
- DPRINTF("xhci_event(): ER full, queueing\n");
-#ifndef ER_FULL_HACK
+ if ((intr->er_ep_idx + 2) % intr->er_size == dp_idx) {
+ DPRINTF("xhci: ER %d full, send ring full error\n", v);
XHCIEvent full = {ER_HOST_CONTROLLER, CC_EVENT_RING_FULL_ERROR};
- xhci_write_event(xhci, &full);
-#endif
- intr->er_full = 1;
- if (((intr->ev_buffer_put+1) % EV_QUEUE) == intr->ev_buffer_get) {
- DPRINTF("xhci: event queue full, dropping event!\n");
- return;
- }
- intr->ev_buffer[intr->ev_buffer_put++] = *event;
- if (intr->ev_buffer_put == EV_QUEUE) {
- intr->ev_buffer_put = 0;
- }
+ xhci_write_event(xhci, &full, v);
+ } else if ((intr->er_ep_idx + 1) % intr->er_size == dp_idx) {
+ DPRINTF("xhci: ER %d full, drop event\n", v);
} else {
xhci_write_event(xhci, event, v);
}
@@ -1124,7 +1033,6 @@ static void xhci_er_reset(XHCIState *xhci, int v)
intr->er_ep_idx = 0;
intr->er_pcs = 1;
- intr->er_full = 0;
DPRINTF("xhci: event ring[%d]:" DMA_ADDR_FMT " [%d]\n",
v, intr->er_start, intr->er_size);
@@ -2978,7 +2886,6 @@ static void xhci_reset(DeviceState *dev)
xhci->intr[i].er_ep_idx = 0;
xhci->intr[i].er_pcs = 1;
- xhci->intr[i].er_full = 0;
xhci->intr[i].ev_buffer_put = 0;
xhci->intr[i].ev_buffer_get = 0;
}
@@ -3368,7 +3275,6 @@ static void xhci_runtime_write(void *ptr, hwaddr reg,
break;
case 0x1c: /* ERDP high */
intr->erdp_high = val;
- xhci_events_update(xhci, v);
break;
default:
trace_usb_xhci_unimplemented("oper write", reg);
@@ -3866,8 +3772,7 @@ static const VMStateDescription vmstate_xhci_event = {
static bool xhci_er_full(void *opaque, int version_id)
{
- struct XHCIInterrupter *intr = opaque;
- return intr->er_full;
+ return false;
}
static const VMStateDescription vmstate_xhci_intr = {
@@ -3891,7 +3796,7 @@ static const VMStateDescription vmstate_xhci_intr = {
VMSTATE_UINT32(er_ep_idx, XHCIInterrupter),
/* event queue (used if ring is full) */
- VMSTATE_BOOL(er_full, XHCIInterrupter),
+ VMSTATE_BOOL(er_full_unused, XHCIInterrupter),
VMSTATE_UINT32_TEST(ev_buffer_put, XHCIInterrupter, xhci_er_full),
VMSTATE_UINT32_TEST(ev_buffer_get, XHCIInterrupter, xhci_er_full),
VMSTATE_STRUCT_ARRAY_TEST(ev_buffer, XHCIInterrupter, EV_QUEUE,
--
1.8.3.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* [Qemu-devel] [PATCH 2/4] xhci: add qemu xhci controller
2017-02-06 11:55 [Qemu-devel] [PATCH 0/4] xhci: cleanups, add qemu-xhci device Gerd Hoffmann
2017-02-06 11:55 ` [Qemu-devel] [PATCH 1/4] xhci: drop ER_FULL_HACK workaround Gerd Hoffmann
@ 2017-02-06 11:55 ` Gerd Hoffmann
2017-02-13 19:01 ` Marcel Apfelbaum
2017-02-06 11:55 ` [Qemu-devel] [PATCH 3/4] xhci: fix nec vendor quirk handling Gerd Hoffmann
2017-02-06 11:55 ` [Qemu-devel] [PATCH 4/4] xhci: drop via vendor command handling Gerd Hoffmann
3 siblings, 1 reply; 6+ messages in thread
From: Gerd Hoffmann @ 2017-02-06 11:55 UTC (permalink / raw)
To: qemu-devel; +Cc: Gerd Hoffmann, Michael S. Tsirkin, Marcel Apfelbaum
Turn existing TYPE_XHCI into an abstract base class.
Create two child classes, TYPE_NEC_XHCI (same name as old xhci
controller) and TYPE_QEMU_XHCI (using an ID from our namespace).
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
docs/specs/pci-ids.txt | 1 +
hw/usb/hcd-xhci.c | 40 ++++++++++++++++++++++++++++++++++++----
include/hw/pci/pci.h | 1 +
3 files changed, 38 insertions(+), 4 deletions(-)
diff --git a/docs/specs/pci-ids.txt b/docs/specs/pci-ids.txt
index 16fdb0c..95adee0 100644
--- a/docs/specs/pci-ids.txt
+++ b/docs/specs/pci-ids.txt
@@ -61,6 +61,7 @@ PCI devices (other than virtio):
1b36:0009 PCI Expander Bridge (-device pxb)
1b36:000a PCI-PCI bridge (multiseat)
1b36:000b PCIe Expander Bridge (-device pxb-pcie)
+1b36:000d PCI xhci usb host adapter
All these devices are documented in docs/specs.
diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 74184ac..887bb39 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -487,7 +487,9 @@ struct XHCIState {
XHCIRing cmd_ring;
};
-#define TYPE_XHCI "nec-usb-xhci"
+#define TYPE_XHCI "base-xhci"
+#define TYPE_NEC_XHCI "nec-usb-xhci"
+#define TYPE_QEMU_XHCI "qemu-xhci"
#define XHCI(obj) \
OBJECT_CHECK(XHCIState, (obj), TYPE_XHCI)
@@ -3868,10 +3870,7 @@ static void xhci_class_init(ObjectClass *klass, void *data)
set_bit(DEVICE_CATEGORY_USB, dc->categories);
k->realize = usb_xhci_realize;
k->exit = usb_xhci_exit;
- k->vendor_id = PCI_VENDOR_ID_NEC;
- k->device_id = PCI_DEVICE_ID_NEC_UPD720200;
k->class_id = PCI_CLASS_SERIAL_USB;
- k->revision = 0x03;
k->is_express = 1;
}
@@ -3880,11 +3879,44 @@ static const TypeInfo xhci_info = {
.parent = TYPE_PCI_DEVICE,
.instance_size = sizeof(XHCIState),
.class_init = xhci_class_init,
+ .abstract = true,
+};
+
+static void nec_xhci_class_init(ObjectClass *klass, void *data)
+{
+ PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
+
+ k->vendor_id = PCI_VENDOR_ID_NEC;
+ k->device_id = PCI_DEVICE_ID_NEC_UPD720200;
+ k->revision = 0x03;
+}
+
+static const TypeInfo nec_xhci_info = {
+ .name = TYPE_NEC_XHCI,
+ .parent = TYPE_XHCI,
+ .class_init = nec_xhci_class_init,
+};
+
+static void qemu_xhci_class_init(ObjectClass *klass, void *data)
+{
+ PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
+
+ k->vendor_id = PCI_VENDOR_ID_REDHAT;
+ k->device_id = PCI_DEVICE_ID_REDHAT_XHCI;
+ k->revision = 0x01;
+}
+
+static const TypeInfo qemu_xhci_info = {
+ .name = TYPE_QEMU_XHCI,
+ .parent = TYPE_XHCI,
+ .class_init = qemu_xhci_class_init,
};
static void xhci_register_types(void)
{
type_register_static(&xhci_info);
+ type_register_static(&nec_xhci_info);
+ type_register_static(&qemu_xhci_info);
}
type_init(xhci_register_types)
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index cbc1fdf..05ef14b 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -97,6 +97,7 @@
#define PCI_DEVICE_ID_REDHAT_BRIDGE_SEAT 0x000a
#define PCI_DEVICE_ID_REDHAT_PXB_PCIE 0x000b
#define PCI_DEVICE_ID_REDHAT_PCIE_RP 0x000c
+#define PCI_DEVICE_ID_REDHAT_XHCI 0x000d
#define PCI_DEVICE_ID_REDHAT_QXL 0x0100
#define FMT_PCIBUS PRIx64
--
1.8.3.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* [Qemu-devel] [PATCH 3/4] xhci: fix nec vendor quirk handling
2017-02-06 11:55 [Qemu-devel] [PATCH 0/4] xhci: cleanups, add qemu-xhci device Gerd Hoffmann
2017-02-06 11:55 ` [Qemu-devel] [PATCH 1/4] xhci: drop ER_FULL_HACK workaround Gerd Hoffmann
2017-02-06 11:55 ` [Qemu-devel] [PATCH 2/4] xhci: add qemu xhci controller Gerd Hoffmann
@ 2017-02-06 11:55 ` Gerd Hoffmann
2017-02-06 11:55 ` [Qemu-devel] [PATCH 4/4] xhci: drop via vendor command handling Gerd Hoffmann
3 siblings, 0 replies; 6+ messages in thread
From: Gerd Hoffmann @ 2017-02-06 11:55 UTC (permalink / raw)
To: qemu-devel; +Cc: Gerd Hoffmann
Only the TYPE_NEC_XHCI controller will have the nec vendor quirks.
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
hw/usb/hcd-xhci.c | 44 +++++++++++++++++++++++++++++---------------
1 file changed, 29 insertions(+), 15 deletions(-)
diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 887bb39..0e25943 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -485,6 +485,8 @@ struct XHCIState {
XHCIInterrupter intr[MAXINTRS];
XHCIRing cmd_ring;
+
+ bool nec_quirks;
};
#define TYPE_XHCI "base-xhci"
@@ -2737,20 +2739,26 @@ static void xhci_process_commands(XHCIState *xhci)
xhci_via_challenge(xhci, trb.parameter);
break;
case CR_VENDOR_NEC_FIRMWARE_REVISION:
- event.type = 48; /* NEC reply */
- event.length = 0x3025;
+ if (xhci->nec_quirks) {
+ event.type = 48; /* NEC reply */
+ event.length = 0x3025;
+ } else {
+ event.ccode = CC_TRB_ERROR;
+ }
break;
case CR_VENDOR_NEC_CHALLENGE_RESPONSE:
- {
- uint32_t chi = trb.parameter >> 32;
- uint32_t clo = trb.parameter;
- uint32_t val = xhci_nec_challenge(chi, clo);
- event.length = val & 0xFFFF;
- event.epid = val >> 16;
- slotid = val >> 24;
- event.type = 48; /* NEC reply */
- }
- break;
+ if (xhci->nec_quirks) {
+ uint32_t chi = trb.parameter >> 32;
+ uint32_t clo = trb.parameter;
+ uint32_t val = xhci_nec_challenge(chi, clo);
+ event.length = val & 0xFFFF;
+ event.epid = val >> 16;
+ slotid = val >> 24;
+ event.type = 48; /* NEC reply */
+ } else {
+ event.ccode = CC_TRB_ERROR;
+ }
+ break;
default:
trace_usb_xhci_unimplemented("command", type);
event.ccode = CC_TRB_ERROR;
@@ -3252,9 +3260,12 @@ static void xhci_runtime_write(void *ptr, hwaddr reg,
intr->erstsz = val & 0xffff;
break;
case 0x10: /* ERSTBA low */
- /* XXX NEC driver bug: it doesn't align this to 64 bytes
- intr->erstba_low = val & 0xffffffc0; */
- intr->erstba_low = val & 0xfffffff0;
+ if (xhci->nec_quirks) {
+ /* NEC driver bug: it doesn't align this to 64 bytes */
+ intr->erstba_low = val & 0xfffffff0;
+ } else {
+ intr->erstba_low = val & 0xffffffc0;
+ }
break;
case 0x14: /* ERSTBA high */
intr->erstba_high = val;
@@ -3549,6 +3560,9 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp)
dev->config[PCI_CACHE_LINE_SIZE] = 0x10;
dev->config[0x60] = 0x30; /* release number */
+ if (strcmp(object_get_typename(OBJECT(dev)), TYPE_NEC_XHCI) == 0) {
+ xhci->nec_quirks = true;
+ }
if (xhci->numintrs > MAXINTRS) {
xhci->numintrs = MAXINTRS;
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* [Qemu-devel] [PATCH 4/4] xhci: drop via vendor command handling
2017-02-06 11:55 [Qemu-devel] [PATCH 0/4] xhci: cleanups, add qemu-xhci device Gerd Hoffmann
` (2 preceding siblings ...)
2017-02-06 11:55 ` [Qemu-devel] [PATCH 3/4] xhci: fix nec vendor quirk handling Gerd Hoffmann
@ 2017-02-06 11:55 ` Gerd Hoffmann
3 siblings, 0 replies; 6+ messages in thread
From: Gerd Hoffmann @ 2017-02-06 11:55 UTC (permalink / raw)
To: qemu-devel; +Cc: Gerd Hoffmann
Seems pretty pointless, we don't emulate an via xhci controller.
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
hw/usb/hcd-xhci.c | 31 -------------------------------
1 file changed, 31 deletions(-)
diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 0e25943..fbf8a8b 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -196,7 +196,6 @@ typedef enum TRBType {
ER_DEVICE_NOTIFICATION,
ER_MFINDEX_WRAP,
/* vendor specific bits */
- CR_VENDOR_VIA_CHALLENGE_RESPONSE = 48,
CR_VENDOR_NEC_FIRMWARE_REVISION = 49,
CR_VENDOR_NEC_CHALLENGE_RESPONSE = 50,
} TRBType;
@@ -552,7 +551,6 @@ static const char *TRBType_names[] = {
[ER_HOST_CONTROLLER] = "ER_HOST_CONTROLLER",
[ER_DEVICE_NOTIFICATION] = "ER_DEVICE_NOTIFICATION",
[ER_MFINDEX_WRAP] = "ER_MFINDEX_WRAP",
- [CR_VENDOR_VIA_CHALLENGE_RESPONSE] = "CR_VENDOR_VIA_CHALLENGE_RESPONSE",
[CR_VENDOR_NEC_FIRMWARE_REVISION] = "CR_VENDOR_NEC_FIRMWARE_REVISION",
[CR_VENDOR_NEC_CHALLENGE_RESPONSE] = "CR_VENDOR_NEC_CHALLENGE_RESPONSE",
};
@@ -2614,32 +2612,6 @@ static uint32_t xhci_nec_challenge(uint32_t hi, uint32_t lo)
return ~val;
}
-static void xhci_via_challenge(XHCIState *xhci, uint64_t addr)
-{
- PCIDevice *pci_dev = PCI_DEVICE(xhci);
- uint32_t buf[8];
- uint32_t obuf[8];
- dma_addr_t paddr = xhci_mask64(addr);
-
- pci_dma_read(pci_dev, paddr, &buf, 32);
-
- memcpy(obuf, buf, sizeof(obuf));
-
- if ((buf[0] & 0xff) == 2) {
- obuf[0] = 0x49932000 + 0x54dc200 * buf[2] + 0x7429b578 * buf[3];
- obuf[0] |= (buf[2] * buf[3]) & 0xff;
- obuf[1] = 0x0132bb37 + 0xe89 * buf[2] + 0xf09 * buf[3];
- obuf[2] = 0x0066c2e9 + 0x2091 * buf[2] + 0x19bd * buf[3];
- obuf[3] = 0xd5281342 + 0x2cc9691 * buf[2] + 0x2367662 * buf[3];
- obuf[4] = 0x0123c75c + 0x1595 * buf[2] + 0x19ec * buf[3];
- obuf[5] = 0x00f695de + 0x26fd * buf[2] + 0x3e9 * buf[3];
- obuf[6] = obuf[2] ^ obuf[3] ^ 0x29472956;
- obuf[7] = obuf[2] ^ obuf[3] ^ 0x65866593;
- }
-
- pci_dma_write(pci_dev, paddr, &obuf, 32);
-}
-
static void xhci_process_commands(XHCIState *xhci)
{
XHCITRB trb;
@@ -2735,9 +2707,6 @@ static void xhci_process_commands(XHCIState *xhci)
case CR_GET_PORT_BANDWIDTH:
event.ccode = xhci_get_port_bandwidth(xhci, trb.parameter);
break;
- case CR_VENDOR_VIA_CHALLENGE_RESPONSE:
- xhci_via_challenge(xhci, trb.parameter);
- break;
case CR_VENDOR_NEC_FIRMWARE_REVISION:
if (xhci->nec_quirks) {
event.type = 48; /* NEC reply */
--
1.8.3.1
^ permalink raw reply related [flat|nested] 6+ messages in thread