2024-03-18 06:44:44

by Jerry Snitselaar

[permalink] [raw]
Subject: [PATCH] crypto: iaa: Fix some errors in IAA documentation

This cleans up the following issues I ran into when trying to use the
scripts and commands in the iaa-crypto.rst document.

- Fix incorrect arguments being passed to accel-config
config-wq.
- Replace --device_name with --driver-name.
- Replace --driver_name with --driver-name.
- Replace --size with --wq-size.
- Add missing --priority argument.
- Add missing accel-config config-engine command after the
config-wq commands.
- Fix wq name passed to accel-config config-wq.
- Add rmmod/modprobe of iaa_crypto to script that disables,
then enables all devices and workqueues to avoid enable-wq
failing with -EEXIST when trying to register to compression
algorithm.
- Fix device name in cases where iaa was used instead of iax.

Cc: Tom Zanussi <[email protected]>
Cc: Jonathan Corbet <[email protected]>
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Jerry Snitselaar <[email protected]>
---
.../driver-api/crypto/iaa/iaa-crypto.rst | 22 ++++++++++++++-----
1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/Documentation/driver-api/crypto/iaa/iaa-crypto.rst b/Documentation/driver-api/crypto/iaa/iaa-crypto.rst
index de587cf9cbed..330d35df5f16 100644
--- a/Documentation/driver-api/crypto/iaa/iaa-crypto.rst
+++ b/Documentation/driver-api/crypto/iaa/iaa-crypto.rst
@@ -179,7 +179,9 @@ has the old 'iax' device naming in place) ::

# configure wq1.0

- accel-config config-wq --group-id=0 --mode=dedicated --type=kernel --name="iaa_crypto" --device_name="crypto" iax1/wq1.0
+ accel-config config-wq --group-id=0 --mode=dedicated --type=kernel --priority=10 --name="iaa_crypto" --driver-name="crypto" iax1/wq1.0
+
+ accel-config config-engine iax1/engine1.0 --group-id=0

# enable IAA device iax1

@@ -536,12 +538,20 @@ The below script automatically does that::

echo "End Disable IAA"

+ echo "Reload iaa_crypto module"
+
+ rmmod iaa_crypto
+ modprobe iaa_crypto
+
+ echo "End Reload iaa_crypto module"
+
#
# configure iaa wqs and devices
#
echo "Configure IAA"
for ((i = 1; i < ${num_iaa} * 2; i += 2)); do
- accel-config config-wq --group-id=0 --mode=dedicated --size=128 --priority=10 --type=kernel --name="iaa_crypto" --driver_name="crypto" iax${i}/wq${i}
+ accel-config config-wq --group-id=0 --mode=dedicated --wq-size=128 --priority=10 --type=kernel --name="iaa_crypto" --driver-name="crypto" iax${i}/wq${i}.0
+ accel-config config-engine iax${i}/engine${i}.0 --group-id=0
done

echo "End Configure IAA"
@@ -552,10 +562,10 @@ The below script automatically does that::
echo "Enable IAA"

for ((i = 1; i < ${num_iaa} * 2; i += 2)); do
- echo enable iaa iaa${i}
- accel-config enable-device iaa${i}
- echo enable wq iaa${i}/wq${i}.0
- accel-config enable-wq iaa${i}/wq${i}.0
+ echo enable iaa iax${i}
+ accel-config enable-device iax${i}
+ echo enable wq iax${i}/wq${i}.0
+ accel-config enable-wq iax${i}/wq${i}.0
done

echo "End Enable IAA"
--
2.41.0



2024-03-18 07:49:32

by Jerry Snitselaar

[permalink] [raw]
Subject: Re: [PATCH] crypto: iaa: Fix some errors in IAA documentation

