Received-SPF: pass (google.com: domain of linux-crypto+bounces-2706-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Mon, 18 Mar 2024 00:49:19 -0700
From: Jerry Snitselaar <jsnitsel@redhat.com>
To: Tom Zanussi <tom.zanussi@linux.intel.com>
Cc: Jonathan Corbet <corbet@lwn.net>, linux-crypto@vger.kernel.org, 
	linux-doc@vger.kernel.org
Subject: Re: [PATCH] crypto: iaa: Fix some errors in IAA documentation
Message-ID: <jhpuhcengkgdpgyb7qsez4lugpa5nhjjn3zqehbcbrtr2xh5md@cc3vz7v2xzdu>
References: <20240318064421.833348-1-jsnitsel@redhat.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240318064421.833348-1-jsnitsel@redhat.com>

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 <tom.zanussi@linux.intel.com>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Cc: linux-crypto@vger.kernel.org
> Cc: linux-doc@vger.kernel.org
> Signed-off-by: Jerry Snitselaar <jsnitsel@redhat.com>
> ---
>  .../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