RE: [PATCH 1/6] virtio: wrap find_vqs

[Gonglei (Arei)]

> -Original Message-
> From: Michael S. Tsirkin [mailto:m...@redhat.com]
> Sent: Thursday, March 30, 2017 4:49 AM
> Subject: [PATCH 1/6] virtio: wrap find_vqs
> 
> We are going to add more parameters to find_vqs, let's wrap the call so
> we don't need to tweak all drivers every time.
> 
> Signed-off-by: Michael S. Tsirkin 
> ---
>  drivers/block/virtio_blk.c | 3 +--
>  drivers/char/virtio_console.c  | 6 +++---
>  drivers/crypto/virtio/virtio_crypto_core.c | 3 +--
>  drivers/gpu/drm/virtio/virtgpu_kms.c   | 3 +--
>  drivers/net/caif/caif_virtio.c | 3 +--
>  drivers/net/virtio_net.c   | 3 +--
>  drivers/rpmsg/virtio_rpmsg_bus.c   | 2 +-
>  drivers/scsi/virtio_scsi.c | 3 +--
>  drivers/virtio/virtio_balloon.c| 3 +--
>  drivers/virtio/virtio_input.c  | 3 +--
>  include/linux/virtio_config.h  | 9 +
>  net/vmw_vsock/virtio_transport.c   | 6 +++---
>  12 files changed, 24 insertions(+), 23 deletions(-)
> 

Acked-by: Gonglei 


> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index 1d4c9f8..c08c30c 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -455,8 +455,7 @@ static int init_vq(struct virtio_blk *vblk)
>   }
> 
>   /* Discover virtqueues and write information to configuration.  */
> - err = vdev->config->find_vqs(vdev, num_vqs, vqs, callbacks, names,
> - );
> + err = virtio_find_vqs(vdev, num_vqs, vqs, callbacks, names, );
>   if (err)
>   goto out;
> 
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index e9b7e0b..5da4c8e 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -1945,9 +1945,9 @@ static int init_vqs(struct ports_device *portdev)
>   }
>   }
>   /* Find the queues. */
> - err = portdev->vdev->config->find_vqs(portdev->vdev, nr_queues, vqs,
> -   io_callbacks,
> -   (const char **)io_names, NULL);
> + err = virtio_find_vqs(portdev->vdev, nr_queues, vqs,
> +   io_callbacks,
> +   (const char **)io_names, NULL);
>   if (err)
>   goto free;
> 
> diff --git a/drivers/crypto/virtio/virtio_crypto_core.c
> b/drivers/crypto/virtio/virtio_crypto_core.c
> index 21472e4..a111cd72 100644
> --- a/drivers/crypto/virtio/virtio_crypto_core.c
> +++ b/drivers/crypto/virtio/virtio_crypto_core.c
> @@ -119,8 +119,7 @@ static int virtcrypto_find_vqs(struct virtio_crypto *vi)
>   names[i] = vi->data_vq[i].name;
>   }
> 
> - ret = vi->vdev->config->find_vqs(vi->vdev, total_vqs, vqs, callbacks,
> -  names, NULL);
> + ret = virtio_find_vqs(vi->vdev, total_vqs, vqs, callbacks, names, NULL);
>   if (ret)
>   goto err_find;
> 
> diff --git a/drivers/gpu/drm/virtio/virtgpu_kms.c
> b/drivers/gpu/drm/virtio/virtgpu_kms.c
> index 4918668..1e1c90b 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_kms.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_kms.c
> @@ -175,8 +175,7 @@ int virtio_gpu_driver_load(struct drm_device *dev,
> unsigned long flags)
>   DRM_INFO("virgl 3d acceleration not supported by guest\n");
>  #endif
> 
> - ret = vgdev->vdev->config->find_vqs(vgdev->vdev, 2, vqs,
> - callbacks, names, NULL);
> + ret = virtio_find_vqs(vgdev->vdev, 2, vqs, callbacks, names, NULL);
>   if (ret) {
>   DRM_ERROR("failed to find virt queues\n");
>   goto err_vqs;
> diff --git a/drivers/net/caif/caif_virtio.c b/drivers/net/caif/caif_virtio.c
> index bc0eb47..6122768 100644
> --- a/drivers/net/caif/caif_virtio.c
> +++ b/drivers/net/caif/caif_virtio.c
> @@ -679,8 +679,7 @@ static int cfv_probe(struct virtio_device *vdev)
>   goto err;
> 
>   /* Get the TX virtio ring. This is a "guest side vring". */
> - err = vdev->config->find_vqs(vdev, 1, >vq_tx, _cbs, ,
> - NULL);
> + err = virtio_find_vqs(vdev, 1, >vq_tx, _cbs, , NULL);
>   if (err)
>   goto err;
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index ea9890d..6802169 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -2079,8 +2079,7 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
>   names[txq2vq(i)] = vi->sq[i].name;
>   }
> 
> - ret = vi->vdev->config->find_vqs(vi->vdev, total_vqs, vqs, callbacks,
> -  names, NULL);
> + ret = virtio_find_vqs(vi->vdev, total_vqs, vqs, callbacks, names, NULL);
>   if (ret)
>   goto err_find;
> 
> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c
> b/drivers/rpmsg/virtio_rpmsg_bus.c
> index 

RE: [PATCH] virtio-crypto: adjust priority of algorithm

[Gonglei (Arei)]

>
> From: Christian Borntraeger [mailto:borntrae...@de.ibm.com]
> Sent: Friday, January 13, 2017 4:28 PM
> To: Gonglei (Arei); virtualizat...@lists.linux-foundation.org;
> linux-crypto@vger.kernel.org; linux-ker...@vger.kernel.org
> Cc: m...@redhat.com; herb...@gondor.apana.org.au
> Subject: Re: [PATCH] virtio-crypto: adjust priority of algorithm
> 
> ACK. Whoever takes this patch might want to fixup 3 typos.
> 
Thanks, I'd better send v2 IMHO.  :)

Regards,
-Gonglei

