[SLOF] [PATCH v2 06/19] virtio-blk: add helpers for filling descriptors

[Nikunj A Dadhania]

Thomas Huth <thuth at redhat.com> writes:

> On 20.01.2016 13:10, Nikunj A Dadhania wrote:
>> Enable virtio_fill_desc/fill_blk_hdr with legacy and modern mode for
>> further use
>> 
>> Signed-off-by: Nikunj A Dadhania <nikunj at linux.vnet.ibm.com>
>> ---
>>  lib/libvirtio/virtio-blk.c | 36 +++++++++++++++++++++---------------
>>  lib/libvirtio/virtio.c     | 19 +++++++++++++++++++
>>  lib/libvirtio/virtio.h     |  4 ++++
>>  3 files changed, 44 insertions(+), 15 deletions(-)
>> 
>> diff --git a/lib/libvirtio/virtio-blk.c b/lib/libvirtio/virtio-blk.c
>> index 1966721..8ae61bc 100644
>> --- a/lib/libvirtio/virtio-blk.c
>> +++ b/lib/libvirtio/virtio-blk.c
>> @@ -13,6 +13,7 @@
>>  #include <stdio.h>
>>  #include <cpu.h>
>>  #include <helpers.h>
>> +#include <byteorder.h>
>>  #include "virtio.h"
>>  #include "virtio-blk.h"
>>  
>> @@ -82,6 +83,19 @@ virtioblk_shutdown(struct virtio_device *dev)
>>  	virtio_reset_device(dev);
>>  }
>>  
>> +static void fill_blk_hdr(struct virtio_blk_req *blkhdr, bool is_modern,
>> +                         uint32_t type, uint32_t ioprio, uint32_t sector)
>> +{
>> +	if (is_modern) {
>> +		blkhdr->type = cpu_to_le32(type);
>> +		blkhdr->ioprio = cpu_to_le32(ioprio);
>> +		blkhdr->sector = cpu_to_le64(sector);
>> +	} else {
>> +		blkhdr->type = type;
>> +		blkhdr->ioprio = ioprio;
>> +		blkhdr->sector = sector;
>> +	}
>> +}
>>  
>>  /**
>>   * Read blocks
>> @@ -137,33 +151,25 @@ virtioblk_read(struct virtio_device *dev, char *buf, long blocknum, long cnt)
>>  	current_used_idx = &vq_used->idx;
>>  
>>  	/* Set up header */
>> -	blkhdr.type = VIRTIO_BLK_T_IN | VIRTIO_BLK_T_BARRIER;
>> -	blkhdr.ioprio = 1;
>> -	blkhdr.sector = blocknum * blk_size / DEFAULT_SECTOR_SIZE;
>> +	fill_blk_hdr(&blkhdr, false, VIRTIO_BLK_T_IN | VIRTIO_BLK_T_BARRIER,
>> +		     1, blocknum * blk_size / DEFAULT_SECTOR_SIZE);
>
> Do you need fill_blk_hdr in a later patch again so that it gets called
> from another location, too? ... if not, I'd maybe fill in that code here
> directly instead of creating a separate function for this.

Only one location, just wanted to simplify and have the is_modern logic
in the function. Otherwise, it will bloat the blk_read.