On Sun, Mar 17, 2024 at 11:44:21PM -0700, Jerry Snitselaar wrote:
> This cleans up the following issues I ran into when trying to use the
> scripts and commands in the iaa-crypto.rst document.
>
> - Fix incorrect arguments being passed to accel-config
> config-wq.
> - Replace --device_name with --driver-name.
> - Replace --driver_name with --driver-name.
> - Replace --size with --wq-size.
> - Add missing --priority argument.
> - Add missing accel-config config-engine command after the
> config-wq commands.
> - Fix wq name passed to accel-config config-wq.
> - Add rmmod/modprobe of iaa_crypto to script that disables,
> then enables all devices and workqueues to avoid enable-wq
> failing with -EEXIST when trying to register to compression
> algorithm.
> - Fix device name in cases where iaa was used instead of iax.
>
> Cc: Tom Zanussi <[email protected]>
> Cc: Jonathan Corbet <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: Jerry Snitselaar <[email protected]>
> ---
> .../driver-api/crypto/iaa/iaa-crypto.rst | 22 ++++++++++++++-----
> 1 file changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/driver-api/crypto/iaa/iaa-crypto.rst b/Documentation/driver-api/crypto/iaa/iaa-crypto.rst
> index de587cf9cbed..330d35df5f16 100644
> --- a/Documentation/driver-api/crypto/iaa/iaa-crypto.rst
> +++ b/Documentation/driver-api/crypto/iaa/iaa-crypto.rst
> @@ -179,7 +179,9 @@ has the old 'iax' device naming in place) ::
>
> # configure wq1.0
>
> - accel-config config-wq --group-id=0 --mode=dedicated --type=kernel --name="iaa_crypto" --device_name="crypto" iax1/wq1.0
> + accel-config config-wq --group-id=0 --mode=dedicated --type=kernel --priority=10 --name="iaa_crypto" --driver-name="crypto" iax1/wq1.0
> +
> + accel-config config-engine iax1/engine1.0 --group-id=0
>
> # enable IAA device iax1
>
> @@ -536,12 +538,20 @@ The below script automatically does that::
>
> echo "End Disable IAA"
>
> + echo "Reload iaa_crypto module"
> +
> + rmmod iaa_crypto
> + modprobe iaa_crypto
> +
> + echo "End Reload iaa_crypto module"
> +
> #
> # configure iaa wqs and devices
> #
> echo "Configure IAA"
> for ((i = 1; i < ${num_iaa} * 2; i += 2)); do
> - accel-config config-wq --group-id=0 --mode=dedicated --size=128 --priority=10 --type=kernel --name="iaa_crypto" --driver_name="crypto" iax${i}/wq${i}
> + accel-config config-wq --group-id=0 --mode=dedicated --wq-size=128 --priority=10 --type=kernel --name="iaa_crypto" --driver-name="crypto" iax${i}/wq${i}.0
> + accel-config config-engine iax${i}/engine${i}.0 --group-id=0
> done
>
> echo "End Configure IAA"
> @@ -552,10 +562,10 @@ The below script automatically does that::
> echo "Enable IAA"
>
> for ((i = 1; i < ${num_iaa} * 2; i += 2)); do
> - echo enable iaa iaa${i}
> - accel-config enable-device iaa${i}
> - echo enable wq iaa${i}/wq${i}.0
> - accel-config enable-wq iaa${i}/wq${i}.0
> + echo enable iaa iax${i}
> + accel-config enable-device iax${i}
> + echo enable wq iax${i}/wq${i}.0
> + accel-config enable-wq iax${i}/wq${i}.0
> done
>
> echo "End Enable IAA"
> --
> 2.41.0
>

In addition to the above, the sections related to the modes seem
to be off to me.

Legacy mode in the Intel IOMMU context is when the IOMMU does not have
scalable mode enabled. If you pass intel_iommu=off the Intel IOMMU
will not be initialized, and I think that would correspond to the No IOMMU
mode instead of Legacy mode. The other suggestion for Legacy mode of
disabling VT-d in the BIOS would also be No IOMMU mode, but in
addition to the dma remapping units being disabled it would disable
interrupt remapping since the DMAR table would no longer be presented
to the OS by the BIOS.

I think the modes should be:

Scalable mode: intel_iommu=on,sm_on
Legacy mode: intel_iommu=on
No IOMMU mode: intel_iommu=off (or VT-d disabled in BIOS)


Since Intel IOMMU and scabale mode have config options that allow them
to be enabled by default, there are different parameter variations
that would match the above cases. I don't know if they need to
be detailed here, or if it would just make it more confusing.


Regards,
Jerry


2024-03-18 15:46:52

by Tom Zanussi

[permalink] [raw]
Subject: Re: [PATCH] crypto: iaa: Fix some errors in IAA documentation

Hi Jerry,