> On 01/13/2017 07:25 AM, Gonglei wrote:
> 
> > Some hardware accelerators (like intel aseni or the s390
>   aesni
> > cpacf functions) have lower priorities than virtio
> > crypto, and those drivers are faster than the same in
> > the host via virtio. So let's lower the priority of
> > virtio-crypto's algorithm, make it's higher than sofeware
>software
> > implimentations but lower than the hardware ones.
>   implementations
> >
> > Suggested-by: Christian Borntraeger <borntrae...@de.ibm.com>
> > Signed-off-by: Gonglei <arei.gong...@huawei.com>
> > ---
> >  drivers/crypto/virtio/virtio_crypto_algs.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/crypto/virtio/virtio_crypto_algs.c
> b/drivers/crypto/virtio/virtio_crypto_algs.c
> > index 6f40a42..4de4740 100644
> > --- a/drivers/crypto/virtio/virtio_crypto_algs.c
> > +++ b/drivers/crypto/virtio/virtio_crypto_algs.c
> > @@ -498,7 +498,7 @@ void virtio_crypto_ablkcipher_finalize_req(
> >  static struct crypto_alg virtio_crypto_algs[] = { {
> > .cra_name = "cbc(aes)",
> > .cra_driver_name = "virtio_crypto_aes_cbc",
> > -   .cra_priority = 501,
> > +   .cra_priority = 150,
> > .cra_flags = CRYPTO_ALG_TYPE_ABLKCIPHER | CRYPTO_ALG_ASYNC,
> > .cra_blocksize = AES_BLOCK_SIZE,
> > .cra_ctxsize  = sizeof(struct virtio_crypto_ablkcipher_ctx),
> >

--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v8 1/1] crypto: add virtio-crypto driver

[Gonglei (Arei)]

> 
> On Thu, Jan 12, 2017 at 03:10:25PM +0100, Christian Borntraeger wrote:
> > On 01/10/2017 01:56 PM, Christian Borntraeger wrote:
> > > On 01/10/2017 01:36 PM, Gonglei (Arei) wrote:
> > >> Hi,
> > >>
> > >>>
> > >>> On 12/15/2016 03:03 AM, Gonglei wrote:
> > >>> [...]
> > >>>> +
> > >>>> +static struct crypto_alg virtio_crypto_algs[] = { {
> > >>>> +  .cra_name = "cbc(aes)",
> > >>>> +  .cra_driver_name = "virtio_crypto_aes_cbc",
> > >>>> +  .cra_priority = 501,
> > >>>
> > >>>
> > >>> This is still higher than the hardware-accelerators (like intel aesni 
> > >>> or the
> > >>> s390 cpacf functions or the arm hw). aesni and s390/cpacf are supported
> by the
> > >>> hardware virtualization and available to the guests. I do not see a way
> how
> > >>> virtio
> > >>> crypto can be faster than that (in the end it might be cpacf/aesni +
> overhead)
> > >>> instead it will very likely be slower.
> > >>> So we should use a number that is higher than software implementations
> but
> > >>> lower than the hw ones.
> > >>>
> > >>> Just grepping around, the software ones seem be be around 100 and the
> > >>> hardware
> > >>> ones around 200-400. So why was 150 not enough?
> > >>>
> > >> I didn't find a documentation about how we use the priority, and I 
> > >> assumed
> > >> people use virtio-crypto will configure hardware accelerators in the
> > >> host. So I choosed the number which bigger than aesni's priority.
> > >
> > > Yes, but the aesni driver will only bind if there is HW support in the 
> > > guest.
> > > And if aesni is available in the guest (or the s390 aes function from 
> > > cpacf)
> > > it will always be faster than the same in the host via virtio.So your 
> > > priority
> > > should be smaller.
> >
> >
> > any opinion on this?
> 
> Going forward, we might add an emulated aesni device and that might
> become slower than virtio. OTOH if or when this happens, we can solve it
> by adding a priority or a feature flag to virtio to raise its priority.
> 
> So I think I agree with Christian here, let's lower the priority.
> Gonglei, could you send a patch like this?
> 
OK, will do.

Thanks,
-Gonglei
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v8 1/1] crypto: add virtio-crypto driver

[Gonglei (Arei)]

Hi,

> 
> On 12/15/2016 03:03 AM, Gonglei wrote:
> [...]
> > +
> > +static struct crypto_alg virtio_crypto_algs[] = { {
> > +   .cra_name = "cbc(aes)",
> > +   .cra_driver_name = "virtio_crypto_aes_cbc",
> > +   .cra_priority = 501,
> 
> 
> This is still higher than the hardware-accelerators (like intel aesni or the
> s390 cpacf functions or the arm hw). aesni and s390/cpacf are supported by the
> hardware virtualization and available to the guests. I do not see a way how
> virtio
> crypto can be faster than that (in the end it might be cpacf/aesni + overhead)
> instead it will very likely be slower.
> So we should use a number that is higher than software implementations but
> lower than the hw ones.
> 
> Just grepping around, the software ones seem be be around 100 and the
> hardware
> ones around 200-400. So why was 150 not enough?
> 
I didn't find a documentation about how we use the priority, and I assumed
people use virtio-crypto will configure hardware accelerators in the
host. So I choosed the number which bigger than aesni's priority.

Regards,
-Gonglei


--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [virtio-dev] Re: [Qemu-devel] [PATCH v7 1/1] crypto: add virtio-crypto driver

[Gonglei (Arei)]

Hi Michael,

>
> >
> >
> > > Subject: RE: [Qemu-devel] [PATCH v7 1/1] crypto: add virtio-crypto driver
> > >
> > > On Thursday, December 15, 2016 8:45 AM, Gonglei (Arei) Wrote:
> > > < > > diff --git a/drivers/crypto/virtio/virtio_crypto_core.c
> > > < > b/drivers/crypto/virtio/virtio_crypto_core.c
> > > < > > new file mode 100644
> > > < > > index 000..c0854a1
> > > < > > --- /dev/null
> > > < > > +++ b/drivers/crypto/virtio/virtio_crypto_core.c
> > > < > > @@ -0,0 +1,474 @@
> > > < > [..]
> > > < > > +
> > > < > > +static void virtcrypto_dataq_callback(struct virtqueue *vq)
> > > < > > +{
> > > < > > +   struct virtio_crypto *vcrypto = vq->vdev->priv;
> > > < > > +   struct virtio_crypto_request *vc_req;
> > > < > > +   unsigned long flags;
> > > < > > +   unsigned int len;
> > > < > > +   struct ablkcipher_request *ablk_req;
> > > < > > +   int error;
> > > < > > +
> > > < > > +   spin_lock_irqsave(>lock, flags);
> > > < >
> > > < > Would it make sense to use a per virtqueue lock
> > > < > like in virtio_blk for example instead of locking on the whole
> > > < > device? OK, it seems you use only one dataqueue, so it
> > > < > may not be that relevant.
> > > < >
> > > < Currently yes, both the backend device (cryptodev-backend-builtin)
> > > < and the frontend driver use one dataqueue.
> > > <
> > >
> > > I think it makes sense to use per virtqueue lock here though it only uses 
> > > one
> > > queue so far,
> > > but in the spec we already have multi queues support.
> > >
> > Yes, I agree. Will do that in V8 soon.
> > Hope to catch up with Michael's pull request for 4.10.
> >
> > Regards,
> > -Gonglei
> 
> I merged v7, this change will have to wait. Sorry.
> 
That's OK. Thanks!

I can post a separate patch after this pull request. 

Regards,
-Gonglei

--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [Qemu-devel] [PATCH v7 1/1] crypto: add virtio-crypto driver

[Gonglei (Arei)]

Regards,
-Gonglei


> -Original Message-
> From: Zeng, Xin [mailto:xin.z...@intel.com]
> Sent: Thursday, December 15, 2016 8:59 AM
> To: Gonglei (Arei); Halil Pasic; linux-ker...@vger.kernel.org;
> qemu-de...@nongnu.org; virtio-...@lists.oasis-open.org;
> virtualizat...@lists.linux-foundation.org; linux-crypto@vger.kernel.org
> Cc: Huangweidong (C); Claudio Fontana; m...@redhat.com; Luonengjun;
> Hanweidong (Randy); Xuquan (Quan Xu); Wanzongshun (Vincent);
> stefa...@redhat.com; Zhoujian (jay, Euler); cornelia.h...@de.ibm.com;
> longpeng; arei.gong...@hotmail.com; da...@davemloft.net; Wubin (H);
> herb...@gondor.apana.org.au
> Subject: RE: [Qemu-devel] [PATCH v7 1/1] crypto: add virtio-crypto driver
> 
> On Thursday, December 15, 2016 8:45 AM, Gonglei (Arei) Wrote:
> < > > diff --git a/drivers/crypto/virtio/virtio_crypto_core.c
> < > b/drivers/crypto/virtio/virtio_crypto_core.c
> < > > new file mode 100644
> < > > index 000..c0854a1
> < > > --- /dev/null
> < > > +++ b/drivers/crypto/virtio/virtio_crypto_core.c
> < > > @@ -0,0 +1,474 @@
> < > [..]
> < > > +
> < > > +static void virtcrypto_dataq_callback(struct virtqueue *vq)
> < > > +{
> < > > +   struct virtio_crypto *vcrypto = vq->vdev->priv;
> < > > +   struct virtio_crypto_request *vc_req;
> < > > +   unsigned long flags;
> < > > +   unsigned int len;
> < > > +   struct ablkcipher_request *ablk_req;
> < > > +   int error;
> < > > +
> < > > +   spin_lock_irqsave(>lock, flags);
> < >
> < > Would it make sense to use a per virtqueue lock
> < > like in virtio_blk for example instead of locking on the whole
> < > device? OK, it seems you use only one dataqueue, so it
> < > may not be that relevant.
> < >
> < Currently yes, both the backend device (cryptodev-backend-builtin)
> < and the frontend driver use one dataqueue.
> <
> 
> I think it makes sense to use per virtqueue lock here though it only uses one
> queue so far,
> but in the spec we already have multi queues support.
> 
Yes, I agree. Will do that in V8 soon. 
Hope to catch up with Michael's pull request for 4.10.

Regards,
-Gonglei

--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [Qemu-devel] [PATCH v7 1/1] crypto: add virtio-crypto driver

[Gonglei (Arei)]

> 
> 
> On 12/14/2016 12:50 PM, Gonglei wrote:
> > diff --git a/drivers/crypto/virtio/virtio_crypto_core.c
> b/drivers/crypto/virtio/virtio_crypto_core.c
> > new file mode 100644
> > index 000..c0854a1
> > --- /dev/null
> > +++ b/drivers/crypto/virtio/virtio_crypto_core.c
> > @@ -0,0 +1,474 @@
> [..]
> > +
> > +static void virtcrypto_dataq_callback(struct virtqueue *vq)
> > +{
> > +   struct virtio_crypto *vcrypto = vq->vdev->priv;
> > +   struct virtio_crypto_request *vc_req;
> > +   unsigned long flags;
> > +   unsigned int len;
> > +   struct ablkcipher_request *ablk_req;
> > +   int error;
> > +
> > +   spin_lock_irqsave(>lock, flags);
> 
> Would it make sense to use a per virtqueue lock
> like in virtio_blk for example instead of locking on the whole
> device? OK, it seems you use only one dataqueue, so it
> may not be that relevant.
> 
Currently yes, both the backend device (cryptodev-backend-builtin)
and the frontend driver use one dataqueue.

Regards,
-Gonglei

--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v6 2/2] crypto: add virtio-crypto driver

[Gonglei (Arei)]

Hi,

>
> Subject: Re: [PATCH v6 2/2] crypto: add virtio-crypto driver
> 
> 
> 
> On 12/12/2016 11:05 PM, Michael S. Tsirkin wrote:
> > On Mon, Dec 12, 2016 at 06:54:07PM +0800, Herbert Xu wrote:
> >> On Mon, Dec 12, 2016 at 06:25:12AM +, Gonglei (Arei) wrote:
> >>> Hi, Michael & Herbert
> >>>
> >>> Because the virtio-crypto device emulation had been in QEMU 2.8,
> >>> would you please merge the virtio-crypto driver for 4.10 if no other
> >>> comments? If so, Miachel pls ack and/or review the patch, then
> >>> Herbert will take it (I asked him last week). Thank you!
> >>>
> >>> Ps: Note on 4.10 merge window timing from Linus
> >>>  https://lkml.org/lkml/2016/12/7/506
> >>>
> >>> Dec 23rd is the deadline for 4.10 merge window.
> >>
> >> Sorry but it's too late for 4.10.  It needed to have been in my
> >> tree before the merge window opened to make it for this cycle.
> >>
> >> Cheers,
> >
> >
> > Objections to me merging this? I'm preparing my tree right now.
> 
> Got this when testing the most recent version on s390x
> 
> [   20.391074] test 0 (128 bit key, 16 byte blocks): [   20.391078] BUG: using
> smp_processor_id() in preemptible [] code: insmod/97
> [   20.391082] caller is virtio_crypto_ablkcipher_setkey+0x44/0x198
> [   20.391085] CPU: 0 PID: 97 Comm: insmod Not tainted
> 4.9.0-02683-gb62a1ab #46
> [   20.391088] Hardware name: IBM  2964 NC9
> 704  (KVM)
> [   20.391405] Stack:
> [   20.391407]0c0eb6d0 0c0eb760
> 0003 
> [   20.391414]0c0eb800 0c0eb778
> 0c0eb778 0020
> [   20.391420] 000a
> 0020 03ff000a
> [   20.391426]03ff000c 0c0eb7c8
>  
> [   20.391432]07c173c8 001126ba
> 0c0eb760 0c0eb7b8
> [   20.391439] Call Trace:
> [   20.391442] ([<0011259e>] show_trace+0x8e/0xe0)
> [   20.391446]  [<00112670>] show_stack+0x80/0xd8
> [   20.391449]  [<00753ab6>] dump_stack+0x96/0xd8
> [   20.391453]  [<007872e6>]
> check_preemption_disabled+0xfe/0x128
> [   20.391456]  [<00839cc4>]
> virtio_crypto_ablkcipher_setkey+0x44/0x198
> [   20.391459]  [<00705a40>]
> skcipher_setkey_ablkcipher+0x50/0x70
> [   20.391476]  [<03ff80002a48>] test_skcipher_speed+0x328/0xb98
> [tcrypt]
> [   20.391492]  [<03ff800063dc>] do_test+0x1c24/0x28e0 [tcrypt]
> [   20.391509]  [<03ff8001006a>] tcrypt_mod_init+0x6a/0x1000
> [tcrypt]
> [   20.391512]  [<001002cc>] do_one_initcall+0xb4/0x148
> [   20.391515]  [<00298632>] do_init_module+0x7a/0x228
> [   20.391519]  [<001fd380>] load_module+0x2428/0x2de0
> [   20.391522]  [<001fde8a>] SyS_init_module+0x152/0x160
> [   20.391526]  [<009f1306>] system_call+0xd6/0x270
> [   20.391528] no locks held by insmod/97.
> 
> Gonglei, any idea? Did not look into it myself yet.
> 
Thanks for report. You must open CONFIG_DEBUG_PREEMPT,
but I didn't do that before. I open some kernel hacking debug switches to test 
it as well today,
and the following patch can fix it.

diff --git a/drivers/crypto/virtio/virtio_crypto_common.h 
b/drivers/crypto/virtio/virtio_crypto_common.h
index 975404b..518dc7ad 100644
--- a/drivers/crypto/virtio/virtio_crypto_common.h
+++ b/drivers/crypto/virtio/virtio_crypto_common.h
@@ -113,7 +113,13 @@ struct virtio_crypto_request {

 static inline int virtio_crypto_get_current_node(void)
 {
-   return topology_physical_package_id(smp_processor_id());
+   int cpu, node;
+
+   cpu = get_cpu();
+   node = topology_physical_package_id(cpu);
+   put_cpu();
+
+   return node;
 }

Meanwhile I find aother problem, will fix it in the following V7.

Regards,
-Gonglei

--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v6 2/2] crypto: add virtio-crypto driver

[Gonglei (Arei)]

>
> Subject: Re: [PATCH v6 2/2] crypto: add virtio-crypto driver
> 
> On Mon, Dec 12, 2016 at 06:54:07PM +0800, Herbert Xu wrote:
> > On Mon, Dec 12, 2016 at 06:25:12AM +0000, Gonglei (Arei) wrote:
> > > Hi, Michael & Herbert
> > >
> > > Because the virtio-crypto device emulation had been in QEMU 2.8,
> > > would you please merge the virtio-crypto driver for 4.10 if no other
> > > comments? If so, Miachel pls ack and/or review the patch, then
> > > Herbert will take it (I asked him last week). Thank you!
> > >
> > > Ps: Note on 4.10 merge window timing from Linus
> > >  https://lkml.org/lkml/2016/12/7/506
> > >
> > > Dec 23rd is the deadline for 4.10 merge window.
> >
> > Sorry but it's too late for 4.10.  It needed to have been in my
> > tree before the merge window opened to make it for this cycle.
> >
> > Cheers,
> 
> 
> Objections to me merging this? I'm preparing my tree right now.
> 
That's great if so since 4.11 merge window opens 
at least three months later.

Do you agree with it Herbert? Thanks.

Regards,
-Gonglei
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v6 2/2] crypto: add virtio-crypto driver

[Gonglei (Arei)]

Hi, Michael & Herbert

Because the virtio-crypto device emulation had been in QEMU 2.8,
would you please merge the virtio-crypto driver for 4.10 if no other
comments? If so, Miachel pls ack and/or review the patch, then
Herbert will take it (I asked him last week). Thank you!

Ps: Note on 4.10 merge window timing from Linus
 https://lkml.org/lkml/2016/12/7/506

Dec 23rd is the deadline for 4.10 merge window.

Regards,
-Gonglei


> -Original Message-
> From: Gonglei (Arei)
> Sent: Thursday, December 08, 2016 12:37 PM
> Subject: [PATCH v6 2/2] crypto: add virtio-crypto driver
> 
> This patch introduces virtio-crypto driver for Linux Kernel.
> 
> The virtio crypto device is a virtual cryptography device
> as well as a kind of virtual hardware accelerator for
> virtual machines. The encryption anddecryption requests
> are placed in the data queue and are ultimately handled by
> thebackend crypto accelerators. The second queue is the
> control queue used to create or destroy sessions for
> symmetric algorithms and will control some advanced features
> in the future. The virtio crypto device provides the following
> cryptoservices: CIPHER, MAC, HASH, and AEAD.
> 
> For more information about virtio-crypto device, please see:
>   http://qemu-project.org/Features/VirtioCrypto
> 
> CC: Michael S. Tsirkin <m...@redhat.com>
> CC: Cornelia Huck <cornelia.h...@de.ibm.com>
> CC: Stefan Hajnoczi <stefa...@redhat.com>
> CC: Herbert Xu <herb...@gondor.apana.org.au>
> CC: Halil Pasic <pa...@linux.vnet.ibm.com>
> CC: David S. Miller <da...@davemloft.net>
> CC: Zeng Xin <xin.z...@intel.com>
> Signed-off-by: Gonglei <arei.gong...@huawei.com>
> ---
>  MAINTAINERS  |   9 +
>  drivers/crypto/Kconfig   |   2 +
>  drivers/crypto/Makefile  |   1 +
>  drivers/crypto/virtio/Kconfig|  10 +
>  drivers/crypto/virtio/Makefile   |   5 +
>  drivers/crypto/virtio/virtio_crypto_algs.c   | 541
> +++
>  drivers/crypto/virtio/virtio_crypto_common.h | 122 ++
>  drivers/crypto/virtio/virtio_crypto_core.c   | 464
> +++
>  drivers/crypto/virtio/virtio_crypto_mgr.c| 264 +
>  include/uapi/linux/Kbuild|   1 +
>  include/uapi/linux/virtio_crypto.h   | 450
> ++
>  include/uapi/linux/virtio_ids.h  |   1 +
>  12 files changed, 1870 insertions(+)
>  create mode 100644 drivers/crypto/virtio/Kconfig
>  create mode 100644 drivers/crypto/virtio/Makefile
>  create mode 100644 drivers/crypto/virtio/virtio_crypto_algs.c
>  create mode 100644 drivers/crypto/virtio/virtio_crypto_common.h
>  create mode 100644 drivers/crypto/virtio/virtio_crypto_core.c
>  create mode 100644 drivers/crypto/virtio/virtio_crypto_mgr.c
>  create mode 100644 include/uapi/linux/virtio_crypto.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index ad9b965..cccaaf0 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12810,6 +12810,7 @@ F:drivers/net/virtio_net.c
>  F:   drivers/block/virtio_blk.c
>  F:   include/linux/virtio_*.h
>  F:   include/uapi/linux/virtio_*.h
> +F:   drivers/crypto/virtio/
> 
>  VIRTIO DRIVERS FOR S390
>  M:   Christian Borntraeger <borntrae...@de.ibm.com>
> @@ -12846,6 +12847,14 @@ S:   Maintained
>  F:   drivers/virtio/virtio_input.c
>  F:   include/uapi/linux/virtio_input.h
> 
> +VIRTIO CRYPTO DRIVER
> +M:  Gonglei <arei.gong...@huawei.com>
> +L:  virtualizat...@lists.linux-foundation.org
> +L:  linux-crypto@vger.kernel.org
> +S:  Maintained
> +F:  drivers/crypto/virtio/
> +F:  include/uapi/linux/virtio_crypto.h
> +
>  VIA RHINE NETWORK DRIVER
>  S:   Orphan
>  F:   drivers/net/ethernet/via/via-rhine.c
> diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig
> index 4d2b81f..7956478 100644
> --- a/drivers/crypto/Kconfig
> +++ b/drivers/crypto/Kconfig
> @@ -555,4 +555,6 @@ config CRYPTO_DEV_ROCKCHIP
> 
>  source "drivers/crypto/chelsio/Kconfig"
> 
> +source "drivers/crypto/virtio/Kconfig"
> +
>  endif # CRYPTO_HW
> diff --git a/drivers/crypto/Makefile b/drivers/crypto/Makefile
> index ad7250f..bc53cb8 100644
> --- a/drivers/crypto/Makefile
> +++ b/drivers/crypto/Makefile
> @@ -32,3 +32,4 @@ obj-$(CONFIG_CRYPTO_DEV_VMX) += vmx/
>  obj-$(CONFIG_CRYPTO_DEV_SUN4I_SS) += sunxi-ss/
>  obj-$(CONFIG_CRYPTO_DEV_ROCKCHIP) += rockchip/
>  obj-$(CONFIG_CRYPTO_DEV_CHELSIO) += chelsio/
> +obj-$(CONFIG_CRYPTO_DEV_VIRTIO) += virtio/
> diff --git a/drivers/crypto/virtio/Kconfig b/drivers/crypto/virtio/Kconfig
> new file mode 100644
> index 000.

RE: [PATCH v6 1/2] sparc: fix a building error reported by kbuild

[Gonglei (Arei)]

Regards,
-Gonglei


> -Original Message-
> From: linux-crypto-ow...@vger.kernel.org
> [mailto:linux-crypto-ow...@vger.kernel.org] On Behalf Of Sam Ravnborg
> Sent: Saturday, December 10, 2016 5:59 AM
> To: Gonglei (Arei)
> Cc: linux-ker...@vger.kernel.org; qemu-de...@nongnu.org;
> virtio-...@lists.oasis-open.org; virtualizat...@lists.linux-foundation.org;
> linux-crypto@vger.kernel.org; Luonengjun; m...@redhat.com;
> stefa...@redhat.com; Huangweidong (C); Wubin (H); xin.z...@intel.com;
> Claudio Fontana; herb...@gondor.apana.org.au; pa...@linux.vnet.ibm.com;
> da...@davemloft.net; Zhoujian (jay, Euler); Hanweidong (Randy);
> arei.gong...@hotmail.com; cornelia.h...@de.ibm.com; Xuquan (Quan Xu);
> longpeng; Wanzongshun (Vincent); sparcli...@vger.kernel.org
> Subject: Re: [PATCH v6 1/2] sparc: fix a building error reported by kbuild
> 
> Hi Gonglei.
> 
> On Thu, Dec 08, 2016 at 12:37:08PM +0800, Gonglei wrote:
> > >> arch/sparc/include/asm/topology_64.h:44:44:
> > error: implicit declaration of function 'cpu_data'
> > [-Werror=implicit-function-declaration]
> >
> >  #define topology_physical_package_id(cpu) (cpu_data(cpu).proc_id)
> >^
> > Let's include cpudata.h in topology_64.h.
> >
> > Cc: Sam Ravnborg <s...@ravnborg.org>
> > Cc: David S. Miller <da...@davemloft.net>
> > Cc: sparcli...@vger.kernel.org
> > Suggested-by: Sam Ravnborg <s...@ravnborg.org>
> > Signed-off-by: Gonglei <arei.gong...@huawei.com>
> Acked-by: Sam Ravnborg <s...@ravnborg.org>
> 
Thanks.

> > ---
> >  arch/sparc/include/asm/topology_64.h | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/arch/sparc/include/asm/topology_64.h
> b/arch/sparc/include/asm/topology_64.h
> > index 7b4898a..2255430 100644
> > --- a/arch/sparc/include/asm/topology_64.h
> > +++ b/arch/sparc/include/asm/topology_64.h
> > @@ -4,6 +4,7 @@
> >  #ifdef CONFIG_NUMA
> >
> >  #include 
> > +#include 
> 
> Nitpick - if you are going to resend this patch, 

It depends on the maintainer's thought. :)

> then please order the two includes in alphabetic order.
> 
> For two includes this looks like bikeshedding, but when we add
> more having them in a defined arder prevents merge conflicts.
> And makes it readable too.
> 
> We also sometimes order the includes with the longest lines topmost,
> and lines with the ame length are ordered alphabetically.
> But this is not seen so often.
> 

Regards,
-Gonglei
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v5 1/1] crypto: add virtio-crypto driver

[Gonglei (Arei)]

Hi Sam,

>
> Subject: Re: [PATCH v5 1/1] crypto: add virtio-crypto driver
> 
> On Mon, Dec 05, 2016 at 03:12:52AM +0000, Gonglei (Arei) wrote:
> > I don't think the root cause of those warnings are introduced by 
> > virtio-crypto
> driver.
> >
> > What's your opinion? Sam and David?
> 
> Root cause here is that arch/sparc/include/asm/topology_64.h
> references cpu_data without including arch/sparc/include/asm/cpudata.h
> 
> I think other architectures pull in the dependency from
> either smp.h or they have it topology.h.
> 
> The easy fix would be to include cpudata.h in
> arch/sparc/include/asm/topology_64.h.
> And that should also be a correct fix.
> 
> Could you include this in your patch-set and build test it?
> 
Sure, I can add this one at the head of my patch set, but because I
haven't sparc environment, so it depends on the kbuild test robot
to test.

Thanks,
-Gonglei
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v5 1/1] crypto: add virtio-crypto driver

[Gonglei (Arei)]

Hi Herbert,

Would you please review and/or ack the virtio_crypto_algs.c?
It is the realization of specified algs based on Linux Crypto Framework.

Thanks!


Regards,
-Gonglei


> -Original Message-
> From: Gonglei (Arei)
> Sent: Thursday, December 01, 2016 8:39 PM
> To: linux-ker...@vger.kernel.org; qemu-de...@nongnu.org;
> virtio-...@lists.oasis-open.org; virtualizat...@lists.linux-foundation.org;
> linux-crypto@vger.kernel.org
> Cc: Luonengjun; m...@redhat.com; stefa...@redhat.com; Huangweidong (C);
> Wubin (H); xin.z...@intel.com; Claudio Fontana;
> herb...@gondor.apana.org.au; pa...@linux.vnet.ibm.com;
> da...@davemloft.net; Zhoujian (jay, Euler); Hanweidong (Randy);
> arei.gong...@hotmail.com; cornelia.h...@de.ibm.com; Xuquan (Quan Xu);
> longpeng; Wanzongshun (Vincent); Gonglei (Arei)
> Subject: [PATCH v5 1/1] crypto: add virtio-crypto driver
> 
> This patch introduces virtio-crypto driver for Linux Kernel.
> 
> The virtio crypto device is a virtual cryptography device
> as well as a kind of virtual hardware accelerator for
> virtual machines. The encryption anddecryption requests
> are placed in the data queue and are ultimately handled by
> thebackend crypto accelerators. The second queue is the
> control queue used to create or destroy sessions for
> symmetric algorithms and will control some advanced features
> in the future. The virtio crypto device provides the following
> cryptoservices: CIPHER, MAC, HASH, and AEAD.
> 
> For more information about virtio-crypto device, please see:
>   http://qemu-project.org/Features/VirtioCrypto
> 
> CC: Michael S. Tsirkin <m...@redhat.com>
> CC: Cornelia Huck <cornelia.h...@de.ibm.com>
> CC: Stefan Hajnoczi <stefa...@redhat.com>
> CC: Herbert Xu <herb...@gondor.apana.org.au>
> CC: Halil Pasic <pa...@linux.vnet.ibm.com>
> CC: David S. Miller <da...@davemloft.net>
> CC: Zeng Xin <xin.z...@intel.com>
> Signed-off-by: Gonglei <arei.gong...@huawei.com>
> ---
>  MAINTAINERS  |   9 +
>  drivers/crypto/Kconfig   |   2 +
>  drivers/crypto/Makefile  |   1 +
>  drivers/crypto/virtio/Kconfig|  10 +
>  drivers/crypto/virtio/Makefile   |   5 +
>  drivers/crypto/virtio/virtio_crypto_algs.c   | 537
> +++
>  drivers/crypto/virtio/virtio_crypto_common.h | 122 ++
>  drivers/crypto/virtio/virtio_crypto_core.c   | 464
> +++
>  drivers/crypto/virtio/virtio_crypto_mgr.c| 264 +
>  include/uapi/linux/Kbuild|   1 +
>  include/uapi/linux/virtio_crypto.h   | 450
> ++
>  include/uapi/linux/virtio_ids.h  |   1 +
>  12 files changed, 1866 insertions(+)
>  create mode 100644 drivers/crypto/virtio/Kconfig
>  create mode 100644 drivers/crypto/virtio/Makefile
>  create mode 100644 drivers/crypto/virtio/virtio_crypto_algs.c
>  create mode 100644 drivers/crypto/virtio/virtio_crypto_common.h
>  create mode 100644 drivers/crypto/virtio/virtio_crypto_core.c
>  create mode 100644 drivers/crypto/virtio/virtio_crypto_mgr.c
>  create mode 100644 include/uapi/linux/virtio_crypto.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index ad9b965..cccaaf0 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12810,6 +12810,7 @@ F:drivers/net/virtio_net.c
>  F:   drivers/block/virtio_blk.c
>  F:   include/linux/virtio_*.h
>  F:   include/uapi/linux/virtio_*.h
> +F:   drivers/crypto/virtio/
> 
>  VIRTIO DRIVERS FOR S390
>  M:   Christian Borntraeger <borntrae...@de.ibm.com>
> @@ -12846,6 +12847,14 @@ S:   Maintained
>  F:   drivers/virtio/virtio_input.c
>  F:   include/uapi/linux/virtio_input.h
> 
> +VIRTIO CRYPTO DRIVER
> +M:  Gonglei <arei.gong...@huawei.com>
> +L:  virtualizat...@lists.linux-foundation.org
> +L:  linux-crypto@vger.kernel.org
> +S:  Maintained
> +F:  drivers/crypto/virtio/
> +F:  include/uapi/linux/virtio_crypto.h
> +
>  VIA RHINE NETWORK DRIVER
>  S:   Orphan
>  F:   drivers/net/ethernet/via/via-rhine.c
> diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig
> index 4d2b81f..7956478 100644
> --- a/drivers/crypto/Kconfig
> +++ b/drivers/crypto/Kconfig
> @@ -555,4 +555,6 @@ config CRYPTO_DEV_ROCKCHIP
> 
>  source "drivers/crypto/chelsio/Kconfig"
> 
> +source "drivers/crypto/virtio/Kconfig"
> +
>  endif # CRYPTO_HW
> diff --git a/drivers/crypto/Makefile b/drivers/crypto/Makefile
> index ad7250f..bc53cb8 100644
> --- a/drivers/crypto/Makefile
> +++ b/drivers/crypto/Makefile
> @@ -32,3 +32,4 @@ obj-$(CONFIG_CRYPTO_DEV_VMX) += vmx/
>  obj-$(CONFIG_CRYPT

RE: [PATCH v5 1/1] crypto: add virtio-crypto driver

[Gonglei (Arei)]

I don't think the root cause of those warnings are introduced by virtio-crypto 
driver.

What's your opinion? Sam and David?

Thanks,
-Gonglei


> -Original Message-
> From: kbuild test robot [mailto:l...@intel.com]
> Sent: Sunday, December 04, 2016 10:40 AM
> Subject: Re: [PATCH v5 1/1] crypto: add virtio-crypto driver
> 
> Hi Gonglei,
> 
> [auto build test ERROR on cryptodev/master]
> [also build test ERROR on v4.9-rc7 next-20161202]
> [if your patch is applied to the wrong git tree, please drop us a note to help
> improve the system]
> 
> url:
> https://github.com/0day-ci/linux/commits/Gonglei/crypto-add-virtio-crypto-dri
> ver/20161202-190424
> base:
> https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git
> master
> config: sparc64-allyesconfig (attached as .config)
> compiler: sparc64-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
> reproduce:
> wget
> https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cr
> oss -O ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # save the attached .config to linux build tree
> make.cross ARCH=sparc64
> 
> All errors (new ones prefixed by >>):
> 
>In file included from arch/sparc/include/asm/topology.h:4:0,
> from include/linux/topology.h:35,
> from include/linux/gfp.h:8,
> from include/linux/kmod.h:22,
> from include/linux/module.h:13,
> from drivers/crypto/virtio/virtio_crypto_mgr.c:21:
>drivers/crypto/virtio/virtio_crypto_common.h: In function
> 'virtio_crypto_get_current_node':
> >> arch/sparc/include/asm/topology_64.h:44:44: error: implicit declaration of
> function 'cpu_data' [-Werror=implicit-function-declaration]
> #define topology_physical_package_id(cpu) (cpu_data(cpu).proc_id)
>^
>drivers/crypto/virtio/virtio_crypto_common.h:116:9: note: in expansion of
> macro 'topology_physical_package_id'
>  return topology_physical_package_id(smp_processor_id());
> ^~~~
> >> arch/sparc/include/asm/topology_64.h:44:57: error: request for member
> 'proc_id' in something not a structure or union
> #define topology_physical_package_id(cpu) (cpu_data(cpu).proc_id)
> ^
>drivers/crypto/virtio/virtio_crypto_common.h:116:9: note: in expansion of
> macro 'topology_physical_package_id'
>  return topology_physical_package_id(smp_processor_id());
> ^~~~
>cc1: some warnings being treated as errors
> 
> vim +/cpu_data +44 arch/sparc/include/asm/topology_64.h
> 
> f5e706ad include/asm-sparc/topology_64.h  Sam Ravnborg
> 2008-07-17  28
> 9d079337 arch/sparc/include/asm/topology_64.h David Miller
> 2009-01-11  29  #define cpumask_of_pcibus(bus)\
> 9d079337 arch/sparc/include/asm/topology_64.h David Miller
> 2009-01-11  30(pcibus_to_node(bus) == -1 ? \
> e9b37512 arch/sparc/include/asm/topology_64.h Rusty Russell
> 2009-03-16  31 cpu_all_mask : \
> 9d079337 arch/sparc/include/asm/topology_64.h David Miller
> 2009-01-11  32 cpumask_of_node(pcibus_to_node(bus)))
> f5e706ad include/asm-sparc/topology_64.h  Sam Ravnborg
> 2008-07-17  33
> 52708d69 arch/sparc/include/asm/topology_64.h Nitin Gupta
> 2015-11-02  34  int __node_distance(int, int);
> 52708d69 arch/sparc/include/asm/topology_64.h Nitin Gupta
> 2015-11-02  35  #define node_distance(a, b) __node_distance(a, b)
> 52708d69 arch/sparc/include/asm/topology_64.h Nitin Gupta
> 2015-11-02  36
> f5e706ad include/asm-sparc/topology_64.h  Sam Ravnborg
> 2008-07-17  37  #else /* CONFIG_NUMA */
> f5e706ad include/asm-sparc/topology_64.h  Sam Ravnborg
> 2008-07-17  38
> f5e706ad include/asm-sparc/topology_64.h  Sam Ravnborg
> 2008-07-17  39  #include 
> f5e706ad include/asm-sparc/topology_64.h  Sam Ravnborg
> 2008-07-17  40
> f5e706ad include/asm-sparc/topology_64.h  Sam Ravnborg
> 2008-07-17  41  #endif /* !(CONFIG_NUMA) */
> f5e706ad include/asm-sparc/topology_64.h  Sam Ravnborg
> 2008-07-17  42
> f5e706ad include/asm-sparc/topology_64.h  Sam Ravnborg
> 2008-07-17  43  #ifdef CONFIG_SMP
> f5e706ad include/asm-sparc/topology_64.h  Sam Ravnborg
> 2008-07-17 @44  #define topology_physical_package_id(cpu)
>   (cpu_data(cpu).proc_id)
> f5e706ad include/asm-sparc/topology_64.h  Sam Ravnborg
> 2008-07-17  45  #define topology_core_id(cpu)
>   (cpu_data(cpu).core_id)
> acc455cf arch/sparc/include/asm/topology_64.h chris hyser
> 2015-04-22  46  #define topology_core_cpumask(cpu)
>   (_core_sib_map[cpu])
> 06931e62 arch/sparc/include/asm/topology_64.h Bartosz Golaszewski
> 2015-05-26  47  #define topology_sibling_cpumask(cpu)
>   (_cpu(cpu_sibling_map, cpu))
> f5e706ad include/asm-sparc/topology_64.h  Sam Ravnborg
> 2008-07-17  48  #endif /* CONFIG_SMP */
> f5e706ad 

RE: [PATCH v4 1/1] crypto: add virtio-crypto driver

[Gonglei (Arei)]

> 
> On Thu, Dec 01, 2016 at 02:27:19AM +, Gonglei (Arei) wrote:
> > > On Tue, Nov 29, 2016 at 08:48:14PM +0800, Gonglei wrote:
> > > > diff --git a/drivers/crypto/virtio/virtio_crypto_algs.c
> > > b/drivers/crypto/virtio/virtio_crypto_algs.c
> > > > new file mode 100644
> > > > index 000..08b077f
> > > > --- /dev/null
> > > > +++ b/drivers/crypto/virtio/virtio_crypto_algs.c
> > > > @@ -0,0 +1,518 @@
> > > > + /* Algorithms supported by virtio crypto device
> > > > +  *
> > > > +  * Authors: Gonglei <arei.gong...@huawei.com>
> > > > +  *
> > > > +  * Copyright 2016 HUAWEI TECHNOLOGIES CO., LTD.
> > > > +  *
> > > > +  * This program is free software; you can redistribute it and/or 
> > > > modify
> > > > +  * it under the terms of the GNU General Public License as published 
> > > > by
> > > > +  * the Free Software Foundation; either version 2 of the License, or
> > > > +  * (at your option) any later version.
> > > > +  *
> > > > +  * This program is distributed in the hope that it will be useful,
> > > > +  * but WITHOUT ANY WARRANTY; without even the implied warranty
> of
> > > > +  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See
> the
> > > > +  * GNU General Public License for more details.
> > > > +  *
> > > > +  * You should have received a copy of the GNU General Public License
> > > > +  * along with this program; if not, see
> <http://www.gnu.org/licenses/>.
> > > > +  */
> > > > +
> > > > +#include 
> > > > +#include 
> > > > +#include 
> > > > +#include 
> > > > +#include 
> > > > +
> > > > +#include 
> > > > +#include "virtio_crypto_common.h"
> > > > +
> > > > +static DEFINE_MUTEX(algs_lock);
> > >
> > > Did you run checkpatch.pl?  I think it encourages you to document what
> > > the lock protects.
> > >
> > Sure. Basically I run checkpatch.py each time. :)
> >
> > # ./scripts/checkpatch.pl 0001-crypto-add-virtio-crypto-driver.patch
> > total: 0 errors, 0 warnings, 1873 lines checked
> >
> > 0001-crypto-add-virtio-crypto-driver.patch has no obvious style problems and
> is ready for submission.
> 
> Looks like a bug in checkpatch.pl:
> 
> # check for spinlock_t definitions without a comment.
> if ($line =~ /^.\s*(struct\s+mutex|spinlock_t)\s+\S+;/ ||
> $line =~ /^.\s*(DEFINE_MUTEX)\s*\(/) {
> my $which = $1;
> if (!ctx_has_comment($first_line, $linenr)) {
> CHK("UNCOMMENTED_DEFINITION",
> "$1 definition without comment\n" . $herecurr);
> }
> }
> 
> Since your mutex definition has the 'static' keyword in front of it
> checkpatch.pl misses it!
> 
Anyway I added the comments. Thanks :)


Regards,
-Gonglei

> Andy: Is this checkpatch.pl behavior intentional?
> 
> Stefan
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v4 1/1] crypto: add virtio-crypto driver

[Gonglei (Arei)]

>
> Subject: Re: [PATCH v4 1/1] crypto: add virtio-crypto driver
> 
> On Thu, Dec 01, 2016 at 02:27:19AM +0000, Gonglei (Arei) wrote:
> > > On Tue, Nov 29, 2016 at 08:48:14PM +0800, Gonglei wrote:
> > > > +static int virtio_crypto_alg_ablkcipher_init_session(
> > > > +   struct virtio_crypto_ablkcipher_ctx *ctx,
> > > > +   uint32_t alg, const uint8_t *key,
> > > > +   unsigned int keylen,
> > > > +   int encrypt)
> > > > +{
> > > > +   struct scatterlist outhdr, key_sg, inhdr, *sgs[3];
> > > > +   unsigned int tmp;
> > > > +   struct virtio_crypto *vcrypto = ctx->vcrypto;
> > > > +   int op = encrypt ? VIRTIO_CRYPTO_OP_ENCRYPT :
> > > VIRTIO_CRYPTO_OP_DECRYPT;
> > > > +   int err;
> > > > +   unsigned int num_out = 0, num_in = 0;
> > > > +
> > > > +   /*
> > > > +* Avoid to do DMA from the stack, switch to using
> > > > +* dynamically-allocated for the key
> > > > +*/
> > > > +   uint8_t *cipher_key = kmalloc(keylen, GFP_ATOMIC);
> > > > +
> > > > +   if (!cipher_key)
> > > > +   return -ENOMEM;
> > > > +
> > > > +   memcpy(cipher_key, key, keylen);
> > >
> > > Are there any rules on handling key material in the kernel?  This buffer
> > > is just kfreed later.  Do you need to zero it out before freeing it?
> > >
> > Good questions. For kernel crypto core, each cipher request should be freed
> > by skcipher_request_free(): zeroize and free request data structure.
> >
> > I need to use kzfree() for key as well. I'll also check other stuffs. 
> > Thanks.
> >
> > > > +
> > > > +   spin_lock(>ctrl_lock);
> > >
> > > The QAT accelerator driver doesn't spin while talking to the device in
> > > virtio_crypto_alg_ablkcipher_init_session().  I didn't find any other
> > > driver examples in the kernel tree, but this function seems like a
> > > weakness in the virtio-crypto device.
> > >
> > The control queues of virtio-net and virtio-console are also be locked
> > Please see:
> >  __send_control_msg() in virtio_console.c and virtio-net's control queue
> > protected by rtnl lock.
> >
> > I didn't want to protect session creations but the virtqueue's operations
> > like what other virtio devices do.
> >
> > > While QEMU is servicing the create session command this vcpu is blocked.
> > > The QEMU global mutex is held so no other vcpu can enter QEMU and the
> > > QMP monitor is also blocked.
> > >
> > > This is a scalability and performance problem.  Can you look at how QAT
> > > avoids this synchronous session setup?
> >
> > For QAT driver, the session creation is synchronous as well because it's a
> > plain software operation which can be completed ASAP.
> 
> I'm mentioning the vmexit and wait for request completion in a spinlock
> because the same type of issue has been a performance bottleneck with
> virtio guest driver in the past.
> 
> If there is a way to avoid spinning then that would be preferred.  It's
> basically a known anti-pattern for virtio guest drivers.
> 
Oh, sorry for my misunderstanding. ;)

> Could you initialize the session on the host side when the first
> asynchronous data is submitted for encryption/decryption instead of
> during init_session()?
> 
I remember I discuss this problem with Alex two/three moths
ago. In some scenarios, its indeed a performance problem, such as each request 
has
different algorithms or keys in HTTP connections. It's performance will be 
better
if we just use one data virtqueue to pass session and data to the backend at 
one time.

But for the batch cipher operations with the same algorithm, the performance is 
poor,
Because we can't do batch operations for those requests. That's the great 
function
of session operations on the control virtqueue. Refer to the our clients 
requirements
and the existing QAT driver, the scenario is even more in NFV.

As your mention here, It's hard to do this IMO, because the backend can't know
the previous session belongs to which requests if we don't pass the session_id
to the backend.

Regards,
-Gonglei
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v4 1/1] crypto: add virtio-crypto driver

[Gonglei (Arei)]

Hi Stefan,

> 
> On Tue, Nov 29, 2016 at 08:48:14PM +0800, Gonglei wrote:
> > diff --git a/drivers/crypto/virtio/virtio_crypto_algs.c
> b/drivers/crypto/virtio/virtio_crypto_algs.c
> > new file mode 100644
> > index 000..08b077f
> > --- /dev/null
> > +++ b/drivers/crypto/virtio/virtio_crypto_algs.c
> > @@ -0,0 +1,518 @@
> > + /* Algorithms supported by virtio crypto device
> > +  *
> > +  * Authors: Gonglei 
> > +  *
> > +  * Copyright 2016 HUAWEI TECHNOLOGIES CO., LTD.
> > +  *
> > +  * This program is free software; you can redistribute it and/or modify
> > +  * it under the terms of the GNU General Public License as published by
> > +  * the Free Software Foundation; either version 2 of the License, or
> > +  * (at your option) any later version.
> > +  *
> > +  * This program is distributed in the hope that it will be useful,
> > +  * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > +  * GNU General Public License for more details.
> > +  *
> > +  * You should have received a copy of the GNU General Public License
> > +  * along with this program; if not, see .
> > +  */
> > +
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +
> > +#include 
> > +#include "virtio_crypto_common.h"
> > +
> > +static DEFINE_MUTEX(algs_lock);
> 
> Did you run checkpatch.pl?  I think it encourages you to document what
> the lock protects.
> 
Sure. Basically I run checkpatch.py each time. :)

# ./scripts/checkpatch.pl 0001-crypto-add-virtio-crypto-driver.patch 
total: 0 errors, 0 warnings, 1873 lines checked

0001-crypto-add-virtio-crypto-driver.patch has no obvious style problems and is 
ready for submission.

> > +static int virtio_crypto_alg_ablkcipher_init_session(
> > +   struct virtio_crypto_ablkcipher_ctx *ctx,
> > +   uint32_t alg, const uint8_t *key,
> > +   unsigned int keylen,
> > +   int encrypt)
> > +{
> > +   struct scatterlist outhdr, key_sg, inhdr, *sgs[3];
> > +   unsigned int tmp;
> > +   struct virtio_crypto *vcrypto = ctx->vcrypto;
> > +   int op = encrypt ? VIRTIO_CRYPTO_OP_ENCRYPT :
> VIRTIO_CRYPTO_OP_DECRYPT;
> > +   int err;
> > +   unsigned int num_out = 0, num_in = 0;
> > +
> > +   /*
> > +* Avoid to do DMA from the stack, switch to using
> > +* dynamically-allocated for the key
> > +*/
> > +   uint8_t *cipher_key = kmalloc(keylen, GFP_ATOMIC);
> > +
> > +   if (!cipher_key)
> > +   return -ENOMEM;
> > +
> > +   memcpy(cipher_key, key, keylen);
> 
> Are there any rules on handling key material in the kernel?  This buffer
> is just kfreed later.  Do you need to zero it out before freeing it?
> 
Good questions. For kernel crypto core, each cipher request should be freed
by skcipher_request_free(): zeroize and free request data structure.

I need to use kzfree() for key as well. I'll also check other stuffs. Thanks. 

> > +
> > +   spin_lock(>ctrl_lock);
> 
> The QAT accelerator driver doesn't spin while talking to the device in
> virtio_crypto_alg_ablkcipher_init_session().  I didn't find any other
> driver examples in the kernel tree, but this function seems like a
> weakness in the virtio-crypto device.
> 
The control queues of virtio-net and virtio-console are also be locked
Please see:
 __send_control_msg() in virtio_console.c and virtio-net's control queue
protected by rtnl lock.

I didn't want to protect session creations but the virtqueue's operations
like what other virtio devices do.

> While QEMU is servicing the create session command this vcpu is blocked.
> The QEMU global mutex is held so no other vcpu can enter QEMU and the
> QMP monitor is also blocked.
> 
> This is a scalability and performance problem.  Can you look at how QAT
> avoids this synchronous session setup?

For QAT driver, the session creation is synchronous as well because it's a
plain software operation which can be completed ASAP.

Regards,
-Gonglei
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [virtio-dev] Re: [PATCH v3] crypto: add virtio-crypto driver

[Gonglei (Arei)]

>
> Subject: Re: [virtio-dev] Re: [PATCH v3] crypto: add virtio-crypto driver
> 
> On Tue, 29 Nov 2016 01:37:44 +0000
> "Gonglei (Arei)" <arei.gong...@huawei.com> wrote:
> 
> > > On Mon, 28 Nov 2016 20:08:23 +0800
> > > Gonglei <arei.gong...@huawei.com> wrote:
> > >
> > > > +static int virtcrypto_update_status(struct virtio_crypto *vcrypto)
> > > > +{
> > > > +   u32 status;
> > > > +   int err;
> > > > +
> > > > +   virtio_cread(vcrypto->vdev,
> > > > +   struct virtio_crypto_config, status, );
> > > > +
> > > > +   /* Ignore unknown (future) status bits */
> > > > +   status &= VIRTIO_CRYPTO_S_HW_READY;
> > >
> > > I'm wondering what the driver really should do if it encounters unknown
> > > status bits.
> > >
> > > I'd expect that new status bits are guarded by a feature bit and that
> > > the device should not set status bits if the respective feature bit has
> > > not been negotiated. Therefore, unknown status bits would be a host
> > > error and the driver should consider the device to be broken.
> > >
> > > Thoughts?
> > >
> > I agree with you.
> >
> > The reasonable way is reset the device if the driver
> > receive an unknown status IMO.
> 
> What about setting FAILED in the generic virtio status? This indicates
> to the host that the driver 'has given up on the device', as the spec
> puts it. If the driver simply resets it, chances are that we will end
> up in the same situation again (after all, that's a host bug).
> 
> Or/additionally use virtio_break_device(), as a quick grep revealed
> that qemu, for one, does not do anything with FAILED. That way at least
> the driver will stop mucking with the device.
> 
I prefer to the second way. The device set the incorrect status,
then the driver prevent the device from being used and print some
error message to notice that.

Patch will go.

Regards,
-Gonglei
> 
> -
> To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
> For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org

--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v3] crypto: add virtio-crypto driver

[Gonglei (Arei)]

> 
> > On Tue, Nov 29, 2016 at 08:22:58AM +, Gonglei (Arei) wrote:
> > > Hi,
> > >
> > > > > > > +source "drivers/crypto/virtio/Kconfig"
> > > > > > > +
> > > > > > >  endif # CRYPTO_HW
> > > > > > > diff --git a/drivers/crypto/Makefile b/drivers/crypto/Makefile
> > > > > > > index ad7250f..bc53cb8 100644
> > > > > > > --- a/drivers/crypto/Makefile
> > > > > > > +++ b/drivers/crypto/Makefile
> > > > > > > @@ -32,3 +32,4 @@ obj-$(CONFIG_CRYPTO_DEV_VMX) += vmx/
> > > > > > >  obj-$(CONFIG_CRYPTO_DEV_SUN4I_SS) += sunxi-ss/
> > > > > > >  obj-$(CONFIG_CRYPTO_DEV_ROCKCHIP) += rockchip/
> > > > > > >  obj-$(CONFIG_CRYPTO_DEV_CHELSIO) += chelsio/
> > > > > > > +obj-$(CONFIG_CRYPTO_DEV_VIRTIO) += virtio/
> > > > > > > diff --git a/drivers/crypto/virtio/Kconfig
> b/drivers/crypto/virtio/Kconfig
> > > > > > > new file mode 100644
> > > > > > > index 000..ceae88c
> > > > > > > --- /dev/null
> > > > > > > +++ b/drivers/crypto/virtio/Kconfig
> > > > > > > @@ -0,0 +1,10 @@
> > > > > > > +config CRYPTO_DEV_VIRTIO
> > > > > > > + tristate "VirtIO crypto driver"
> > > > > > > + depends on VIRTIO
> > > > > > > +select CRYPTO_AEAD
> > > > > > > +select CRYPTO_AUTHENC
> > > > > > > +select CRYPTO_BLKCIPHER
> > > > > >
> > > > > > Inconsistent tab vs space whitespace usage.
> > > > > >
> > > > Will fix.
> > > >
> > > > > > > + default m
> > > > > > > + help
> > > > > > > +   This driver provides support for virtio crypto device. If you
> > > > > > > +   choose 'M' here, this module will be called virtio-crypto.
> > > > > >
> > > > > > All the other virtio drivers use underscore ('_') instead of hyphen
> > > > > > ('-').
> > > > >
> > > > > Except virtio-rng.
> > > > >
> > > > > >  I suggest calling it virtio_crypto for consistency.
> > > > > >
> > > > OK, I will change the Makefile to fix it.
> > > >
> > >
> > > I tried to do this, but I failed. Because virtio_crypto.ko
> > > consists of more than one source file which include tree files currently,
> > > and virtio_crypto.ko matchs the name of virtio_crypto.c.
> > > That's different with all other virtio drivers.
> >
> > Can you rename virtio_crypto.c to virtio_crypto_core.c?
> 
> +1
> 
> I think that's the way to go.

Cool, here we go.

Regards,
-Gonglei
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v3] crypto: add virtio-crypto driver

[Gonglei (Arei)]

> 
> On Tue, Nov 29, 2016 at 08:22:58AM +, Gonglei (Arei) wrote:
> > Hi,
> >
> > > > > > +source "drivers/crypto/virtio/Kconfig"
> > > > > > +
> > > > > >  endif # CRYPTO_HW
> > > > > > diff --git a/drivers/crypto/Makefile b/drivers/crypto/Makefile
> > > > > > index ad7250f..bc53cb8 100644
> > > > > > --- a/drivers/crypto/Makefile
> > > > > > +++ b/drivers/crypto/Makefile
> > > > > > @@ -32,3 +32,4 @@ obj-$(CONFIG_CRYPTO_DEV_VMX) += vmx/
> > > > > >  obj-$(CONFIG_CRYPTO_DEV_SUN4I_SS) += sunxi-ss/
> > > > > >  obj-$(CONFIG_CRYPTO_DEV_ROCKCHIP) += rockchip/
> > > > > >  obj-$(CONFIG_CRYPTO_DEV_CHELSIO) += chelsio/
> > > > > > +obj-$(CONFIG_CRYPTO_DEV_VIRTIO) += virtio/
> > > > > > diff --git a/drivers/crypto/virtio/Kconfig 
> > > > > > b/drivers/crypto/virtio/Kconfig
> > > > > > new file mode 100644
> > > > > > index 000..ceae88c
> > > > > > --- /dev/null
> > > > > > +++ b/drivers/crypto/virtio/Kconfig
> > > > > > @@ -0,0 +1,10 @@
> > > > > > +config CRYPTO_DEV_VIRTIO
> > > > > > +   tristate "VirtIO crypto driver"
> > > > > > +   depends on VIRTIO
> > > > > > +select CRYPTO_AEAD
> > > > > > +select CRYPTO_AUTHENC
> > > > > > +select CRYPTO_BLKCIPHER
> > > > >
> > > > > Inconsistent tab vs space whitespace usage.
> > > > >
> > > Will fix.
> > >
> > > > > > +   default m
> > > > > > +   help
> > > > > > + This driver provides support for virtio crypto device. If you
> > > > > > + choose 'M' here, this module will be called virtio-crypto.
> > > > >
> > > > > All the other virtio drivers use underscore ('_') instead of hyphen
> > > > > ('-').
> > > >
> > > > Except virtio-rng.
> > > >
> > > > >  I suggest calling it virtio_crypto for consistency.
> > > > >
> > > OK, I will change the Makefile to fix it.
> > >
> >
> > I tried to do this, but I failed. Because virtio_crypto.ko
> > consists of more than one source file which include tree files currently,
> > and virtio_crypto.ko matchs the name of virtio_crypto.c.
> > That's different with all other virtio drivers.
> 
> Can you rename virtio_crypto.c to virtio_crypto_core.c?
> 
I'm fine with virtio_crypto_core.c. What about you? Michael?

Regards,
-Gonglei

> I'm not very familiar with the kernel Makefile infrastructure so maybe
> there's a better way of doing it.
> 
> Stefan
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v3] crypto: add virtio-crypto driver

[Gonglei (Arei)]

Hi,

> > > > +source "drivers/crypto/virtio/Kconfig"
> > > > +
> > > >  endif # CRYPTO_HW
> > > > diff --git a/drivers/crypto/Makefile b/drivers/crypto/Makefile
> > > > index ad7250f..bc53cb8 100644
> > > > --- a/drivers/crypto/Makefile
> > > > +++ b/drivers/crypto/Makefile
> > > > @@ -32,3 +32,4 @@ obj-$(CONFIG_CRYPTO_DEV_VMX) += vmx/
> > > >  obj-$(CONFIG_CRYPTO_DEV_SUN4I_SS) += sunxi-ss/
> > > >  obj-$(CONFIG_CRYPTO_DEV_ROCKCHIP) += rockchip/
> > > >  obj-$(CONFIG_CRYPTO_DEV_CHELSIO) += chelsio/
> > > > +obj-$(CONFIG_CRYPTO_DEV_VIRTIO) += virtio/
> > > > diff --git a/drivers/crypto/virtio/Kconfig 
> > > > b/drivers/crypto/virtio/Kconfig
> > > > new file mode 100644
> > > > index 000..ceae88c
> > > > --- /dev/null
> > > > +++ b/drivers/crypto/virtio/Kconfig
> > > > @@ -0,0 +1,10 @@
> > > > +config CRYPTO_DEV_VIRTIO
> > > > +   tristate "VirtIO crypto driver"
> > > > +   depends on VIRTIO
> > > > +select CRYPTO_AEAD
> > > > +select CRYPTO_AUTHENC
> > > > +select CRYPTO_BLKCIPHER
> > >
> > > Inconsistent tab vs space whitespace usage.
> > >
> Will fix.
> 
> > > > +   default m
> > > > +   help
> > > > + This driver provides support for virtio crypto device. If you
> > > > + choose 'M' here, this module will be called virtio-crypto.
> > >
> > > All the other virtio drivers use underscore ('_') instead of hyphen
> > > ('-').
> >
> > Except virtio-rng.
> >
> > >  I suggest calling it virtio_crypto for consistency.
> > >
> OK, I will change the Makefile to fix it.
> 

I tried to do this, but I failed. Because virtio_crypto.ko
consists of more than one source file which include tree files currently,
and virtio_crypto.ko matchs the name of virtio_crypto.c.
That's different with all other virtio drivers.

The Makefile can't address this situation well, I googled it, and I find a way 
in
 
http://stackoverflow.com/questions/13606075/building-a-kernel-module-from-several-source-files-which-one-of-them-has-the-sam

 Proper way to fix in kernel make file would be as:

# 
obj-m += module.o

#append other source files except module.c which would be include by default
module-objs += src1.o src2.o

Unfortunately it doesn't work because virtio_crypto.c isn't compiled.

# insmod virtio_crypto.ko 
insmod: ERROR: could not insert module virtio_crypto.ko: Unknown symbol in 
module

# dmesg
[74339.311801] virtio_crypto: Unknown symbol virtqueue_is_broken (err 0)
[74339.311816] virtio_crypto: Unknown symbol crypto_register_algs (err 0)
[74339.311833] virtio_crypto: Unknown symbol virtqueue_add_sgs (err 0)
[74339.311839] virtio_crypto: Unknown symbol virtqueue_get_buf (err 0)
[74339.311844] virtio_crypto: Unknown symbol virtqueue_kick (err 0)
[74339.311854] virtio_crypto: Unknown symbol crypto_ablkcipher_type (err 0)
[74339.311860] virtio_crypto: Unknown symbol crypto_unregister_algs (err 0)

It seems that I have no choice but to name the module to 'virtio-crypto' for 
simplicity.

Regards,
-Gonglei
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v3] crypto: add virtio-crypto driver

