[PATCH v2 1/3] doc: swiotlb: iommu/dma: Clarify swiotlb=force option applies only to dma-direct

[PATCH v2 1/3] doc: swiotlb: iommu/dma: Clarify swiotlb=force option applies only to dma-direct

[T.J. Mercier]

IOMMU implementations now sometimes bounce memory through SWIOTLB to
achieve cacheline alignment [1], or prevent DMA attacks by untrusted
devices [2]. These uses of SWIOTLB differ conceptually from historical
use which was a solution to the problem of device addressing
limitations that prevent DMA to some portion of system memory
(typically beyond 4 GiB). IOMMUs also solve the problem of device
addressing limitations and therefore eliminate the need for SWIOTLB for
that purpose. However as mentioned, IOMMUs can use SWIOTLB for other
purposes.

The swiotlb=force kernel command line parameter does not impact IOMMU
related use of SWIOTLB, and that is intentional. IOMMUs cannot be forced
to use SWIOTLB for all buffers. Update the documentation for the swiotlb
parameter to clarify that SWIOTLB use can only be forced in scenarios
where an IOMMU is not involved.

[1] https://lore.kernel.org/all/20230612153201.554742-16-catalin.marinas@arm.com
[2] https://lore.kernel.org/all/20190906061452.30791-1-baolu.lu@linux.intel.com/
Signed-off-by: T.J. Mercier <tjmercier@google.com>
---
 Documentation/admin-guide/kernel-parameters.txt | 1 +
 Documentation/arch/x86/x86_64/boot-options.rst  | 3 +++
 2 files changed, 4 insertions(+)

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..a37139d1752f 100644
--- a/Documentation/arch/x86/x86_64/boot-options.rst
+++ b/Documentation/arch/x86/x86_64/boot-options.rst
@@ -292,6 +292,9 @@ implementation:
         Prereserve that many 2K slots for the software IO bounce buffering.
       force
         Force all IO through the software TLB.
+        Hardware IOMMU implementations can use SWIOTLB bounce buffering in
+        some circumstances, but they cannot be forced to always use them, so
+        this option only has an effect when no hardware IOMMU is involved.
       noforce
         Do not initialize the software TLB.
 
-- 
2.45.0.rc1.225.g2a3ae87e7f-goog

[PATCH v2 2/3] doc: swiotlb: Document SWIOTLB areas parameter

[T.J. Mercier]

Commit 20347fca71a3 ("swiotlb: split up the global swiotlb lock") added
the ability to specify the number of SWIOTLB areas, but boot-options.rst
was not updated as part of that commit.

Reported-by: Michael Kelley <mhklinux@outlook.com>
Fixes: 20347fca71a3 ("swiotlb: split up the global swiotlb lock")
Signed-off-by: T.J. Mercier <tjmercier@google.com>
---
 Documentation/arch/x86/x86_64/boot-options.rst | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/arch/x86/x86_64/boot-options.rst b/Documentation/arch/x86/x86_64/boot-options.rst
index a37139d1752f..18161657b301 100644
--- a/Documentation/arch/x86/x86_64/boot-options.rst
+++ b/Documentation/arch/x86/x86_64/boot-options.rst
@@ -287,9 +287,11 @@ iommu options only relevant to the AMD GART hardware IOMMU:
 iommu options only relevant to the software bounce buffering (SWIOTLB) IOMMU
 implementation:
 
-    swiotlb=<slots>[,force,noforce]
+    swiotlb=<slots>[,<areas>,force,noforce]
       <slots>
         Prereserve that many 2K slots for the software IO bounce buffering.
+      <areas>
+        Number of SWIOTLB areas with their own lock. Must be a power of 2.
       force
         Force all IO through the software TLB.
         Hardware IOMMU implementations can use SWIOTLB bounce buffering in
-- 
2.45.0.rc1.225.g2a3ae87e7f-goog

Re: [PATCH v2 2/3] doc: swiotlb: Document SWIOTLB areas parameter

[Petr Tesařík]

On Tue,  7 May 2024 01:34:59 +0000
"T.J. Mercier" <tjmercier@google.com> wrote:

