Re: [Qemu-devel] [PATCH v4 08/13] virtio-crypto: add control queue handler

["Gonglei (Arei)" <arei.gonglei@huawei.com>]

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