RE: [PATCH v4 01/13] cryptodev: introduce cryptodev backend interface

["Gonglei (Arei)" <arei.gonglei@huawei.com>]

> -----Original Message-----
> From: Stefan Hajnoczi [mailto:stefanha@redhat.com]
> Sent: Tuesday, October 04, 2016 12:11 AM
> Subject: Re: [PATCH v4 01/13] cryptodev: introduce cryptodev backend
> interface
> 
> On Wed, Sep 28, 2016 at 04:25:40PM +0800, Gonglei wrote:
> > diff --git a/backends/cryptodev.c b/backends/cryptodev.c
> > new file mode 100644
> > index 0000000..a15904b
> > --- /dev/null
> > +++ b/backends/cryptodev.c
> > @@ -0,0 +1,175 @@
> > +/*
> > + * QEMU Crypto Device Implement
> 
> s/Implement/Implementation/
> 
> > diff --git a/include/sysemu/cryptodev.h b/include/sysemu/cryptodev.h
> > new file mode 100644
> > index 0000000..cc3c3be
> > --- /dev/null
> > +++ b/include/sysemu/cryptodev.h
> > @@ -0,0 +1,145 @@
> > +/*
> > + * QEMU Crypto Device Implement
> 
> s/Implement/Implementation/
> 
OK.

> > + *
> > + * Copyright (c) 2016 HUAWEI TECHNOLOGIES CO., LTD.
> > + *
> > + * Authors:
> > + *    Gonglei <arei.gonglei@huawei.com>
> > + *
> > + * This library is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU Lesser General Public
> > + * License as published by the Free Software Foundation; either
> > + * version 2 of the License, or (at your option) any later version.
> > + *
> > + * This library is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> GNU
> > + * Lesser General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU Lesser General Public
> > + * License along with this library; if not, see
> <http://www.gnu.org/licenses/>.
> > + *
> > + */
> > +#ifndef QCRYPTO_CRYPTODEV_H
> > +#define QCRYPTO_CRYPTODEV_H
> > +
> > +#include "qom/object.h"
> > +#include "qemu-common.h"
> > +
> > +/**
> > + * QCryptoCryptoDevBackend:
> > + *
> > + * The QCryptoCryptoDevBackend object is an interface
> > + * for different cryptodev backends, which provides crypto
> > + * operation wrapper.
> 
> I suggest calling it CryptoDevBackend since that's shorter and doesn't
> repeat any information.  I'm not sure why "QCrypto" is necessary.
> 
Ok, and Daniel explained the reason in another reply. I'll rename those stuff.

> > + *
> > + */
> > +
> > +#define TYPE_QCRYPTO_CRYPTODEV_BACKEND "cryptodev-backend"
> > +
> > +#define QCRYPTO_CRYPTODEV_BACKEND(obj) \
> > +    OBJECT_CHECK(QCryptoCryptoDevBackend, \
> > +                 (obj), TYPE_QCRYPTO_CRYPTODEV_BACKEND)
> > +#define QCRYPTO_CRYPTODEV_BACKEND_GET_CLASS(obj) \
> > +    OBJECT_GET_CLASS(QCryptoCryptoDevBackendClass, \
> > +                 (obj), TYPE_QCRYPTO_CRYPTODEV_BACKEND)
> > +#define QCRYPTO_CRYPTODEV_BACKEND_CLASS(klass) \
> > +    OBJECT_CLASS_CHECK(QCryptoCryptoDevBackendClass, \
> > +                (klass), TYPE_QCRYPTO_CRYPTODEV_BACKEND)
> > +
> > +
> > +#define MAX_CRYPTO_QUEUE_NUM  64
> > +
> > +typedef struct QCryptoCryptoDevBackendConf
> QCryptoCryptoDevBackendConf;
> > +typedef struct QCryptoCryptoDevBackendPeers
> QCryptoCryptoDevBackendPeers;
> > +typedef struct QCryptoCryptoDevBackendClientState
> > +                     QCryptoCryptoDevBackendClientState;
> > +typedef struct QCryptoCryptoDevBackend QCryptoCryptoDevBackend;
> > +
> > +
> > +typedef struct QCryptoCryptoDevBackendClass {
> > +    ObjectClass parent_class;
> > +
> > +    void (*init)(QCryptoCryptoDevBackend *backend, Error **errp);
> > +    void (*cleanup)(QCryptoCryptoDevBackend *backend, Error **errp);
> > +} QCryptoCryptoDevBackendClass;
> > +
> > +
> > +struct QCryptoCryptoDevBackendClientState {
> 
> The naming could be simplified: CryptoDevClient.
> 
> > +    char *model;
> > +    char *name;
> > +    char info_str[128];
> 
> This should probably be char *info_str unless there is a specific reason
> to use a fixed-size buffer.
> 
OK.

> > +    unsigned int queue_index;
> > +    QTAILQ_ENTRY(QCryptoCryptoDevBackendClientState) next;
> > +};
> > +
> > +struct QCryptoCryptoDevBackendPeers {
> > +    QCryptoCryptoDevBackendClientState
> *ccs[MAX_CRYPTO_QUEUE_NUM];
> > +    uint32_t queues;
> > +};
> > +
> > +struct QCryptoCryptoDevBackendConf {
> > +    QCryptoCryptoDevBackendPeers peers;
> > +
> > +    /* Supported service mask */
> > +    uint32_t crypto_services;
> > +
> > +    /* Detailed algorithms mask */
> > +    uint32_t cipher_algo_l;
> > +    uint32_t cipher_algo_h;
> > +    uint32_t hash_algo;
> > +    uint32_t mac_algo_l;
> > +    uint32_t mac_algo_h;
> > +    uint32_t asym_algo;
> > +    uint32_t kdf_algo;
> > +    uint32_t aead_algo;
> > +    uint32_t primitive_algo;
> > +};
> > +
> > +struct QCryptoCryptoDevBackend {
> > +    Object parent_obj;
> > +
> > +    int ready;
> 
> Please use bool.  That way it's clear the field only takes true and
> false values.
> 
OK.

> > +    QCryptoCryptoDevBackendConf conf;
> > +};
> > +
> > +/**
> > + * qcrypto_cryptodev_backend_new_client:
> > + * @model: the cryptodev backend model
> > + * @name: the cryptodev backend name, can be NULL
> > + *
> > + * Creates a new cryptodev backend client object
> > + * with the @name in the mode @mode.
> 
> s/mode/model/
> 
Yes.


Regards,
-Gonglei