[PATCH] iommu/dma: Respect SWIOTLB force_bounce

[PATCH] iommu/dma: Respect SWIOTLB force_bounce

[T.J. Mercier]

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.

Fixes: 861370f49ce4 ("iommu/dma: force bouncing if the size is not cacheline-aligned")
Signed-off-by: T.J. Mercier <tjmercier@google.com>
---
 drivers/iommu/dma-iommu.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index e4cb26f6a943..755d66b49dff 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -606,7 +606,8 @@ static bool dev_use_swiotlb(struct device *dev, size_t size,
 {
 	return IS_ENABLED(CONFIG_SWIOTLB) &&
 		(dev_is_untrusted(dev) ||
-		 dma_kmalloc_needs_bounce(dev, size, dir));
+		 dma_kmalloc_needs_bounce(dev, size, dir) ||
+		 is_swiotlb_force_bounce(dev));
 }
 
 static bool dev_use_sg_swiotlb(struct device *dev, struct scatterlist *sg,
@@ -632,7 +633,7 @@ static bool dev_use_sg_swiotlb(struct device *dev, struct scatterlist *sg,
 				return true;
 	}
 
-	return false;
+	return is_swiotlb_force_bounce(dev);
 }
 
 /**
-- 
2.45.0.rc0.197.gbae5840b3b-goog

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

[Christoph Hellwig]

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.

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

[Robin Murphy]

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.

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

[T.J. Mercier]

On Thu, May 2, 2024 at 5:50 AM Robin Murphy <robin.murphy@arm.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".

Thanks,
T.J.

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@arm.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.

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

[T.J. Mercier]

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

Ok I think clarifying SWIOTLB as it applies to dma-direct (and not any
of the other uses you've mentioned above) would do it. This?

diff --git a/Documentation/admin-guide/kernel-parameters.txt
b/Documentation/admin-guide/kernel-parameters.txt
index 213d0719e2b7..84c582ac246c 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -6486,6 +6486,7 @@
                                 to a power of 2.
                        force -- force using of bounce buffers even if they
                                 wouldn't be automatically used by the kernel
+                                where a hardware IOMMU is not involved
                        noforce -- Never use bounce buffers (for debugging)

        switches=       [HW,M68k,EARLY]
diff --git a/Documentation/arch/x86/x86_64/boot-options.rst
b/Documentation/arch/x86/x86_64/boot-options.rst
index 137432d34109..066b4bc81583 100644
--- a/Documentation/arch/x86/x86_64/boot-options.rst
+++ b/Documentation/arch/x86/x86_64/boot-options.rst
@@ -285,7 +285,7 @@ iommu options only relevant to the AMD GART hardware IOMMU:
       Always panic when IOMMU overflows.

 iommu options only relevant to the software bounce buffering (SWIOTLB) IOMMU
-implementation:
+implementation where a hardware IOMMU is not involved:

     swiotlb=<slots>[,force,noforce]
       <slots>