From patchwork Tue Feb 28 03:09:17 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: David Gibson X-Patchwork-Id: 143316 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 D98D8B6FA2 for ; Tue, 28 Feb 2012 14:10:02 +1100 (EST) Received: from localhost ([::1]:40022 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S2DRv-00050x-3R for incoming@patchwork.ozlabs.org; Mon, 27 Feb 2012 22:09:59 -0500 Received: from eggs.gnu.org ([208.118.235.92]:49386) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S2DRm-00050h-7P for qemu-devel@nongnu.org; Mon, 27 Feb 2012 22:09:52 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1S2DRk-00079J-3D for qemu-devel@nongnu.org; Mon, 27 Feb 2012 22:09:49 -0500 Received: from e23smtp05.au.ibm.com ([202.81.31.147]:59073) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S2DRi-000788-Jx for qemu-devel@nongnu.org; Mon, 27 Feb 2012 22:09:48 -0500 Received: from /spool/local by e23smtp05.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 28 Feb 2012 03:05:44 +1000 Received: from d23relay03.au.ibm.com (202.81.31.245) by e23smtp05.au.ibm.com (202.81.31.211) with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted; Tue, 28 Feb 2012 03:05:43 +1000 Received: from d23av01.au.ibm.com (d23av01.au.ibm.com [9.190.234.96]) by d23relay03.au.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id q1S39NV31490954 for ; Tue, 28 Feb 2012 14:09:25 +1100 Received: from d23av01.au.ibm.com (loopback [127.0.0.1]) by d23av01.au.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id q1S39MLE004077 for ; Tue, 28 Feb 2012 14:09:23 +1100 Received: from ozlabs.au.ibm.com (ozlabs.au.ibm.com [9.190.163.12]) by d23av01.au.ibm.com (8.14.4/8.13.1/NCO v10.0 AVin) with ESMTP id q1S39MpR004071; Tue, 28 Feb 2012 14:09:22 +1100 Received: by ozlabs.au.ibm.com (Postfix, from userid 1010) id 6F0FC73A0A; Tue, 28 Feb 2012 14:09:22 +1100 (EST) Date: Tue, 28 Feb 2012 14:09:17 +1100 From: David Gibson To: Gerd Hoffmann Message-ID: <20120228030917.GL3433@truffala.fritz.box> Mail-Followup-To: Gerd Hoffmann , anthony@codemonkey.ws, qemu-devel@nongnu.org, Wei Yang References: <1330043012-30556-1-git-send-email-david@gibson.dropbear.id.au> <1330043012-30556-4-git-send-email-david@gibson.dropbear.id.au> <4F4B8F6E.6010703@redhat.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <4F4B8F6E.6010703@redhat.com> User-Agent: Mutt/1.5.21 (2010-09-15) x-cbid: 12022717-1396-0000-0000-000000BCB3E9 X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 202.81.31.147 Cc: Wei Yang , qemu-devel@nongnu.org, anthony@codemonkey.ws Subject: Re: [Qemu-devel] [PATCH 3/6] USB OHCI bug fixes 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 On Mon, Feb 27, 2012 at 03:13:02PM +0100, Gerd Hoffmann wrote: > On 02/24/12 01:23, David Gibson wrote: > > From: Wei Yang > > > > This patch fixes two bugs in the OHCI device where the device writes > > back data to system memory that should be exclusively under the > > control of the guest side driver. > > Looks good. Fails checkpatch though. Oops, missed one of the overlong lines. checkpatch clean revised version below. > What is your merge plan btw? Wanna pick me this up for the usb queue? > Or do you need just my ack? If you could put it in your queue that would be great. From a4c996ba5e71dfd9f84abcf9365693229f707213 Mon Sep 17 00:00:00 2001 From: Wei Yang Date: Wed, 22 Feb 2012 12:02:06 +1100 Subject: [PATCH] USB OHCI bug fixes This patch fixes two bugs in the OHCI device where the device writes back data to system memory that should be exclusively under the control of the guest side driver. In OHCI specification Section 5.2.7, it mentioned "In all cases, Host Controller Driver is responsible for the insertion and removal of all Endpoint Descriptors in the various Host Controller Endpoint Descriptor lists". In the ohci_frame_boundary(), ohci_put_hcca() writes the entire hcca back including the interrupt ED lists which should be under driver control. This violates the specification and can race with a host driver updating that list at the same time. In the OHCI Spec Section 4.6, Transfer Descriptor Queue Processing, it mentioned "Since the TD pointed to by TailP is not accessed by the HC, the Host Controller Driver can initialize that TD and link at least one other to it without creating a coherency or synchronization problem". While the function ohci_put_ed() writes the entire endpoint descriptor back including the TailP which should under driver control. This violate the specification and can race with a host driver updating the TD list at the same time. In each case the solution is to make sure we don't write data which is under driver control. Cc: Gerd Hoffman Signed-off-by: Wei Yang Signed-off-by: David Gibson --- hw/usb-ohci.c | 18 ++++++++++++++++-- 1 files changed, 16 insertions(+), 2 deletions(-) diff --git a/hw/usb-ohci.c b/hw/usb-ohci.c index 7aa19fe..a4b4ef9 100644 --- a/hw/usb-ohci.c +++ b/hw/usb-ohci.c @@ -122,6 +122,8 @@ struct ohci_hcca { uint16_t frame, pad; uint32_t done; }; +#define HCCA_WRITEBACK_OFFSET offsetof(struct ohci_hcca, frame) +#define HCCA_WRITEBACK_SIZE 8 /* frame, pad, done */ static void ohci_bus_stop(OHCIState *ohci); static void ohci_async_cancel_device(OHCIState *ohci, USBDevice *dev); @@ -189,6 +191,10 @@ struct ohci_ed { uint32_t head; uint32_t next; }; +#define ED_TAILP_OFFSET offsetof(struct ohci_ed, tail) +#define ED_PART1_SIZE ED_TAILP_OFFSET +#define ED_HEADP_OFFSET offsetof(struct ohci_ed, head) +#define ED_PART2_SIZE (sizeof(struct ohci_ed) - ED_HEADP_OFFSET) /* General transfer descriptor */ struct ohci_td { @@ -569,7 +575,13 @@ static inline int ohci_read_hcca(OHCIState *ohci, static inline int ohci_put_ed(OHCIState *ohci, uint32_t addr, struct ohci_ed *ed) { - return put_dwords(ohci, addr, (uint32_t *)ed, sizeof(*ed) >> 2); + /* ed->tail is under control of the HCD, so we need to split + * the write back into two parts + */ + put_dwords(ohci, addr, (uint32_t *)ed, ED_PART1_SIZE >> 2); + return put_dwords(ohci, addr + ED_HEADP_OFFSET, + (uint32_t *)((char *)ed + ED_HEADP_OFFSET), + ED_PART2_SIZE >> 2); } static inline int ohci_put_td(OHCIState *ohci, @@ -588,7 +600,9 @@ static inline int ohci_put_iso_td(OHCIState *ohci, static inline int ohci_put_hcca(OHCIState *ohci, uint32_t addr, struct ohci_hcca *hcca) { - cpu_physical_memory_write(addr + ohci->localmem_base, hcca, sizeof(*hcca)); + cpu_physical_memory_write(addr + ohci->localmem_base + HCCA_WRITEBACK_OFFSET, + (char *)hcca + HCCA_WRITEBACK_OFFSET, + HCCA_WRITEBACK_SIZE); return 1; }