Date:   Wed, 26 Aug 2020 14:26:40 -0400
From:   Alex Deucher <alexdeucher@...il.com>
To:     Tom Murphy <murphyt7@....ie>
Cc:     Logan Gunthorpe <logang@...tatee.com>, kvm@...r.kernel.org,
        David Airlie <airlied@...ux.ie>,
        Maling list - DRI developers 
        <dri-devel@...ts.freedesktop.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        linux-tegra@...r.kernel.org, Julien Grall <julien.grall@....com>,
        Thierry Reding <thierry.reding@...il.com>,
        Will Deacon <will@...nel.org>,
        Marek Szyprowski <m.szyprowski@...sung.com>,
        Jean-Philippe Brucker <jean-philippe@...aro.org>,
        linux-samsung-soc@...r.kernel.org, Marc Zyngier <maz@...nel.org>,
        Krzysztof Kozlowski <krzk@...nel.org>,
        Jonathan Hunter <jonathanh@...dia.com>,
        Christoph Hellwig <hch@...radead.org>,
        linux-rockchip@...ts.infradead.org, Andy Gross <agross@...nel.org>,
        linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
        linux-s390@...r.kernel.org,
        linux-arm-msm <linux-arm-msm@...r.kernel.org>,
        Intel Graphics Development <intel-gfx@...ts.freedesktop.org>,
        Alex Williamson <alex.williamson@...hat.com>,
        linux-mediatek@...ts.infradead.org,
        Rodrigo Vivi <rodrigo.vivi@...el.com>,
        Matthias Brugger <matthias.bgg@...il.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        "open list:VIRTIO CORE, NET..." 
        <virtualization@...ts.linux-foundation.org>,
        Gerald Schaefer <gerald.schaefer@...ibm.com>,
        David Woodhouse <dwmw2@...radead.org>,
        Cornelia Huck <cohuck@...hat.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "list@....net:IOMMU DRIVERS <iommu@...ts.linux-foundation.org>, Joerg
        Roedel <joro@...tes.org>," <iommu@...ts.linux-foundation.org>,
        Kukjin Kim <kgene@...nel.org>,
        Robin Murphy <robin.murphy@....com>
Subject: Re: [PATCH 0/8] Convert the intel iommu driver to the dma-iommu api

On Mon, Aug 24, 2020 at 2:56 AM Tom Murphy <murphyt7@....ie> wrote:
>
> Hi Logan/All,
>
> I have added a check for the sg_dma_len == 0 :
> """
>  } __sgt_iter(struct scatterlist *sgl, bool dma) {
>         struct sgt_iter s = { .sgp = sgl };
>
> +       if (sgl && sg_dma_len(sgl) == 0)
> +           s.sgp = NULL;
>
>         if (s.sgp) {
>             .....
> """
> at location [1].
> but it doens't fix the problem.
>
> You're right though, this change does need to be made, this code
> doesn't handle pages of sg_dma_len(sg) == 0 correctly
> So my guess is that we have more bugs in other parts of the i915
> driver (or there is a problem with my "sg_dma_len == 0" fix above).
> I have been trying to spot where else the code might be buggy but I
> haven't had any luck so far.
>
> I'm doing a microconfernce (at LPC 2020) this wednesdays [1] on this
> if you're interested in attending.
> I'm hoping I can chat about it with a few people and find how can
> reproduce and fix this issues. I don't have any more time I can give
> to this unfortunately and it would be a shame for the work to go to
> waste.
>
> [0] https://github.com/torvalds/linux/blob/d012a7190fc1fd72ed48911e77ca97ba4521bccd/drivers/gpu/drm/i915/i915_scatterlist.h#L28
> [1] https://linuxplumbersconf.org/event/7/contributions/846/
>
> On Fri, 29 May 2020 at 22:21, Logan Gunthorpe <logang@...tatee.com> wrote:
> >
> >
> >
> > On 2020-05-29 3:11 p.m., Marek Szyprowski wrote:
> > > Patches are pending:
> > > https://lore.kernel.org/linux-iommu/20200513132114.6046-1-m.szyprowski@samsung.com/T/
> >
> > Cool, nice! Though, I still don't think that fixes the issue in
> > i915_scatterlist.h given it still ignores sg_dma_len() and strictly
> > relies on sg_next()/sg_is_last() to stop iterating -- and I suspect this
> > is the bug that got in Tom's way.
> >
> > >> However, as Robin pointed out, there are other ugly tricks like stopping
> > >> iterating through the SGL when sg_dma_len() is zero. For example, the
> > >> AMD driver appears to use drm_prime_sg_to_page_addr_arrays() which does
> > >> this trick and thus likely isn't buggy (otherwise, I'd expect someone to
> > >> have complained by now seeing AMD has already switched to IOMMU-DMA.

We ran into the same issue with amdgpu and radeon when the AMD IOMMU
driver was converted and had to fix it as well.  The relevant fixes
were:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=42e67b479eab6d26459b80b4867298232b0435e7
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=0199172f933342d8b1011aae2054a695c25726f4
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=47f7826c520ecd92ffbffe59ecaa2fe61e42ec70
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=c0f83d164fb8f3a2b7bc379a6c1e27d1123a9eab

Alex

> > >
> > > I'm not sure that this is a trick. Stopping at zero sg_dma_len() was
> > > somewhere documented.
> >
> > Well whatever you want to call it, it is ugly to have some drivers doing
> > one thing with the returned value and others assuming there's an extra
> > zero at the end. It just causes confusion for people reading/copying the
> > code. It would be better if they are all consistent. However, I concede
> > stopping at zero should not be broken, presently.
> >
> > Logan
> _______________________________________________
> dri-devel mailing list
> dri-devel@...ts.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Powered by blists - more mailing lists