[Gonglei (Arei)]

>
> > > > +
> > > > +/* Note: kernel crypto API realization */
> > > > +static int virtio_crypto_ablkcipher_setkey(struct crypto_ablkcipher 
> > > > *tfm,
> > > > +const uint8_t *key,
> > > > +unsigned int keylen)
> > > > +{
> > > > +   struct virtio_crypto_ablkcipher_ctx *ctx =
> > crypto_ablkcipher_ctx(tfm);
> > > > +   int ret;
> > > > +
> > > > +   spin_lock(>lock);
> > > > +
> > > > +   if (!ctx->vcrypto) {
> > > > +   /* New key */
> > > > +   int node = virtio_crypto_get_current_node();
> > > > +   struct virtio_crypto *vcrypto =
> > > > + virtcrypto_get_dev_node(node);
> > > > +   if (!vcrypto) {
> > > > +   vcrypto = virtcrypto_devmgr_get_first();
> >
> > Is this the standard way to do this? How does this work with
> > multiple crypto devices (e.g. with different capabilities)?
> >
> Actually there is a simple schedule algorithms in virtcrypto_get_dev_node(),
> which return the device used fewest on the node.
> 
> If we don't find a device in the node, select the first on as default.
> But I forgot to check the first devices whether the device has started here.
> 
Oh, the virtcrypto_get_dev_node() had done this work, the calling of
virtcrypto_devmgr_get_first() here is superfluous. Will remove it.

Regards,
-Gonglei

--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v3] crypto: add virtio-crypto driver

[Gonglei (Arei)]

Hi Halil,

> 
> On 11/28/2016 06:19 PM, Michael S. Tsirkin wrote:
> >>> +static int virtio_crypto_alg_ablkcipher_init_session(
> >>> > > + struct virtio_crypto_ablkcipher_ctx *ctx,
> >>> > > + uint32_t alg, const uint8_t *key,
> >>> > > + unsigned int keylen,
> >>> > > + int encrypt)
> >>> > > +{
> >>> > > + struct scatterlist outhdr, key_sg, inhdr, *sgs[3];
> >>> > > + unsigned int tmp;
> >>> > > + struct virtio_crypto *vcrypto = ctx->vcrypto;
> >>> > > + int op = encrypt ? VIRTIO_CRYPTO_OP_ENCRYPT :
> VIRTIO_CRYPTO_OP_DECRYPT;
> >>> > > + int err;
> >>> > > + unsigned int num_out = 0, num_in = 0;
> >>> > > +
> >>> > > + /*
> >>> > > +  * Avoid to do DMA from the stack, switch to using
> >>> > > +  * dynamically-allocated for the key
> >>> > > +  */
> >>> > > + uint8_t *cipher_key = kmalloc(keylen, GFP_ATOMIC);
> >>> > > +
> >>> > > + if (!cipher_key)
> >>> > > + return -ENOMEM;
> >>> > > +
> >>> > > + memcpy(cipher_key, key, keylen);
> >>> > > +
> >>> > > + spin_lock(>ctrl_lock);
> >>> > > + /* Pad ctrl header */
> >>> > > + vcrypto->ctrl.header.opcode =
> >>> > > + cpu_to_le32(VIRTIO_CRYPTO_CIPHER_CREATE_SESSION);
> >>> > > + vcrypto->ctrl.header.algo = cpu_to_le32(alg);
> >>> > > + /* Set the default dataqueue id to 0 */
> >>> > > + vcrypto->ctrl.header.queue_id = 0;
> >>> > > +
> >>> > > + vcrypto->input.status = cpu_to_le32(VIRTIO_CRYPTO_ERR);
> >>> > > + /* Pad cipher's parameters */
> >>> > > + vcrypto->ctrl.u.sym_create_session.op_type =
> >>> > > + cpu_to_le32(VIRTIO_CRYPTO_SYM_OP_CIPHER);
> >>> > > + vcrypto->ctrl.u.sym_create_session.u.cipher.para.algo =
> >>> > > + vcrypto->ctrl.header.algo;
> >>> > > + vcrypto->ctrl.u.sym_create_session.u.cipher.para.keylen =
> >>> > > + cpu_to_le32(keylen);
> >>> > > + vcrypto->ctrl.u.sym_create_session.u.cipher.para.op =
> >>> > > + cpu_to_le32(op);
> >>> > > +
> >>> > > + sg_init_one(, >ctrl, sizeof(vcrypto->ctrl));
> >>> > > + sgs[num_out++] = 
> >>> > > +
> >>> > > + /* Set key */
> >>> > > + sg_init_one(_sg, cipher_key, keylen);
> >>> > > + sgs[num_out++] = _sg;
> >>> > > +
> >>> > > + /* Return status and session id back */
> >>> > > + sg_init_one(, >input, sizeof(vcrypto->input));
> >>> > > + sgs[num_out + num_in++] = 
> >>> > > +
> >>> > > + err = virtqueue_add_sgs(vcrypto->ctrl_vq, sgs, num_out,
> >>> > > + num_in, vcrypto, GFP_ATOMIC);
> >>> > > + if (err < 0) {
> >>> > > + spin_unlock(>ctrl_lock);
> >>> > > + kfree(cipher_key);
> >>> > > + return err;
> >>> > > + }
> >>> > > + virtqueue_kick(vcrypto->ctrl_vq);
> >>> > > +
> >>> > > + /*
> >>> > > +  * Spin for a response, the kick causes an ioport write, 
> >>> > > trapping
> >>> > > +  * into the hypervisor, so the request should be handled 
> >>> > > immediately.
> >>> > > +  */
> 
> I have my doubts about this comment (and about the code below too). Is
> 'kick causes an ioport write' true for every transport/architecture?
> If we relay on this property maybe the documentation of notify should
> mention it.
> 
Actually it isn't true for every transport, see the call trace:
 /**
 * virtqueue_kick - update after add_buf
 * @vq: the struct virtqueue
 *
 * After one or more virtqueue_add_* calls, invoke this to kick
 * the other side.
 *
 * Caller must ensure we don't call this with other virtqueue
 * operations at the same time (except where noted).
 *
 * Returns false if kick failed, otherwise true.
 */
bool virtqueue_kick(struct virtqueue *vq)
{
if (virtqueue_kick_prepare(vq))
return virtqueue_notify(vq);
return true;
}

If virtqueue_kick_prepare return ture, then notify which causes an ioport write.

Let me remove the comments avoid confusions. 

> I know we have the same message in virtio-net.
> 
Yes, I just migrated it from virtio-net. ;)

> >>> > > + while (!virtqueue_get_buf(vcrypto->ctrl_vq, ) &&
> >>> > > +!virtqueue_is_broken(vcrypto->ctrl_vq))
> >>> > > + cpu_relax();
> > this spin under lock is kind of ugly.
> > Why do we need to hold it while spinning?
> > to prevent submitting more than one request?
> > Isn't there a way to control this within crypto core?
> >
> > unlock
> > relax
> > lock
> >
> > would be better.
> >
> >>> > > +
> >>> > > + if (le32_to_cpu(vcrypto->input.status) != VIRTIO_CRYPTO_OK) {
> >>> > > + spin_unlock(>ctrl_lock);
> >>> > > + pr_err("virtio_crypto: Create session failed status: 
> >>> > > %u\n",
> >>> > > + le32_to_cpu(vcrypto->input.status));
> >>> > > + kfree(cipher_key);
> >>> > > + return -EINVAL;
> >>> > > + }
> >>> > > + spin_unlock(>ctrl_lock);
> >>> > > +
> > You 

RE: [PATCH v3] crypto: add virtio-crypto driver

[Gonglei (Arei)]

Hi Michael and Stefan,

>
> Subject: Re: [PATCH v3] crypto: add virtio-crypto driver
> 
> On Mon, Nov 28, 2016 at 04:20:55PM +, Stefan Hajnoczi wrote:
> > On Mon, Nov 28, 2016 at 08:08:23PM +0800, Gonglei wrote:
> > > This patch introduces virtio-crypto driver for Linux Kernel.
> > >
> > > The virtio crypto device is a virtual cryptography device
> > > as well as a kind of virtual hardware accelerator for
> > > virtual machines. The encryption anddecryption requests
> > > are placed in the data queue and are ultimately handled by
> > > thebackend crypto accelerators. The second queue is the
> > > control queue used to create or destroy sessions for
> > > symmetric algorithms and will control some advanced features
> > > in the future. The virtio crypto device provides the following
> > > cryptoservices: CIPHER, MAC, HASH, and AEAD.
> > >
> > > For more information about virtio-crypto device, please see:
> > >   http://qemu-project.org/Features/VirtioCrypto
> >
> > I've left some comments below.
> >

Thanks! Make pretty senses.

> > >
> > > CC: Michael S. Tsirkin 
> > > CC: Cornelia Huck 
> > > CC: Stefan Hajnoczi 
> > > CC: Herbert Xu 
> > > CC: Halil Pasic 
> > > CC: David S. Miller 
> > > CC: Zeng Xin 
> > > Signed-off-by: Gonglei 
> > > ---
> > >  MAINTAINERS  |   9 +
> > >  drivers/crypto/Kconfig   |   2 +
> > >  drivers/crypto/Makefile  |   1 +
> > >  drivers/crypto/virtio/Kconfig|  10 +
> > >  drivers/crypto/virtio/Makefile   |   5 +
> > >  drivers/crypto/virtio/virtio_crypto.c| 451
> +++
> > >  drivers/crypto/virtio/virtio_crypto_algs.c   | 525
> +++
> > >  drivers/crypto/virtio/virtio_crypto_common.h | 124 +++
> > >  drivers/crypto/virtio/virtio_crypto_mgr.c| 258 +
> > >  include/uapi/linux/Kbuild|   1 +
> > >  include/uapi/linux/virtio_crypto.h   | 450
> +++
> > >  include/uapi/linux/virtio_ids.h  |   1 +
> > >  12 files changed, 1837 insertions(+)
> > >  create mode 100644 drivers/crypto/virtio/Kconfig
> > >  create mode 100644 drivers/crypto/virtio/Makefile
> > >  create mode 100644 drivers/crypto/virtio/virtio_crypto.c
> > >  create mode 100644 drivers/crypto/virtio/virtio_crypto_algs.c
> > >  create mode 100644 drivers/crypto/virtio/virtio_crypto_common.h
> > >  create mode 100644 drivers/crypto/virtio/virtio_crypto_mgr.c
> > >  create mode 100644 include/uapi/linux/virtio_crypto.h
> > >
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index ad9b965..cccaaf0 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -12810,6 +12810,7 @@ F:drivers/net/virtio_net.c
> > >  F:   drivers/block/virtio_blk.c
> > >  F:   include/linux/virtio_*.h
> > >  F:   include/uapi/linux/virtio_*.h
> > > +F:   drivers/crypto/virtio/
> > >
> > >  VIRTIO DRIVERS FOR S390
> > >  M:   Christian Borntraeger 
> > > @@ -12846,6 +12847,14 @@ S:   Maintained
> > >  F:   drivers/virtio/virtio_input.c
> > >  F:   include/uapi/linux/virtio_input.h
> > >
> > > +VIRTIO CRYPTO DRIVER
> > > +M:  Gonglei 
> > > +L:  virtualizat...@lists.linux-foundation.org
> > > +L:  linux-crypto@vger.kernel.org
> > > +S:  Maintained
> > > +F:  drivers/crypto/virtio/
> > > +F:  include/uapi/linux/virtio_crypto.h
> > > +
> > >  VIA RHINE NETWORK DRIVER
> > >  S:   Orphan
> > >  F:   drivers/net/ethernet/via/via-rhine.c
> > > diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig
> > > index 4d2b81f..7956478 100644
> > > --- a/drivers/crypto/Kconfig
> > > +++ b/drivers/crypto/Kconfig
> > > @@ -555,4 +555,6 @@ config CRYPTO_DEV_ROCKCHIP
> > >
> > >  source "drivers/crypto/chelsio/Kconfig"
> > >
> > > +source "drivers/crypto/virtio/Kconfig"
> > > +
> > >  endif # CRYPTO_HW
> > > diff --git a/drivers/crypto/Makefile b/drivers/crypto/Makefile
> > > index ad7250f..bc53cb8 100644
> > > --- a/drivers/crypto/Makefile
> > > +++ b/drivers/crypto/Makefile
> > > @@ -32,3 +32,4 @@ obj-$(CONFIG_CRYPTO_DEV_VMX) += vmx/
> > >  obj-$(CONFIG_CRYPTO_DEV_SUN4I_SS) += sunxi-ss/
> > >  obj-$(CONFIG_CRYPTO_DEV_ROCKCHIP) += rockchip/
> > >  obj-$(CONFIG_CRYPTO_DEV_CHELSIO) += chelsio/
> > > +obj-$(CONFIG_CRYPTO_DEV_VIRTIO) += virtio/
> > > diff --git a/drivers/crypto/virtio/Kconfig b/drivers/crypto/virtio/Kconfig
> > > new file mode 100644
> > > index 000..ceae88c
> > > --- /dev/null
> > > +++ b/drivers/crypto/virtio/Kconfig
> > > @@ -0,0 +1,10 @@
> > > +config CRYPTO_DEV_VIRTIO
> > > + tristate "VirtIO crypto driver"
> > > + depends on VIRTIO
> > > +select CRYPTO_AEAD
> > > +select CRYPTO_AUTHENC
> > > +select CRYPTO_BLKCIPHER
> >
> 

RE: [virtio-dev] Re: [PATCH v3] crypto: add virtio-crypto driver

[Gonglei (Arei)]

>
> Subject: [virtio-dev] Re: [PATCH v3] crypto: add virtio-crypto driver
> 
> On Mon, 28 Nov 2016 20:08:23 +0800
> Gonglei  wrote:
> 
> > +static int virtcrypto_update_status(struct virtio_crypto *vcrypto)
> > +{
> > +   u32 status;
> > +   int err;
> > +
> > +   virtio_cread(vcrypto->vdev,
> > +   struct virtio_crypto_config, status, );
> > +
> > +   /* Ignore unknown (future) status bits */
> > +   status &= VIRTIO_CRYPTO_S_HW_READY;
> 
> I'm wondering what the driver really should do if it encounters unknown
> status bits.
> 
> I'd expect that new status bits are guarded by a feature bit and that
> the device should not set status bits if the respective feature bit has
> not been negotiated. Therefore, unknown status bits would be a host
> error and the driver should consider the device to be broken.
> 
> Thoughts?
> 
I agree with you. 

The reasonable way is reset the device if the driver
receive an unknown status IMO.

Regards,
-Gonglei

> > +
> > +   if (vcrypto->status == status)
> > +   return 0;
> > +
> > +   vcrypto->status = status;
> > +
> > +   if (vcrypto->status & VIRTIO_CRYPTO_S_HW_READY) {
> > +   err = virtcrypto_dev_start(vcrypto);
> > +   if (err) {
> > +   dev_err(>vdev->dev,
> > +   "Failed to start virtio crypto device.\n");
> > +   virtcrypto_dev_stop(vcrypto);
> > +   return -EPERM;
> > +   }
> > +   dev_info(>vdev->dev, "Accelerator is ready\n");
> > +   } else {
> > +   virtcrypto_dev_stop(vcrypto);
> > +   dev_info(>vdev->dev, "Accelerator is not ready\n");
> > +   }
> > +
> > +   return 0;
> > +}
> > +
> 
> 
> -
> To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
> For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org

--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [virtio-dev] Re: [PATCH v2 2/2] crypto: add virtio-crypto driver

[Gonglei (Arei)]

> 
> From: Michael S. Tsirkin [mailto:m...@redhat.com]
> Sent: Monday, November 28, 2016 1:09 PM
> To: Gonglei (Arei)
> Subject: Re: [virtio-dev] Re: [PATCH v2 2/2] crypto: add virtio-crypto driver
> 
> On Mon, Nov 28, 2016 at 04:47:21AM +, Gonglei (Arei) wrote:
> > Michael, I'd like to add virtio-crypto stuff to your maintaining part likes
> > the virtio-net/blk parts so that the corresponding patches
> > can be CC'ed to you too because the virtio-crypto doesn't lay in
> > driver/virtio directory. Will you?
> >
> > Thanks!
> > -Gonglei
> 
> Sure, that's fine.
> 
Will do in the next version.  :)

Thanks,
-Gonglei
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [virtio-dev] Re: [PATCH v2 2/2] crypto: add virtio-crypto driver

[Gonglei (Arei)]

Hi Michael,

>
> Subject: [virtio-dev] Re: [PATCH v2 2/2] crypto: add virtio-crypto driver
> 
> On Tue, Nov 22, 2016 at 04:10:23PM +0800, Gonglei wrote:
> > This patch introduces virtio-crypto driver for Linux Kernel.
> >
> > The virtio crypto device is a virtual cryptography device
> > as well as a kind of virtual hardware accelerator for
> > virtual machines. The encryption anddecryption requests
> > are placed in the data queue and are ultimately handled by
> > thebackend crypto accelerators. The second queue is the
> > control queue used to create or destroy sessions for
> > symmetric algorithms and will control some advanced features
> > in the future. The virtio crypto device provides the following
> > cryptoservices: CIPHER, MAC, HASH, and AEAD.
> >
> > For more information about virtio-crypto device, please see:
> >   http://qemu-project.org/Features/VirtioCrypto
> >
> > CC: Michael S. Tsirkin 
> > CC: Cornelia Huck 
> > CC: Stefan Hajnoczi 
> > CC: Herbert Xu 
> > CC: Halil Pasic 
> > CC: David S. Miller 
> > CC: Zeng Xin 
> > Signed-off-by: Gonglei 
> > ---
> >  MAINTAINERS  |   8 +
> >  drivers/crypto/Kconfig   |   2 +
> >  drivers/crypto/Makefile  |   1 +
> >  drivers/crypto/virtio/Kconfig|  10 +
> >  drivers/crypto/virtio/Makefile   |   5 +
> >  drivers/crypto/virtio/virtio_crypto.c| 444
> +++
> >  drivers/crypto/virtio/virtio_crypto_algs.c   | 524
> +++
> >  drivers/crypto/virtio/virtio_crypto_common.h | 124 +++
> >  drivers/crypto/virtio/virtio_crypto_mgr.c| 258 +
> >  include/uapi/linux/Kbuild|   1 +
> >  include/uapi/linux/virtio_crypto.h   | 435
> ++
> >  include/uapi/linux/virtio_ids.h  |   1 +
> >  12 files changed, 1813 insertions(+)
> >  create mode 100644 drivers/crypto/virtio/Kconfig
> >  create mode 100644 drivers/crypto/virtio/Makefile
> >  create mode 100644 drivers/crypto/virtio/virtio_crypto.c
> >  create mode 100644 drivers/crypto/virtio/virtio_crypto_algs.c
> >  create mode 100644 drivers/crypto/virtio/virtio_crypto_common.h
> >  create mode 100644 drivers/crypto/virtio/virtio_crypto_mgr.c
> >  create mode 100644 include/uapi/linux/virtio_crypto.h
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 411e3b8..d6b18bb 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -12844,6 +12844,14 @@ S: Maintained
> >  F: drivers/virtio/virtio_input.c
> >  F: include/uapi/linux/virtio_input.h
> >
> > +VIRTIO CRYPTO DRIVER
> > +M:  Gonglei 
> > +L:  virtualizat...@lists.linux-foundation.org
> > +L:  linux-crypto@vger.kernel.org
> > +S:  Maintained
> > +F:  drivers/crypto/virtio/
> > +F:  include/uapi/linux/virtio_crypto.h
> > +
> >  VIA RHINE NETWORK DRIVER
> >  S: Orphan
> >  F: drivers/net/ethernet/via/via-rhine.c
> > diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig
> > index 4d2b81f..7956478 100644
> > --- a/drivers/crypto/Kconfig
> > +++ b/drivers/crypto/Kconfig
> > @@ -555,4 +555,6 @@ config CRYPTO_DEV_ROCKCHIP
> >
> >  source "drivers/crypto/chelsio/Kconfig"
> >
> > +source "drivers/crypto/virtio/Kconfig"
> > +
> >  endif # CRYPTO_HW
> > diff --git a/drivers/crypto/Makefile b/drivers/crypto/Makefile
> > index ad7250f..bc53cb8 100644
> > --- a/drivers/crypto/Makefile
> > +++ b/drivers/crypto/Makefile
> > @@ -32,3 +32,4 @@ obj-$(CONFIG_CRYPTO_DEV_VMX) += vmx/
> >  obj-$(CONFIG_CRYPTO_DEV_SUN4I_SS) += sunxi-ss/
> >  obj-$(CONFIG_CRYPTO_DEV_ROCKCHIP) += rockchip/
> >  obj-$(CONFIG_CRYPTO_DEV_CHELSIO) += chelsio/
> > +obj-$(CONFIG_CRYPTO_DEV_VIRTIO) += virtio/
> > diff --git a/drivers/crypto/virtio/Kconfig b/drivers/crypto/virtio/Kconfig
> > new file mode 100644
> > index 000..ceae88c
> > --- /dev/null
> > +++ b/drivers/crypto/virtio/Kconfig
> > @@ -0,0 +1,10 @@
> > +config CRYPTO_DEV_VIRTIO
> > +   tristate "VirtIO crypto driver"
> > +   depends on VIRTIO
> > +select CRYPTO_AEAD
> > +select CRYPTO_AUTHENC
> > +select CRYPTO_BLKCIPHER
> > +   default m
> > +   help
> > + This driver provides support for virtio crypto device. If you
> > + choose 'M' here, this module will be called virtio-crypto.
> > diff --git a/drivers/crypto/virtio/Makefile b/drivers/crypto/virtio/Makefile
> > new file mode 100644
> > index 000..a316e92
> > --- /dev/null
> > +++ b/drivers/crypto/virtio/Makefile
> > @@ -0,0 +1,5 @@
> > +obj-$(CONFIG_CRYPTO_DEV_VIRTIO) += virtio-crypto.o
> > +virtio-crypto-objs := \
> > +   virtio_crypto_algs.o \
> > +   virtio_crypto_mgr.o \
> > +   virtio_crypto.o
> > diff --git a/drivers/crypto/virtio/virtio_crypto.c
> b/drivers/crypto/virtio/virtio_crypto.c
> > new file mode 100644
> > index 

RE: [virtio-dev] Re: [PATCH v2 1/2] virtio: introduce little edian functions for virtio_cread/write# family

[Gonglei (Arei)]

>
> Subject: Re: [virtio-dev] Re: [PATCH v2 1/2] virtio: introduce little edian
> functions for virtio_cread/write# family
> 
> On Mon, Nov 28, 2016 at 04:34:56AM +0200, Michael S. Tsirkin wrote:
> > On Mon, Nov 28, 2016 at 01:56:04AM +, Gonglei (Arei) wrote:
> > > Hi Michael,
> > >
> > > Thanks for your feedback firstly!
> > >
> > > > -Original Message-
> > > > From: virtio-...@lists.oasis-open.org
> [mailto:virtio-...@lists.oasis-open.org]
> > > > On Behalf Of Michael S. Tsirkin
> > > > Sent: Sunday, November 27, 2016 11:33 AM
> > > > To: Gonglei (Arei)
> > > > Subject: [virtio-dev] Re: [PATCH v2 1/2] virtio: introduce little edian
> functions for
> > > > virtio_cread/write# family
> > > >
> > > > On Tue, Nov 22, 2016 at 04:10:22PM +0800, Gonglei wrote:
> > > > > Virtio modern devices are always little edian, let's introduce
> > > > > the LE functions for read/write configuration space for
> > > > > virtio modern devices, which avoid complaint by Sparse when
> > > > > we use the virtio_creaed/virtio_cwrite in VIRTIO_1 devices.
> > > > >
> > > > > Signed-off-by: Gonglei <arei.gong...@huawei.com>
> > > > > ---
> > > > >  include/linux/virtio_config.h | 45
> > > > +++
> > > > >  1 file changed, 45 insertions(+)
> > > > >
> > > > > diff --git a/include/linux/virtio_config.h 
> > > > > b/include/linux/virtio_config.h
> > > > > index 26c155b..de05707 100644
> > > > > --- a/include/linux/virtio_config.h
> > > > > +++ b/include/linux/virtio_config.h
> > > > > @@ -414,4 +414,49 @@ static inline void virtio_cwrite64(struct
> virtio_device
> > > > *vdev,
> > > > >   _r; 
> > > > > \
> > > > >   })
> > > > >
> > > > > +static inline __le16 virtio_cread16_le(struct virtio_device *vdev,
> > > > > +  unsigned int offset)
> > > > > +{
> > > > > + __le16 ret;
> > > > > +
> > > > > + vdev->config->get(vdev, offset, , sizeof(ret));
> > > > > + return ret;
> > > > > +}
> > > > > +
> > > > > +static inline void virtio_cwrite16_le(struct virtio_device *vdev,
> > > > > +unsigned int offset, __le16 val)
> > > > > +{
> > > > > + vdev->config->set(vdev, offset, , sizeof(val));
> > > > > +}
> > > > > +
> > > > > +static inline __le32 virtio_cread32_le(struct virtio_device *vdev,
> > > > > +  unsigned int offset)
> > > > > +{
> > > > > + __le32 ret;
> > > > > +
> > > > > + vdev->config->get(vdev, offset, , sizeof(ret));
> > > > > + return ret;
> > > > > +}
> > > > > +
> > > > > +static inline void virtio_cwrite32_le(struct virtio_device *vdev,
> > > > > +unsigned int offset, __le32 val)
> > > > > +{
> > > > > + vdev->config->set(vdev, offset, , sizeof(val));
> > > > > +}
> > > > > +
> > > > > +static inline __le64 virtio_cread64_le(struct virtio_device *vdev,
> > > > > +  unsigned int offset)
> > > > > +{
> > > > > + __le64 ret;
> > > > > +
> > > > > + __virtio_cread_many(vdev, offset, , 1, sizeof(ret));
> > > > > + return ret;
> > > > > +}
> > > > > +
> > > > > +static inline void virtio_cwrite64_le(struct virtio_device *vdev,
> > > > > +unsigned int offset, __le64 val)
> > > > > +{
> > > > > + vdev->config->set(vdev, offset, , sizeof(val));
> > > > > +}
> > > > > +
> > > > >  #endif /* _LINUX_VIRTIO_CONFIG_H */
> > > >
> > > > Could you please better explain what is the issue you are facing?
> > > > virtio_cwrite/virtio_cread all accept and return native types.
> > > >
> > > virtio_cwrite/virtio_cread are used to write/read config

