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

[Daniel P. Berrange]

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.

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.

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.

> 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

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.

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

> 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


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