[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: RE: [PATCH v2 00/15] virtio-crypto: introduce framework and device emulation
Hi Daniel, Thanks for your comments fristly, please see my embedded reply. Regards, -Gonglei > -----Original Message----- > From: Daniel P. Berrange [mailto:berrange@redhat.com] > Sent: Tuesday, September 13, 2016 4:58 PM > To: Gonglei (Arei) > Cc: qemu-devel@nongnu.org; virtio-dev@lists.oasis-open.org; Huangpeng > (Peter); Luonengjun; mst@redhat.com; stefanha@redhat.com; > pbonzini@redhat.com; Huangweidong (C); mike.caraman@nxp.com; > agraf@suse.de; xin.zeng@intel.com; Claudio Fontana; nmorey@kalray.eu; > vincent.jardin@6wind.com > Subject: Re: [PATCH v2 00/15] virtio-crypto: introduce framework and device > emulation > > On Tue, Sep 13, 2016 at 11:52:06AM +0800, Gonglei wrote: > > Changes since v1: > > - rmmove mixed endian-ness handler for virtio-crypto device, just > > use little-endian. [mst] > > - add sg list support according virtio-crypto spec v10 (will be posted soon). > > - fix a memory leak in session handler. > > - add a feature page link in qemu.org > (http://qemu-project.org/Features/VirtioCrypto) > > - fix some trivial problems, sush as 's/Since 2.7/Since 2.8/g' in > qapi-schema.json > > - rebase the latest qemu master tree. > > > > Please review, thanks! > > > > This patch series realize the framework and emulation of a new > > virtio crypto device, which is similar with virtio net device. > > > > - I introduce the cryptodev backend as the client of virtio crypto device > > which can be realized by different methods, such as cryptodev-linux in my > series, > > vhost-crypto kernel module, vhost-user etc. > > - The patch set abides by the virtio crypto speccification. > > - The virtio crypto support symmetric algorithms (including CIPHER and > algorithm chainning) > > at present, except HASH, MAC and AEAD services. > > - unsupport hot plug/unplug cryptodev client at this moment. > > > > Cryptodev-linux is a device that allows access to Linux kernel cryptographic > drivers; > > thus allowing of userspace applications to take advantage of hardware > accelerators. > > It can be found here: > > > > http://cryptodev-linux.org/ > > > > (please use the latest version) > > > > To use the cryptodev-linux as the client, the cryptodev.ko should be insert on > the host. > > > > # enter cryptodev-linux module root directory, then > > make;make install > > > The cryptodev kernel module is not upstream and shows no sign of > ever being likely to be accepted & merged upstream. On that basis, > support for it in QEMU has a firm NACK from me, as trying to support > out of tree kernel modules long term never ends well. This is > particularly bad because it appears to be the only impl backend > you've provided in this series. > OK, I agree with you :) But if we support multiple backends, can we keep cryptodev-linux module as one option? > IMHO, the first default backend should ought to use the internal > QEMU crypto APIs for its impl, which delegate to nettle/gcrypt, > which in turn can take care of using the kernel for acceleration > if they so choose. > Will work on this later. > > then configuring QEMU shows: > > > > [...] > > jemalloc support no > > avx2 optimization no > > cryptodev-linux support yes > > > > QEMU can then be started using the following parameters: > > > > qemu-system-x86_64 \ > > [...] \ > > -cryptodev type=cryptodev-linux,id=cryptodev0 \ > > -device virtio-crypto-pci,id=crypto0,cryptodev=cryptodev0 \ > > [...] > > > > The front-end linux kernel driver (Experimental at present) is publicly > accessible from: > > > > https://github.com/gongleiarei/virtio-crypto-linux-driver.git > > > > After insmod virtio-crypto.ko, you also can use cryptodev-linux test the crypto > function > > in the guest. For example: > > > > > > linux-guest:/home/gonglei/cryptodev-linux/tests # ./cipher - > > requested cipher CRYPTO_AES_CBC, got cbc(aes) with driver > virtio_crypto_aes_cbc > > AES Test passed > > requested cipher CRYPTO_AES_CBC, got cbc(aes) with driver > virtio_crypto_aes_cbc > > requested cipher CRYPTO_AES_CBC, got cbc(aes) with driver > virtio_crypto_aes_cbc > > Test passed > > > > QEMU code also can be accessible from: > > > > https://github.com/gongleiarei/qemu.git > > > > branch virtio-crypto > > > > For more information, please see: > > http://qemu-project.org/Features/VirtioCrypto > > > > > > Gonglei (15): > > crypto: introduce cryptodev backend and crypto legacy hardware > > crypto: introduce crypto queue handler > > crypto: add cryptoLegacyHW stuff > > crypto: add symetric algorithms support > > crypto: add cryptodev-linux as a cryptodev backend > > crypto: add internal handle logic layer > > virtio-crypto: introduce virtio-crypto.h > > virtio-crypto-pci: add virtio crypto pci support > > virtio-crypto: add virtio crypto realization > > virtio-crypto: set capacity of crypto legacy hardware > > virtio-crypto: add control queue handler > > virtio-crypto: add destroy session logic > > virtio-crypto: get correct input data address for each request > > virtio-crypto: add data virtqueue processing handler > > virtio-crypto: support scatter gather list > > > > configure | 16 + > > crypto/Makefile.objs | 3 + > > crypto/crypto-queue.c | 206 +++++ > > crypto/crypto.c | 378 +++++++++ > > As a general point, please don't take the plain 'crypto' > namespace - that's way too generic a term. > Ok, I'll use 'cryptodev' as the prefix in the following series. > I'd expect to see 'cryptodev.c' and 'cryptodev-queue.c' > really, since these files appear specific to the cryptodev > OK, it makes senses. > > crypto/cryptodev-linux.c | 419 ++++++++++ > > hw/core/qdev-properties-system.c | 86 ++ > > hw/virtio/Makefile.objs | 3 +- > > hw/virtio/virtio-crypto-pci.c | 71 ++ > > hw/virtio/virtio-crypto.c | 1013 > ++++++++++++++++++++++++ > > hw/virtio/virtio-pci.h | 15 + > > include/crypto/crypto-clients.h | 39 + > > I'd expect header file names to match the name of the > .c file containing the impl. You've not added any > crypto-clients.c file - if the code is in crypto.c > then the crypto-clients.h content should be in > crypto.h too. > Agree. > > include/crypto/crypto-queue.h | 69 ++ > > include/crypto/crypto.h | 192 +++++ > > Again, cryptodev.h and crypto-queue.h are preferrable > OK. > > Regards, > Daniel > -- > |: http://berrange.com -o- > http://www.flickr.com/photos/dberrange/ :| > |: http://libvirt.org -o- > http://virt-manager.org :| > |: http://autobuild.org -o- > http://search.cpan.org/~danberr/ :| > |: http://entangle-photo.org -o- > http://live.gnome.org/gtk-vnc :|
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]