>
> From: Daniel P. Berrange [mailto:berra...@redhat.com]
> Sent: Tuesday, September 13, 2016 5:14 PM
> Subject: Re: [PATCH v2 01/15] crypto: introduce cryptodev backend and crypto
> legacy hardware
>
> On Tue, Sep 13, 2016 at 11:52:07AM +0800, Gonglei wrote:
> > cryptodev backend is used to realize the active work for
> > virtual crypto device. CryptoLegacyHW device is a cryptographic
> > hardware device seen by the virtual machine.
> > The relationship between cryptodev backend and legacy hadware
> > as follow:
> > crypto_legacy_hw device (1)--->(n) cryptodev client backend
> >
> > Signed-off-by: Gonglei <arei.gong...@huawei.com>
>
> > diff --git a/crypto/crypto.c b/crypto/crypto.c
> > new file mode 100644
> > index 0000000..fbc6497
> > --- /dev/null
> > +++ b/crypto/crypto.c
> > @@ -0,0 +1,171 @@
> > +/*
> > + * QEMU Crypto Device Implement
> > + *
> > + * Copyright (c) 2016 HUAWEI TECHNOLOGIES CO., LTD.
> > + *
> > + * Authors:
> > + * Gonglei <arei.gong...@huawei.com>
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining a
> copy
> > + * of this software and associated documentation files (the "Software"), to
> deal
> > + * in the Software without restriction, including without limitation the
> > rights
> > + * to use, copy, modify, merge, publish, distribute, sublicense, and/or
> > sell
> > + * copies of the Software, and to permit persons to whom the Software is
> > + * furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice shall be included
> > in
> > + * all copies or substantial portions of the Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> EXPRESS OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO
> EVENT SHALL
> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
> DAMAGES OR OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> ARISING FROM,
> > + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> OTHER DEALINGS IN
> > + * THE SOFTWARE.
> > + */
>
> New files are expected to be submitted under the GPLv2+ license,
> unless they're header files imported from an external project,
> which this is not.
>
> The same license mistake is made across other files / patches
> in this series, so I won't repeat the comment - just fix all
> of them to be GPLv2+ please.
>
> > +#include "qemu/osdep.h"
> > +#include "sysemu/sysemu.h"
> > +#include "qapi/error.h"
> > +#include "qemu/iov.h"
> > +#include "qapi-visit.h"
> > +#include "qapi/opts-visitor.h"
> > +
> > +#include "crypto/crypto.h"
> > +#include "qemu/config-file.h"
> > +#include "monitor/monitor.h"
> > +
> > +
> > +static QTAILQ_HEAD(, CryptoClientState) crypto_clients;
> > +
> > +QemuOptsList qemu_cryptodev_opts = {
> > + .name = "cryptodev",
> > + .implied_opt_name = "type",
> > + .head = QTAILQ_HEAD_INITIALIZER(qemu_cryptodev_opts.head),
> > + .desc = {
> > + /*
> > + * no elements => accept any params
> > + * validation will happen later
> > + */
> > + { /* end of list */ }
> > + },
> > +};
>
> No new code should be adding more command line options for
> device backends.
>
> Your backend impl should be using QOM to define a object,
> which can be created via the -object command line arg,
> or object_add monitor command.
>
Any backgrounds about this rule? Maybe I missed something.
> If you're not familiar with this, take a look at the
> crypto/secret.c file which is a pretty simple example of
> how to use QOM to define a new user creatable object.
>
OK, thanks.
> I'm going to skip reviewing any of the .c code in the
> crypto/ dir for now, since that will all change quite a
> bit when you switch over to QOM.
>
OK.
> > diff --git a/include/crypto/crypto.h b/include/crypto/crypto.h
> > new file mode 100644
> > index 0000000..f93f6f9
> > --- /dev/null
> > +++ b/include/crypto/crypto.h
>
> > +#ifndef QCRYPTO_CRYPTO_H__
> > +#define QCRYPTO_CRYPTO_H__
> > +
> > +#include "qemu/queue.h"
> > +#include "qapi-types.h"
> > +
> > +typedef void (CryptoPoll)(CryptoClientState *, bool);
> > +typedef void (CryptoCleanup) (CryptoClientState *);
> > +typedef void (CryptoClientDestructor)(CryptoClientState *);
> > +typedef void (CryptoHWStatusChanged)(CryptoClientState *);
> > +
> > +typedef struct CryptoClientInfo {
> > + CryptoClientOptionsKind type;
> > + size_t size;
> > +
> > + CryptoCleanup *cleanup;
> > + CryptoPoll *poll;
> > + CryptoHWStatusChanged *hw_status_changed;
> > +} CryptoClientInfo;
> > +
> > +struct CryptoClientState {
> > + CryptoClientInfo *info;
> > + int ready;
> > + QTAILQ_ENTRY(CryptoClientState) next;
> > + CryptoClientState *peer;
> > + char *model;
> > + char *name;
> > + char info_str[256];
> > + CryptoClientDestructor *destructor;
> > +};
> > +
> > +int crypto_client_init(QemuOpts *opts, Error **errp);
> > +int crypto_init_clients(void);
> > +
> > +CryptoClientState *new_crypto_client(CryptoClientInfo *info,
> > + CryptoClientState *peer,
> > + const char *model,
> > + const char *name);
> > +
> > +#endif /* QCRYPTO_CRYPTO_H__ */
>
> For all files in the crypto sub-directory I've been trying
> to stick to the strict convention that all methods must
> follow the naming scheme qcrypto_[filename]_XX
> eg if you rename this file to cryptodev as requested,
> your methods should all follow the naming convention
> of 'qcrypto_cryptdev_XXX'.
>
> Similarly all structs would be QCryptoCryptoDevXXX
>
OK.
> Finally the .h file should contain full inline documentation.
> At start there should be a general description of the file
> and its purpose, along with example usages. Then there should
> be formal documentation for every single method in the .h
> file describing the method semantics, parameters and return
> values. See include/crypto/cipher.h for an example.
>
OK, make senses to me.
> > diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
> > index b113fcf..82843a9 100644
> > --- a/include/qemu/typedefs.h
> > +++ b/include/qemu/typedefs.h
> > @@ -94,5 +94,6 @@ typedef struct SSIBus SSIBus;
> > typedef struct uWireSlave uWireSlave;
> > typedef struct VirtIODevice VirtIODevice;
> > typedef struct Visitor Visitor;
> > +typedef struct CryptoClientState CryptoClientState;
>
> Don't add to this file - anything that wants to see
> the CryptoClientState typedef should explicitly include
> the crypto/cryptodev.h file or whatever the master
> definition lives.
>
OK.
> > diff --git a/qapi-schema.json b/qapi-schema.json
> > index c4f3674..46f7993 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -4582,3 +4582,49 @@
> > # Since: 2.7
> > ##
> > { 'command': 'query-hotpluggable-cpus', 'returns': ['HotpluggableCPU'] }
> > +
> > +##
> > +# @CryptoLegacyHWOptions
> > +#
> > +# Create a new cryptographic hardware device.
> > +#
> > +# @cryptodev: #optional id of -cryptodev to connect to
> > +#
> > +# @model: #optional device model (Only virtio at present)
> > +#
> > +# @vectors: #optional number of MSI-x vectors, 0 to disable MSI-X
> > +#
> > +# Since 2.8
> > +##
> > +{ 'struct': 'CryptoLegacyHWOptions',
> > + 'data': {
> > + '*cryptodev': 'str',
> > + '*model': 'str',
> > + '*vectors': 'uint32' } }
> > +
> > +##
> > +# @CryptoClientOptions
> > +#
> > +# A discriminated record of crypto device traits.
> > +#
> > +# Since 2.8
> > +#
> > +##
> > +{ 'union': 'CryptoClientOptions',
> > + 'data': {'legacy-hw': 'CryptoLegacyHWOptions'} }
> > +
> > +##
> > +# @Cryptodev
> > +#
> > +# Captures the configuration of a crypto device.
> > +#
> > +# @id: identifier for monitor commands.
> > +#
> > +# @opts: device type specific properties
> > +#
> > +# Since 2.8
> > +##
> > +{ 'struct': 'Cryptodev',
> > + 'data': {
> > + 'id': 'str',
> > + 'opts': 'CryptoClientOptions' } }
>
> All crypto related QAPI additions should be in qapi/crypto.json
>
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 :|