> Commit 20347fca71a3 ("swiotlb: split up the global swiotlb lock") added
> the ability to specify the number of SWIOTLB areas, but boot-options.rst
> was not updated as part of that commit.
> 
> Reported-by: Michael Kelley <mhklinux@outlook.com>
> Fixes: 20347fca71a3 ("swiotlb: split up the global swiotlb lock")
> Signed-off-by: T.J. Mercier <tjmercier@google.com>
> ---
>  Documentation/arch/x86/x86_64/boot-options.rst | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/arch/x86/x86_64/boot-options.rst b/Documentation/arch/x86/x86_64/boot-options.rst
> index a37139d1752f..18161657b301 100644
> --- a/Documentation/arch/x86/x86_64/boot-options.rst
> +++ b/Documentation/arch/x86/x86_64/boot-options.rst
> @@ -287,9 +287,11 @@ iommu options only relevant to the AMD GART hardware IOMMU:
>  iommu options only relevant to the software bounce buffering (SWIOTLB) IOMMU
>  implementation:
>  
> -    swiotlb=<slots>[,force,noforce]
> +    swiotlb=<slots>[,<areas>,force,noforce]

This is not quite how this parameter is parsed. All the following
specifications are valid:

- swiotlb=512        // only slots
- swiotlb=,4         // only areas
- swiotlb=512,4      // slots and areas
- swiotlb=force      // default size
- swiotlb=512,force  // default areas
- swiotlb=512,4,force // explicitly specify everything

I believe the syntax should be somethig like:

  swiotlb={ | [<slots>][,<areas>],}{ force | noforce }

Petr T

>        <slots>
>          Prereserve that many 2K slots for the software IO bounce buffering.
> +      <areas>
> +        Number of SWIOTLB areas with their own lock. Must be a power of 2.
>        force
>          Force all IO through the software TLB.
>          Hardware IOMMU implementations can use SWIOTLB bounce buffering in

Re: [PATCH v2 2/3] doc: swiotlb: Document SWIOTLB areas parameter

[T.J. Mercier]

On Wed, May 8, 2024 at 11:14 PM Petr Tesařík <petr@tesarici.cz> wrote:
>
> On Tue,  7 May 2024 01:34:59 +0000
> "T.J. Mercier" <tjmercier@google.com> wrote:
>
> > Commit 20347fca71a3 ("swiotlb: split up the global swiotlb lock") added
> > the ability to specify the number of SWIOTLB areas, but boot-options.rst
> > was not updated as part of that commit.
> >
> > Reported-by: Michael Kelley <mhklinux@outlook.com>
> > Fixes: 20347fca71a3 ("swiotlb: split up the global swiotlb lock")
> > Signed-off-by: T.J. Mercier <tjmercier@google.com>
> > ---
> >  Documentation/arch/x86/x86_64/boot-options.rst | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/arch/x86/x86_64/boot-options.rst b/Documentation/arch/x86/x86_64/boot-options.rst
> > index a37139d1752f..18161657b301 100644
> > --- a/Documentation/arch/x86/x86_64/boot-options.rst
> > +++ b/Documentation/arch/x86/x86_64/boot-options.rst
> > @@ -287,9 +287,11 @@ iommu options only relevant to the AMD GART hardware IOMMU:
> >  iommu options only relevant to the software bounce buffering (SWIOTLB) IOMMU
> >  implementation:
> >
> > -    swiotlb=<slots>[,force,noforce]
> > +    swiotlb=<slots>[,<areas>,force,noforce]
>
> This is not quite how this parameter is parsed. All the following
> specifications are valid:
>
> - swiotlb=512        // only slots
> - swiotlb=,4         // only areas
> - swiotlb=512,4      // slots and areas
> - swiotlb=force      // default size
> - swiotlb=512,force  // default areas
> - swiotlb=512,4,force // explicitly specify everything
>
> I believe the syntax should be somethig like:
>
>   swiotlb={ | [<slots>][,<areas>],}{ force | noforce }
>
> Petr T
>
What does the leading | mean in front of slots?  How about brackets
around force/noforce since they're also optional and mutually
exclusive? The Rebooting section uses double brackets instead of
braces for groupings like that. Only weird thing here is the
force/noforce comma being potentially unneeded, but that's getting
pretty picky and I think the intent is clear.

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

Re: [PATCH v2 2/3] doc: swiotlb: Document SWIOTLB areas parameter

[Petr Tesařík]

