DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] examples/vhost_crypto: fix zero copy
@ 2018-10-30 14:48 Fan Zhang
  2018-10-30 19:38 ` Mattias Rönnblom
  2018-11-14 11:16 ` [dpdk-dev] [PATCH v2] " Fan Zhang
  0 siblings, 2 replies; 7+ messages in thread
From: Fan Zhang @ 2018-10-30 14:48 UTC (permalink / raw)
  To: dev; +Cc: maxime.coquelin

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);
 
 		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
+			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);
+			ret = -EPERM;
+			goto error_exit;
+		}
+
 		if (dev_info.max_nb_queue_pairs < info->qid + 1) {
 			RTE_LOG(ERR, USER1, "Number of queues cannot over %u",
 					dev_info.max_nb_queue_pairs);
-- 
2.13.6

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [dpdk-dev] [PATCH] examples/vhost_crypto: fix zero copy
  2018-10-30 14:48 [dpdk-dev] [PATCH] examples/vhost_crypto: fix zero copy Fan Zhang
@ 2018-10-30 19:38 ` Mattias Rönnblom
  2018-11-09 11:40   ` Maxime Coquelin
  2018-11-14  8:46   ` Zhang, Roy Fan
  2018-11-14 11:16 ` [dpdk-dev] [PATCH v2] " Fan Zhang
  1 sibling, 2 replies; 7+ messages in thread
From: Mattias Rönnblom @ 2018-10-30 19:38 UTC (permalink / raw)
  To: Fan Zhang, dev; +Cc: maxime.coquelin

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".

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [dpdk-dev] [PATCH] examples/vhost_crypto: fix zero copy
  2018-10-30 19:38 ` Mattias Rönnblom
@ 2018-11-09 11:40   ` Maxime Coquelin
  2018-11-14  8:46   ` Zhang, Roy Fan
  1 sibling, 0 replies; 7+ messages in thread
From: Maxime Coquelin @ 2018-11-09 11:40 UTC (permalink / raw)
  To: Mattias Rönnblom, Fan Zhang, dev

Hi Fan,

Could you please have a look at Mattias comments and reply?

Thanks in advance,
Maxime

On 10/30/18 8:38 PM, Mattias Rönnblom wrote:
> 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".

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [dpdk-dev] [PATCH] examples/vhost_crypto: fix zero copy
  2018-10-30 19:38 ` Mattias Rönnblom
  2018-11-09 11:40   ` Maxime Coquelin
@ 2018-11-14  8:46   ` Zhang, Roy Fan
  1 sibling, 0 replies; 7+ messages in thread
From: Zhang, Roy Fan @ 2018-11-14  8:46 UTC (permalink / raw)
  To: Mattias Rönnblom, dev; +Cc: maxime.coquelin

Hi Mattias,

Sorry for the late reply. Comments inline.

> -----Original Message-----
> From: Mattias Rönnblom [mailto:mattias.ronnblom@ericsson.com]
> Sent: Tuesday, October 30, 2018 7:38 PM
> To: Zhang, Roy Fan <roy.fan.zhang@intel.com>; dev@dpdk.org
> Cc: maxime.coquelin@redhat.com
> Subject: Re: [dpdk-dev] [PATCH] examples/vhost_crypto: fix zero copy
> 
> On 2018-10-30 15:48, Fan Zhang wrote:
> > This patch fixes the zero copy enable problem for vhost crypto sample
> > application.
> 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").
No problem, I can change that. 
> 
> >   		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?
These two PMD names are defined inside the PMDs and are not exposed to the vhost library.
Also these two PMDs are the ones that copy to the end of the packet for computation purposes, which will cause the problem for virtio case.
 
> 
> > +			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".
Thanks I will change that.

Regards,
Fan

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [dpdk-dev] [PATCH v2] examples/vhost_crypto: fix zero copy
  2018-10-30 14:48 [dpdk-dev] [PATCH] examples/vhost_crypto: fix zero copy Fan Zhang
  2018-10-30 19:38 ` Mattias Rönnblom
@ 2018-11-14 11:16 ` Fan Zhang
  2018-11-16 10:48   ` Maxime Coquelin
  1 sibling, 1 reply; 7+ messages in thread
From: Fan Zhang @ 2018-11-14 11:16 UTC (permalink / raw)
  To: dev; +Cc: maxime.coquelin

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>
---
v2:
- removed unecessary checks
- Changed log message when zero-copy is not applicable.

 examples/vhost_crypto/main.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/examples/vhost_crypto/main.c b/examples/vhost_crypto/main.c
index cbb5e49d2..f08babd97 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,6 +443,9 @@ free_resource(void)
 		struct lcore_option *lo = &options.los[i];
 		struct vhost_crypto_info *info = options.infos[i];
 
+		if (!info)
+			continue;
+
 		rte_mempool_free(info->cop_pool);
 		rte_mempool_free(info->sess_pool);
 
@@ -493,6 +497,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
+			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 in %s\n",
+					dev_info.driver_name);
+			ret = -EPERM;
+			goto error_exit;
+		}
+
 		if (dev_info.max_nb_queue_pairs < info->qid + 1) {
 			RTE_LOG(ERR, USER1, "Number of queues cannot over %u",
 					dev_info.max_nb_queue_pairs);
-- 
2.13.6

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [dpdk-dev] [PATCH v2] examples/vhost_crypto: fix zero copy
  2018-11-14 11:16 ` [dpdk-dev] [PATCH v2] " Fan Zhang
@ 2018-11-16 10:48   ` Maxime Coquelin
  2018-11-16 15:23     ` Ferruh Yigit
  0 siblings, 1 reply; 7+ messages in thread
From: Maxime Coquelin @ 2018-11-16 10:48 UTC (permalink / raw)
  To: Fan Zhang, dev



On 11/14/18 12:16 PM, 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>
> ---
> v2:
> - removed unecessary checks
> - Changed log message when zero-copy is not applicable.
> 
>   examples/vhost_crypto/main.c | 17 +++++++++++++++++
>   1 file changed, 17 insertions(+)
> 

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks!
Maxime

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [dpdk-dev] [PATCH v2] examples/vhost_crypto: fix zero copy
  2018-11-16 10:48   ` Maxime Coquelin
@ 2018-11-16 15:23     ` Ferruh Yigit
  0 siblings, 0 replies; 7+ messages in thread
From: Ferruh Yigit @ 2018-11-16 15:23 UTC (permalink / raw)
  To: Maxime Coquelin, Fan Zhang, dev

On 11/16/2018 10:48 AM, Maxime Coquelin wrote:
> 
> 
> On 11/14/18 12:16 PM, 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>
>> ---
>> v2:
>> - removed unecessary checks
>> - Changed log message when zero-copy is not applicable.
>>
>>   examples/vhost_crypto/main.c | 17 +++++++++++++++++
>>   1 file changed, 17 insertions(+)
>>
> 
> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Applied to dpdk-next-net/master, thanks.

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2018-11-16 15:23 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-30 14:48 [dpdk-dev] [PATCH] examples/vhost_crypto: fix zero copy Fan Zhang
2018-10-30 19:38 ` Mattias Rönnblom
2018-11-09 11:40   ` Maxime Coquelin
2018-11-14  8:46   ` Zhang, Roy Fan
2018-11-14 11:16 ` [dpdk-dev] [PATCH v2] " Fan Zhang
2018-11-16 10:48   ` Maxime Coquelin
2018-11-16 15:23     ` Ferruh Yigit

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git