On 06/06/2011 06:38 AM, Gerd Hoffmann wrote:
> This patch adds support for keeping multiple queues going at the same
> time. One slow device will not affect other devices any more.
>
> The patch adds code to manage EHCIQueue structs. It also does a number
> of changes to the state machine:
>
> * The state machine will never ever stop in EXECUTING any more.
> Instead it will continue with the next queue (aka HORIZONTALQH) when
> the usb device returns USB_RET_ASYNC.
> * The state machine will stop processing when it figures it walks in
> circles (easy to figure now that we have a EHCIQueue struct for each
> QH we've processed). The bailout logic should not be needed any
> more. For now it is still in, but will assert() in case it triggers.
> * The state machine will just skip queues with a async USBPacket in
> flight.
> * The state machine will resume processing as soon as the async
> USBPacket is finished.
>
> The patch also takes care to flush the QH struct back to guest memory
> when needed, so we don't get stale data when (re-)loading it from guest
> memory in FETCHQH state.
>
> It also makes the writeback code to not touch the first three dwords of
> the QH struct as the EHCI must not write them. This actually fixes a
> bug where QH chaining changes (next ptr) by the linux ehci driver where
> overwritten by the emulated EHCI.
>
> Signed-off-by: Gerd Hoffmann <kra...@redhat.com>
> ---
> hw/usb-ehci.c | 171
> ++++++++++++++++++++++++++++++++++++++++++++++-----------
> trace-events | 5 +-
> 2 files changed, 142 insertions(+), 34 deletions(-)
>
> diff --git a/hw/usb-ehci.c b/hw/usb-ehci.c
> index 3656a35..5cbb675 100644
> --- a/hw/usb-ehci.c
> +++ b/hw/usb-ehci.c
> @@ -345,6 +345,9 @@ enum async_state {
>
> struct EHCIQueue {
> EHCIState *ehci;
> + QTAILQ_ENTRY(EHCIQueue) next;
> + bool async_schedule;
> + uint32_t seen, ts;
>
> /* cached data from guest - needs to be flushed
> * when guest removes an entry (doorbell, handshake sequence)
> @@ -400,7 +403,7 @@ struct EHCIState {
> int pstate; // Current state in periodic schedule
> USBPort ports[NB_PORTS];
> uint32_t usbsts_pending;
> - EHCIQueue queue;
> + QTAILQ_HEAD(, EHCIQueue) queues;
>
> uint32_t a_fetch_addr; // which address to look at next
> uint32_t p_fetch_addr; // which address to look at next
> @@ -594,9 +597,9 @@ static int ehci_get_fetch_addr(EHCIState *s, int async)
> return async ? s->a_fetch_addr : s->p_fetch_addr;
> }
>
> -static void ehci_trace_qh(EHCIState *s, target_phys_addr_t addr, EHCIqh *qh)
> +static void ehci_trace_qh(EHCIQueue *q, target_phys_addr_t addr, EHCIqh *qh)
> {
> - trace_usb_ehci_qh(addr, qh->next,
> + trace_usb_ehci_qh(q, addr, qh->next,
> qh->current_qtd, qh->next_qtd, qh->altnext_qtd,
> get_field(qh->epchar, QH_EPCHAR_RL),
> get_field(qh->epchar, QH_EPCHAR_MPLEN),
> @@ -609,9 +612,9 @@ static void ehci_trace_qh(EHCIState *s,
> target_phys_addr_t addr, EHCIqh *qh)
> (bool)(qh->epchar & QH_EPCHAR_I));
> }
>
> -static void ehci_trace_qtd(EHCIState *s, target_phys_addr_t addr, EHCIqtd
> *qtd)
> +static void ehci_trace_qtd(EHCIQueue *q, target_phys_addr_t addr, EHCIqtd
> *qtd)
> {
> - trace_usb_ehci_qtd(addr, qtd->next, qtd->altnext,
> + trace_usb_ehci_qtd(q, addr, qtd->next, qtd->altnext,
> get_field(qtd->token, QTD_TOKEN_TBYTES),
> get_field(qtd->token, QTD_TOKEN_CPAGE),
> get_field(qtd->token, QTD_TOKEN_CERR),
> @@ -628,6 +631,69 @@ static void ehci_trace_itd(EHCIState *s,
> target_phys_addr_t addr, EHCIitd *itd)
> trace_usb_ehci_itd(addr, itd->next);
> }
>
> +/* queue management */
> +
> +static EHCIQueue *ehci_alloc_queue(EHCIState *ehci, int async)
> +{
> + EHCIQueue *q;
> +
> + q = qemu_mallocz(sizeof(*q));
> + q->ehci = ehci;
> + q->async_schedule = async;
> + QTAILQ_INSERT_HEAD(&ehci->queues, q, next);
> + trace_usb_ehci_queue_action(q, "alloc");
> + return q;
> +}
> +
> +static void ehci_free_queue(EHCIQueue *q)
> +{
> + trace_usb_ehci_queue_action(q, "free");
> + if (q->async == EHCI_ASYNC_INFLIGHT) {
> + usb_cancel_packet(&q->packet);
> + }
> + QTAILQ_REMOVE(&q->ehci->queues, q, next);
> + qemu_free(q);
> +}
> +
> +static EHCIQueue *ehci_find_queue_by_qh(EHCIState *ehci, uint32_t addr)
> +{
> + EHCIQueue *q;
> +
> + QTAILQ_FOREACH(q, &ehci->queues, next) {
> + if (addr == q->qhaddr) {
> + return q;
> + }
> + }
> + return NULL;
> +}
> +
> +static void ehci_queues_rip_unused(EHCIState *ehci)
> +{
> + EHCIQueue *q, *tmp;
> +
> + QTAILQ_FOREACH_SAFE(q, &ehci->queues, next, tmp) {
> + if (q->seen) {
> + q->seen = 0;
> + q->ts = ehci->last_run_usec;
> + continue;
> + }
> + if (ehci->last_run_usec < q->ts + 250000) {
> + /* allow 0.25 sec idle */
> + continue;
> + }
> + ehci_free_queue(q);
> + }
> +}
> +
> +static void ehci_queues_rip_all(EHCIState *ehci)
> +{
> + EHCIQueue *q, *tmp;
> +
> + QTAILQ_FOREACH_SAFE(q, &ehci->queues, next, tmp) {
> + ehci_free_queue(q);
> + }
> +}
> +
> /* Attach or detach a device on root hub */
>
> static void ehci_attach(USBPort *port)
> @@ -697,6 +763,7 @@ static void ehci_reset(void *opaque)
> usb_attach(&s->ports[i], s->ports[i].dev);
> }
> }
> + ehci_queues_rip_all(s);
> }
>
> static uint32_t ehci_mem_readb(void *ptr, target_phys_addr_t addr)
> @@ -1022,8 +1089,8 @@ static void ehci_async_complete_packet(USBDevice *dev,
> USBPacket *packet)
> {
> EHCIQueue *q = container_of(packet, EHCIQueue, packet);
>
> - DPRINTF("Async packet complete\n");
> - assert(q->async == EHIC_ASYNC_INFLIGHT);
> + trace_usb_ehci_queue_action(q, "wakeup");
> + assert(q->async == EHCI_ASYNC_INFLIGHT);
> q->async = EHCI_ASYNC_FINISHED;
> q->usb_status = packet->len;
> }
> @@ -1032,10 +1099,7 @@ static void ehci_execute_complete(EHCIQueue *q)
> {
> int c_err, reload;
>
> - if (q->async == EHCI_ASYNC_INFLIGHT) {
> - DPRINTF("not done yet\n");
> - return;
> - }
> + assert(q->async != EHCI_ASYNC_INFLIGHT);
> q->async = EHCI_ASYNC_NONE;
>
> DPRINTF("execute_complete: qhaddr 0x%x, next %x, qtdaddr 0x%x, status
> %d\n",
> @@ -1185,10 +1249,6 @@ static int ehci_execute(EHCIQueue *q)
> return USB_RET_PROCERR;
> }
>
> - if (ret == USB_RET_ASYNC) {
> - q->async = EHCI_ASYNC_INFLIGHT;
> - }
> -
> return ret;
> }
>
> @@ -1328,10 +1388,12 @@ static int ehci_state_waitlisthead(EHCIState *ehci,
> int async)
> ehci_set_usbsts(ehci, USBSTS_REC);
> }
>
> + ehci_queues_rip_unused(ehci);
> +
> /* Find the head of the list (4.9.1.1) */
> for(i = 0; i < MAX_QH; i++) {
> get_dwords(NLPTR_GET(entry), (uint32_t *) &qh, sizeof(EHCIqh) >> 2);
> - ehci_trace_qh(ehci, NLPTR_GET(entry), &qh);
> + ehci_trace_qh(NULL, NLPTR_GET(entry), &qh);
>
> if (qh.epchar & QH_EPCHAR_H) {
> if (async) {
> @@ -1419,11 +1481,34 @@ static EHCIQueue *ehci_state_fetchqh(EHCIState *ehci,
> int async)
> int reload;
>
> entry = ehci_get_fetch_addr(ehci, async);
> - q = &ehci->queue; /* temporary */
> + q = ehci_find_queue_by_qh(ehci, entry);
> + if (NULL == q) {
> + q = ehci_alloc_queue(ehci, async);
> + }
> q->qhaddr = entry;
> + q->seen++;
> +
> + if (q->seen > 1) {
> + /* we are going in circles -- stop processing */
> + ehci_set_state(ehci, async, EST_ACTIVE);
> + q = NULL;
> + goto out;
> + }
>
> get_dwords(NLPTR_GET(q->qhaddr), (uint32_t *) &q->qh, sizeof(EHCIqh) >>
> 2);
> - ehci_trace_qh(ehci, NLPTR_GET(q->qhaddr), &q->qh);
> + ehci_trace_qh(q, NLPTR_GET(q->qhaddr), &q->qh);
> +
> + if (q->async == EHCI_ASYNC_INFLIGHT) {
> + /* I/O still in progress -- skip queue */
> + ehci_set_state(ehci, async, EST_HORIZONTALQH);
> + goto out;
> + }
> + if (q->async == EHCI_ASYNC_FINISHED) {
> + /* I/O finished -- continue processing queue */
> + trace_usb_ehci_queue_action(q, "resume");
> + ehci_set_state(ehci, async, EST_EXECUTING);
> + goto out;
> + }
>
> if (async && (q->qh.epchar & QH_EPCHAR_H)) {
>
> @@ -1542,7 +1627,7 @@ static int ehci_state_fetchqtd(EHCIQueue *q, int async)
> int again = 0;
>
> get_dwords(NLPTR_GET(q->qtdaddr),(uint32_t *) &q->qtd, sizeof(EHCIqtd)
> >> 2);
> - ehci_trace_qtd(q->ehci, NLPTR_GET(q->qtdaddr), &q->qtd);
> + ehci_trace_qtd(q, NLPTR_GET(q->qtdaddr), &q->qtd);
>
> if (q->qtd.token & QTD_TOKEN_ACTIVE) {
> ehci_set_state(q->ehci, async, EST_EXECUTE);
> @@ -1570,6 +1655,23 @@ static int ehci_state_horizqh(EHCIQueue *q, int async)
> return again;
> }
>
> +/*
> + * Write the qh back to guest physical memory. This step isn't
> + * in the EHCI spec but we need to do it since we don't share
> + * physical memory with our guest VM.
> + *
> + * The first three bytes are read-only for the EHCI, so skip them
> + * when writing back the qh.
> + */
> +static void ehci_flush_qh(EHCIQueue *q)
> +{
> + uint32_t *qh = (uint32_t *) &q->qh;
> + uint32_t dwords = sizeof(EHCIqh) >> 2;
> + uint32_t addr = NLPTR_GET(q->qhaddr);
> +
> + put_dwords(addr + 3 * sizeof(uint32_t), qh + 3, dwords - 3);
> +}
3 bytes or 3 words? Comment above says skip 3 bytes.
David
> +
> static int ehci_state_execute(EHCIQueue *q, int async)
> {
> int again = 0;
> @@ -1614,12 +1716,18 @@ static int ehci_state_execute(EHCIQueue *q, int async)
> again = -1;
> goto out;
> }
> - ehci_set_state(q->ehci, async, EST_EXECUTING);
> -
> - if (q->usb_status != USB_RET_ASYNC) {
> + if (q->usb_status == USB_RET_ASYNC) {
> + ehci_flush_qh(q);
> + trace_usb_ehci_queue_action(q, "suspend");
> + q->async = EHCI_ASYNC_INFLIGHT;
> + ehci_set_state(q->ehci, async, EST_HORIZONTALQH);
> again = 1;
> + goto out;
> }
>
> + ehci_set_state(q->ehci, async, EST_EXECUTING);
> + again = 1;
> +
> out:
> return again;
> }
> @@ -1660,13 +1768,6 @@ static int ehci_state_executing(EHCIQueue *q, int
> async)
> set_field(&q->qh.altnext_qtd, nakcnt, QH_ALTNEXT_NAKCNT);
> }
>
> - /*
> - * Write the qh back to guest physical memory. This step isn't
> - * in the EHCI spec but we need to do it since we don't share
> - * physical memory with our guest VM.
> - */
> - put_dwords(NLPTR_GET(q->qhaddr), (uint32_t *) &q->qh, sizeof(EHCIqh) >>
> 2);
> -
> /* 4.10.5 */
> if ((q->usb_status == USB_RET_NAK) || (q->qh.token & QTD_TOKEN_ACTIVE)) {
> ehci_set_state(q->ehci, async, EST_HORIZONTALQH);
> @@ -1677,6 +1778,7 @@ static int ehci_state_executing(EHCIQueue *q, int async)
> again = 1;
>
> out:
> + ehci_flush_qh(q);
> return again;
> }
>
> @@ -1686,7 +1788,7 @@ static int ehci_state_writeback(EHCIQueue *q, int async)
> int again = 0;
>
> /* Write back the QTD from the QH area */
> - ehci_trace_qtd(q->ehci, NLPTR_GET(q->qtdaddr), (EHCIqtd*)
> &q->qh.next_qtd);
> + ehci_trace_qtd(q, NLPTR_GET(q->qtdaddr), (EHCIqtd*) &q->qh.next_qtd);
> put_dwords(NLPTR_GET(q->qtdaddr),(uint32_t *) &q->qh.next_qtd,
> sizeof(EHCIqtd) >> 2);
>
> @@ -1720,11 +1822,14 @@ static void ehci_advance_state(EHCIState *ehci,
> * something is wrong with the linked list. TO-DO: why is
> * this hack needed?
> */
> + assert(iter < MAX_ITERATIONS);
> +#if 0
> if (iter > MAX_ITERATIONS) {
> DPRINTF("\n*** advance_state: bailing on MAX
> ITERATIONS***\n");
> ehci_set_state(ehci, async, EST_ACTIVE);
> break;
> }
> +#endif
> }
> switch(ehci_get_state(ehci, async)) {
> case EST_WAITLISTHEAD:
> @@ -1762,7 +1867,7 @@ static void ehci_advance_state(EHCIState *ehci,
> break;
>
> case EST_EXECUTING:
> - q = &ehci->queue; /* temporary */
> + assert(q != NULL);
> again = ehci_state_executing(q, async);
> break;
>
> @@ -1773,6 +1878,7 @@ static void ehci_advance_state(EHCIState *ehci,
> default:
> fprintf(stderr, "Bad state!\n");
> again = -1;
> + assert(0);
> break;
> }
>
> @@ -1780,6 +1886,7 @@ static void ehci_advance_state(EHCIState *ehci,
> fprintf(stderr, "processing error - resetting ehci HC\n");
> ehci_reset(ehci);
> again = 0;
> + assert(0);
> }
> }
> while (again);
> @@ -2070,7 +2177,7 @@ static int usb_ehci_initfn(PCIDevice *dev)
> }
>
> s->frame_timer = qemu_new_timer_ns(vm_clock, ehci_frame_timer, s);
> - s->queue.ehci = s;
> + QTAILQ_INIT(&s->queues);
>
> qemu_register_reset(ehci_reset, s);
>
> diff --git a/trace-events b/trace-events
> index 3426e14..51e2e7c 100644
> --- a/trace-events
> +++ b/trace-events
> @@ -201,13 +201,14 @@ disable usb_ehci_mmio_writel(uint32_t addr, const char
> *str, uint32_t val) "wr m
> disable usb_ehci_mmio_change(uint32_t addr, const char *str, uint32_t new,
> uint32_t old) "ch mmio %04x [%s] = %x (old: %x)"
> disable usb_ehci_usbsts(const char *sts, int state) "usbsts %s %d"
> disable usb_ehci_state(const char *schedule, const char *state) "%s schedule
> %s"
> -disable usb_ehci_qh(uint32_t addr, uint32_t next, uint32_t c_qtd, uint32_t
> n_qtd, uint32_t a_qtd, int rl, int mplen, int eps, int ep, int devaddr, int
> c, int h, int dtc, int i) "QH @ %08x: next %08x qtds %08x,%08x,%08x - rl %d,
> mplen %d, eps %d, ep %d, dev %d, c %d, h %d, dtc %d, i %d"
> -disable usb_ehci_qtd(uint32_t addr, uint32_t next, uint32_t altnext, int
> tbytes, int cpage, int cerr, int pid, int ioc, int active, int halt, int
> babble, int xacterr) "QH @ %08x: next %08x altnext %08x - tbytes %d, cpage
> %d, cerr %d, pid %d, ioc %d, active %d, halt %d, babble %d, xacterr %d"
> +disable usb_ehci_qh(void *q, uint32_t addr, uint32_t next, uint32_t c_qtd,
> uint32_t n_qtd, uint32_t a_qtd, int rl, int mplen, int eps, int ep, int
> devaddr, int c, int h, int dtc, int i) "q %p - QH @ %08x: next %08x qtds
> %08x,%08x,%08x - rl %d, mplen %d, eps %d, ep %d, dev %d, c %d, h %d, dtc %d,
> i %d"
> +disable usb_ehci_qtd(void *q, uint32_t addr, uint32_t next, uint32_t
> altnext, int tbytes, int cpage, int cerr, int pid, int ioc, int active, int
> halt, int babble, int xacterr) "q %p - QTD @ %08x: next %08x altnext %08x -
> tbytes %d, cpage %d, cerr %d, pid %d, ioc %d, active %d, halt %d, babble %d,
> xacterr %d"
> disable usb_ehci_itd(uint32_t addr, uint32_t next) "ITD @ %08x: next %08x"
> disable usb_ehci_port_attach(uint32_t port, const char *device) "attach port
> #%d - %s"
> disable usb_ehci_port_detach(uint32_t port) "detach port #%d"
> disable usb_ehci_port_reset(uint32_t port, int enable) "reset port #%d - %d"
> disable usb_ehci_data(int rw, uint32_t cpage, uint32_t offset, uint32_t
> addr, uint32_t len, uint32_t bufpos) "write %d, cpage %d, offset 0x%03x, addr
> 0x%08x, len %d, bufpos %d"
> +disable usb_ehci_queue_action(void *q, const char *action) "q %p: %s"
>
> # hw/usb-desc.c
> disable usb_desc_device(int addr, int len, int ret) "dev %d query device,
> len %d, ret %d"
