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 + > "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
> > > 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
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
> > 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
On Tue, 29 Nov 2016 09:25:49 + Stefan Hajnocziwrote: > 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
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
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
> > > > > + > > > > +/* 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
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
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
> > 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
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
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
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
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
On Mon, 28 Nov 2016 20:08:23 +0800 Gongleiwrote: > +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