On Sun, 2024-03-17 at 23:44 -0700, Jerry Snitselaar wrote:
> This cleans up the following issues I ran into when trying to use the
> scripts and commands in the iaa-crypto.rst document.
>
> - Fix incorrect arguments being passed to accel-config
>   config-wq.
>     - Replace --device_name with --driver-name.
>     - Replace --driver_name with --driver-name.
>     - Replace --size with --wq-size.
>     - Add missing --priority argument.
> - Add missing accel-config config-engine command after the
>   config-wq commands.
> - Fix wq name passed to accel-config config-wq.
> - Add rmmod/modprobe of iaa_crypto to script that disables,
>   then enables all devices and workqueues to avoid enable-wq
>   failing with -EEXIST when trying to register to compression
>   algorithm.
> - Fix device name in cases where iaa was used instead of iax.
>

Yeah, I have these fixes in my own test scripts as well - I thought I
had updated the docs with them but obviously not :-/

Thanks for noticing/testing and fixing them.

Reviewed-by: Tom Zanussi <[email protected]>

> Cc: Tom Zanussi <[email protected]>
> Cc: Jonathan Corbet <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: Jerry Snitselaar <[email protected]>
> ---
>  .../driver-api/crypto/iaa/iaa-crypto.rst      | 22 ++++++++++++++---
> --
>  1 file changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/driver-api/crypto/iaa/iaa-crypto.rst
> b/Documentation/driver-api/crypto/iaa/iaa-crypto.rst
> index de587cf9cbed..330d35df5f16 100644
> --- a/Documentation/driver-api/crypto/iaa/iaa-crypto.rst
> +++ b/Documentation/driver-api/crypto/iaa/iaa-crypto.rst
> @@ -179,7 +179,9 @@ has the old 'iax' device naming in place) ::
>  
>    # configure wq1.0
>  
> -  accel-config config-wq --group-id=0 --mode=dedicated --type=kernel
> --name="iaa_crypto" --device_name="crypto" iax1/wq1.0
> +  accel-config config-wq --group-id=0 --mode=dedicated --type=kernel
> --priority=10 --name="iaa_crypto" --driver-name="crypto" iax1/wq1.0
> +
> +  accel-config config-engine iax1/engine1.0 --group-id=0
>  
>    # enable IAA device iax1
>  
> @@ -536,12 +538,20 @@ The below script automatically does that::
>  
>    echo "End Disable IAA"
>  
> +  echo "Reload iaa_crypto module"
> +
> +  rmmod iaa_crypto
> +  modprobe iaa_crypto
> +
> +  echo "End Reload iaa_crypto module"
> +
>    #
>    # configure iaa wqs and devices
>    #
>    echo "Configure IAA"
>    for ((i = 1; i < ${num_iaa} * 2; i += 2)); do
> -      accel-config config-wq --group-id=0 --mode=dedicated --
> size=128 --priority=10 --type=kernel --name="iaa_crypto" --
> driver_name="crypto" iax${i}/wq${i}
> +      accel-config config-wq --group-id=0 --mode=dedicated --wq-
> size=128 --priority=10 --type=kernel --name="iaa_crypto" --driver-
> name="crypto" iax${i}/wq${i}.0
> +      accel-config config-engine iax${i}/engine${i}.0 --group-id=0
>    done
>  
>    echo "End Configure IAA"
> @@ -552,10 +562,10 @@ The below script automatically does that::
>    echo "Enable IAA"
>  
>    for ((i = 1; i < ${num_iaa} * 2; i += 2)); do
> -      echo enable iaa iaa${i}
> -      accel-config enable-device iaa${i}
> -      echo enable wq iaa${i}/wq${i}.0
> -      accel-config enable-wq iaa${i}/wq${i}.0
> +      echo enable iaa iax${i}
> +      accel-config enable-device iax${i}
> +      echo enable wq iax${i}/wq${i}.0
> +      accel-config enable-wq iax${i}/wq${i}.0
>    done
>  
>    echo "End Enable IAA"


2024-03-18 16:26:41

by Tom Zanussi

[permalink] [raw]
Subject: Re: [PATCH] crypto: iaa: Fix some errors in IAA documentation

Hi Jerry,

