Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1531261 > unrolled thread

[PATCH v3] virtio-crypto: add Linux driver

Back to article view | Back to linux.kernel

Contents

  [PATCH v3] virtio-crypto: add Linux driver Gonglei <arei.gonglei@huawei.com> - 2016-11-28 13:20 +0100
    Re: [PATCH v3] crypto: add virtio-crypto driver Cornelia Huck <cornelia.huck@de.ibm.com> - 2016-11-28 14:20 +0100
      RE: [virtio-dev] Re: [PATCH v3] crypto: add virtio-crypto driver "Gonglei (Arei)" <arei.gonglei@huawei.com> - 2016-11-29 02:50 +0100
        Re: [virtio-dev] Re: [PATCH v3] crypto: add virtio-crypto driver Cornelia Huck <cornelia.huck@de.ibm.com> - 2016-11-29 10:50 +0100
          RE: [virtio-dev] Re: [PATCH v3] crypto: add virtio-crypto driver "Gonglei (Arei)" <arei.gonglei@huawei.com> - 2016-11-29 13:00 +0100
    Re: [PATCH v3] crypto: add virtio-crypto driver Halil Pasic <pasic@linux.vnet.ibm.com> - 2016-11-28 18:40 +0100
      RE: [PATCH v3] crypto: add virtio-crypto driver "Gonglei (Arei)" <arei.gonglei@huawei.com> - 2016-11-29 04:50 +0100
    RE: [PATCH v3] crypto: add virtio-crypto driver "Gonglei (Arei)" <arei.gonglei@huawei.com> - 2016-11-29 07:50 +0100
    RE: [PATCH v3] crypto: add virtio-crypto driver "Gonglei (Arei)" <arei.gonglei@huawei.com> - 2016-11-29 09:30 +0100
      Re: [PATCH v3] crypto: add virtio-crypto driver Stefan Hajnoczi <stefanha@redhat.com> - 2016-11-29 10:30 +0100
        Re: [PATCH v3] crypto: add virtio-crypto driver Cornelia Huck <cornelia.huck@de.ibm.com> - 2016-11-29 10:40 +0100
          RE: [PATCH v3] crypto: add virtio-crypto driver "Gonglei (Arei)" <arei.gonglei@huawei.com> - 2016-11-29 12:50 +0100
        RE: [PATCH v3] crypto: add virtio-crypto driver "Gonglei (Arei)" <arei.gonglei@huawei.com> - 2016-11-29 10:40 +0100

#1531261 — [PATCH v3] virtio-crypto: add Linux driver

v3:
 - set cpu affinity when data queues are not equal to the number of online cpus. [Michael]
 - add TODO comments for cpu hotplug (changing the relationship of binding virtqueue and cpu)
 - use __u32/64 in the config space since the virtio->get() doesn't support byte-swap yet. [Michael]
 - drop the whole patch 1 of v2 because the above reason.
 - add VERSION_1 check at the beginning of virtcrypto_probe()
 - s/-1/EPERM/g in virtcrypto_update_status(), don't change err to EFAULT then. [Michael]
 - add reset operation before delete the virtqueus. [Micheal]
 - drop an unnecessiry spin_lock calling in virtcrypto_freeze(), avoid possible dead lock. [Micheal]
 - redefine parameter alg's type in order to use a cast for it. [Michael]
 - pad all structures to have the same size in one union, and add a member to
   show the union's size in virtio_crypto.h. [Michael]
 - update MAINTAINER file to add virtio-crypto stuff to Michael's entry so that
   the corresponding patches can be CC'ed to you too because the virtio-crypto
   doesn't lay in driver/virtio directory. 

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:

The patch 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 on this file. 


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 (1):
  crypto: add virtio-crypto driver

 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

-- 
1.8.3.1

[toc] | [next] | [standalone]

#1531298 — Re: [PATCH v3] crypto: add virtio-crypto driver

On Mon, 28 Nov 2016 20:08:23 +0800
Gonglei <arei.gonglei@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, &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?

> +
> +	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(&vcrypto->vdev->dev,
> +				"Failed to start virtio crypto device.\n");
> +			virtcrypto_dev_stop(vcrypto);
> +			return -EPERM;
> +		}
> +		dev_info(&vcrypto->vdev->dev, "Accelerator is ready\n");
> +	} else {
> +		virtcrypto_dev_stop(vcrypto);
> +		dev_info(&vcrypto->vdev->dev, "Accelerator is not ready\n");
> +	}
> +
> +	return 0;
> +}
> +

[toc] | [prev] | [next] | [standalone]

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

>
> Subject: [virtio-dev] Re: [PATCH v3] crypto: add virtio-crypto driver
> 
> On Mon, 28 Nov 2016 20:08:23 +0800
> Gonglei <arei.gonglei@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, &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(&vcrypto->vdev->dev,
> > +				"Failed to start virtio crypto device.\n");
> > +			virtcrypto_dev_stop(vcrypto);
> > +			return -EPERM;
> > +		}
> > +		dev_info(&vcrypto->vdev->dev, "Accelerator is ready\n");
> > +	} else {
> > +		virtcrypto_dev_stop(vcrypto);
> > +		dev_info(&vcrypto->vdev->dev, "Accelerator is not ready\n");
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> 
> 
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[toc] | [prev] | [next] | [standalone]

#1532062 — Re: [virtio-dev] Re: [PATCH v3] crypto: add virtio-crypto driver

On Tue, 29 Nov 2016 01:37:44 +0000
"Gonglei (Arei)" <arei.gonglei@huawei.com> wrote:

> > On Mon, 28 Nov 2016 20:08:23 +0800
> > Gonglei <arei.gonglei@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, &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.

[toc] | [prev] | [next] | [standalone]

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

>
> Subject: Re: [virtio-dev] Re: [PATCH v3] crypto: add virtio-crypto driver
> 
> On Tue, 29 Nov 2016 01:37:44 +0000
> "Gonglei (Arei)" <arei.gonglei@huawei.com> wrote:
> 
> > > On Mon, 28 Nov 2016 20:08:23 +0800
> > > Gonglei <arei.gonglei@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, &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-unsubscribe@lists.oasis-open.org
> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[toc] | [prev] | [next] | [standalone]

#1531476 — Re: [PATCH v3] crypto: add virtio-crypto driver

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(&vcrypto->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(&outhdr, &vcrypto->ctrl, sizeof(vcrypto->ctrl));
>>> > > +	sgs[num_out++] = &outhdr;
>>> > > +
>>> > > +	/* Set key */
>>> > > +	sg_init_one(&key_sg, cipher_key, keylen);
>>> > > +	sgs[num_out++] = &key_sg;
>>> > > +
>>> > > +	/* Return status and session id back */
>>> > > +	sg_init_one(&inhdr, &vcrypto->input, sizeof(vcrypto->input));
>>> > > +	sgs[num_out + num_in++] = &inhdr;
>>> > > +
>>> > > +	err = virtqueue_add_sgs(vcrypto->ctrl_vq, sgs, num_out,
>>> > > +				num_in, vcrypto, GFP_ATOMIC);
>>> > > +	if (err < 0) {
>>> > > +		spin_unlock(&vcrypto->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.

I know we have the same message in virtio-net.

>>> > > +	while (!virtqueue_get_buf(vcrypto->ctrl_vq, &tmp) &&
>>> > > +	       !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(&vcrypto->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(&vcrypto->ctrl_lock);
>>> > > +
> You drop lock here. If someone is trying to submit multiple
> requests, then the below will be racy as it might overwrite
> new result with previous data.
> 

Was going to object on this too but Michael was faster.

Halil

>>> > > +	spin_lock(&ctx->lock);
>>> > > +	if (encrypt)
>>> > > +		ctx->enc_sess_info.session_id =
>>> > > +			le64_to_cpu(vcrypto->input.session_id);
>>> > > +	else
>>> > > +		ctx->dec_sess_info.session_id =
>>> > > +			le64_to_cpu(vcrypto->input.session_id);
>>> > > +	spin_unlock(&ctx->lock);
>>> > > +
>>> > > +	kfree(cipher_key);
>>> > > +	return 0;
>>> > > +}
>>> > > +

[toc] | [prev] | [next] | [standalone]

#1531883 — RE: [PATCH v3] crypto: add virtio-crypto driver

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(&vcrypto->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(&outhdr, &vcrypto->ctrl, sizeof(vcrypto->ctrl));
> >>> > > +	sgs[num_out++] = &outhdr;
> >>> > > +
> >>> > > +	/* Set key */
> >>> > > +	sg_init_one(&key_sg, cipher_key, keylen);
> >>> > > +	sgs[num_out++] = &key_sg;
> >>> > > +
> >>> > > +	/* Return status and session id back */
> >>> > > +	sg_init_one(&inhdr, &vcrypto->input, sizeof(vcrypto->input));
> >>> > > +	sgs[num_out + num_in++] = &inhdr;
> >>> > > +
> >>> > > +	err = virtqueue_add_sgs(vcrypto->ctrl_vq, sgs, num_out,
> >>> > > +				num_in, vcrypto, GFP_ATOMIC);
> >>> > > +	if (err < 0) {
> >>> > > +		spin_unlock(&vcrypto->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, &tmp) &&
> >>> > > +	       !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(&vcrypto->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(&vcrypto->ctrl_lock);
> >>> > > +
> > You drop lock here. If someone is trying to submit multiple
> > requests, then the below will be racy as it might overwrite
> > new result with previous data.
> >
> 
> Was going to object on this too but Michael was faster.
> 
Will fix.

Thanks,
-Gonglei

> Halil
> 
> >>> > > +	spin_lock(&ctx->lock);
> >>> > > +	if (encrypt)
> >>> > > +		ctx->enc_sess_info.session_id =
> >>> > > +			le64_to_cpu(vcrypto->input.session_id);
> >>> > > +	else
> >>> > > +		ctx->dec_sess_info.session_id =
> >>> > > +			le64_to_cpu(vcrypto->input.session_id);
> >>> > > +	spin_unlock(&ctx->lock);
> >>> > > +
> >>> > > +	kfree(cipher_key);
> >>> > > +	return 0;
> >>> > > +}
> >>> > > +

[toc] | [prev] | [next] | [standalone]

#1531935 — RE: [PATCH v3] crypto: add virtio-crypto driver

>
> > > > +
> > > > +/* 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(&ctx->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

[toc] | [prev] | [next] | [standalone]

#1531992 — RE: [PATCH v3] crypto: add virtio-crypto driver

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

[toc] | [prev] | [next] | [standalone]

#1532036 — Re: [PATCH v3] crypto: add virtio-crypto driver

[Multipart message — attachments visible in raw view] — view raw

On Tue, Nov 29, 2016 at 08:22:58AM +0000, 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 0000000..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 not very familiar with the kernel Makefile infrastructure so maybe
there's a better way of doing it.

Stefan

[toc] | [prev] | [next] | [standalone]

#1532044 — Re: [PATCH v3] crypto: add virtio-crypto driver

On Tue, 29 Nov 2016 09:25:49 +0000
Stefan Hajnoczi <stefanha@redhat.com> wrote:

> On Tue, Nov 29, 2016 at 08:22:58AM +0000, 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 0000000..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.

[toc] | [prev] | [next] | [standalone]

#1532196 — RE: [PATCH v3] crypto: add virtio-crypto driver

> 
> > On Tue, Nov 29, 2016 at 08:22:58AM +0000, 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 0000000..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

[toc] | [prev] | [next] | [standalone]

#1532052 — RE: [PATCH v3] crypto: add virtio-crypto driver

> 
> On Tue, Nov 29, 2016 at 08:22:58AM +0000, 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 0000000..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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web