RE: [virtio-dev] Re: [PATCH v2 1/2] virtio: introduce little edian functions for virtio_cread/write# family

[Gonglei (Arei)]

>
> Subject: Re: [virtio-dev] Re: [PATCH v2 1/2] virtio: introduce little edian
> functions for virtio_cread/write# family
> 
> On Mon, Nov 28, 2016 at 01:56:04AM +, Gonglei (Arei) wrote:
> > Hi Michael,
> >
> > Thanks for your feedback firstly!
> >
> > > -Original Message-
> > > From: virtio-...@lists.oasis-open.org
> [mailto:virtio-...@lists.oasis-open.org]
> > > On Behalf Of Michael S. Tsirkin
> > > Sent: Sunday, November 27, 2016 11:33 AM
> > > To: Gonglei (Arei)
> > > Subject: [virtio-dev] Re: [PATCH v2 1/2] virtio: introduce little edian 
> > > functions
> for
> > > virtio_cread/write# family
> > >
> > > On Tue, Nov 22, 2016 at 04:10:22PM +0800, Gonglei wrote:
> > > > Virtio modern devices are always little edian, let's introduce
> > > > the LE functions for read/write configuration space for
> > > > virtio modern devices, which avoid complaint by Sparse when
> > > > we use the virtio_creaed/virtio_cwrite in VIRTIO_1 devices.
> > > >
> > > > Signed-off-by: Gonglei <arei.gong...@huawei.com>
> > > > ---
> > > >  include/linux/virtio_config.h | 45
> > > +++
> > > >  1 file changed, 45 insertions(+)
> > > >
> > > > diff --git a/include/linux/virtio_config.h 
> > > > b/include/linux/virtio_config.h
> > > > index 26c155b..de05707 100644
> > > > --- a/include/linux/virtio_config.h
> > > > +++ b/include/linux/virtio_config.h
> > > > @@ -414,4 +414,49 @@ static inline void virtio_cwrite64(struct
> virtio_device
> > > *vdev,
> > > > _r; 
> > > > \
> > > > })
> > > >
> > > > +static inline __le16 virtio_cread16_le(struct virtio_device *vdev,
> > > > +unsigned int offset)
> > > > +{
> > > > +   __le16 ret;
> > > > +
> > > > +   vdev->config->get(vdev, offset, , sizeof(ret));
> > > > +   return ret;
> > > > +}
> > > > +
> > > > +static inline void virtio_cwrite16_le(struct virtio_device *vdev,
> > > > +  unsigned int offset, __le16 val)
> > > > +{
> > > > +   vdev->config->set(vdev, offset, , sizeof(val));
> > > > +}
> > > > +
> > > > +static inline __le32 virtio_cread32_le(struct virtio_device *vdev,
> > > > +unsigned int offset)
> > > > +{
> > > > +   __le32 ret;
> > > > +
> > > > +   vdev->config->get(vdev, offset, , sizeof(ret));
> > > > +   return ret;
> > > > +}
> > > > +
> > > > +static inline void virtio_cwrite32_le(struct virtio_device *vdev,
> > > > +  unsigned int offset, __le32 val)
> > > > +{
> > > > +   vdev->config->set(vdev, offset, , sizeof(val));
> > > > +}
> > > > +
> > > > +static inline __le64 virtio_cread64_le(struct virtio_device *vdev,
> > > > +unsigned int offset)
> > > > +{
> > > > +   __le64 ret;
> > > > +
> > > > +   __virtio_cread_many(vdev, offset, , 1, sizeof(ret));
> > > > +   return ret;
> > > > +}
> > > > +
> > > > +static inline void virtio_cwrite64_le(struct virtio_device *vdev,
> > > > +  unsigned int offset, __le64 val)
> > > > +{
> > > > +   vdev->config->set(vdev, offset, , sizeof(val));
> > > > +}
> > > > +
> > > >  #endif /* _LINUX_VIRTIO_CONFIG_H */
> > >
> > > Could you please better explain what is the issue you are facing?
> > > virtio_cwrite/virtio_cread all accept and return native types.
> > >
> > virtio_cwrite/virtio_cread are used to write/read configuration
> > space for virtio devices. For virtio-crypto device, I used __le32/64 
> > directly
> > in struct virtio_crypto_config. The sparse reports warnings if I use
> virtio_cread()
> > for virtio-crypto device.
> 
> I suspect that's because you are doing cread into an le32 variable.
> 
> 
Yes.

