RE: [PATCH v4 04/13] cryptodev: introduce a new cryptodev backend

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

> -----Original Message-----
> From: Stefan Hajnoczi [mailto:stefanha@redhat.com]
> Sent: Tuesday, October 04, 2016 12:32 AM
> Subject: Re: [PATCH v4 04/13] cryptodev: introduce a new cryptodev backend
> 
> On Wed, Sep 28, 2016 at 04:25:43PM +0800, Gonglei wrote:
> > +/* Max number of symetrical sessions */
> 
> s/symetrical/symmetric/
> 
> But why does the comment say "symetrical" when the constant name
> MAX_NUM_SESSIONS seems to be a global limit for *all* sessions (not just
> symmetric)?
> 
> > +#define MAX_NUM_SESSIONS 256
> 
> The guest can only have 256 sessions open?
> 
For cipher API backend, it's a experimental cryptodev backend, which
can't be applied in production environment because of the poor performance. 
The limit is just for simplifying code logic, of course we can increase the number
as well, but it doesn't necessary IMO.

> What are the limits of real crypto libraries and accelerators?
> 
The hardware accelerators maybe have a limit, for example the
Intel QAT pmd driver in DPDK limit the maximum num of session is 2048.

> > +
> > +
> > +struct QCryptoCryptoDevBackendBuiltin {
> > +    QCryptoCryptoDevBackend parent_obj;
> > +
> > +    QCryptoCryptoDevBackendBuiltinSession
> *sessions[MAX_NUM_SESSIONS];
> > +};
> > +
> > +static void qcrypto_cryptodev_backend_builtin_init(
> > +             QCryptoCryptoDevBackend *backend, Error **errp)
> > +{
> > +    /* Only support one queue */
> > +    int queues = MAX(backend->conf.peers.queues, 1);
> > +    size_t i;
> > +    QCryptoCryptoDevBackendClientState *cc;
> > +
> > +    for (i = 0; i < queues; i++) {
> > +        cc = qcrypto_cryptodev_backend_new_client(
> > +                  "cryptodev-builtin", NULL);
> > +        snprintf(cc->info_str, sizeof(cc->info_str),
> > +                 "cryptodev-builtin%lu", i);
> > +        cc->queue_index = i;
> > +
> > +        backend->conf.peers.ccs[i] = cc;
> > +    }
> > +
> > +    backend->conf.crypto_services =
> > +                         1u << VIRTIO_CRYPTO_SERVICE_CIPHER |
> > +                         1u << VIRTIO_CRYPTO_SERVICE_HASH |
> > +                         1u << VIRTIO_CRYPTO_SERVICE_MAC;
> > +    backend->conf.cipher_algo_l = 1u <<
> VIRTIO_CRYPTO_CIPHER_AES_CBC;
> > +    backend->conf.hash_algo = 1u << VIRTIO_CRYPTO_HASH_SHA1;
> > +}
> > +
> > +static int
> > +qcrypto_cryptodev_backend_builtin_get_unused_session_index(
> > +      QCryptoCryptoDevBackendBuiltin *builtin)
> > +{
> > +    size_t i;
> > +
> > +    for (i = 0; i < MAX_NUM_SESSIONS; i++) {
> > +        if (builtin->sessions[i] == NULL) {
> > +            return i;
> > +        }
> > +    }
> > +
> > +    return -1;
> > +}
> > +
> > +static int
> > +qcrypto_cryptodev_backend_builtin_get_algo(uint32_t key_len,
> > +                                           Error **errp)
> > +{
> > +    int algo;
> > +
> > +    if (key_len == 128 / 8) {
> > +        algo = QCRYPTO_CIPHER_ALG_AES_128;
> > +    } else if (key_len == 192 / 8) {
> > +        algo = QCRYPTO_CIPHER_ALG_AES_192;
> > +    } else if (key_len == 256 / 8) {
> > +        algo = QCRYPTO_CIPHER_ALG_AES_256;
> > +    } else {
> > +        error_setg(errp, "unsupported key length :%u", key_len);
> > +        return -1;
> > +    }
> > +
> > +    return algo;
> > +}
> > +
> > +static int qcrypto_cryptodev_backend_builtin_create_cipher_session(
> > +                    QCryptoCryptoDevBackendBuiltin *builtin,
> > +                    QCryptoCryptoDevBackendSymSessionInfo
> *sess_info,
> > +                    Error **errp)
> > +{
> > +    int algo;
> > +    int mode;
> > +    QCryptoCipher *cipher;
> > +    int index;
> > +    QCryptoCryptoDevBackendBuiltinSession *sess;
> > +
> > +    if (sess_info->op_type != VIRTIO_CRYPTO_SYM_OP_CIPHER) {
> > +        error_setg(errp, "unsupported optype :%u", sess_info->op_type);
> > +        return -1;
> > +    }
> > +
> > +    index =
> qcrypto_cryptodev_backend_builtin_get_unused_session_index(builtin);
> 
> Feel free to omit the function name prefix for static functions.  These
> names are very long.
> 
> > +    if (index < 0) {
> > +        error_setg(errp, "the total number of created session
> exceed %u",
> 
> "Total number of sessions created exceeds %u"
> 
> > +                  MAX_NUM_SESSIONS);
> > +        return -1;
> > +    }
> > +
> > +    switch (sess_info->cipher_alg) {
> > +    case VIRTIO_CRYPTO_CIPHER_AES_ECB:
> > +        algo =
> qcrypto_cryptodev_backend_builtin_get_algo(sess_info->key_len,
> > +
> errp);
> > +        if (algo < 0)  {
> > +            return -1;
> > +        }
> > +        mode = QCRYPTO_CIPHER_MODE_ECB;
> > +        break;
> > +    case VIRTIO_CRYPTO_CIPHER_AES_CBC:
> > +        algo =
> qcrypto_cryptodev_backend_builtin_get_algo(sess_info->key_len,
> > +
> errp);
> > +        if (algo < 0)  {
> > +            return -1;
> > +        }
> > +        mode = QCRYPTO_CIPHER_MODE_CBC;
> > +        break;
> > +    case VIRTIO_CRYPTO_CIPHER_AES_CTR:
> > +        algo =
> qcrypto_cryptodev_backend_builtin_get_algo(sess_info->key_len,
> > +
> errp);
> > +        if (algo < 0)  {
> > +            return -1;
> > +        }
> > +        mode = QCRYPTO_CIPHER_MODE_CTR;
> > +        break;
> > +    default:
> > +        error_setg(errp, "unsupported cipher alg :%u",
> > +                   sess_info->cipher_alg);
> > +        return -1;
> > +    }
> 
> Code duplication can be eliminated:
> 
> switch (sess_info->cipher_alg) {
> case VIRTIO_CRYPTO_CIPHER_AES_ECB:
>     mode = QCRYPTO_CIPHER_MODE_ECB;
>     break;
> ...
> }
> 
> algo = qcrypto_cryptodev_backend_builtin_get_algo(sess_info->key_len,
>                                                   errp);
> if (algo < 0) {
>     return -1;
> }
> 
Make sense. :)

> > +static void qcrypto_cryptodev_backend_builtin_cleanup(
> > +             QCryptoCryptoDevBackend *backend,
> > +             Error **errp)
> > +{
> > +    QCryptoCryptoDevBackendBuiltin *builtin =
> > +
> QCRYPTO_CRYPTODEV_BACKEND_BUILTIN(backend);
> > +    size_t i;
> > +    int queues = backend->conf.peers.queues;
> > +    QCryptoCryptoDevBackendClientState *cc;
> > +
> > +    for (i = 0; i < MAX_NUM_SESSIONS; i++) {
> > +        if (builtin->sessions[i] != NULL) {
> > +            qcrypto_cryptodev_backend_builtin_sym_close_session(
> > +                    backend, i, 0, errp);
> > +        }
> > +    }
> > +
> > +    for (i = 0; i < queues; i++) {
> 
> This device doesn't seem to support queues because queue_index is
> ignored by all functions that take it.
> 
> Perhaps there should be an error if the device is realized with more
> than 1 queue?

Yes, we can add the check for cryptodev-builtin backend.


Regards,
-Gonglei