Re: [Qemu-devel] [PATCH v4 04/13] cryptodev: introduce a new cryptodev backend

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 5758 bytes --]

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?

What are the limits of real crypto libraries and accelerators?

> +
> +
> +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;
}

> +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?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]