> 
> > Furthermore, it means the warnings exist for all VIRTIO_1 devices because
> &g

RE: [virtio-dev] Re: [PATCH v2 1/2] virtio: introduce little edian functions for virtio_cread/write# family

[Gonglei (Arei)]

Hi Michael,

Thanks for your feedback firstly!

> -Original Message-
> From: virtio-...@lists.oasis-open.org [mailto:virtio-...@lists.oasis-open.org]
> On Behalf Of Michael S. Tsirkin
> Sent: Sunday, November 27, 2016 11:33 AM
> To: Gonglei (Arei)
> Subject: [virtio-dev] Re: [PATCH v2 1/2] virtio: introduce little edian 
> functions for
> virtio_cread/write# family
> 
> On Tue, Nov 22, 2016 at 04:10:22PM +0800, Gonglei wrote:
> > Virtio modern devices are always little edian, let's introduce
> > the LE functions for read/write configuration space for
> > virtio modern devices, which avoid complaint by Sparse when
> > we use the virtio_creaed/virtio_cwrite in VIRTIO_1 devices.
> >
> > Signed-off-by: Gonglei <arei.gong...@huawei.com>
> > ---
> >  include/linux/virtio_config.h | 45
> +++
> >  1 file changed, 45 insertions(+)
> >
> > diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
> > index 26c155b..de05707 100644
> > --- a/include/linux/virtio_config.h
> > +++ b/include/linux/virtio_config.h
> > @@ -414,4 +414,49 @@ static inline void virtio_cwrite64(struct virtio_device
> *vdev,
> > _r; \
> > })
> >
> > +static inline __le16 virtio_cread16_le(struct virtio_device *vdev,
> > +unsigned int offset)
> > +{
> > +   __le16 ret;
> > +
> > +   vdev->config->get(vdev, offset, , sizeof(ret));
> > +   return ret;
> > +}
> > +
> > +static inline void virtio_cwrite16_le(struct virtio_device *vdev,
> > +  unsigned int offset, __le16 val)
> > +{
> > +   vdev->config->set(vdev, offset, , sizeof(val));
> > +}
> > +
> > +static inline __le32 virtio_cread32_le(struct virtio_device *vdev,
> > +unsigned int offset)
> > +{
> > +   __le32 ret;
> > +
> > +   vdev->config->get(vdev, offset, , sizeof(ret));
> > +   return ret;
> > +}
> > +
> > +static inline void virtio_cwrite32_le(struct virtio_device *vdev,
> > +  unsigned int offset, __le32 val)
> > +{
> > +   vdev->config->set(vdev, offset, , sizeof(val));
> > +}
> > +
> > +static inline __le64 virtio_cread64_le(struct virtio_device *vdev,
> > +unsigned int offset)
> > +{
> > +   __le64 ret;
> > +
> > +   __virtio_cread_many(vdev, offset, , 1, sizeof(ret));
> > +   return ret;
> > +}
> > +
> > +static inline void virtio_cwrite64_le(struct virtio_device *vdev,
> > +  unsigned int offset, __le64 val)
> > +{
> > +   vdev->config->set(vdev, offset, , sizeof(val));
> > +}
> > +
> >  #endif /* _LINUX_VIRTIO_CONFIG_H */
> 
> Could you please better explain what is the issue you are facing?
> virtio_cwrite/virtio_cread all accept and return native types.
> 
virtio_cwrite/virtio_cread are used to write/read configuration
space for virtio devices. For virtio-crypto device, I used __le32/64 directly
in struct virtio_crypto_config. The sparse reports warnings if I use 
virtio_cread()
for virtio-crypto device.

