Date: Thu, 2 May 2024 17:54:04 +0100
From: Robin Murphy <robin.murphy@....com>
To: "T.J. Mercier" <tjmercier@...gle.com>
Cc: Christoph Hellwig <hch@...radead.org>, Joerg Roedel <joro@...tes.org>,
 Will Deacon <will@...nel.org>, Andrew Morton <akpm@...ux-foundation.org>,
 Catalin Marinas <catalin.marinas@....com>, isaacmanjarres@...gle.com,
 iommu@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] iommu/dma: Respect SWIOTLB force_bounce

On 02/05/2024 5:02 pm, T.J. Mercier wrote:
> On Thu, May 2, 2024 at 5:50 AM Robin Murphy <robin.murphy@....com> wrote:
>>
>> On 02/05/2024 6:07 am, Christoph Hellwig wrote:
>>> On Wed, May 01, 2024 at 08:13:18PM +0000, T.J. Mercier wrote:
>>>> iommu_dma_map_page and iommu_dma_map_sg conditionally use SWIOTLB, but
>>>> checking if force_bounce is set for the device is not part of that
>>>> condition. Check if devices have requested to force SWIOTLB use as part
>>>> of deciding to take the existing SWIOTLB paths.
>>>
>>> This fails to explain why you'd want this somewhat surprising behavior,
>>> and why you consider it a bug fix.
>>
>> Indeed, it's rather intentional that the "swiotlb=force" argument
>> doesn't affect iommu-dma, since that's primarily for weeding out drivers
>> making dodgy assumptions about DMA addresses, and iommu-dma is
>> inherently even better at that already.
>>
>> Beyond that I think this change also seems likely to interact badly with
>> CC_ATTR_GUEST_MEM_ENCRYPT on x86, where we invoke the SWIOTLB_FORCE flag
>> for dma-direct, but expect that an IOMMU can provide a decrypted view
>> in-place, thus bouncing in that path would be unnecessarily detrimental.
>>
>> Thanks,
>> Robin.
> 
> I encountered this while testing a change to DMA direct which makes
> sure that sg_dma_mark_swiotlb is called there like it is here. (Right
> now the SG_DMA_SWIOTLB flag is set only if dma_map_sgtable takes the
> IOMMU path, but not if SWIOTLB is used on the direct path.) While I
> agree IOMMU + force_bounce is an unusual config, I found it equally
> surprising that swiotlb=force wasn't doing what is advertised, or even
> giving a warning/error. Since the iommu-dma code is already set up for
> conditionally bouncing through SWIOTLB, it looked straightforward to
> give what's asked for in the case of swiotlb=force. If it's
> intentional that SWIOTLB options don't affect IOMMU code, then should
> we just warn about it here when it's ignored? The presence of a
> warning like that would also be a suggestion of, "you probably don't
> actually want what you're asking for with this configuration you've
> specified".

Traditionally, user-facing "SWIOTLB" refers to what is now dma-direct, 
in its context of bouncing to make DMA mappings accessible to devices 
with memory access limitations. The fact that the IOMMU implementations 
(originally Intel, now iommu-dma) also co-opted some of the SWIOTLB 
machinery for the very different purpose of isolation of memory 
*outside* non-page-aligned DMA mappings was always more of an internal 
implementation detail.

The newest use for enforcing non-coherent cacheline alignment blurs the 
boundary a little since its purpose is again largely orthogonal to those 
cases, however it's also one to which "swiotlb=force" is semantically 
kind of meaningless once you think about it (how does one forcibly align 
a buffer which is already suitably aligned?)

If there's any issue here I'd say it's only that the description in 
kernel-parameters.txt still hasn't been updated since "automatically 
used by the kernel" *did* solely imply a device DMA mask limitation.

Thanks,
Robin.

Powered by blists - more mailing lists