On Mon, 2024-03-18 at 00:49 -0700, Jerry Snitselaar wrote:
> On Sun, Mar 17, 2024 at 11:44:21PM -0700, Jerry Snitselaar wrote:
> > This cleans up the following issues I ran into when trying to use
> > the
> > scripts and commands in the iaa-crypto.rst document.
> >
> > - Fix incorrect arguments being passed to accel-config
> >   config-wq.
> >     - Replace --device_name with --driver-name.
> >     - Replace --driver_name with --driver-name.
> >     - Replace --size with --wq-size.
> >     - Add missing --priority argument.
> > - Add missing accel-config config-engine command after the
> >   config-wq commands.
> > - Fix wq name passed to accel-config config-wq.
> > - Add rmmod/modprobe of iaa_crypto to script that disables,
> >   then enables all devices and workqueues to avoid enable-wq
> >   failing with -EEXIST when trying to register to compression
> >   algorithm.
> > - Fix device name in cases where iaa was used instead of iax.
> >
> > Cc: Tom Zanussi <[email protected]>
> > Cc: Jonathan Corbet <[email protected]>
> > Cc: [email protected]
> > Cc: [email protected]
> > Signed-off-by: Jerry Snitselaar <[email protected]>
> > ---
> >  .../driver-api/crypto/iaa/iaa-crypto.rst      | 22 ++++++++++++++-
> > ----
> >  1 file changed, 16 insertions(+), 6 deletions(-)
> >
> > diff --git a/Documentation/driver-api/crypto/iaa/iaa-crypto.rst
> > b/Documentation/driver-api/crypto/iaa/iaa-crypto.rst
> > index de587cf9cbed..330d35df5f16 100644
> > --- a/Documentation/driver-api/crypto/iaa/iaa-crypto.rst
> > +++ b/Documentation/driver-api/crypto/iaa/iaa-crypto.rst
> > @@ -179,7 +179,9 @@ has the old 'iax' device naming in place) ::
> >  
> >    # configure wq1.0
> >  
> > -  accel-config config-wq --group-id=0 --mode=dedicated --
> > type=kernel --name="iaa_crypto" --device_name="crypto" iax1/wq1.0
> > +  accel-config config-wq --group-id=0 --mode=dedicated --
> > type=kernel --priority=10 --name="iaa_crypto" --driver-
> > name="crypto" iax1/wq1.0
> > +
> > +  accel-config config-engine iax1/engine1.0 --group-id=0
> >  
> >    # enable IAA device iax1
> >  
> > @@ -536,12 +538,20 @@ The below script automatically does that::
> >  
> >    echo "End Disable IAA"
> >  
> > +  echo "Reload iaa_crypto module"
> > +
> > +  rmmod iaa_crypto
> > +  modprobe iaa_crypto
> > +
> > +  echo "End Reload iaa_crypto module"
> > +
> >    #
> >    # configure iaa wqs and devices
> >    #
> >    echo "Configure IAA"
> >    for ((i = 1; i < ${num_iaa} * 2; i += 2)); do
> > -      accel-config config-wq --group-id=0 --mode=dedicated --
> > size=128 --priority=10 --type=kernel --name="iaa_crypto" --
> > driver_name="crypto" iax${i}/wq${i}
> > +      accel-config config-wq --group-id=0 --mode=dedicated --wq-
> > size=128 --priority=10 --type=kernel --name="iaa_crypto" --driver-
> > name="crypto" iax${i}/wq${i}.0
> > +      accel-config config-engine iax${i}/engine${i}.0 --group-id=0
> >    done
> >  
> >    echo "End Configure IAA"
> > @@ -552,10 +562,10 @@ The below script automatically does that::
> >    echo "Enable IAA"
> >  
> >    for ((i = 1; i < ${num_iaa} * 2; i += 2)); do
> > -      echo enable iaa iaa${i}
> > -      accel-config enable-device iaa${i}
> > -      echo enable wq iaa${i}/wq${i}.0
> > -      accel-config enable-wq iaa${i}/wq${i}.0
> > +      echo enable iaa iax${i}
> > +      accel-config enable-device iax${i}
> > +      echo enable wq iax${i}/wq${i}.0
> > +      accel-config enable-wq iax${i}/wq${i}.0
> >    done
> >  
> >    echo "End Enable IAA"
> > --
> > 2.41.0
> >
>
> In addition to the above, the sections related to the modes seem
> to be off to me.
>
> Legacy mode in the Intel IOMMU context is when the IOMMU does not
> have
> scalable mode enabled. If you pass intel_iommu=off the Intel IOMMU
> will not be initialized, and I think that would correspond to the No
> IOMMU
> mode instead of Legacy mode. The other suggestion for Legacy mode of
> disabling VT-d in the BIOS would also be No IOMMU mode, but in
> addition to the dma remapping units being disabled it would disable
> interrupt remapping since the DMAR table would no longer be presented
> to the OS by the BIOS.
>
> I think the modes should be:
>
> Scalable mode: intel_iommu=on,sm_on
> Legacy mode: intel_iommu=on
> No IOMMU mode: intel_iommu=off (or VT-d disabled in BIOS)
>

