linux-kernel - Re: [PATCH v5] drm/i915: stop using swiotlb

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <db9f787e-c3e4-d353-da57-80cb7a135d86@linux.intel.com>
Date:   Tue, 9 Aug 2022 12:36:50 +0100
From:   Tvrtko Ursulin <tvrtko.ursulin@...ux.intel.com>
To:     "Hellstrom, Thomas" <thomas.hellstrom@...el.com>,
        "daniel@...ll.ch" <daniel@...ll.ch>,
        "joonas.lahtinen@...ux.intel.com" <joonas.lahtinen@...ux.intel.com>,
        "Vivi, Rodrigo" <rodrigo.vivi@...el.com>,
        "bob.beckett@...labora.com" <bob.beckett@...labora.com>,
        "jani.nikula@...ux.intel.com" <jani.nikula@...ux.intel.com>,
        "airlied@...ux.ie" <airlied@...ux.ie>
Cc:     "hch@....de" <hch@....de>,
        "Ursulin, Tvrtko" <tvrtko.ursulin@...el.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
        "kernel@...labora.com" <kernel@...labora.com>,
        "Auld, Matthew" <matthew.auld@...el.com>,
        "intel-gfx@...ts.freedesktop.org" <intel-gfx@...ts.freedesktop.org>
Subject: Re: [PATCH v5] drm/i915: stop using swiotlb


On 08/08/2022 16:48, Hellstrom, Thomas wrote:
> Hi, [back from vacation]
> 
> On Tue, 2022-07-26 at 16:39 +0100, Robert Beckett wrote:
>> Calling swiotlb functions directly is nowadays considered harmful.
>> See
>> https://lore.kernel.org/intel-gfx/20220711082614.GA29487@lst.de/
>>
>> Replace swiotlb_max_segment() calls with dma_max_mapping_size().
>> In i915_gem_object_get_pages_internal() no longer consider
>> max_segment
>> only if CONFIG_SWIOTLB is enabled. There can be other (iommu related)
>> causes of specific max segment sizes.
>>
>> Cc: Christoph Hellwig <hch@....de>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin@...ux.intel.com>
>> Cc: Thomas Hellstrom <thomas.hellstrom@...el.com>
>> Cc: Matthew Auld <matthew.auld@...el.com>
>>
>> v2: - restore UINT_MAX clamp in i915_sg_segment_size()
>>      - drop PAGE_SIZE check as it will always be >= PAGE_SIZE
>> v3: - actually clamp to UINT_MAX in i915_sg_segment_size()
>> v4: - round down max segment size to PAGE_SIZE
>> v5: - fix checkpatch whitespace issue
>>
>> Reviewed-by: Christoph Hellwig <hch@....de>
>> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@...el.com>
>> Signed-off-by: Robert Beckett <bob.beckett@...labora.com>
> 
> Hmm,
> 
> This whole thing looks a bit strange to me since with SWIOTLB actually
> used for i915, the driver should malfunction anyway as it doesn't do
> any dma_sync_sg_for_cpu() or dma_sync_sg_for_device(), and the driver
> assumes all coherent dma. Is that SWIOTLB=force kernel option still
> available?

Don't know about these - but pretty sure in the past we had i915 break 
if we did not respect swiotlb_max_segment.

Digging through git history at least running as Xen dom0 looks to have 
been impacted, but commits such as abb0deacb5a6 ("drm/i915: Fallback to 
single PAGE_SIZE segments for DMA remapping") are older and suggest 
problem was generic. 1625e7e549c5 ("drm/i915: make compact dma scatter 
lists creation work with SWIOTLB backend.") as well. So it looks it did 
work behind swiotlb despite those missing calls you highlighted.

> Also, correct me if I'm wrong, but the original driver segment size
> appears to mean "the largest contiguous area that can be handled either
> by the device or the dma mapping layer" rather than the total space
> available for dma mappings? Not completely sure what
> dma_max_mapping_size() is returning, though?

AFAIU looks to be compatible on paper at least.:

dma_max_mapping_size -> "Returns the maximum size of a mapping for the 
device."

So an individual mapping.

But then in case of swiotlb is implemented in swiotlb_max_mapping_size, 
and not the same code as swiotlb_max_segment. I agree, ideally if 
someone could clarify they are returning the same thing or there is a 
miss somewhere.

Regards,

Tvrtko