diff mbox

virtio: Remove global variables in block and 9p driver

Message ID 1484659001-13844-1-git-send-email-thuth@redhat.com
State Accepted
Headers show

Commit Message

Thomas Huth Jan. 17, 2017, 1:16 p.m. UTC 
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(-)

Comments

Laurent Vivier Jan. 18, 2017, 7:57 a.m. UTC | #1 
On 17/01/2017 14: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>

Reviewed-by: Laurent Vivier <lvivier@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;
>  
>  	/* Tell HV that setup succeeded */
>  	status |= VIRTIO_STAT_DRIVER_OK;
> diff --git a/lib/libvirtio/virtio-blk.c b/lib/libvirtio/virtio-blk.c
> index cdfdd10..9eea99d 100644
> --- a/lib/libvirtio/virtio-blk.c
> +++ b/lib/libvirtio/virtio-blk.c
> @@ -21,8 +21,6 @@
>  #define DEFAULT_SECTOR_SIZE 512
>  #define DRIVER_FEATURE_SUPPORT  (VIRTIO_BLK_F_BLK_SIZE | VIRTIO_F_VERSION_1)
>  
> -static struct vqs vq;
> -
>  /**
>   * Initialize virtio-block device.
>   * @param  dev  pointer to virtio device information
> @@ -30,7 +28,7 @@ static struct vqs vq;
>  int
>  virtioblk_init(struct virtio_device *dev)
>  {
> -	struct vring_avail *vq_avail;
> +	struct vqs vq;
>  	int blk_size = DEFAULT_SECTOR_SIZE;
>  	uint64_t features;
>  	int status = VIRTIO_STAT_ACKNOWLEDGE;
> @@ -59,9 +57,8 @@ virtioblk_init(struct virtio_device *dev)
>  	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;
>  
>  	/* Tell HV that setup succeeded */
>  	status |= VIRTIO_STAT_DRIVER_OK;
>
Alexey Kardashevskiy Feb. 1, 2017, 2:14 a.m. UTC | #2 
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?



>  
>  	/* Tell HV that setup succeeded */
>  	status |= VIRTIO_STAT_DRIVER_OK;
> diff --git a/lib/libvirtio/virtio-blk.c b/lib/libvirtio/virtio-blk.c
> index cdfdd10..9eea99d 100644
> --- a/lib/libvirtio/virtio-blk.c
> +++ b/lib/libvirtio/virtio-blk.c
> @@ -21,8 +21,6 @@
>  #define DEFAULT_SECTOR_SIZE 512
>  #define DRIVER_FEATURE_SUPPORT  (VIRTIO_BLK_F_BLK_SIZE | VIRTIO_F_VERSION_1)
>  
> -static struct vqs vq;
> -
>  /**
>   * Initialize virtio-block device.
>   * @param  dev  pointer to virtio device information
> @@ -30,7 +28,7 @@ static struct vqs vq;
>  int
>  virtioblk_init(struct virtio_device *dev)
>  {
> -	struct vring_avail *vq_avail;
> +	struct vqs vq;
>  	int blk_size = DEFAULT_SECTOR_SIZE;
>  	uint64_t features;
>  	int status = VIRTIO_STAT_ACKNOWLEDGE;
> @@ -59,9 +57,8 @@ virtioblk_init(struct virtio_device *dev)
>  	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;
>  
>  	/* Tell HV that setup succeeded */
>  	status |= VIRTIO_STAT_DRIVER_OK;
>
Thomas Huth Feb. 1, 2017, 8:08 a.m. UTC | #3 
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.

 Thomas
Alexey Kardashevskiy Feb. 3, 2017, 3:17 a.m. UTC | #4 
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>

Thanks, applied.

> ---
>  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;
>  
>  	/* Tell HV that setup succeeded */
>  	status |= VIRTIO_STAT_DRIVER_OK;
> diff --git a/lib/libvirtio/virtio-blk.c b/lib/libvirtio/virtio-blk.c
> index cdfdd10..9eea99d 100644
> --- a/lib/libvirtio/virtio-blk.c
> +++ b/lib/libvirtio/virtio-blk.c
> @@ -21,8 +21,6 @@
>  #define DEFAULT_SECTOR_SIZE 512
>  #define DRIVER_FEATURE_SUPPORT  (VIRTIO_BLK_F_BLK_SIZE | VIRTIO_F_VERSION_1)
>  
> -static struct vqs vq;
> -
>  /**
>   * Initialize virtio-block device.
>   * @param  dev  pointer to virtio device information
> @@ -30,7 +28,7 @@ static struct vqs vq;
>  int
>  virtioblk_init(struct virtio_device *dev)
>  {
> -	struct vring_avail *vq_avail;
> +	struct vqs vq;
>  	int blk_size = DEFAULT_SECTOR_SIZE;
>  	uint64_t features;
>  	int status = VIRTIO_STAT_ACKNOWLEDGE;
> @@ -59,9 +57,8 @@ virtioblk_init(struct virtio_device *dev)
>  	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;
>  
>  	/* Tell HV that setup succeeded */
>  	status |= VIRTIO_STAT_DRIVER_OK;
>
diff mbox

Patch

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;
 
 	/* Tell HV that setup succeeded */
 	status |= VIRTIO_STAT_DRIVER_OK;
diff --git a/lib/libvirtio/virtio-blk.c b/lib/libvirtio/virtio-blk.c
index cdfdd10..9eea99d 100644
--- a/lib/libvirtio/virtio-blk.c
+++ b/lib/libvirtio/virtio-blk.c
@@ -21,8 +21,6 @@ 
 #define DEFAULT_SECTOR_SIZE 512
 #define DRIVER_FEATURE_SUPPORT  (VIRTIO_BLK_F_BLK_SIZE | VIRTIO_F_VERSION_1)
 
-static struct vqs vq;
-
 /**
  * Initialize virtio-block device.
  * @param  dev  pointer to virtio device information
@@ -30,7 +28,7 @@  static struct vqs vq;
 int
 virtioblk_init(struct virtio_device *dev)
 {
-	struct vring_avail *vq_avail;
+	struct vqs vq;
 	int blk_size = DEFAULT_SECTOR_SIZE;
 	uint64_t features;
 	int status = VIRTIO_STAT_ACKNOWLEDGE;
@@ -59,9 +57,8 @@  virtioblk_init(struct virtio_device *dev)
 	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;
 
 	/* Tell HV that setup succeeded */
 	status |= VIRTIO_STAT_DRIVER_OK;