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

[Auger Eric]

Hi Peter,

First of all, please forgive me for the delay.
On 8/15/19 3:54 PM, Peter Xu wrote:
> 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?
Indeed I don't think it is needed and besides iov is not freed!

I got inspired from hw/net/virtio-net.c. To be honest I don't get why
the g_memdup is needed there either. The out_sg gets duplicated and
commands work on the duplicated data and not in place.
> 
>> +        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.
yes. I initialized 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)?
That's unclear to me. Similarly when checking against virtio-net.c, the
element is pushed back to the used ring and len is set to the size of
the status with:

/*
 * Control virtqueue data structures
 *
 * The control virtqueue expects a header in the first sg entry
 * and an ack/status response in the last entry.  Data for the
 * command goes in between.
 */
> 
>> +        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? :)
The state is made of red black trees, lists. For the former there is no
VMSTATE* ready. I am working on it but I think this should be handled
separately
> 
>> +};
>> +
>> +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.
virtio_init does:
    if (vdev->config_len) {
        vdev->config = g_malloc0(config_size);

but I should be homogeneous and then remove s->config.domain_range.start
= 0;
> 
>> +    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,
>