Furthermore, it means the warnings exist for all VIRTIO_1 devices because
they are definitely LE, which it's not necessary to use 
virtio_to_cpu/cpu_to_virtio.


PS: I googled a discussion about this topic for virtio-input device, pls see:
 http://linux.kernel.narkive.com/3argfbWz/patch-1-1-add-virtio-input-driver

Regards,
-Gonglei

> If you want it in LE format, swap it!
> 
> 
> 
> > --
> > 1.8.3.1
> >
> 
> -
> To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
> For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org

--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v2 0/2] virtio-crypto: add Linux driver

[Gonglei (Arei)]

Hi,

> -Original Message-
> From: Gonglei (Arei)
> Sent: Tuesday, November 22, 2016 4:10 PM
> To: linux-ker...@vger.kernel.org; qemu-de...@nongnu.org;
> virtio-...@lists.oasis-open.org; virtualizat...@lists.linux-foundation.org;
> linux-crypto@vger.kernel.org
> Subject: [PATCH v2 0/2] virtio-crypto: add Linux driver
> 
> The virtio crypto device is a virtual cryptography device
> as well as a kind of virtual hardware accelerator for
> virtual machines. The encryption anddecryption requests
> are placed in the data queue and are ultimately handled by
> thebackend crypto accelerators. The second queue is the
> control queue used to create or destroy sessions for
> symmetric algorithms and will control some advanced features
> in the future. The virtio crypto device provides the following
> cryptoservices: CIPHER, MAC, HASH, and AEAD.
> 
> For more information about virtio-crypto device, please see:
>   http://qemu-project.org/Features/VirtioCrypto
> 
> For better reviewing:
> 
> Patch 1 introduces the little edian functions for VIRTIO_1
> devices.
> 
> Patch 2 mainly includes five files:
>  1) virtio_crypto.h is the header file for virtio-crypto device,
> which is based on the virtio-crypto specification.
>  2) virtio_crypto.c is the entry of the driver module,
> which is similar with other virtio devices, such as virtio-net,
> virtio-input etc.
>  3) virtio_crypto_mgr.c is used to manage the virtio
> crypto devices in the system. We support up to 32 virtio-crypto
> devices currently. I use a global list to store the virtio crypto
> devices which refer to Intel QAT driver. Meanwhile, the file
> includs the functions of add/del/search/start/stop for virtio
> crypto devices.
>  4) virtio_crypto_common.h is a private header file for virtio
> crypto driver, includes structure definations, and function declarations.
>  5) virtio_crypto_algs.c is the realization of algs based on Linux Crypto
> Framwork,
> which can register different crypto algorithms. Currently it's only support
> AES-CBC.
> The Crypto guys can mainly focus to this file.
> 
> Actually I have no idea the virtio-crypto driver should be gone in whose
> tree, Michael's or Herbert's?
> 
> Would you give me a feedback? Thanks a lot!
> 
Ping?