Yes, I think you're correct, those make more sense.

> Since Intel IOMMU and scabale mode have config options that allow
> them
> to be enabled by default, there are different parameter variations
> that would match the above cases. I don't know if they need to
> be detailed here, or if it would just make it more confusing.
>

Personally, I think it would be useful to have them detailed and might
lessen confusion for people setting things up and/or debugging a setup.

Thanks,

Tom

> Regards,
> Jerry
>


2024-03-18 17:59:03

by Jerry Snitselaar

[permalink] [raw]
Subject: Re: [PATCH] crypto: iaa: Fix some errors in IAA documentation

On Mon, Mar 18, 2024 at 11:26:31AM -0500, Tom Zanussi wrote:
> Hi Jerry,
>
> On Mon, 2024-03-18 at 00:49 -0700, Jerry Snitselaar wrote:
> > On Sun, Mar 17, 2024 at 11:44:21PM -0700, Jerry Snitselaar wrote:
> > > This cleans up the following issues I ran into when trying to use
> > > the
> > > scripts and commands in the iaa-crypto.rst document.
> > >
> > > - Fix incorrect arguments being passed to accel-config
> > > ? config-wq.
> > > ??? - Replace --device_name with --driver-name.
> > > ??? - Replace --driver_name with --driver-name.
> > > ??? - Replace --size with --wq-size.
> > > ??? - Add missing --priority argument.
> > > - Add missing accel-config config-engine command after the
> > > ? config-wq commands.
> > > - Fix wq name passed to accel-config config-wq.
> > > - Add rmmod/modprobe of iaa_crypto to script that disables,
> > > ? then enables all devices and workqueues to avoid enable-wq
> > > ? failing with -EEXIST when trying to register to compression
> > > ? algorithm.
> > > - Fix device name in cases where iaa was used instead of iax.
> > >
> > > Cc: Tom Zanussi <[email protected]>
> > > Cc: Jonathan Corbet <[email protected]>
> > > Cc: [email protected]
> > > Cc: [email protected]
> > > Signed-off-by: Jerry Snitselaar <[email protected]>
> > > ---
> > > ?.../driver-api/crypto/iaa/iaa-crypto.rst????? | 22 ++++++++++++++-
> > > ----
> > > ?1 file changed, 16 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/Documentation/driver-api/crypto/iaa/iaa-crypto.rst
> > > b/Documentation/driver-api/crypto/iaa/iaa-crypto.rst
> > > index de587cf9cbed..330d35df5f16 100644
> > > --- a/Documentation/driver-api/crypto/iaa/iaa-crypto.rst
> > > +++ b/Documentation/driver-api/crypto/iaa/iaa-crypto.rst
> > > @@ -179,7 +179,9 @@ has the old 'iax' device naming in place) ::
> > > ?
> > > ?? # configure wq1.0
> > > ?
> > > -? accel-config config-wq --group-id=0 --mode=dedicated --
> > > type=kernel --name="iaa_crypto" --device_name="crypto" iax1/wq1.0
> > > +? accel-config config-wq --group-id=0 --mode=dedicated --
> > > type=kernel --priority=10 --name="iaa_crypto" --driver-
> > > name="crypto" iax1/wq1.0
> > > +
> > > +? accel-config config-engine iax1/engine1.0 --group-id=0
> > > ?
> > > ?? # enable IAA device iax1
> > > ?
> > > @@ -536,12 +538,20 @@ The below script automatically does that::
> > > ?
> > > ?? echo "End Disable IAA"
> > > ?
> > > +? echo "Reload iaa_crypto module"
> > > +
> > > +? rmmod iaa_crypto
> > > +? modprobe iaa_crypto
> > > +
> > > +? echo "End Reload iaa_crypto module"
> > > +
> > > ?? #
> > > ?? # configure iaa wqs and devices
> > > ?? #
> > > ?? echo "Configure IAA"
> > > ?? for ((i = 1; i < ${num_iaa} * 2; i += 2)); do
> > > -????? accel-config config-wq --group-id=0 --mode=dedicated --
> > > size=128 --priority=10 --type=kernel --name="iaa_crypto" --
> > > driver_name="crypto" iax${i}/wq${i}
> > > +????? accel-config config-wq --group-id=0 --mode=dedicated --wq-
> > > size=128 --priority=10 --type=kernel --name="iaa_crypto" --driver-
> > > name="crypto" iax${i}/wq${i}.0
> > > +????? accel-config config-engine iax${i}/engine${i}.0 --group-id=0
> > > ?? done
> > > ?
> > > ?? echo "End Configure IAA"
> > > @@ -552,10 +562,10 @@ The below script automatically does that::
> > > ?? echo "Enable IAA"
> > > ?
> > > ?? for ((i = 1; i < ${num_iaa} * 2; i += 2)); do
> > > -????? echo enable iaa iaa${i}
> > > -????? accel-config enable-device iaa${i}
> > > -????? echo enable wq iaa${i}/wq${i}.0
> > > -????? accel-config enable-wq iaa${i}/wq${i}.0
> > > +????? echo enable iaa iax${i}
> > > +????? accel-config enable-device iax${i}
> > > +????? echo enable wq iax${i}/wq${i}.0
> > > +????? accel-config enable-wq iax${i}/wq${i}.0
> > > ?? done
> > > ?
> > > ?? echo "End Enable IAA"
> > > --
> > > 2.41.0
> > >
> >
> > In addition to the above, the sections related to the modes seem
> > to be off to me.
> >
> > Legacy mode in the Intel IOMMU context is when the IOMMU does not
> > have
> > scalable mode enabled. If you pass intel_iommu=off the Intel IOMMU
> > will not be initialized, and I think that would correspond to the No
> > IOMMU
> > mode instead of Legacy mode. The other suggestion for Legacy mode of
> > disabling VT-d in the BIOS would also be No IOMMU mode, but in
> > addition to the dma remapping units being disabled it would disable
> > interrupt remapping since the DMAR table would no longer be presented
> > to the OS by the BIOS.
> >
> > I think the modes should be:
> >
> > Scalable mode: intel_iommu=on,sm_on
> > Legacy mode: intel_iommu=on
> > No IOMMU mode: intel_iommu=off (or VT-d disabled in BIOS)
> >
>
> Yes, I think you're correct, those make more sense.
>
> > Since Intel IOMMU and scabale mode have config options that allow
> > them
> > to be enabled by default, there are different parameter variations
> > that would match the above cases. I don't know if they need to
> > be detailed here, or if it would just make it more confusing.
> >
>
> Personally, I think it would be useful to have them detailed and might
> lessen confusion for people setting things up and/or debugging a setup.
>
> Thanks,
>
> Tom

