From patchwork Wed Mar 9 07:50:13 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Magnus Damm X-Patchwork-Id: 8543801 Return-Path: X-Original-To: patchwork-linux-arm@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork2.web.kernel.org (Postfix) with ESMTP id AF4E1C0553 for ; Wed, 9 Mar 2016 07:52:05 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id BD24E201FA for ; Wed, 9 Mar 2016 07:52:04 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.9]) (using TLSv1.2 with cipher AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id C09B02015E for ; Wed, 9 Mar 2016 07:52:03 +0000 (UTC) Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1adYtC-0000Y3-8N; Wed, 09 Mar 2016 07:50:38 +0000 Received: from mail-vk0-x233.google.com ([2607:f8b0:400c:c05::233]) by bombadil.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1adYt8-0000N2-Mf for linux-arm-kernel@lists.infradead.org; Wed, 09 Mar 2016 07:50:35 +0000 Received: by mail-vk0-x233.google.com with SMTP id k1so46592622vkb.0 for ; Tue, 08 Mar 2016 23:50:13 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc; bh=UNl4W3Q5A1Yndv7rl0R8Xb6JbWTr3JIadp8EW3tjhgU=; b=uICQK/ENLmYWhBJVHupzM2DWcDmbF/T/U/n8eU6cocN1xTIXA1yF6o4lVW54PF7mmh /AknEp9rJGu5lFab9J8lYMTCCMWrprQLIN1BmuCAH23ipsZhStN3bInrUSEELHvbqhur zqflHjN1elIcLS2GnGQ6/qc/dy2PUZpmcv4ki7G7guB5m4zhCBUqkXQ996X744zewP6c EEsc80jA+M2fIorsqaW3Da/6Yu1K9r50AKyFtB0UAAMxYep57WeEc3a3R351pYuh5pJu 2pUk7iCi7dxoCL8Ulc9/hj2Vqv6CFyNmoKA21lkLNqBt5JBQYVZJFNkZtqJOzglxvokh uIUw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc; bh=UNl4W3Q5A1Yndv7rl0R8Xb6JbWTr3JIadp8EW3tjhgU=; b=Ag4y4op54ASyzpV+jEhb2S34JJPnDuY87LRDLZF375GsIc4nAUOD22jAm555x4+eFv vxwm4MI1QjksacB0nJ076JmR393bhWG5faX3Et8ZbozPdc7EGeEhyDl23Q2SaGpmbl1W R3a7BhGLJ8InGDi/8pXotTMaiKXo0Na3CmFXULzDq1usbTOe++1zyK+yhBtCxAVhFiBR LmDsWcEEb1ZO+xGKSMhe4hC+za8tDLUB9CycD2DVbHXqloxWbUbVHDn7Htw5E7JtoWNx PGX925jk07+T5Qy4MeCHpSBkSB4uL/rgbk3aT0fhm0zJjxvddVOz39ax1LguMxdZPK5M A19Q== X-Gm-Message-State: AD7BkJJtidIzBb0p/jn1I6eVt3nL+pwOUEJiXcaJKG/DKg2RBcfxS+F+zFJhrvI2+8r+cxCNzv6d5AP7PAWKaA== MIME-Version: 1.0 X-Received: by 10.31.171.143 with SMTP id u137mr31189257vke.88.1457509813157; Tue, 08 Mar 2016 23:50:13 -0800 (PST) Received: by 10.103.91.130 with HTTP; Tue, 8 Mar 2016 23:50:13 -0800 (PST) In-Reply-To: <4812a34857b081e45c36d7e887840f3675da74dc.1450457730.git.robin.murphy@arm.com> References: <9a84191ed813c23db7901f8c73f514d081495bdf.1450457730.git.robin.murphy@arm.com> <4812a34857b081e45c36d7e887840f3675da74dc.1450457730.git.robin.murphy@arm.com> Date: Wed, 9 Mar 2016 16:50:13 +0900 Message-ID: Subject: Re: [PATCH 2/3] iommu/dma: Use correct offset in map_sg From: Magnus Damm To: Robin Murphy X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20160308_235034_897168_896F133E X-CRM114-Status: GOOD ( 21.47 ) X-Spam-Score: -2.7 (--) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: joro , iommu@lists.linux-foundation.org, "linux-arm-kernel@lists.infradead.org" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Spam-Status: No, score=-4.1 required=5.0 tests=BAYES_00, DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED, FREEMAIL_FROM, RCVD_IN_DNSWL_MED, RP_MATCHES_RCVD, T_DKIM_INVALID, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Sat, Dec 19, 2015 at 2:01 AM, Robin Murphy wrote: > When mapping a non-page-aligned scatterlist entry, we copy the original > offset to the output DMA address before aligning it to hand off to > iommu_map_sg(), then later adding the IOVA page address portion to get > the final mapped address. However, when the IOVA page size is smaller > than the CPU page size, it is the offset within the IOVA page we want, > not that within the CPU page, which can easily be larger than an IOVA > page and thus result in an incorrect final address. > > Fix the bug by taking only the IOVA-aligned part of the offset as the > basis of the DMA address, not the whole thing. > > Signed-off-by: Robin Murphy > --- > drivers/iommu/dma-iommu.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c > index 982e716..03811e3 100644 > --- a/drivers/iommu/dma-iommu.c > +++ b/drivers/iommu/dma-iommu.c > @@ -458,7 +458,7 @@ int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg, > size_t s_length = s->length; > size_t pad_len = (mask - iova_len + 1) & mask; > > - sg_dma_address(s) = s->offset; > + sg_dma_address(s) = s_offset; > sg_dma_len(s) = s_length; > s->offset -= s_offset; > s_length = iova_align(iovad, s_length + s_offset); > -- > 1.9.1 Hi Robin, Thanks a lot for your fix! While I don't have any doubt that your patch fixes a real issue I wonder if another update is needed. Depending on what is expected perhaps just the comment above the code wants an update or maybe the "un-swizzling" needs more work. With this patch applied the code looks semi-complete to me at this point. Currently the comment just above the hunk says: /* * Work out how much IOVA space we need, and align the segments to * IOVA granules for the IOMMU driver to handle. With some clever * trickery we can modify the list in-place, but reversibly, by * hiding the original data in the as-yet-unused DMA fields. */ With your fix the "original data" is no longer stored in the unused DMA fields. Instead the s_offset value is stored as modified in sg_dma_address() which in turn will make the iommu_dma_map_sg() function return with modified sg->s_offset both on success and failure. Perhaps this is intentional design, or maybe __invalidate_sg() and __finalize_sg() both need to support roll back? Any ideas? Thanks, / magnus My untested hack to support roll back on top of next-20160308 does something like this... --- 0001/drivers/iommu/dma-iommu.c +++ work/drivers/iommu/dma-iommu.c 2016-03-09 16:33:21.250513000 +0900 @@ -392,7 +392,7 @@ void iommu_dma_unmap_page(struct device * Handling IOVA concatenation can come later, if needed */ static int __finalise_sg(struct device *dev, struct scatterlist *sg, int nents, - dma_addr_t dma_addr) + dma_addr_t dma_addr, struct iova_domain *iovad) { struct scatterlist *s; int i; @@ -405,7 +405,7 @@ static int __finalise_sg(struct device * s->offset = s_offset; s->length = s_length; - sg_dma_address(s) = dma_addr + s_offset; + sg_dma_address(s) = dma_addr + iova_offset(iovad, s_offset); dma_addr += s_dma_len; } return i; @@ -455,11 +455,13 @@ int iommu_dma_map_sg(struct device *dev, * hiding the original data in the as-yet-unused DMA fields. */ for_each_sg(sg, s, nents, i) { - size_t s_offset = iova_offset(iovad, s->offset); + size_t s_offset = s->offset; size_t s_length = s->length; sg_dma_address(s) = s_offset; sg_dma_len(s) = s_length; + + s_offset = iova_offset(iovad, s_offset); s->offset -= s_offset; s_length = iova_align(iovad, s_length + s_offset); s->length = s_length; @@ -494,7 +496,7 @@ int iommu_dma_map_sg(struct device *dev, if (iommu_map_sg(domain, dma_addr, sg, nents, prot) < iova_len) goto out_free_iova; - return __finalise_sg(dev, sg, nents, dma_addr); + return __finalise_sg(dev, sg, nents, dma_addr, iovad); out_free_iova: __free_iova(iovad, iova);