From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:53797) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TVE9z-0008V3-Tv for qemu-devel@nongnu.org; Sun, 04 Nov 2012 23:19:41 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TVE9w-0005DA-Vi for qemu-devel@nongnu.org; Sun, 04 Nov 2012 23:19:39 -0500 Received: from e23smtp03.au.ibm.com ([202.81.31.145]:60964) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TVE9v-0005BQ-UJ for qemu-devel@nongnu.org; Sun, 04 Nov 2012 23:19:36 -0500 Received: from /spool/local by e23smtp03.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 5 Nov 2012 14:16:29 +1000 Received: from d23av02.au.ibm.com (d23av02.au.ibm.com [9.190.235.138]) by d23relay04.au.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id qA549EXR64028914 for ; Mon, 5 Nov 2012 15:09:15 +1100 Received: from d23av02.au.ibm.com (loopback [127.0.0.1]) by d23av02.au.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id qA54JQRi016190 for ; Mon, 5 Nov 2012 15:19:26 +1100 Date: Mon, 5 Nov 2012 15:20:48 +1100 From: David Gibson Message-ID: <20121105042048.GY27695@truffula.fritz.box> References: <1351823524-7149-1-git-send-email-david@gibson.dropbear.id.au> <509376CB.8000905@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <509376CB.8000905@redhat.com> Subject: Re: [Qemu-devel] [PATCH] xhci: Fix some DMA host endian bugs List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Gerd Hoffmann Cc: aliguori@us.ibm.com, qemu-devel@nongnu.org On Fri, Nov 02, 2012 at 08:31:23AM +0100, Gerd Hoffmann wrote: > Hi, > > > +static inline void xhci_dma_write_u32s(XHCIState *xhci, dma_addr_t addr, > > + uint32_t *buf, size_t len) > > +{ > > + int i; > > + > > + for (i = 0; i < (len / sizeof(uint32_t)); i++) { > > + buf[i] = cpu_to_le32(buf[i]); > > + } > > + pci_dma_write(&xhci->pci_dev, addr, buf, len); > > +} > > I think we should use a temporary buffer here, otherwise you leave the > values byteswapped in buf which likely has unwanted side effects. Here's an updated version that uses a temporary buffer. >>From 588a8f874c8d5a658ef95e35164e182a915091db Mon Sep 17 00:00:00 2001 From: David Gibson Date: Mon, 5 Nov 2012 14:29:01 +1100 Subject: [PATCH] xhci: Fix some DMA host endian bugs The xhci device does correct endian switches on the results of some DMAs but not all. In particular, there are many DMAs of what are essentially arrays of 32-bit integers which never get byteswapped. This causes them to be interpreted incorrectly on big-endian hosts, since (as per the xhci spec) these arrays are always little-endian in guest memory. This patch adds some helper functions to fix these bugs. This may not be all the endian bugs in the xhci code, but it's certainly some of them and the Linux guest xhci driver certainly gets further with these fixes. Signed-off-by: David Gibson --- v2: * Change xhci_dma_write_u32s to use a temporary buffer rather than altering the given data in place. --- hw/usb/hcd-xhci.c | 81 +++++++++++++++++++++++++++++++++++------------------ 1 file changed, 54 insertions(+), 27 deletions(-) diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c index 7b65741..1465685 100644 --- a/hw/usb/hcd-xhci.c +++ b/hw/usb/hcd-xhci.c @@ -607,6 +607,34 @@ static inline dma_addr_t xhci_mask64(uint64_t addr) } } +static inline void xhci_dma_read_u32s(XHCIState *xhci, dma_addr_t addr, + uint32_t *buf, size_t len) +{ + int i; + + assert((len % sizeof(uint32_t)) == 0); + + pci_dma_read(&xhci->pci_dev, addr, buf, len); + + for (i = 0; i < (len / sizeof(uint32_t)); i++) { + buf[i] = le32_to_cpu(buf[i]); + } +} + +static inline void xhci_dma_write_u32s(XHCIState *xhci, dma_addr_t addr, + uint32_t *buf, size_t len) +{ + int i; + uint32_t tmp[len / sizeof(uint32_t)]; + + assert((len % sizeof(uint32_t)) == 0); + + for (i = 0; i < (len / sizeof(uint32_t)); i++) { + tmp[i] = cpu_to_le32(buf[i]); + } + pci_dma_write(&xhci->pci_dev, addr, tmp, len); +} + static XHCIPort *xhci_lookup_port(XHCIState *xhci, struct USBPort *uport) { int index; @@ -1018,14 +1046,14 @@ static void xhci_set_ep_state(XHCIState *xhci, XHCIEPContext *epctx, { uint32_t ctx[5]; - pci_dma_read(&xhci->pci_dev, epctx->pctx, ctx, sizeof(ctx)); + xhci_dma_read_u32s(xhci, epctx->pctx, ctx, sizeof(ctx)); ctx[0] &= ~EP_STATE_MASK; ctx[0] |= state; ctx[2] = epctx->ring.dequeue | epctx->ring.ccs; ctx[3] = (epctx->ring.dequeue >> 16) >> 16; DPRINTF("xhci: set epctx: " DMA_ADDR_FMT " state=%d dequeue=%08x%08x\n", epctx->pctx, state, ctx[3], ctx[2]); - pci_dma_write(&xhci->pci_dev, epctx->pctx, ctx, sizeof(ctx)); + xhci_dma_write_u32s(xhci, epctx->pctx, ctx, sizeof(ctx)); epctx->state = state; } @@ -1857,14 +1885,14 @@ static TRBCCode xhci_address_slot(XHCIState *xhci, unsigned int slotid, assert(slotid >= 1 && slotid <= xhci->numslots); dcbaap = xhci_addr64(xhci->dcbaap_low, xhci->dcbaap_high); - pci_dma_read(&xhci->pci_dev, dcbaap + 8*slotid, &poctx, sizeof(poctx)); + poctx = ldq_le_pci_dma(&xhci->pci_dev, dcbaap + 8*slotid); ictx = xhci_mask64(pictx); - octx = xhci_mask64(le64_to_cpu(poctx)); + octx = xhci_mask64(poctx); DPRINTF("xhci: input context at "DMA_ADDR_FMT"\n", ictx); DPRINTF("xhci: output context at "DMA_ADDR_FMT"\n", octx); - pci_dma_read(&xhci->pci_dev, ictx, ictl_ctx, sizeof(ictl_ctx)); + xhci_dma_read_u32s(xhci, ictx, ictl_ctx, sizeof(ictl_ctx)); if (ictl_ctx[0] != 0x0 || ictl_ctx[1] != 0x3) { fprintf(stderr, "xhci: invalid input context control %08x %08x\n", @@ -1872,8 +1900,8 @@ static TRBCCode xhci_address_slot(XHCIState *xhci, unsigned int slotid, return CC_TRB_ERROR; } - pci_dma_read(&xhci->pci_dev, ictx+32, slot_ctx, sizeof(slot_ctx)); - pci_dma_read(&xhci->pci_dev, ictx+64, ep0_ctx, sizeof(ep0_ctx)); + xhci_dma_read_u32s(xhci, ictx+32, slot_ctx, sizeof(slot_ctx)); + xhci_dma_read_u32s(xhci, ictx+64, ep0_ctx, sizeof(ep0_ctx)); DPRINTF("xhci: input slot context: %08x %08x %08x %08x\n", slot_ctx[0], slot_ctx[1], slot_ctx[2], slot_ctx[3]); @@ -1923,8 +1951,8 @@ static TRBCCode xhci_address_slot(XHCIState *xhci, unsigned int slotid, DPRINTF("xhci: output ep0 context: %08x %08x %08x %08x %08x\n", ep0_ctx[0], ep0_ctx[1], ep0_ctx[2], ep0_ctx[3], ep0_ctx[4]); - pci_dma_write(&xhci->pci_dev, octx, slot_ctx, sizeof(slot_ctx)); - pci_dma_write(&xhci->pci_dev, octx+32, ep0_ctx, sizeof(ep0_ctx)); + xhci_dma_write_u32s(xhci, octx, slot_ctx, sizeof(slot_ctx)); + xhci_dma_write_u32s(xhci, octx+32, ep0_ctx, sizeof(ep0_ctx)); return res; } @@ -1957,17 +1985,17 @@ static TRBCCode xhci_configure_slot(XHCIState *xhci, unsigned int slotid, } } - pci_dma_read(&xhci->pci_dev, octx, slot_ctx, sizeof(slot_ctx)); + xhci_dma_read_u32s(xhci, octx, slot_ctx, sizeof(slot_ctx)); slot_ctx[3] &= ~(SLOT_STATE_MASK << SLOT_STATE_SHIFT); slot_ctx[3] |= SLOT_ADDRESSED << SLOT_STATE_SHIFT; DPRINTF("xhci: output slot context: %08x %08x %08x %08x\n", slot_ctx[0], slot_ctx[1], slot_ctx[2], slot_ctx[3]); - pci_dma_write(&xhci->pci_dev, octx, slot_ctx, sizeof(slot_ctx)); + xhci_dma_write_u32s(xhci, octx, slot_ctx, sizeof(slot_ctx)); return CC_SUCCESS; } - pci_dma_read(&xhci->pci_dev, ictx, ictl_ctx, sizeof(ictl_ctx)); + xhci_dma_read_u32s(xhci, ictx, ictl_ctx, sizeof(ictl_ctx)); if ((ictl_ctx[0] & 0x3) != 0x0 || (ictl_ctx[1] & 0x3) != 0x1) { fprintf(stderr, "xhci: invalid input context control %08x %08x\n", @@ -1975,8 +2003,8 @@ static TRBCCode xhci_configure_slot(XHCIState *xhci, unsigned int slotid, return CC_TRB_ERROR; } - pci_dma_read(&xhci->pci_dev, ictx+32, islot_ctx, sizeof(islot_ctx)); - pci_dma_read(&xhci->pci_dev, octx, slot_ctx, sizeof(slot_ctx)); + xhci_dma_read_u32s(xhci, ictx+32, islot_ctx, sizeof(islot_ctx)); + xhci_dma_read_u32s(xhci, octx, slot_ctx, sizeof(slot_ctx)); if (SLOT_STATE(slot_ctx[3]) < SLOT_ADDRESSED) { fprintf(stderr, "xhci: invalid slot state %08x\n", slot_ctx[3]); @@ -1988,8 +2016,7 @@ static TRBCCode xhci_configure_slot(XHCIState *xhci, unsigned int slotid, xhci_disable_ep(xhci, slotid, i); } if (ictl_ctx[1] & (1<pci_dev, ictx+32+(32*i), ep_ctx, - sizeof(ep_ctx)); + xhci_dma_read_u32s(xhci, ictx+32+(32*i), ep_ctx, sizeof(ep_ctx)); DPRINTF("xhci: input ep%d.%d context: %08x %08x %08x %08x %08x\n", i/2, i%2, ep_ctx[0], ep_ctx[1], ep_ctx[2], ep_ctx[3], ep_ctx[4]); @@ -2001,7 +2028,7 @@ static TRBCCode xhci_configure_slot(XHCIState *xhci, unsigned int slotid, DPRINTF("xhci: output ep%d.%d context: %08x %08x %08x %08x %08x\n", i/2, i%2, ep_ctx[0], ep_ctx[1], ep_ctx[2], ep_ctx[3], ep_ctx[4]); - pci_dma_write(&xhci->pci_dev, octx+(32*i), ep_ctx, sizeof(ep_ctx)); + xhci_dma_write_u32s(xhci, octx+(32*i), ep_ctx, sizeof(ep_ctx)); } } @@ -2013,7 +2040,7 @@ static TRBCCode xhci_configure_slot(XHCIState *xhci, unsigned int slotid, DPRINTF("xhci: output slot context: %08x %08x %08x %08x\n", slot_ctx[0], slot_ctx[1], slot_ctx[2], slot_ctx[3]); - pci_dma_write(&xhci->pci_dev, octx, slot_ctx, sizeof(slot_ctx)); + xhci_dma_write_u32s(xhci, octx, slot_ctx, sizeof(slot_ctx)); return CC_SUCCESS; } @@ -2038,7 +2065,7 @@ static TRBCCode xhci_evaluate_slot(XHCIState *xhci, unsigned int slotid, DPRINTF("xhci: input context at "DMA_ADDR_FMT"\n", ictx); DPRINTF("xhci: output context at "DMA_ADDR_FMT"\n", octx); - pci_dma_read(&xhci->pci_dev, ictx, ictl_ctx, sizeof(ictl_ctx)); + xhci_dma_read_u32s(xhci, ictx, ictl_ctx, sizeof(ictl_ctx)); if (ictl_ctx[0] != 0x0 || ictl_ctx[1] & ~0x3) { fprintf(stderr, "xhci: invalid input context control %08x %08x\n", @@ -2047,12 +2074,12 @@ static TRBCCode xhci_evaluate_slot(XHCIState *xhci, unsigned int slotid, } if (ictl_ctx[1] & 0x1) { - pci_dma_read(&xhci->pci_dev, ictx+32, islot_ctx, sizeof(islot_ctx)); + xhci_dma_read_u32s(xhci, ictx+32, islot_ctx, sizeof(islot_ctx)); DPRINTF("xhci: input slot context: %08x %08x %08x %08x\n", islot_ctx[0], islot_ctx[1], islot_ctx[2], islot_ctx[3]); - pci_dma_read(&xhci->pci_dev, octx, slot_ctx, sizeof(slot_ctx)); + xhci_dma_read_u32s(xhci, octx, slot_ctx, sizeof(slot_ctx)); slot_ctx[1] &= ~0xFFFF; /* max exit latency */ slot_ctx[1] |= islot_ctx[1] & 0xFFFF; @@ -2062,17 +2089,17 @@ static TRBCCode xhci_evaluate_slot(XHCIState *xhci, unsigned int slotid, DPRINTF("xhci: output slot context: %08x %08x %08x %08x\n", slot_ctx[0], slot_ctx[1], slot_ctx[2], slot_ctx[3]); - pci_dma_write(&xhci->pci_dev, octx, slot_ctx, sizeof(slot_ctx)); + xhci_dma_write_u32s(xhci, octx, slot_ctx, sizeof(slot_ctx)); } if (ictl_ctx[1] & 0x2) { - pci_dma_read(&xhci->pci_dev, ictx+64, iep0_ctx, sizeof(iep0_ctx)); + xhci_dma_read_u32s(xhci, ictx+64, iep0_ctx, sizeof(iep0_ctx)); DPRINTF("xhci: input ep0 context: %08x %08x %08x %08x %08x\n", iep0_ctx[0], iep0_ctx[1], iep0_ctx[2], iep0_ctx[3], iep0_ctx[4]); - pci_dma_read(&xhci->pci_dev, octx+32, ep0_ctx, sizeof(ep0_ctx)); + xhci_dma_read_u32s(xhci, octx+32, ep0_ctx, sizeof(ep0_ctx)); ep0_ctx[1] &= ~0xFFFF0000; /* max packet size*/ ep0_ctx[1] |= iep0_ctx[1] & 0xFFFF0000; @@ -2080,7 +2107,7 @@ static TRBCCode xhci_evaluate_slot(XHCIState *xhci, unsigned int slotid, DPRINTF("xhci: output ep0 context: %08x %08x %08x %08x %08x\n", ep0_ctx[0], ep0_ctx[1], ep0_ctx[2], ep0_ctx[3], ep0_ctx[4]); - pci_dma_write(&xhci->pci_dev, octx+32, ep0_ctx, sizeof(ep0_ctx)); + xhci_dma_write_u32s(xhci, octx+32, ep0_ctx, sizeof(ep0_ctx)); } return CC_SUCCESS; @@ -2105,12 +2132,12 @@ static TRBCCode xhci_reset_slot(XHCIState *xhci, unsigned int slotid) } } - pci_dma_read(&xhci->pci_dev, octx, slot_ctx, sizeof(slot_ctx)); + xhci_dma_read_u32s(xhci, octx, slot_ctx, sizeof(slot_ctx)); slot_ctx[3] &= ~(SLOT_STATE_MASK << SLOT_STATE_SHIFT); slot_ctx[3] |= SLOT_DEFAULT << SLOT_STATE_SHIFT; DPRINTF("xhci: output slot context: %08x %08x %08x %08x\n", slot_ctx[0], slot_ctx[1], slot_ctx[2], slot_ctx[3]); - pci_dma_write(&xhci->pci_dev, octx, slot_ctx, sizeof(slot_ctx)); + xhci_dma_write_u32s(xhci, octx, slot_ctx, sizeof(slot_ctx)); return CC_SUCCESS; } -- 1.7.10.4 -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson