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

2016-11-29 Thread Gonglei (Arei) 
>
> Subject: Re: [virtio-dev] Re: [PATCH v3] crypto: add virtio-crypto driver
> 
> On Tue, 29 Nov 2016 01:37:44 +
> "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

2016-11-29 Thread 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: [virtio-dev] Re: [PATCH v3] crypto: add virtio-crypto driver

2016-11-29 Thread Cornelia Huck 
On Tue, 29 Nov 2016 01:37:44 +
"Gonglei (Arei)"  wrote:

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

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.

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

2016-11-29 Thread 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

2016-11-29 Thread Cornelia Huck 
On Tue, 29 Nov 2016 09:25:49 +
Stefan Hajnoczi  wrote:

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

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

2016-11-29 Thread Stefan Hajnoczi 
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 not very familiar with the kernel Makefile infrastructure so maybe
there's a better way of doing it.

Stefan


signature.asc
Description: PGP signature

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

2016-11-29 Thread 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

2016-11-28 Thread 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

2016-11-28 Thread 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

2016-11-28 Thread 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 <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.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 <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 # CRY

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

2016-11-28 Thread Gonglei (Arei) 
>
> Subject: [virtio-dev] Re: [PATCH v3] crypto: add virtio-crypto driver
> 
> 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.

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: [PATCH v3] crypto: add virtio-crypto driver

2016-11-28 Thread Halil Pasic 


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.

I know we have the same message in 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 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(>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(>lock);
>>> > > +
>>> > > +   kfree(cipher_key);
>>> > > +   return 0;
>>> > > +}
>>> > > +

--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to

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

2016-11-28 Thread Michael S. Tsirkin 
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.
> 
> > 
> > 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
> 
> Inconsistent tab vs space whitespace usage.
> 
> > +   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.
> 
> >

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

2016-11-28 Thread Stefan Hajnoczi 
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.

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

Inconsistent tab vs space whitespace usage.

> + 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
('-').  I suggest calling it virtio_crypto for consistency.

> 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 := \
> +

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

2016-11-28 Thread kbuild test robot 
Hi Gonglei,

[auto build test ERROR on cryptodev/master]
[also build test ERROR on v4.9-rc7 next-20161128]
[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-driver/20161128-214706
base:   
https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git master
config: sparc-allmodconfig (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.cross
 -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=sparc 

All error/warnings (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:118: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:118: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 +/topology_physical_package_id +118 
drivers/crypto/virtio/virtio_crypto_common.h

   102  };
   103  
   104  int virtcrypto_devmgr_add_dev(struct virtio_crypto *vcrypto_dev);
   105  struct list_head *virtcrypto_devmgr_get_head(void);
   106  void virtcrypto_devmgr_rm_dev(struct virtio_crypto *vcrypto_dev);
   107  struct virtio_crypto *virtcrypto_devmgr_get_first(void);
   108  int virtcrypto_dev_in_use(struct virtio_crypto *vcrypto_dev);
   109  int virtcrypto_dev_get(struct virtio_crypto *vcrypto_dev);
   110  void virtcrypto_dev_put(struct virtio_crypto *vcrypto_dev);
   111  int virtcrypto_dev_started(struct virtio_crypto *vcrypto_dev);
   112  struct virtio_crypto *virtcrypto_get_dev_node(int node);
   113  int virtcrypto_dev_start(struct virtio_crypto *vcrypto);
   114  void virtcrypto_dev_stop(struct virtio_crypto *vcrypto);
   115  
   116  static inline int virtio_crypto_get_current_node(void)
   117  {
 > 118  return topology_physical_package_id(smp_processor_id());
   119  }
   120  
   121  int virtio_crypto_algs_register(void);
   122  void virtio_crypto_algs_unregister(void);
   123  
   124  #endif /* _VIRITO_CRYPTO_COMMON_H */

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip

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

2016-11-28 Thread Cornelia Huck 
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?

> +
> + 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 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