Hi Tom,

This is what I came up with last night for the kernel parameters when thinking about it:


| mode \ default enable | intel_iommu + /sm + | intel_iommu + / sm - | intel_iommu - / sm + | intel_iommu - / sm - |
|-----------------------+---------------------+----------------------+-----------------------+----------------------|
| Scalable Mode | nothing | intel_iommu=sm_on | intel_iommu=on | intel_iommu=on,sm_on |
| Legacy Mode | intel_iommu=sm_off | nothing | intel_iommu=on,sm_off | intel_iommu=on |
| No IOMMU Mode | intel_iommu=off | intel_iommu=off | nothing | nothing |


Regards,
Jerry



>
> > Regards,
> > Jerry
> >
>


2024-03-18 19:03:19

by Tom Zanussi

[permalink] [raw]
Subject: Re: [PATCH] crypto: iaa: Fix some errors in IAA documentation

On Mon, 2024-03-18 at 10:58 -0700, Jerry Snitselaar wrote:
> On Mon, Mar 18, 2024 at 11:26:31AM -0500, Tom Zanussi wrote:
> > Hi Jerry,
> >
> > On Mon, 2024-03-18 at 00:49 -0700, Jerry Snitselaar wrote:
> > > On Sun, Mar 17, 2024 at 11:44:21PM -0700, Jerry Snitselaar wrote:
> > > > This cleans up the following issues I ran into when trying to use
> > > > the
> > > > scripts and commands in the iaa-crypto.rst document.
> > > >
> > > > - Fix incorrect arguments being passed to accel-config
> > > >   config-wq.
> > > >     - Replace --device_name with --driver-name.
> > > >     - Replace --driver_name with --driver-name.
> > > >     - Replace --size with --wq-size.
> > > >     - Add missing --priority argument.
> > > > - Add missing accel-config config-engine command after the
> > > >   config-wq commands.
> > > > - Fix wq name passed to accel-config config-wq.
> > > > - Add rmmod/modprobe of iaa_crypto to script that disables,
> > > >   then enables all devices and workqueues to avoid enable-wq
> > > >   failing with -EEXIST when trying to register to compression
> > > >   algorithm.
> > > > - Fix device name in cases where iaa was used instead of iax.
> > > >
> > > > Cc: Tom Zanussi <[email protected]>
> > > > Cc: Jonathan Corbet <[email protected]>
> > > > Cc: [email protected]
> > > > Cc: [email protected]
> > > > Signed-off-by: Jerry Snitselaar <[email protected]>
> > > > ---
> > > >  .../driver-api/crypto/iaa/iaa-crypto.rst      | 22 ++++++++++++++-
> > > > ----
> > > >  1 file changed, 16 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/Documentation/driver-api/crypto/iaa/iaa-crypto.rst
> > > > b/Documentation/driver-api/crypto/iaa/iaa-crypto.rst
> > > > index de587cf9cbed..330d35df5f16 100644
> > > > --- a/Documentation/driver-api/crypto/iaa/iaa-crypto.rst
> > > > +++ b/Documentation/driver-api/crypto/iaa/iaa-crypto.rst
> > > > @@ -179,7 +179,9 @@ has the old 'iax' device naming in place) ::
> > > >  
> > > >    # configure wq1.0
> > > >  
> > > > -  accel-config config-wq --group-id=0 --mode=dedicated --
> > > > type=kernel --name="iaa_crypto" --device_name="crypto" iax1/wq1.0
> > > > +  accel-config config-wq --group-id=0 --mode=dedicated --
> > > > type=kernel --priority=10 --name="iaa_crypto" --driver-
> > > > name="crypto" iax1/wq1.0
> > > > +
> > > > +  accel-config config-engine iax1/engine1.0 --group-id=0
> > > >  
> > > >    # enable IAA device iax1
> > > >  
> > > > @@ -536,12 +538,20 @@ The below script automatically does that::
> > > >  
> > > >    echo "End Disable IAA"
> > > >  
> > > > +  echo "Reload iaa_crypto module"
> > > > +
> > > > +  rmmod iaa_crypto
> > > > +  modprobe iaa_crypto
> > > > +
> > > > +  echo "End Reload iaa_crypto module"
> > > > +
> > > >    #
> > > >    # configure iaa wqs and devices
> > > >    #
> > > >    echo "Configure IAA"
> > > >    for ((i = 1; i < ${num_iaa} * 2; i += 2)); do
> > > > -      accel-config config-wq --group-id=0 --mode=dedicated --
> > > > size=128 --priority=10 --type=kernel --name="iaa_crypto" --
> > > > driver_name="crypto" iax${i}/wq${i}
> > > > +      accel-config config-wq --group-id=0 --mode=dedicated --wq-
> > > > size=128 --priority=10 --type=kernel --name="iaa_crypto" --driver-
> > > > name="crypto" iax${i}/wq${i}.0
> > > > +      accel-config config-engine iax${i}/engine${i}.0 --group-id=0
> > > >    done
> > > >  
> > > >    echo "End Configure IAA"
> > > > @@ -552,10 +562,10 @@ The below script automatically does that::
> > > >    echo "Enable IAA"
> > > >  
> > > >    for ((i = 1; i < ${num_iaa} * 2; i += 2)); do
> > > > -      echo enable iaa iaa${i}
> > > > -      accel-config enable-device iaa${i}
> > > > -      echo enable wq iaa${i}/wq${i}.0
> > > > -      accel-config enable-wq iaa${i}/wq${i}.0
> > > > +      echo enable iaa iax${i}
> > > > +      accel-config enable-device iax${i}
> > > > +      echo enable wq iax${i}/wq${i}.0
> > > > +      accel-config enable-wq iax${i}/wq${i}.0
> > > >    done
> > > >  
> > > >    echo "End Enable IAA"
> > > > --
> > > > 2.41.0
> > > >
> > >
> > > In addition to the above, the sections related to the modes seem
> > > to be off to me.
> > >
> > > Legacy mode in the Intel IOMMU context is when the IOMMU does not
> > > have
> > > scalable mode enabled. If you pass intel_iommu=off the Intel IOMMU
> > > will not be initialized, and I think that would correspond to the No
> > > IOMMU
> > > mode instead of Legacy mode. The other suggestion for Legacy mode of
> > > disabling VT-d in the BIOS would also be No IOMMU mode, but in
> > > addition to the dma remapping units being disabled it would disable
> > > interrupt remapping since the DMAR table would no longer be presented
> > > to the OS by the BIOS.
> > >
> > > I think the modes should be:
> > >
> > > Scalable mode: intel_iommu=on,sm_on
> > > Legacy mode: intel_iommu=on
> > > No IOMMU mode: intel_iommu=off (or VT-d disabled in BIOS)
> > >
> >
> > Yes, I think you're correct, those make more sense.
> >
> > > Since Intel IOMMU and scabale mode have config options that allow
> > > them
> > > to be enabled by default, there are different parameter variations
> > > that would match the above cases. I don't know if they need to
> > > be detailed here, or if it would just make it more confusing.
> > >
> >
> > Personally, I think it would be useful to have them detailed and might
> > lessen confusion for people setting things up and/or debugging a setup.
> >
> > Thanks,
> >
> > Tom
>
> Hi Tom,
>
> This is what I came up with last night for the kernel parameters when thinking about it:
>
>
> > mode \ default enable | intel_iommu + /sm + | intel_iommu + / sm - | intel_iommu - / sm +  | intel_iommu - / sm - |
> > -----------------------+---------------------+----------------------+-----------------------+----------------------|
> > Scalable Mode         | nothing             | intel_iommu=sm_on    | intel_iommu=on        | intel_iommu=on,sm_on |
> > Legacy Mode           | intel_iommu=sm_off  | nothing              | intel_iommu=on,sm_off | intel_iommu=on       |
> > No IOMMU Mode         | intel_iommu=off     | intel_iommu=off      | nothing               | nothing              |
>

