[SLOF] [PATCH] virtio: Remove global variables in block and 9p driver

[Alexey Kardashevskiy]

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 at 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.

===
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;
 }


I am still missing the point, right? :)


-- 
Alexey