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

[Stefan Hajnoczi <stefanha@redhat.com>]

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

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

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

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

Attachment: signature.asc
Description: PGP signature