[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 00/15] virtio-crypto: introduce framework and
From: |
Gonglei (Arei) |
Subject: |
Re: [Qemu-devel] [PATCH v2 00/15] virtio-crypto: introduce framework and device emulation |
Date: |
Tue, 13 Sep 2016 09:45:05 +0000 |
Hi Daniel,
Thanks for your comments fristly, please see my embedded reply.
Regards,
-Gonglei
> -----Original Message-----
> From: Daniel P. Berrange [mailto:address@hidden
> Sent: Tuesday, September 13, 2016 4:58 PM
> To: Gonglei (Arei)
> Cc: address@hidden; address@hidden; Huangpeng
> (Peter); Luonengjun; address@hidden; address@hidden;
> address@hidden; Huangweidong (C); address@hidden;
> address@hidden; address@hidden; Claudio Fontana; address@hidden;
> address@hidden
> 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 :|
- [Qemu-devel] [PATCH v2 08/15] virtio-crypto-pci: add virtio crypto pci support, (continued)
- [Qemu-devel] [PATCH v2 08/15] virtio-crypto-pci: add virtio crypto pci support, Gonglei, 2016/09/12
- [Qemu-devel] [PATCH v2 05/15] crypto: add cryptodev-linux as a cryptodev backend, Gonglei, 2016/09/12
- [Qemu-devel] [PATCH v2 04/15] crypto: add symetric algorithms support, Gonglei, 2016/09/12
- [Qemu-devel] [PATCH v2 12/15] virtio-crypto: add destroy session logic, Gonglei, 2016/09/12
- [Qemu-devel] [PATCH v2 07/15] virtio-crypto: introduce virtio-crypto.h, Gonglei, 2016/09/12
- [Qemu-devel] [PATCH v2 11/15] virtio-crypto: add control queue handler, Gonglei, 2016/09/12
- [Qemu-devel] [PATCH v2 13/15] virtio-crypto: get correct input data address for each request, Gonglei, 2016/09/12
- [Qemu-devel] [PATCH v2 14/15] virtio-crypto: add data virtqueue processing handler, Gonglei, 2016/09/12
- Re: [Qemu-devel] [PATCH v2 00/15] virtio-crypto: introduce framework and device emulation, Daniel P. Berrange, 2016/09/13
- Re: [Qemu-devel] [PATCH v2 00/15] virtio-crypto: introduce framework and device emulation,
Gonglei (Arei) <=