>
>>  	/* Determine descriptor index */
>>  	id = (vq_avail->idx * 3) % vq_size;
>>  
>>  	/* Set up virtqueue descriptor for header */
>>  	desc = &vq_desc[id];
>> -	desc->addr = (uint64_t)&blkhdr;
>> -	desc->len = sizeof(struct virtio_blk_req);
>> -	desc->flags = VRING_DESC_F_NEXT;
>> -	desc->next = (id + 1) % vq_size;
>> +	virtio_fill_desc(desc, false, (uint64_t)&blkhdr, sizeof(struct virtio_blk_req),
>> +		  VRING_DESC_F_NEXT, (id + 1) % vq_size);
>>  
>>  	/* Set up virtqueue descriptor for data */
>>  	desc = &vq_desc[(id + 1) % vq_size];
>> -	desc->addr = (uint64_t)buf;
>> -	desc->len = cnt * blk_size;
>> -	desc->flags = VRING_DESC_F_NEXT | VRING_DESC_F_WRITE;
>> -	desc->next = (id + 2) % vq_size;
>> +	virtio_fill_desc(desc, false, (uint64_t)buf, cnt * blk_size,
>> +		  VRING_DESC_F_NEXT | VRING_DESC_F_WRITE, (id + 2) % vq_size);
>>  
>>  	/* Set up virtqueue descriptor for status */
>>  	desc = &vq_desc[(id + 2) % vq_size];
>> -	desc->addr = (uint64_t)&status;
>> -	desc->len = 1;
>> -	desc->flags = VRING_DESC_F_WRITE;
>> -	desc->next = 0;
>> +	virtio_fill_desc(desc, false, (uint64_t)&status, 1, VRING_DESC_F_WRITE, 0);
>>  
>>  	vq_avail->ring[vq_avail->idx % vq_size] = id;
>>  	mb();
>> diff --git a/lib/libvirtio/virtio.c b/lib/libvirtio/virtio.c
>> index b5be2e4..3a75e44 100644
>> --- a/lib/libvirtio/virtio.c
>> +++ b/lib/libvirtio/virtio.c
>> @@ -113,6 +113,25 @@ struct vring_used *virtio_get_vring_used(struct virtio_device *dev, int queue)
>>  			       * sizeof(struct vring_avail));
>>  }
>>  
>> +/**
>> + * Fill the virtio ring descriptor depending on the legacy mode or virtio 1.0
>> + */
>> +void virtio_fill_desc(struct vring_desc *desc, bool is_modern,
>> +                      uint64_t addr, uint32_t len,
>> +                      uint16_t flags, uint16_t next)
>> +{
>> +	if (is_modern) {
>> +		desc->addr = cpu_to_le64(addr);
>> +		desc->len = cpu_to_le32(len);
>> +		desc->flags = cpu_to_le16(flags);
>> +		desc->next = cpu_to_le16(next);
>> +	} else {
>> +		desc->addr = addr;
>> +		desc->len = len;
>> +		desc->flags = flags;
>> +		desc->next = next;
>> +	}
>> +}
>>  
>>  /**
>>   * Reset virtio device
>> diff --git a/lib/libvirtio/virtio.h b/lib/libvirtio/virtio.h
>> index e41f47d..c26282f 100644
>> --- a/lib/libvirtio/virtio.h
>> +++ b/lib/libvirtio/virtio.h
>> @@ -14,6 +14,7 @@
>>  #define _LIBVIRTIO_H
>>  
>>  #include <stdint.h>
>> +#include <stdbool.h>
>>  
>>  /* Device status bits */
>>  #define VIRTIO_STAT_ACKNOWLEDGE		1
>> @@ -83,6 +84,9 @@ extern int virtio_get_qsize(struct virtio_device *dev, int queue);
>>  extern struct vring_desc *virtio_get_vring_desc(struct virtio_device *dev, int queue);
>>  extern struct vring_avail *virtio_get_vring_avail(struct virtio_device *dev, int queue);
>>  extern struct vring_used *virtio_get_vring_used(struct virtio_device *dev, int queue);
>> +extern void virtio_fill_desc(struct vring_desc *desc, bool is_modern,
>> +                             uint64_t addr, uint32_t len,
>> +                             uint16_t flags, uint16_t next);
>>  extern int virtio_queue_init_vq(struct virtio_device *dev, struct vqs *vq, unsigned int id);
>>  
>>  extern void virtio_reset_device(struct virtio_device *dev);
>
> Anyway, patch also looks ok already as it is, so:
>
> Reviewed-by: Thomas Huth <thuth at redhat.com>