Any ideas? Thanks.

Regards,
-Gonglei

> 
> v2:
>  - stop doing DMA from the stack, CONFIG_VMAP_STACK=y [Salvatore]
>  - convert __virtio32/64 to __le32/64 in virtio_crypto.h
>  - remove VIRTIO_CRYPTO_S_STARTED based on the lastest virtio crypto spec.
>  - introduces the little edian functions for VIRTIO_1 devices in patch 1.
> 
> Gonglei (2):
>   virtio: introduce little edian functions for virtio_cread/write#
> family
>   crypto: add virtio-crypto driver
> 
>  MAINTAINERS  |   8 +
>  drivers/crypto/Kconfig   |   2 +
>  drivers/crypto/Makefile  |   1 +
>  drivers/crypto/virtio/Kconfig|  10 +
>  drivers/crypto/virtio/Makefile   |   5 +
>  drivers/crypto/virtio/virtio_crypto.c| 444
> +++
>  drivers/crypto/virtio/virtio_crypto_algs.c   | 524
> +++
>  drivers/crypto/virtio/virtio_crypto_common.h | 124 +++
>  drivers/crypto/virtio/virtio_crypto_mgr.c| 258 +
>  include/linux/virtio_config.h|  45 +++
>  include/uapi/linux/Kbuild|   1 +
>  include/uapi/linux/virtio_crypto.h   | 435
> ++
>  include/uapi/linux/virtio_ids.h  |   1 +
>  13 files changed, 1858 insertions(+)
>  create mode 100644 drivers/crypto/virtio/Kconfig
>  create mode 100644 drivers/crypto/virtio/Makefile
>  create mode 100644 drivers/crypto/virtio/virtio_crypto.c
>  create mode 100644 drivers/crypto/virtio/virtio_crypto_algs.c
>  create mode 100644 drivers/crypto/virtio/virtio_crypto_common.h
>  create mode 100644 drivers/crypto/virtio/virtio_crypto_mgr.c
>  create mode 100644 include/uapi/linux/virtio_crypto.h
> 
> --
> 1.8.3.1
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] crypto: add virtio-crypto driver

