Message ID | dcfa2ec0-d745-16e5-6235-a99b83cd22ed@ozlabs.ru |
---|---|
State | Not Applicable |
Headers | show |
On 02.02.2017 01:00, Alexey Kardashevskiy wrote: > On 01/02/17 19:08, Thomas Huth wrote: >> On 01.02.2017 03:14, Alexey Kardashevskiy wrote: >>> On 18/01/17 00:16, Thomas Huth wrote: >>>> No need for a global variable to store the virtqueue information here, >>>> thus let's remove the global vq variable and make it local to the >>>> init function instead. >>>> >>>> Signed-off-by: Thomas Huth <thuth@redhat.com> >>>> --- >>>> lib/libvirtio/virtio-9p.c | 9 +++------ >>>> lib/libvirtio/virtio-blk.c | 9 +++------ >>>> 2 files changed, 6 insertions(+), 12 deletions(-) >>>> >>>> diff --git a/lib/libvirtio/virtio-9p.c b/lib/libvirtio/virtio-9p.c >>>> index 6e9c379..fb329b3 100644 >>>> --- a/lib/libvirtio/virtio-9p.c >>>> +++ b/lib/libvirtio/virtio-9p.c >>>> @@ -20,8 +20,6 @@ >>>> #include "p9.h" >>>> #include "virtio-internal.h" >>>> >>>> -static struct vqs vq; >>>> - >>>> /** >>>> * Notes for 9P Server config: >>>> * >>>> @@ -163,7 +161,7 @@ static int virtio_9p_transact(void *opaque, uint8_t *tx, int tx_size, uint8_t *r >>>> int virtio_9p_init(struct virtio_device *dev, void *tx_buf, void *rx_buf, >>>> int buf_size) >>>> { >>>> - struct vring_avail *vq_avail; >>>> + struct vqs vq; >>>> int status = VIRTIO_STAT_ACKNOWLEDGE; >>>> >>>> /* Check for double open */ >>>> @@ -195,9 +193,8 @@ int virtio_9p_init(struct virtio_device *dev, void *tx_buf, void *rx_buf, >>>> if (virtio_queue_init_vq(dev, &vq, 0)) >>>> goto dev_error; >>>> >>>> - vq_avail = virtio_get_vring_avail(dev, 0); >>>> - vq_avail->flags = virtio_cpu_to_modern16(dev, VRING_AVAIL_F_NO_INTERRUPT); >>>> - vq_avail->idx = 0; >>>> + vq.avail->flags = virtio_cpu_to_modern16(dev, VRING_AVAIL_F_NO_INTERRUPT); >>>> + vq.avail->idx = 0; >>> >>> >>> Here is some black magic happening which I cannot understand. @vq is a >>> local struct, it is not used anywhere except virtio_queue_init_vq() but >>> even there the only used bit is vq->desc which is allocated in >>> virtio_queue_init_vq() and then written to @dev via virtio_set_qaddr(). >>> >>> I'd think that we do not need @vq parameter in virtio_queue_init_vq() and >>> we can drop these local @vq from "virtio_.*_init", what do I miss? >> >> You missed that the "virtqueue available ring" is used again in >> virtio_9p_transact(). The code gets a pointer to the ring there with >> virtio_get_vring_avail(). So no, this "black magic" can not be dropped, >> it is a required setup step. > > I was not talking about the ring itself, I was talking about the vq thingy > which is a descriptor of the ring. [...] > The code gets individual members of vq but not a pointer to vq itself (the > thing which the patch makes local instead of removing it). [...] > I am still missing the point, right? :) Right. It's a device, and the VRING_AVAIL_F_NO_INTERRUPT flag is read from the *device* side (thus in this case QEMU). You can not simply remove this, as far as I can tell. Thomas
=== static int virtio_9p_transact(void *opaque, uint8_t *tx, int tx_size, uint8_t *rx, uint32_t *rx_size) { struct virtio_device *dev = opaque; struct vring_desc *desc; int id, i; uint32_t vq_size; struct vring_desc *vq_desc; struct vring_avail *vq_avail; struct vring_used *vq_used; volatile uint16_t *current_used_idx; uint16_t last_used_idx, avail_idx; /* Virt IO queues. */ . vq_size = virtio_get_qsize(dev, 0); vq_desc = virtio_get_vring_desc(dev, 0); vq_avail = virtio_get_vring_avail(dev, 0); vq_used = virtio_get_vring_used(dev, 0) === The code gets individual members of vq but not a pointer to vq itself (the thing which the patch makes local instead of removing it). Basically, virtio_queue_init_vq() can be reduced to this: diff --git a/lib/libvirtio/virtio-9p.c b/lib/libvirtio/virtio-9p.c index fb329b3..c4140f3 100644 --- a/lib/libvirtio/virtio-9p.c +++ b/lib/libvirtio/virtio-9p.c @@ -161,7 +161,6 @@ static int virtio_9p_transact(void *opaque, uint8_t *tx, int tx_size, uint8_t *r int virtio_9p_init(struct virtio_device *dev, void *tx_buf, void *rx_buf, int buf_size) { - struct vqs vq; int status = VIRTIO_STAT_ACKNOWLEDGE; /* Check for double open */ @@ -190,12 +189,9 @@ int virtio_9p_init(struct virtio_device *dev, void *tx_buf, void *rx_buf, virtio_set_guest_features(dev, 0); } - if (virtio_queue_init_vq(dev, &vq, 0)) + if (virtio_queue_init_vq(dev, 0)) goto dev_error; - vq.avail->flags = virtio_cpu_to_modern16(dev, VRING_AVAIL_F_NO_INTERRUPT); - vq.avail->idx = 0; - /* Tell HV that setup succeeded */ status |= VIRTIO_STAT_DRIVER_OK; virtio_set_status(dev, status); diff --git a/lib/libvirtio/virtio.c b/lib/libvirtio/virtio.c index f189941..aefe150 100644 --- a/lib/libvirtio/virtio.c +++ b/lib/libvirtio/virtio.c @@ -385,19 +385,18 @@ void virtio_set_qaddr(struct virtio_device *dev, int queue, unsigned long qaddr) } } -int virtio_queue_init_vq(struct virtio_device *dev, struct vqs *vq, unsigned int id) +int virtio_queue_init_vq(struct virtio_device *dev, unsigned int id) { - vq->size = virtio_get_qsize(dev, id); - vq->desc = SLOF_alloc_mem_aligned(virtio_vring_size(vq->size), 4096); - if (!vq->desc) { + size_t size; + void *desc; + size = virtio_get_qsize(dev, id); + desc = SLOF_alloc_mem_aligned(virtio_vring_size(vq->size), 4096); + if (!desc) { printf("memory allocation failed!\n"); return -1; } - memset(vq->desc, 0, virtio_vring_size(vq->size)); - virtio_set_qaddr(dev, id, (unsigned long)vq->desc); - vq->avail = virtio_get_vring_avail(dev, id); - vq->used = virtio_get_vring_used(dev, id); - vq->id = id; + memset(desc, 0, virtio_vring_size(vq->size)); + virtio_set_qaddr(dev, id, (unsigned long)desc); return 0;