Re: [PATCH] iommu/dma: Respect SWIOTLB force_bounce

[Robin Murphy]

On 02/05/2024 5:02 pm, T.J. Mercier wrote:
> On Thu, May 2, 2024 at 5:50 AM Robin Murphy <robin.murphy@xxxxxxx> 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.