Re: [Qemu-devel] [PATCH v2 00/15] virtio-crypto: introduce framework and device emulation

[Daniel P. Berrange]

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.

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.

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

I'd expect to see 'cryptodev.c' and 'cryptodev-queue.c'
really, since these files appear specific to the cryptodev

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

>  include/crypto/crypto-queue.h                  |   69 ++
>  include/crypto/crypto.h                        |  192 +++++

Again, cryptodev.h and crypto-queue.h are preferrable


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