Re: [Qemu-devel] [PATCH v2 01/15] crypto: introduce cryptodev backend and crypto legacy hardware

[Gonglei (Arei)]

>
> From: Daniel P. Berrange [mailto:address@hidden
> 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 <address@hidden>
> 
> > 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 <address@hidden>
> > + *
> > + * 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 :|