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