Very nice. I think it would need a little explanation on how to read
the table and mention of how the defaults correspond to actual config
options e.g. sm+ = CONFIG_INTEL_IOMMU_SCALABLE_MODE_DEFAULT_ON=y,
etc...

Thanks,

Tom

>
> Regards,
> Jerry
>
>
>
> >
> > > Regards,
> > > Jerry
> > >
> >
>


2024-03-18 19:47:01

by Jerry Snitselaar

[permalink] [raw]
Subject: Re: [PATCH] crypto: iaa: Fix some errors in IAA documentation

On Mon, Mar 18, 2024 at 02:01:18PM -0500, Tom Zanussi wrote:
...
> >
> > This is what I came up with last night for the kernel parameters when thinking about it:
> >
> >
> > > mode \ default enable | intel_iommu + /sm + | intel_iommu + / sm - | intel_iommu - / sm +? | intel_iommu - / sm - |
> > > -----------------------+---------------------+----------------------+-----------------------+----------------------|
> > > Scalable Mode???????? | nothing???????????? | intel_iommu=sm_on??? | intel_iommu=on??????? | intel_iommu=on,sm_on |
> > > Legacy Mode?????????? | intel_iommu=sm_off? | nothing????????????? | intel_iommu=on,sm_off | intel_iommu=on?????? |
> > > No IOMMU Mode???????? | intel_iommu=off???? | intel_iommu=off????? | nothing?????????????? | nothing????????????? |
> >
>
> Very nice. I think it would need a little explanation on how to read
> the table and mention of how the defaults correspond to actual config
> options e.g. sm+ = CONFIG_INTEL_IOMMU_SCALABLE_MODE_DEFAULT_ON=y,
> etc...
>
> Thanks,
>
> Tom
>

Yes, if something like this were to go into the doc it would need more
explanation. This was me just trying to map out the different
combinations last night in an org-mode table in emacs.