[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v4 08/13] virtio-crypto: add control queue handl
From: |
Gonglei (Arei) |
Subject: |
Re: [Qemu-devel] [PATCH v4 08/13] virtio-crypto: add control queue handler |
Date: |
Wed, 5 Oct 2016 03:38:20 +0000 |
> -----Original Message-----
> From: Stefan Hajnoczi [mailto:address@hidden
> 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