From patchwork Fri Mar 2 20:27:12 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Hans de Goede X-Patchwork-Id: 144340 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id D3AC4B6F9F for ; Sat, 3 Mar 2012 07:38:53 +1100 (EST) Received: from localhost ([::1]:37116 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S3Z4L-0005Zc-IA for incoming@patchwork.ozlabs.org; Fri, 02 Mar 2012 15:27:13 -0500 Received: from eggs.gnu.org ([208.118.235.92]:32921) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S3Z2u-0002Ti-OZ for qemu-devel@nongnu.org; Fri, 02 Mar 2012 15:26:04 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1S3Z2s-0007BF-DZ for qemu-devel@nongnu.org; Fri, 02 Mar 2012 15:25:44 -0500 Received: from mx1.redhat.com ([209.132.183.28]:44500) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S3Z2s-0007Ao-5h for qemu-devel@nongnu.org; Fri, 02 Mar 2012 15:25:42 -0500 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q22KPe5K021859 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Fri, 2 Mar 2012 15:25:40 -0500 Received: from shalem.localdomain.com (vpn1-7-1.ams2.redhat.com [10.36.7.1]) by int-mx02.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id q22KPWZZ008746; Fri, 2 Mar 2012 15:25:39 -0500 From: Hans de Goede To: Gerd Hoffmann Date: Fri, 2 Mar 2012 21:27:12 +0100 Message-Id: <1330720040-24507-6-git-send-email-hdegoede@redhat.com> In-Reply-To: <1330720040-24507-1-git-send-email-hdegoede@redhat.com> References: <1330720040-24507-1-git-send-email-hdegoede@redhat.com> X-Scanned-By: MIMEDefang 2.67 on 10.5.11.12 X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 209.132.183.28 Cc: Hans de Goede , qemu-devel@nongnu.org Subject: [Qemu-devel] [PATCH 05/13] usb-ehci: Drop cached qhs when the doorbell gets rung X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org The purpose of the IAAD bit / the doorbell is to make the ehci controller forget about cached qhs, this is mainly used when cancelling transactions, the qh is unlinked from the async schedule and then the doorbell gets rung, once the doorbell is acked by the controller the hcd knows that the qh is no longer in use and that it can do something else with the memory, such as re-use it for a new qh! But we keep our struct representing this qh around for circa 250 ms. This allows for a (mightily large) race window where the following could happen: -hcd submits a qh at address 0xdeadbeef -our ehci code sees the qh, sends a request to a usb-device, gets a result of USB_RET_ASYNC, sets the async_state of the qh to EHCI_ASYNC_INFLIGHT -hcd unlinks the qh at address 0xdeadbeef -hcd rings the doorbell, wait for us to ack it -hcd re-uses the qh at address 0xdeadbeef -our ehci code sees the qh, looks in the async_queue, sees there already is a qh at address 0xdeadbeef there with async_state of EHCI_ASYNC_INFLIGHT, does nothing -the *original* (which the hcd thinks it has cancelled) transaction finishes -our ehci code sees the qh on yet another pass through the async list, looks in the async_queue, sees there already is a qh at address 0xdeadbeef there with async_state of EHCI_ASYNC_COMPLETED, and finished the transaction with the results of the *original* transaction. Not good (tm), this patch fixes this race by removing all qhs which have not been seen during the last cycle through the async list immidiately when the doorbell is rung. Note this patch does not fix any actually observed problem, but upon reading of the EHCI spec it became apparent to me that the above race could happen and the usb-ehci behavior from before this patch is not good. Signed-off-by: Hans de Goede --- hw/usb-ehci.c | 33 +++++++++++++++++---------------- 1 files changed, 17 insertions(+), 16 deletions(-) diff --git a/hw/usb-ehci.c b/hw/usb-ehci.c index d384fcc..b349003 100644 --- a/hw/usb-ehci.c +++ b/hw/usb-ehci.c @@ -697,7 +697,7 @@ static EHCIQueue *ehci_find_queue_by_qh(EHCIState *ehci, uint32_t addr, return NULL; } -static void ehci_queues_rip_unused(EHCIState *ehci, int async) +static void ehci_queues_rip_unused(EHCIState *ehci, int async, int flush) { EHCIQueueHead *head = async ? &ehci->aqueues : &ehci->pqueues; EHCIQueue *q, *tmp; @@ -708,7 +708,7 @@ static void ehci_queues_rip_unused(EHCIState *ehci, int async) q->ts = ehci->last_run_ns; continue; } - if (ehci->last_run_ns < q->ts + 250000000) { + if (!flush && ehci->last_run_ns < q->ts + 250000000) { /* allow 0.25 sec idle */ continue; } @@ -1537,7 +1537,7 @@ static int ehci_state_waitlisthead(EHCIState *ehci, int async) ehci_set_usbsts(ehci, USBSTS_REC); } - ehci_queues_rip_unused(ehci, async); + ehci_queues_rip_unused(ehci, async, 0); /* Find the head of the list (4.9.1.1) */ for(i = 0; i < MAX_QH; i++) { @@ -2093,18 +2093,7 @@ static void ehci_advance_async_state(EHCIState *ehci) break; } - /* If the doorbell is set, the guest wants to make a change to the - * schedule. The host controller needs to release cached data. - * (section 4.8.2) - */ - if (ehci->usbcmd & USBCMD_IAAD) { - DPRINTF("ASYNC: doorbell request acknowledged\n"); - ehci->usbcmd &= ~USBCMD_IAAD; - ehci_set_interrupt(ehci, USBSTS_IAA); - break; - } - - /* make sure guest has acknowledged */ + /* make sure guest has acknowledged the doorbell interrupt */ /* TO-DO: is this really needed? */ if (ehci->usbsts & USBSTS_IAA) { DPRINTF("IAA status bit still set.\n"); @@ -2118,6 +2107,18 @@ static void ehci_advance_async_state(EHCIState *ehci) ehci_set_state(ehci, async, EST_WAITLISTHEAD); ehci_advance_state(ehci, async); + + /* If the doorbell is set, the guest wants to make a change to the + * schedule. The host controller needs to release cached data. + * (section 4.8.2) + */ + if (ehci->usbcmd & USBCMD_IAAD) { + /* Remove all unseen qhs from the async qhs queue */ + ehci_queues_rip_unused(ehci, async, 1); + DPRINTF("ASYNC: doorbell request acknowledged\n"); + ehci->usbcmd &= ~USBCMD_IAAD; + ehci_set_interrupt(ehci, USBSTS_IAA); + } break; default: @@ -2167,7 +2168,7 @@ static void ehci_advance_periodic_state(EHCIState *ehci) ehci_set_fetch_addr(ehci, async,entry); ehci_set_state(ehci, async, EST_FETCHENTRY); ehci_advance_state(ehci, async); - ehci_queues_rip_unused(ehci, async); + ehci_queues_rip_unused(ehci, async, 0); break; default: