[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: RE: [PATCH v4 08/13] virtio-crypto: add control queue handler
> -----Original Message----- > From: Stefan Hajnoczi [mailto:stefanha@redhat.com] > Sent: Tuesday, October 04, 2016 6:09 PM > Subject: Re: [PATCH v4 08/13] virtio-crypto: add control queue handler > > 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. > Yes, it makes very sense. Thanks. > 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 = cipher_para->algo; > > + info->key_len = cipher_para->keylen; > > + info->direction = cipher_para->op; > > Endianness? Use the virtio_ldl_p() family of functions to load values > from the guest. > > This same issue is present in the rest of the code. I won't mentioned > it again but please fix all occurrences. > Will fix them. > > + len = info->key_len; > > + /* get cipher key */ > > + if (len > 0) { > > + DPRINTF("keylen=%" PRIu32 "\n", info->key_len); > > + key_gpa = cipher_out->key_addr; > > + > > + key_hva = 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. > OK. > > +static void virtio_crypto_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq) > > +{ > > + VirtIOCrypto *vcrypto = 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 = 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. > I noticed your patch set introduced virtio_error(), and it seems that merged a few days ago. I'll fix them. > > + > > + iov_cnt = elem->in_num; > > + iov = elem->in_sg; > > + s = iov_to_buf(iov, iov_cnt, 0, &ctrl, sizeof(ctrl)); > > + assert(s == sizeof(ctrl)); > > This assert is always true because you checked iov_size() above. Please > move that check down here and drop the assert. OK. Regards, -Gonglei
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]