On Thu, Aug 29, 2019 at 02:18:42PM +0200, Auger Eric wrote: > 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.
Oh true, I found that it's because of calling of iov_discard_front(). Please have a look at 771b6ed37e3. Though it seems to me that virtio-iommu does not truncate iovs so it should not be needed. > > > >> + 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. > */ I was referencing the balloon code when reading the patch, e.g., virtio_balloon_handle_output(). Though after I read more carefully I see that other places are using it as you described. Now I tend to agree with you, because virtqueue_push() who calls virtqueue_unmap_sg() used the len to unmap in_sg[] rather than out_sg[]. So please ignore my previous comment. (then I'm not sure whether the usage in the balloon code was correct now...) > > > >> + 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 Fair enough. Would you mind to add a similar comment above unmigratable? Thanks, -- Peter Xu