On Thu, 9 May 2024 09:04:16 -0700
"T.J. Mercier" <tjmercier@google.com> wrote:

> On Wed, May 8, 2024 at 11:14 PM Petr Tesařík <petr@tesarici.cz> wrote:
> >
> > On Tue,  7 May 2024 01:34:59 +0000
> > "T.J. Mercier" <tjmercier@google.com> wrote:
> >  
> > > Commit 20347fca71a3 ("swiotlb: split up the global swiotlb lock") added
> > > the ability to specify the number of SWIOTLB areas, but boot-options.rst
> > > was not updated as part of that commit.
> > >
> > > Reported-by: Michael Kelley <mhklinux@outlook.com>
> > > Fixes: 20347fca71a3 ("swiotlb: split up the global swiotlb lock")
> > > Signed-off-by: T.J. Mercier <tjmercier@google.com>
> > > ---
> > >  Documentation/arch/x86/x86_64/boot-options.rst | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/Documentation/arch/x86/x86_64/boot-options.rst b/Documentation/arch/x86/x86_64/boot-options.rst
> > > index a37139d1752f..18161657b301 100644
> > > --- a/Documentation/arch/x86/x86_64/boot-options.rst
> > > +++ b/Documentation/arch/x86/x86_64/boot-options.rst
> > > @@ -287,9 +287,11 @@ iommu options only relevant to the AMD GART hardware IOMMU:
> > >  iommu options only relevant to the software bounce buffering (SWIOTLB) IOMMU
> > >  implementation:
> > >
> > > -    swiotlb=<slots>[,force,noforce]
> > > +    swiotlb=<slots>[,<areas>,force,noforce]  
> >
> > This is not quite how this parameter is parsed. All the following
> > specifications are valid:
> >
> > - swiotlb=512        // only slots
> > - swiotlb=,4         // only areas
> > - swiotlb=512,4      // slots and areas
> > - swiotlb=force      // default size
> > - swiotlb=512,force  // default areas
> > - swiotlb=512,4,force // explicitly specify everything
> >
> > I believe the syntax should be somethig like:
> >
> >   swiotlb={ | [<slots>][,<areas>],}{ force | noforce }
> >
> > Petr T
> >  
> What does the leading | mean in front of slots?  How about brackets

I wanted to mark somehow that "force" and "noforce" alone do not
require a leading comma, but if you specify slots and/or areas, the
comma is required, like you write below.

> around force/noforce since they're also optional and mutually
> exclusive?

Right. I missed that.

> The Rebooting section uses double brackets instead of
> braces for groupings like that.

TBH I don't know what syntax is used here. Use anything that can capture
the variants I listed above.

> Only weird thing here is the
> force/noforce comma being potentially unneeded, but that's getting
> pretty picky and I think the intent is clear.
> 
> swiotlb=[<slots>][,<areas>][, [force] | [noforce] ]

No objections from my side.

Petr T

[PATCH v2 3/3] doc: x86/iommu: Update and reorder iommu options

[T.J. Mercier]

Several options were missing from the top of the IOMMU section that were
detailed below. Add the missing options and reorder them to match the
order in which they are documented.

Signed-off-by: T.J. Mercier <tjmercier@google.com>
---
 Documentation/arch/x86/x86_64/boot-options.rst | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/arch/x86/x86_64/boot-options.rst b/Documentation/arch/x86/x86_64/boot-options.rst
index 18161657b301..04baefcc1712 100644
--- a/Documentation/arch/x86/x86_64/boot-options.rst
+++ b/Documentation/arch/x86/x86_64/boot-options.rst
@@ -242,9 +242,9 @@ Multiple x86-64 PCI-DMA mapping implementations exist, for example:
 
 ::
 
-  iommu=[<size>][,noagp][,off][,force][,noforce]
-  [,memaper[=<order>]][,merge][,fullflush][,nomerge]
-  [,noaperture]
+  iommu=[off][,noforce][,force][,soft][,<size>]
+  [,allowed][,fullflush][,nofullflush][,memaper[=<order>]]
+  [,merge][,nomerge][,noaperture][,noagp][,panic]
 
 General iommu options:
 
-- 
2.45.0.rc1.225.g2a3ae87e7f-goog

