From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42498) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1brckh-0007PY-6H for qemu-devel@nongnu.org; Tue, 04 Oct 2016 23:20:16 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1brckc-0003bm-Mz for qemu-devel@nongnu.org; Tue, 04 Oct 2016 23:20:14 -0400 Received: from szxga03-in.huawei.com ([119.145.14.66]:52522) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1brckb-0003VH-Q1 for qemu-devel@nongnu.org; Tue, 04 Oct 2016 23:20:10 -0400 From: "Gonglei (Arei)" Date: Wed, 5 Oct 2016 03:19:44 +0000 Message-ID: <33183CC9F5247A488A2544077AF19020B03E658A@SZXEMA503-MBS.china.huawei.com> References: <1475051152-400276-1-git-send-email-arei.gonglei@huawei.com> <1475051152-400276-5-git-send-email-arei.gonglei@huawei.com> <20161003163140.GX10245@stefanha-x1.localdomain> In-Reply-To: <20161003163140.GX10245@stefanha-x1.localdomain> Content-Language: zh-CN Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH v4 04/13] cryptodev: introduce a new cryptodev backend List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: "qemu-devel@nongnu.org" , "virtio-dev@lists.oasis-open.org" , Luonengjun , "mst@redhat.com" , "pbonzini@redhat.com" , "berrange@redhat.com" , "Huangweidong (C)" , "Wubin (H)" , "mike.caraman@nxp.com" , "agraf@suse.de" , "xin.zeng@intel.com" , Claudio Fontana , "nmorey@kalray.eu" , "vincent.jardin@6wind.com" , "Zhoujian (jay, Euler)" , "Hanweidong (Randy)" , "Huangpeng (Peter)" > -----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 backen= d >=20 > On Wed, Sep 28, 2016 at 04:25:43PM +0800, Gonglei wrote: > > +/* Max number of symetrical sessions */ >=20 > s/symetrical/symmetric/ >=20 > 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)? >=20 > > +#define MAX_NUM_SESSIONS 256 >=20 > The guest can only have 256 sessions open? >=20 For cipher API backend, it's a experimental cryptodev backend, which can't be applied in production environment because of the poor performance.= =20 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? >=20 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 =3D MAX(backend->conf.peers.queues, 1); > > + size_t i; > > + QCryptoCryptoDevBackendClientState *cc; > > + > > + for (i =3D 0; i < queues; i++) { > > + cc =3D qcrypto_cryptodev_backend_new_client( > > + "cryptodev-builtin", NULL); > > + snprintf(cc->info_str, sizeof(cc->info_str), > > + "cryptodev-builtin%lu", i); > > + cc->queue_index =3D i; > > + > > + backend->conf.peers.ccs[i] =3D cc; > > + } > > + > > + backend->conf.crypto_services =3D > > + 1u << VIRTIO_CRYPTO_SERVICE_CIPHER | > > + 1u << VIRTIO_CRYPTO_SERVICE_HASH | > > + 1u << VIRTIO_CRYPTO_SERVICE_MAC; > > + backend->conf.cipher_algo_l =3D 1u << > VIRTIO_CRYPTO_CIPHER_AES_CBC; > > + backend->conf.hash_algo =3D 1u << VIRTIO_CRYPTO_HASH_SHA1; > > +} > > + > > +static int > > +qcrypto_cryptodev_backend_builtin_get_unused_session_index( > > + QCryptoCryptoDevBackendBuiltin *builtin) > > +{ > > + size_t i; > > + > > + for (i =3D 0; i < MAX_NUM_SESSIONS; i++) { > > + if (builtin->sessions[i] =3D=3D NULL) { > > + return i; > > + } > > + } > > + > > + return -1; > > +} > > + > > +static int > > +qcrypto_cryptodev_backend_builtin_get_algo(uint32_t key_len, > > + Error **errp) > > +{ > > + int algo; > > + > > + if (key_len =3D=3D 128 / 8) { > > + algo =3D QCRYPTO_CIPHER_ALG_AES_128; > > + } else if (key_len =3D=3D 192 / 8) { > > + algo =3D QCRYPTO_CIPHER_ALG_AES_192; > > + } else if (key_len =3D=3D 256 / 8) { > > + algo =3D 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 !=3D VIRTIO_CRYPTO_SYM_OP_CIPHER) { > > + error_setg(errp, "unsupported optype :%u", sess_info->op_type)= ; > > + return -1; > > + } > > + > > + index =3D > qcrypto_cryptodev_backend_builtin_get_unused_session_index(builtin); >=20 > Feel free to omit the function name prefix for static functions. These > names are very long. >=20 > > + if (index < 0) { > > + error_setg(errp, "the total number of created session > exceed %u", >=20 > "Total number of sessions created exceeds %u" >=20 > > + MAX_NUM_SESSIONS); > > + return -1; > > + } > > + > > + switch (sess_info->cipher_alg) { > > + case VIRTIO_CRYPTO_CIPHER_AES_ECB: > > + algo =3D > qcrypto_cryptodev_backend_builtin_get_algo(sess_info->key_len, > > + > errp); > > + if (algo < 0) { > > + return -1; > > + } > > + mode =3D QCRYPTO_CIPHER_MODE_ECB; > > + break; > > + case VIRTIO_CRYPTO_CIPHER_AES_CBC: > > + algo =3D > qcrypto_cryptodev_backend_builtin_get_algo(sess_info->key_len, > > + > errp); > > + if (algo < 0) { > > + return -1; > > + } > > + mode =3D QCRYPTO_CIPHER_MODE_CBC; > > + break; > > + case VIRTIO_CRYPTO_CIPHER_AES_CTR: > > + algo =3D > qcrypto_cryptodev_backend_builtin_get_algo(sess_info->key_len, > > + > errp); > > + if (algo < 0) { > > + return -1; > > + } > > + mode =3D QCRYPTO_CIPHER_MODE_CTR; > > + break; > > + default: > > + error_setg(errp, "unsupported cipher alg :%u", > > + sess_info->cipher_alg); > > + return -1; > > + } >=20 > Code duplication can be eliminated: >=20 > switch (sess_info->cipher_alg) { > case VIRTIO_CRYPTO_CIPHER_AES_ECB: > mode =3D QCRYPTO_CIPHER_MODE_ECB; > break; > ... > } >=20 > algo =3D qcrypto_cryptodev_backend_builtin_get_algo(sess_info->key_len, > errp); > if (algo < 0) { > return -1; > } >=20 Make sense. :) > > +static void qcrypto_cryptodev_backend_builtin_cleanup( > > + QCryptoCryptoDevBackend *backend, > > + Error **errp) > > +{ > > + QCryptoCryptoDevBackendBuiltin *builtin =3D > > + > QCRYPTO_CRYPTODEV_BACKEND_BUILTIN(backend); > > + size_t i; > > + int queues =3D backend->conf.peers.queues; > > + QCryptoCryptoDevBackendClientState *cc; > > + > > + for (i =3D 0; i < MAX_NUM_SESSIONS; i++) { > > + if (builtin->sessions[i] !=3D NULL) { > > + qcrypto_cryptodev_backend_builtin_sym_close_session( > > + backend, i, 0, errp); > > + } > > + } > > + > > + for (i =3D 0; i < queues; i++) { >=20 > This device doesn't seem to support queues because queue_index is > ignored by all functions that take it. >=20 > 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