Re: [Qemu-devel] [PATCH for-4.2 v10 03/15] virtio-iommu: Add skeleton

From: Peter Xu <peterx@redhat.com>
To: Eric Auger <eric.auger@redhat.com>
Cc: peter.maydell@linaro.org, kevin.tian@intel.com, mst@redhat.com,
	tn@semihalf.com, qemu-devel@nongnu.org,
	alex.williamson@redhat.com, qemu-arm@nongnu.org,
	jean-philippe@linaro.org, bharat.bhushan@nxp.com,
	eric.auger.pro@gmail.com
Subject: Re: [Qemu-devel] [PATCH for-4.2 v10 03/15] virtio-iommu: Add skeleton
Date: Thu, 15 Aug 2019 21:54:00 +0800	[thread overview]
Message-ID: <20190815135400.GC8463@xz-x1> (raw)
In-Reply-To: <20190730172137.23114-4-eric.auger@redhat.com>

On Tue, Jul 30, 2019 at 07:21:25PM +0200, Eric Auger wrote:
> +static void virtio_iommu_handle_command(VirtIODevice *vdev, VirtQueue *vq)
> +{
> +    VirtIOIOMMU *s = VIRTIO_IOMMU(vdev);
> +    struct virtio_iommu_req_head head;
> +    struct virtio_iommu_req_tail tail;

[1]

> +    VirtQueueElement *elem;
> +    unsigned int iov_cnt;
> +    struct iovec *iov;
> +    size_t sz;
> +
> +    for (;;) {
> +        elem = virtqueue_pop(vq, sizeof(VirtQueueElement));
> +        if (!elem) {
> +            return;
> +        }
> +
> +        if (iov_size(elem->in_sg, elem->in_num) < sizeof(tail) ||
> +            iov_size(elem->out_sg, elem->out_num) < sizeof(head)) {
> +            virtio_error(vdev, "virtio-iommu bad head/tail size");
> +            virtqueue_detach_element(vq, elem, 0);
> +            g_free(elem);
> +            break;
> +        }
> +
> +        iov_cnt = elem->out_num;
> +        iov = g_memdup(elem->out_sg, sizeof(struct iovec) * elem->out_num);

Could I ask why memdup is needed here?

> +        sz = iov_to_buf(iov, iov_cnt, 0, &head, sizeof(head));
> +        if (unlikely(sz != sizeof(head))) {
> +            tail.status = VIRTIO_IOMMU_S_DEVERR;

Do you need to zero the reserved bits to make sure it won't contain
garbage?  Same question to below uses of tail.

> +            goto out;
> +        }
> +        qemu_mutex_lock(&s->mutex);
> +        switch (head.type) {
> +        case VIRTIO_IOMMU_T_ATTACH:
> +            tail.status = virtio_iommu_handle_attach(s, iov, iov_cnt);
> +            break;
> +        case VIRTIO_IOMMU_T_DETACH:
> +            tail.status = virtio_iommu_handle_detach(s, iov, iov_cnt);
> +            break;
> +        case VIRTIO_IOMMU_T_MAP:
> +            tail.status = virtio_iommu_handle_map(s, iov, iov_cnt);
> +            break;
> +        case VIRTIO_IOMMU_T_UNMAP:
> +            tail.status = virtio_iommu_handle_unmap(s, iov, iov_cnt);
> +            break;
> +        default:
> +            tail.status = VIRTIO_IOMMU_S_UNSUPP;
> +        }
> +        qemu_mutex_unlock(&s->mutex);
> +
> +out:
> +        sz = iov_from_buf(elem->in_sg, elem->in_num, 0,
> +                          &tail, sizeof(tail));
> +        assert(sz == sizeof(tail));
> +
> +        virtqueue_push(vq, elem, sizeof(tail));

s/tail/head/ (though they are the same size)?

> +        virtio_notify(vdev, vq);
> +        g_free(elem);
> +    }
> +}

[...]

> +static void virtio_iommu_set_features(VirtIODevice *vdev, uint64_t val)
> +{
> +    VirtIOIOMMU *dev = VIRTIO_IOMMU(vdev);
> +
> +    dev->acked_features = val;
> +    trace_virtio_iommu_set_features(dev->acked_features);
> +}
> +
> +static const VMStateDescription vmstate_virtio_iommu_device = {
> +    .name = "virtio-iommu-device",
> +    .unmigratable = 1,

Curious, is there explicit reason to not support migration from the
first version? :)

> +};
> +
> +static void virtio_iommu_device_realize(DeviceState *dev, Error **errp)
> +{
> +    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> +    VirtIOIOMMU *s = VIRTIO_IOMMU(dev);
> +
> +    virtio_init(vdev, "virtio-iommu", VIRTIO_ID_IOMMU,
> +                sizeof(struct virtio_iommu_config));
> +
> +    s->req_vq = virtio_add_queue(vdev, VIOMMU_DEFAULT_QUEUE_SIZE,
> +                             virtio_iommu_handle_command);
> +    s->event_vq = virtio_add_queue(vdev, VIOMMU_DEFAULT_QUEUE_SIZE, NULL);
> +
> +    s->config.page_size_mask = TARGET_PAGE_MASK;
> +    s->config.input_range.end = -1UL;
> +    s->config.domain_range.start = 0;

Zero input_range.start = 0?  After all domain_range.start is zeroed.

> +    s->config.domain_range.end = 32;
> +
> +    virtio_add_feature(&s->features, VIRTIO_RING_F_EVENT_IDX);
> +    virtio_add_feature(&s->features, VIRTIO_RING_F_INDIRECT_DESC);
> +    virtio_add_feature(&s->features, VIRTIO_F_VERSION_1);
> +    virtio_add_feature(&s->features, VIRTIO_IOMMU_F_INPUT_RANGE);
> +    virtio_add_feature(&s->features, VIRTIO_IOMMU_F_DOMAIN_RANGE);
> +    virtio_add_feature(&s->features, VIRTIO_IOMMU_F_MAP_UNMAP);
> +    virtio_add_feature(&s->features, VIRTIO_IOMMU_F_BYPASS);
> +    virtio_add_feature(&s->features, VIRTIO_IOMMU_F_MMIO);
> +}

Regards,

-- 
Peter Xu