Re: [PATCH v2 1/3] doc: swiotlb: iommu/dma: Clarify swiotlb=force option applies only to dma-direct

[Petr Tesařík]

On Tue,  7 May 2024 01:34:58 +0000
"T.J. Mercier" <tjmercier@google.com> wrote:

> IOMMU implementations now sometimes bounce memory through SWIOTLB to
> achieve cacheline alignment [1], or prevent DMA attacks by untrusted
> devices [2]. These uses of SWIOTLB differ conceptually from historical
> use which was a solution to the problem of device addressing
> limitations that prevent DMA to some portion of system memory
> (typically beyond 4 GiB). IOMMUs also solve the problem of device
> addressing limitations and therefore eliminate the need for SWIOTLB for
> that purpose. However as mentioned, IOMMUs can use SWIOTLB for other
> purposes.
> 
> The swiotlb=force kernel command line parameter does not impact IOMMU
> related use of SWIOTLB, and that is intentional. IOMMUs cannot be forced
> to use SWIOTLB for all buffers. Update the documentation for the swiotlb
> parameter to clarify that SWIOTLB use can only be forced in scenarios
> where an IOMMU is not involved.
> 
> [1] https://lore.kernel.org/all/20230612153201.554742-16-catalin.marinas@arm.com
> [2] https://lore.kernel.org/all/20190906061452.30791-1-baolu.lu@linux.intel.com/
> Signed-off-by: T.J. Mercier <tjmercier@google.com>

Looks good to me now.

Reviewed-by: Petr Tesarik <petr@tesarici.cz>

Petr T

> ---
>  Documentation/admin-guide/kernel-parameters.txt | 1 +
>  Documentation/arch/x86/x86_64/boot-options.rst  | 3 +++
>  2 files changed, 4 insertions(+)
> 
> 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..a37139d1752f 100644
> --- a/Documentation/arch/x86/x86_64/boot-options.rst
> +++ b/Documentation/arch/x86/x86_64/boot-options.rst
> @@ -292,6 +292,9 @@ implementation:
>          Prereserve that many 2K slots for the software IO bounce buffering.
>        force
>          Force all IO through the software TLB.
> +        Hardware IOMMU implementations can use SWIOTLB bounce buffering in
> +        some circumstances, but they cannot be forced to always use them, so
> +        this option only has an effect when no hardware IOMMU is involved.
>        noforce
>          Do not initialize the software TLB.
>  

Re: [PATCH v2 1/3] doc: swiotlb: iommu/dma: Clarify swiotlb=force option applies only to dma-direct

[Dave Hansen]

So, I know get_maintainer.pl doesn't work great for files that are used
by a lot of subsystems, but it doesn't seem _super_ hard to find
relevant maintainers for this stuff.

There are many IOMMU and swiotlb folks in MAINTAINERS who aren't cc'd
here.  I'd be great to get an ack from those folks.
iommu@lists.linux.dev seems to pop up pretty frequently.

I'd also have zero objections to a patch to:

	Documentation/arch/x86/x86_64/boot-options.rst

that goes through another tree.

Re: [PATCH v2 1/3] doc: swiotlb: iommu/dma: Clarify swiotlb=force option applies only to dma-direct

[T.J. Mercier]

On Thu, May 9, 2024 at 1:18 PM Dave Hansen <dave.hansen@intel.com> wrote:
>
> So, I know get_maintainer.pl doesn't work great for files that are used
> by a lot of subsystems, but it doesn't seem _super_ hard to find
> relevant maintainers for this stuff.
>
> There are many IOMMU and swiotlb folks in MAINTAINERS who aren't cc'd
> here.  I'd be great to get an ack from those folks.
> iommu@lists.linux.dev seems to pop up pretty frequently.
>
> I'd also have zero objections to a patch to:
>
>         Documentation/arch/x86/x86_64/boot-options.rst
>
> that goes through another tree.

Sorry about that. I did manually CC iommu@lists.linux.dev on the
original version of this patch after a thread [1] on this topic on the
iommu list, but I neglected to manually add it for this v2. There will
be a v3 for the second patch, so I'll make sure to include the iommu
list then.

[1] https://lore.kernel.org/all/CABdmKX1HdXccWp9chz-Y_-Hh5TPry-4WRcVf4fUXKV=Og3dVTg@mail.gmail.com/