[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-arm] [PATCH for-4.2 v10 03/15] virtio-iommu: Add skeleton
From: |
Auger Eric |
Subject: |
Re: [Qemu-arm] [PATCH for-4.2 v10 03/15] virtio-iommu: Add skeleton |
Date: |
Thu, 29 Aug 2019 14:18:42 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.4.0 |
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,
>