From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53787) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1brMf8-0002l7-Aj for qemu-devel@nongnu.org; Tue, 04 Oct 2016 06:09:30 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1brMf5-0002nB-0k for qemu-devel@nongnu.org; Tue, 04 Oct 2016 06:09:26 -0400 Received: from mx1.redhat.com ([209.132.183.28]:56940) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1brMf4-0002mE-MT for qemu-devel@nongnu.org; Tue, 04 Oct 2016 06:09:22 -0400 Date: Tue, 4 Oct 2016 11:09:14 +0100 From: Stefan Hajnoczi Message-ID: <20161004100914.GG4587@stefanha-x1.localdomain> References: <1475051152-400276-1-git-send-email-arei.gonglei@huawei.com> <1475051152-400276-9-git-send-email-arei.gonglei@huawei.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="/i8j2F0k9BYX4qLc" Content-Disposition: inline In-Reply-To: <1475051152-400276-9-git-send-email-arei.gonglei@huawei.com> Subject: Re: [Qemu-devel] [PATCH v4 08/13] virtio-crypto: add control queue handler List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Gonglei Cc: qemu-devel@nongnu.org, virtio-dev@lists.oasis-open.org, luonengjun@huawei.com, mst@redhat.com, pbonzini@redhat.com, berrange@redhat.com, weidong.huang@huawei.com, wu.wubin@huawei.com, mike.caraman@nxp.com, agraf@suse.de, xin.zeng@intel.com, claudio.fontana@huawei.com, nmorey@kalray.eu, vincent.jardin@6wind.com, jianjay.zhou@huawei.com, hanweidong@huawei.com, peter.huangpeng@huawei.com --/i8j2F0k9BYX4qLc Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Sep 28, 2016 at 04:25:47PM +0800, Gonglei wrote: > -static void virtio_crypto_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq) > +static inline int virtio_crypto_vq2q(int queue_index) > +{ > + return queue_index; > +} Please document this function. I think it takes a virtqueue index and returns the crypto queue. The ctrl virtqueue is after the op virtqueues so the input value doesn't need to be adjusted. Without this information it's hard to understand the function. > + > +static void > +virtio_crypto_cipher_session_helper(VirtIODevice *vdev, > + QCryptoCryptoDevBackendSymSessionInfo *info, > + struct virtio_crypto_cipher_session_para *cipher_para, > + struct virtio_crypto_cipher_session_output *cipher_out) > +{ > + hwaddr key_gpa; > + void *key_hva; > + hwaddr len; > + > + info->cipher_alg =3D cipher_para->algo; > + info->key_len =3D cipher_para->keylen; > + info->direction =3D cipher_para->op; Endianness? Use the virtio_ldl_p() family of functions to load values =66rom the guest. This same issue is present in the rest of the code. I won't mentioned it again but please fix all occurrences. > + len =3D info->key_len; > + /* get cipher key */ > + if (len > 0) { > + DPRINTF("keylen=3D%" PRIu32 "\n", info->key_len); > + key_gpa =3D cipher_out->key_addr; > + > + key_hva =3D cpu_physical_memory_map(key_gpa, &len, 0); virtio devices should not use cpu_physical_memory_map(). Please see my reply to the virtio-crypto specification about scatter-gather I/O. > +static void virtio_crypto_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq) > +{ > + VirtIOCrypto *vcrypto =3D VIRTIO_CRYPTO(vdev); > + struct virtio_crypto_op_ctrl_req ctrl; > + VirtQueueElement *elem; > + size_t s; > + struct iovec *iov; > + unsigned int iov_cnt; > + uint32_t queue_id; > + uint32_t opcode; > + > + for (;;) { > + elem =3D virtqueue_pop(vq, sizeof(VirtQueueElement)); > + if (!elem) { > + break; > + } > + if (elem->in_num < 1 || > + iov_size(elem->in_sg, elem->in_num) < sizeof(ctrl)) { > + error_report("virtio-crypto ctrl missing headers"); > + exit(1); > + } Please use virtio_error() instead. virtio devices should not call exit(1). There are other instances of this throughout the code, please fix all of them. > + > + iov_cnt =3D elem->in_num; > + iov =3D elem->in_sg; > + s =3D iov_to_buf(iov, iov_cnt, 0, &ctrl, sizeof(ctrl)); > + assert(s =3D=3D sizeof(ctrl)); This assert is always true because you checked iov_size() above. Please move that check down here and drop the assert. --/i8j2F0k9BYX4qLc Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQEcBAEBAgAGBQJX83/KAAoJEJykq7OBq3PIW0gH/10xndFcYsMZTQBH6dSQw4Q4 krZtdRYaxF3J8EnkLk+UMOGG0cZWxP6djgNgQNIMQeXkckbioTBjuWqSfxR5e7y0 1r+hz4aCrHH3oIMyCf3XBqExvC9v0Ai5GWpV/2rcK+DSYSS7TqgXApxgvlPteb/z PTnU4aHXROiOO491e2Wok1h9QZoqOm1G2voRLOz2a5tVh0Sls0oXUlFNpVd9JQTD FQgr5hhVGkhfWT7zuAfUtiJmA70cO3lDIwnohVZ+5RRnQ+jQeDPh292Zs5nHuyNS SqZfwcivLiHKfswgGEkbLMp8mGbZXx4XfEEA1z8bRxsk67PGqkAmmJxJq+e5cnI= =WV6g -----END PGP SIGNATURE----- --/i8j2F0k9BYX4qLc--