Re: [dpdk-dev] [PATCH] examples/vhost_crypto: fix zero copy

["Mattias Rönnblom" <mattias.ronnblom@ericsson.com>]

On 2018-10-30 15:48, Fan Zhang wrote:
> This patch fixes the zero copy enable problem for vhost crypto
> sample application.
> 
> For some Crypto PMDs such as AESNI-MB and AESNI-GCM the data to
> be processed will be made a copy in the same buffer but next to the
> data. For example, to encrypt 64 bytes data the PMD will copy this
> data from offset 64 to offset 123. This requires the application
> provides the buffer with at least double of the data size.
> 
> However there is no way for VMs to know this limitation. When
> zero-copy is enabled in Vhost the PMD may overwrite the buffer
> next to the VM data to be processed, and further cause problems
> such as Segmentation Fault or even worse, crashes the VM.
> 
> To fix the problem the user should avoid enabling the zero copy
> for these Crypto PMDs. This patch adds the checking of the PMD
> names to see if zero copy can be applied.
> 
> Fixes: 709521f4c2cd ("examples/vhost_crypto: support multi-core")
> 
> Signed-off-by: Fan Zhang <roy.fan.zhang@intel.com>
> ---
>   examples/vhost_crypto/main.c | 23 +++++++++++++++++++++--
>   1 file changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/examples/vhost_crypto/main.c b/examples/vhost_crypto/main.c
> index cbb5e49d2..887e3eb6f 100644
> --- a/examples/vhost_crypto/main.c
> +++ b/examples/vhost_crypto/main.c
> @@ -4,6 +4,7 @@
>   
>   #include <stdio.h>
>   #include <stdlib.h>
> +#include <string.h>
>   #include <unistd.h>
>   #include <stdbool.h>
>   #include <assert.h>
> @@ -442,8 +443,13 @@ free_resource(void)
>   		struct lcore_option *lo = &options.los[i];
>   		struct vhost_crypto_info *info = options.infos[i];
>   
> -		rte_mempool_free(info->cop_pool);
> -		rte_mempool_free(info->sess_pool);
> +		if (!info)
> +			continue;
> +
> +		if (info->cop_pool)
> +			rte_mempool_free(info->cop_pool);
> +		if (info->sess_pool)
> +			rte_mempool_free(info->sess_pool);
>   

rte_mempool_free() already does a NULL-check (as per libc free() 
convention), and if you are to do a NULL-check it should be an explicit 
one ("!= NULL").

>   		for (j = 0; j < lo->nb_sockets; j++) {
>   			rte_vhost_driver_unregister(lo->socket_files[i]);
> @@ -493,6 +499,19 @@ main(int argc, char *argv[])
>   		info->nb_vids = lo->nb_sockets;
>   
>   		rte_cryptodev_info_get(info->cid, &dev_info);
> +		if (options.zero_copy == RTE_VHOST_CRYPTO_ZERO_COPY_ENABLE) {
> +#define VHOST_CRYPTO_CDEV_NAME_AESNI_MB_PMD	crypto_aesni_mb
> +#define VHOST_CRYPTO_CDEV_NAME_AESNI_GCM_PMD	crypto_aesni_gcm

What's the purpose of these defines?

> +			if (strstr(dev_info.driver_name,
> +				RTE_STR(VHOST_CRYPTO_CDEV_NAME_AESNI_MB_PMD)) ||
> +				strstr(dev_info.driver_name,
> +				RTE_STR(VHOST_CRYPTO_CDEV_NAME_AESNI_GCM_PMD)))
> +			RTE_LOG(ERR, USER1, "Cannot enable Zero Copy to %s\n",
> +					dev_info.driver_name);

"Zero Copy to" should probably be "zero-copy in" or "Zero-copy in".