Linux Virtualization - Dec 2016 - [Qemu-devel] [PATCH v7 1/1] crypto: add virtio-crypto driver

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 0000000..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(&vcrypto->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.
> +	do {
> +		virtqueue_disable_cb(vq);
> +		while ((vc_req = virtqueue_get_buf(vq, &len)) != NULL) {
> +			if (vc_req->type == VIRTIO_CRYPTO_SYM_OP_CIPHER) {
> +				switch (vc_req->status) {
> +				case VIRTIO_CRYPTO_OK:
> +					error = 0;
> +					break;
> +				case VIRTIO_CRYPTO_INVSESS:
> +				case VIRTIO_CRYPTO_ERR:
> +					error = -EINVAL;
> +					break;
> +				case VIRTIO_CRYPTO_BADMSG:
> +					error = -EBADMSG;
> +					break;
> +				default:
> +					error = -EIO;
> +					break;
> +				}
> +				ablk_req = vc_req->ablkcipher_req;
> +				virtcrypto_clear_request(vc_req);
> +
> +				spin_unlock_irqrestore(&vcrypto->lock, flags);
> +				/* Finish the encrypt or decrypt process */
> +				ablk_req->base.complete(&ablk_req->base, error);
> +				spin_lock_irqsave(&vcrypto->lock, flags);
> +			}
> +		}
> +	} while (!virtqueue_enable_cb(vq));
> +	spin_unlock_irqrestore(&vcrypto->lock, flags);
> +}

> 
> 
> 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 0000000..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(&vcrypto->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

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 0000000..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(&vcrypto->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.

< Regards,
< -Gonglei