[Gonglei (Arei)]

Hi Michael,

May I should convert all __virtio32/64 to le32/64 in virtio_crypto.h ?


> +#define VIRTIO_CRYPTO_OPCODE(service, op)   (((service) << 8) | (op))
> +
> +struct virtio_crypto_ctrl_header {
> +#define VIRTIO_CRYPTO_CIPHER_CREATE_SESSION \
> +VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_CIPHER, 0x02)
> +#define VIRTIO_CRYPTO_CIPHER_DESTROY_SESSION \
> +VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_CIPHER, 0x03)
> +#define VIRTIO_CRYPTO_HASH_CREATE_SESSION \
> +VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_HASH, 0x02)
> +#define VIRTIO_CRYPTO_HASH_DESTROY_SESSION \
> +VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_HASH, 0x03)
> +#define VIRTIO_CRYPTO_MAC_CREATE_SESSION \
> +VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_MAC, 0x02)
> +#define VIRTIO_CRYPTO_MAC_DESTROY_SESSION \
> +VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_MAC, 0x03)
> +#define VIRTIO_CRYPTO_AEAD_CREATE_SESSION \
> +VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_AEAD, 0x02)
> +#define VIRTIO_CRYPTO_AEAD_DESTROY_SESSION \
> +VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_AEAD, 0x03)
> + __virtio32 opcode;
> + __virtio32 algo;
> + __virtio32 flag;
> + /* data virtqueue id */
> + __virtio32 queue_id;
> +};
> +
> +struct virtio_crypto_cipher_session_para {
> +#define VIRTIO_CRYPTO_NO_CIPHER 0
> +#define VIRTIO_CRYPTO_CIPHER_ARC4   1
> +#define VIRTIO_CRYPTO_CIPHER_AES_ECB2
> +#define VIRTIO_CRYPTO_CIPHER_AES_CBC3
> +#define VIRTIO_CRYPTO_CIPHER_AES_CTR4
> +#define VIRTIO_CRYPTO_CIPHER_DES_ECB5
> +#define VIRTIO_CRYPTO_CIPHER_DES_CBC6
> +#define VIRTIO_CRYPTO_CIPHER_3DES_ECB   7
> +#define VIRTIO_CRYPTO_CIPHER_3DES_CBC   8
> +#define VIRTIO_CRYPTO_CIPHER_3DES_CTR   9
> +#define VIRTIO_CRYPTO_CIPHER_KASUMI_F8  10
> +#define VIRTIO_CRYPTO_CIPHER_SNOW3G_UEA211
> +#define VIRTIO_CRYPTO_CIPHER_AES_F8 12
> +#define VIRTIO_CRYPTO_CIPHER_AES_XTS13
> +#define VIRTIO_CRYPTO_CIPHER_ZUC_EEA3   14
> + __virtio32 algo;
> + /* length of key */
> + __virtio32 keylen;
> +
> +#define VIRTIO_CRYPTO_OP_ENCRYPT  1
> +#define VIRTIO_CRYPTO_OP_DECRYPT  2
> + /* encrypt or decrypt */
> + __virtio32 op;
> + __virtio32 padding;
> +};
> +
> +struct virtio_crypto_session_input {
> + /* Device-writable part */
> + __virtio64 session_id;
> + __virtio32 status;
> + __virtio32 padding;
> +};
> +
> +struct virtio_crypto_cipher_session_req {
> + struct virtio_crypto_cipher_session_para para;
> +};
> +
> +struct virtio_crypto_hash_session_para {
> +#define VIRTIO_CRYPTO_NO_HASH0
> +#define VIRTIO_CRYPTO_HASH_MD5   1
> +#define VIRTIO_CRYPTO_HASH_SHA1  2
> +#define VIRTIO_CRYPTO_HASH_SHA_224   3
> +#define VIRTIO_CRYPTO_HASH_SHA_256   4
> +#define VIRTIO_CRYPTO_HASH_SHA_384   5
> +#define VIRTIO_CRYPTO_HASH_SHA_512   6
> +#define VIRTIO_CRYPTO_HASH_SHA3_224  7
> +#define VIRTIO_CRYPTO_HASH_SHA3_256  8
> +#define VIRTIO_CRYPTO_HASH_SHA3_384  9
> +#define VIRTIO_CRYPTO_HASH_SHA3_512  10
> +#define VIRTIO_CRYPTO_HASH_SHA3_SHAKE128  11
> +#define VIRTIO_CRYPTO_HASH_SHA3_SHAKE256  12
> + __virtio32 algo;
> + /* hash result length */
> + __virtio32 hash_result_len;
> +};
> +
> +struct virtio_crypto_hash_create_session_req {
> + struct virtio_crypto_hash_session_para para;
> +};
> +
> +struct virtio_crypto_mac_session_para {
> +#define VIRTIO_CRYPTO_NO_MAC   0
> +#define VIRTIO_CRYPTO_MAC_HMAC_MD5 1
> +#define VIRTIO_CRYPTO_MAC_HMAC_SHA12
> +#define VIRTIO_CRYPTO_MAC_HMAC_SHA_224 3
> +#define VIRTIO_CRYPTO_MAC_HMAC_SHA_256 4
> +#define VIRTIO_CRYPTO_MAC_HMAC_SHA_384 5
> +#define VIRTIO_CRYPTO_MAC_HMAC_SHA_512 6
> +#define VIRTIO_CRYPTO_MAC_CMAC_3DES25
> +#define VIRTIO_CRYPTO_MAC_CMAC_AES 26
> +#define VIRTIO_CRYPTO_MAC_KASUMI_F927
> +#define VIRTIO_CRYPTO_MAC_SNOW3G_UIA2  28
> +#define VIRTIO_CRYPTO_MAC_GMAC_AES 41
> +#define VIRTIO_CRYPTO_MAC_GMAC_TWOFISH 42
> +#define VIRTIO_CRYPTO_MAC_CBCMAC_AES   49
> +#define VIRTIO_CRYPTO_MAC_CBCMAC_KASUMI_F9 50
> +#define VIRTIO_CRYPTO_MAC_XCBC_AES 53
> + __virtio32 algo;
> + /* hash result length */
> + __virtio32 hash_result_len;
> + /* length of authenticated key */
> + __virtio32 auth_key_len;
> + __virtio32 padding;
> +};
> +
> +struct virtio_crypto_mac_create_session_req {
> + struct virtio_crypto_mac_session_para para;
> +};
> +
> +struct virtio_crypto_aead_session_para {
> +#define VIRTIO_CRYPTO_NO_AEAD 0
> +#define VIRTIO_CRYPTO_AEAD_GCM1
> +#define VIRTIO_CRYPTO_AEAD_CCM2
> +#define VIRTIO_CRYPTO_AEAD_CHACHA20_POLY1305