[PATCHv3 0/2] Fixes for kvmtool virtio-net part

[PATCHv3 0/2] Fixes for kvmtool virtio-net part

[Fan Du]

Changelog:
v2:
  - Rebase with Will kvmtool.git tree[1]
  - Drop patch2 from v1, as commit f83dc816a9c7 has already fix it
v3:
  - Remove -Wunused-variable warnings

Fan Du (2):
  kvmtool: Introduce downscript option for virtio-net
  kvmtool: Restrict virtio queue number to 1 when vhost on

 include/kvm/virtio-net.h |  1 +
 virtio/net.c             | 53 +++++++++++++++++++++++++++++++++++++-----------
 2 files changed, 42 insertions(+), 12 deletions(-)

-- 
1.8.3.1

[PATCHv3 1/2] kvmtool: Introduce downscript option for virtio-net

[Fan Du]

To detach tap device automatically from bridge when exiting,
just like what the reverse of "script" does.

Signed-off-by: Fan Du <fan.du@intel.com>
---
 include/kvm/virtio-net.h |  1 +
 virtio/net.c             | 49 ++++++++++++++++++++++++++++++++++++------------
 2 files changed, 38 insertions(+), 12 deletions(-)

diff --git a/include/kvm/virtio-net.h b/include/kvm/virtio-net.h
index f435cc3..d136a09 100644
--- a/include/kvm/virtio-net.h
+++ b/include/kvm/virtio-net.h
@@ -9,6 +9,7 @@ struct virtio_net_params {
 	const char *guest_ip;
 	const char *host_ip;
 	const char *script;
+	const char *downscript;
 	const char *trans;
 	const char *tapif;
 	char guest_mac[6];
diff --git a/virtio/net.c b/virtio/net.c
index 4a6a855..d343615 100644
--- a/virtio/net.c
+++ b/virtio/net.c
@@ -294,10 +294,29 @@ static int virtio_net_request_tap(struct net_dev *ndev, struct ifreq *ifr,
 	return ret;
 }
 
+static int virtio_net_exec_script(const char* script, char *tap_name)
+{
+	int pid;
+	int status;
+
+	pid = fork();
+	if (pid == 0) {
+		execl(script, script, tap_name, NULL);
+		_exit(1);
+	} else {
+		waitpid(pid, &status, 0);
+		if (WIFEXITED(status) && WEXITSTATUS(status) != 0) {
+			pr_warning("Fail to setup tap by %s", script);
+			return -1;
+		}
+	}
+	return 0;
+}
+
 static bool virtio_net__tap_init(struct net_dev *ndev)
 {
 	int sock = socket(AF_INET, SOCK_STREAM, 0);
-	int pid, status, offload, hdr_len;
+	int offload, hdr_len;
 	struct sockaddr_in sin = {0};
 	struct ifreq ifr;
 	const struct virtio_net_params *params = ndev->params;
@@ -339,17 +358,8 @@ static bool virtio_net__tap_init(struct net_dev *ndev)
 	}
 
 	if (strcmp(params->script, "none")) {
-		pid = fork();
-		if (pid == 0) {
-			execl(params->script, params->script, ndev->tap_name, NULL);
-			_exit(1);
-		} else {
-			waitpid(pid, &status, 0);
-			if (WIFEXITED(status) && WEXITSTATUS(status) != 0) {
-				pr_warning("Fail to setup tap by %s", params->script);
-				goto fail;
-			}
-		}
+		if(virtio_net_exec_script(params->script, ndev->tap_name) < 0)
+			goto fail;
 	} else if (!skipconf) {
 		memset(&ifr, 0, sizeof(ifr));
 		strncpy(ifr.ifr_name, ndev->tap_name, sizeof(ndev->tap_name));
@@ -702,6 +712,8 @@ static int set_net_param(struct kvm *kvm, struct virtio_net_params *p,
 			die("Unknown network mode %s, please use user, tap or none", kvm->cfg.network);
 	} else if (strcmp(param, "script") == 0) {
 		p->script = strdup(val);
+	} else if (strcmp(param, "downscript") == 0) {
+		p->downscript = strdup(val);
 	} else if (strcmp(param, "guest_ip") == 0) {
 		p->guest_ip = strdup(val);
 	} else if (strcmp(param, "host_ip") == 0) {
@@ -877,6 +889,19 @@ virtio_dev_init(virtio_net__init);
 
 int virtio_net__exit(struct kvm *kvm)
 {
+	struct virtio_net_params *params;
+	struct net_dev *ndev;
+	struct list_head *ptr;
+
+	list_for_each(ptr, &ndevs) {
+		ndev = list_entry(ptr, struct net_dev, list);
+		params = ndev->params;
+		/* Cleanup any tap device which attached to bridge */
+		if (ndev->mode == NET_MODE_TAP &&
+		    strcmp(params->downscript, "none")) {
+			virtio_net_exec_script(params->downscript, ndev->tap_name);
+		}
+	}
 	return 0;
 }
 virtio_dev_exit(virtio_net__exit);
-- 
1.8.3.1

Re: [PATCHv3 1/2] kvmtool: Introduce downscript option for virtio-net

[Andre Przywara]

Hi,

thanks for the rework, that looks good to me, some minor nits below.
Not sure if that requires a new version, maybe Will can fix that up when
he applies it.

On 21/07/15 07:18, Fan Du wrote:
> To detach tap device automatically from bridge when exiting,
> just like what the reverse of "script" does.
> 
> Signed-off-by: Fan Du <fan.du@intel.com>
> ---
>  include/kvm/virtio-net.h |  1 +
>  virtio/net.c             | 49 ++++++++++++++++++++++++++++++++++++------------
>  2 files changed, 38 insertions(+), 12 deletions(-)
> 
> diff --git a/include/kvm/virtio-net.h b/include/kvm/virtio-net.h
> index f435cc3..d136a09 100644
> --- a/include/kvm/virtio-net.h
> +++ b/include/kvm/virtio-net.h
> @@ -9,6 +9,7 @@ struct virtio_net_params {
>  	const char *guest_ip;
>  	const char *host_ip;
>  	const char *script;
> +	const char *downscript;
>  	const char *trans;
>  	const char *tapif;
>  	char guest_mac[6];
> diff --git a/virtio/net.c b/virtio/net.c
> index 4a6a855..d343615 100644
> --- a/virtio/net.c
> +++ b/virtio/net.c
> @@ -294,10 +294,29 @@ static int virtio_net_request_tap(struct net_dev *ndev, struct ifreq *ifr,
>  	return ret;
>  }
>  
> +static int virtio_net_exec_script(const char* script, char *tap_name)

can you make tap_name a "const char *" as well?

> +{
> +	int pid;

fork() returns a value of type "pid_t", so pid should be of that type
too. I know it was int before, but let's fix it when we rewrite it anyway.

> +	int status;
> +
> +	pid = fork();
> +	if (pid == 0) {
> +		execl(script, script, tap_name, NULL);
> +		_exit(1);
> +	} else {
> +		waitpid(pid, &status, 0);
> +		if (WIFEXITED(status) && WEXITSTATUS(status) != 0) {
> +			pr_warning("Fail to setup tap by %s", script);
> +			return -1;
> +		}
> +	}
> +	return 0;
> +}
> +
>  static bool virtio_net__tap_init(struct net_dev *ndev)
>  {
>  	int sock = socket(AF_INET, SOCK_STREAM, 0);
> -	int pid, status, offload, hdr_len;
> +	int offload, hdr_len;
>  	struct sockaddr_in sin = {0};
>  	struct ifreq ifr;
>  	const struct virtio_net_params *params = ndev->params;
> @@ -339,17 +358,8 @@ static bool virtio_net__tap_init(struct net_dev *ndev)
>  	}
>  
>  	if (strcmp(params->script, "none")) {
> -		pid = fork();
> -		if (pid == 0) {
> -			execl(params->script, params->script, ndev->tap_name, NULL);
> -			_exit(1);
> -		} else {
> -			waitpid(pid, &status, 0);
> -			if (WIFEXITED(status) && WEXITSTATUS(status) != 0) {
> -				pr_warning("Fail to setup tap by %s", params->script);
> -				goto fail;
> -			}
> -		}
> +		if(virtio_net_exec_script(params->script, ndev->tap_name) < 0)
> +			goto fail;
>  	} else if (!skipconf) {
>  		memset(&ifr, 0, sizeof(ifr));
>  		strncpy(ifr.ifr_name, ndev->tap_name, sizeof(ndev->tap_name));
> @@ -702,6 +712,8 @@ static int set_net_param(struct kvm *kvm, struct virtio_net_params *p,
>  			die("Unknown network mode %s, please use user, tap or none", kvm->cfg.network);
>  	} else if (strcmp(param, "script") == 0) {
>  		p->script = strdup(val);
> +	} else if (strcmp(param, "downscript") == 0) {
> +		p->downscript = strdup(val);
>  	} else if (strcmp(param, "guest_ip") == 0) {
>  		p->guest_ip = strdup(val);
>  	} else if (strcmp(param, "host_ip") == 0) {
> @@ -877,6 +889,19 @@ virtio_dev_init(virtio_net__init);
>  
>  int virtio_net__exit(struct kvm *kvm)
>  {
> +	struct virtio_net_params *params;
> +	struct net_dev *ndev;
> +	struct list_head *ptr;
> +
> +	list_for_each(ptr, &ndevs) {
> +		ndev = list_entry(ptr, struct net_dev, list);
> +		params = ndev->params;
> +		/* Cleanup any tap device which attached to bridge */
> +		if (ndev->mode == NET_MODE_TAP &&
> +		    strcmp(params->downscript, "none")) {
> +			virtio_net_exec_script(params->downscript, ndev->tap_name);
> +		}

You don't need the braces here since the condition is only a one-liner.

Cheers,
Andre.

> +	}
>  	return 0;
>  }
>  virtio_dev_exit(virtio_net__exit);
> 

Re: [PATCHv3 1/2] kvmtool: Introduce downscript option for virtio-net

[Will Deacon]

On Tue, Jul 21, 2015 at 10:44:31AM +0100, Andre Przywara wrote:
> thanks for the rework, that looks good to me, some minor nits below.
> Not sure if that requires a new version, maybe Will can fix that up when
> he applies it.

Nah, please send a new version with these points addressed.

Will

[PATCHv4 kvmtool] kvmtool: Introduce downscript option for virtio-net

[Fan Du]

To detach tap device automatically from bridge when exiting,
just like what the reverse of "script" does.

Signed-off-by: Fan Du <fan.du@intel.com>
---
 include/kvm/virtio-net.h |  1 +
 virtio/net.c             | 49 ++++++++++++++++++++++++++++++++++++------------
 2 files changed, 38 insertions(+), 12 deletions(-)

diff --git a/include/kvm/virtio-net.h b/include/kvm/virtio-net.h
index f435cc3..d136a09 100644
--- a/include/kvm/virtio-net.h
+++ b/include/kvm/virtio-net.h
@@ -9,6 +9,7 @@ struct virtio_net_params {
 	const char *guest_ip;
 	const char *host_ip;
 	const char *script;
+	const char *downscript;
 	const char *trans;
 	const char *tapif;
 	char guest_mac[6];
diff --git a/virtio/net.c b/virtio/net.c
index 4a6a855..a6e58db 100644
--- a/virtio/net.c
+++ b/virtio/net.c
@@ -294,10 +294,29 @@ static int virtio_net_request_tap(struct net_dev *ndev, struct ifreq *ifr,
 	return ret;
 }
 
+static int virtio_net_exec_script(const char* script, const char *tap_name)
+{
+	pid_t pid;
+	int status;
+
+	pid = fork();
+	if (pid == 0) {
+		execl(script, script, tap_name, NULL);
+		_exit(1);
+	} else {
+		waitpid(pid, &status, 0);
+		if (WIFEXITED(status) && WEXITSTATUS(status) != 0) {
+			pr_warning("Fail to setup tap by %s", script);
+			return -1;
+		}
+	}
+	return 0;
+}
+
 static bool virtio_net__tap_init(struct net_dev *ndev)
 {
 	int sock = socket(AF_INET, SOCK_STREAM, 0);
-	int pid, status, offload, hdr_len;
+	int offload, hdr_len;
 	struct sockaddr_in sin = {0};
 	struct ifreq ifr;
 	const struct virtio_net_params *params = ndev->params;
@@ -339,17 +358,8 @@ static bool virtio_net__tap_init(struct net_dev *ndev)
 	}
 
 	if (strcmp(params->script, "none")) {
-		pid = fork();
-		if (pid == 0) {
-			execl(params->script, params->script, ndev->tap_name, NULL);
-			_exit(1);
-		} else {
-			waitpid(pid, &status, 0);
-			if (WIFEXITED(status) && WEXITSTATUS(status) != 0) {
-				pr_warning("Fail to setup tap by %s", params->script);
-				goto fail;
-			}
-		}
+		if(virtio_net_exec_script(params->script, ndev->tap_name) < 0)
+			goto fail;
 	} else if (!skipconf) {
 		memset(&ifr, 0, sizeof(ifr));
 		strncpy(ifr.ifr_name, ndev->tap_name, sizeof(ndev->tap_name));
@@ -702,6 +712,8 @@ static int set_net_param(struct kvm *kvm, struct virtio_net_params *p,
 			die("Unknown network mode %s, please use user, tap or none", kvm->cfg.network);
 	} else if (strcmp(param, "script") == 0) {
 		p->script = strdup(val);
+	} else if (strcmp(param, "downscript") == 0) {
+		p->downscript = strdup(val);
 	} else if (strcmp(param, "guest_ip") == 0) {
 		p->guest_ip = strdup(val);
 	} else if (strcmp(param, "host_ip") == 0) {
@@ -740,6 +752,7 @@ int netdev_parser(const struct option *opt, const char *arg, int unset)
 		.guest_ip	= DEFAULT_GUEST_ADDR,
 		.host_ip	= DEFAULT_HOST_ADDR,
 		.script		= DEFAULT_SCRIPT,
+		.downscript	= DEFAULT_SCRIPT,
 		.mode		= NET_MODE_TAP,
 	};
 
@@ -877,6 +890,18 @@ virtio_dev_init(virtio_net__init);
 
 int virtio_net__exit(struct kvm *kvm)
 {
+	struct virtio_net_params *params;
+	struct net_dev *ndev;
+	struct list_head *ptr;
+
+	list_for_each(ptr, &ndevs) {
+		ndev = list_entry(ptr, struct net_dev, list);
+		params = ndev->params;
+		/* Cleanup any tap device which attached to bridge */
+		if (ndev->mode == NET_MODE_TAP &&
+		    strcmp(params->downscript, "none"))
+			virtio_net_exec_script(params->downscript, ndev->tap_name);
+	}
 	return 0;
 }
 virtio_dev_exit(virtio_net__exit);
-- 
1.8.3.1

[PATCHv3 2/2] kvmtool: Restrict virtio queue number to 1 when vhost on

[Fan Du]

vhost kernel driver does not support mutiple queue yet,
Tweak queue number will fail with "--net mode=tap,vhost=1,mq=2"
as below when lkvm trying to set ring kick fd for queue 2:

VHOST_SET_VRING_KICK failed: No buffer space available

Error on this scenario, and overide with the default one queue
configuration.

Signed-off-by: Fan Du <fan.du@intel.com>
---
 virtio/net.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/virtio/net.c b/virtio/net.c
index d343615..21a80f3 100644
--- a/virtio/net.c
+++ b/virtio/net.c
@@ -730,6 +730,10 @@ static int set_net_param(struct kvm *kvm, struct virtio_net_params *p,
 		p->mq = atoi(val);
 	} else
 		die("Unknown network parameter %s", param);
+	if (p->vhost && p->mq > 1) {
+		p->mq = 1;
+		pr_err("vhost does not support mq yet, overide mq to 1.");
+	}
 
 	return 0;
 }
-- 
1.8.3.1

Re: [PATCHv3 2/2] kvmtool: Restrict virtio queue number to 1 when vhost on

[Andre Przywara]

Hi,

On 21/07/15 07:18, Fan Du wrote:
> vhost kernel driver does not support mutiple queue yet,
> Tweak queue number will fail with "--net mode=tap,vhost=1,mq=2"
> as below when lkvm trying to set ring kick fd for queue 2:
> 
> VHOST_SET_VRING_KICK failed: No buffer space available
> 
> Error on this scenario, and overide with the default one queue
> configuration.

I don't like the idea of overriding an explicitly given command line
parameter (mq=2).
So why do you provide mq=2 in the first place if you know that the
kernel does not support it?
I'd rather see the error message to be more descriptive in that case.
That would help the user to understand what's going on, also it would
still work should the kernel ever support multiple queues in the future.

Cheers,
Andre.

> 
> Signed-off-by: Fan Du <fan.du@intel.com>
> ---
>  virtio/net.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/virtio/net.c b/virtio/net.c
> index d343615..21a80f3 100644
> --- a/virtio/net.c
> +++ b/virtio/net.c
> @@ -730,6 +730,10 @@ static int set_net_param(struct kvm *kvm, struct virtio_net_params *p,
>  		p->mq = atoi(val);
>  	} else
>  		die("Unknown network parameter %s", param);
> +	if (p->vhost && p->mq > 1) {
> +		p->mq = 1;
> +		pr_err("vhost does not support mq yet, overide mq to 1.");
> +	}
>  
>  	return 0;
>  }
> 

RE: [PATCHv3 2/2] kvmtool: Restrict virtio queue number to 1 when vhost on

[Du, Fan]

>-----Original Message-----
>From: Andre Przywara [mailto:andre.przywara@arm.com]
>Sent: Tuesday, July 21, 2015 5:45 PM
>To: Du, Fan; Will Deacon
>Cc: kvm@vger.kernel.org; Marc Zyngier
>Subject: Re: [PATCHv3 2/2] kvmtool: Restrict virtio queue number to 1 when
>vhost on
>
>Hi,
>
>On 21/07/15 07:18, Fan Du wrote:
>> vhost kernel driver does not support mutiple queue yet,
>> Tweak queue number will fail with "--net mode=tap,vhost=1,mq=2"
>> as below when lkvm trying to set ring kick fd for queue 2:
>>
>> VHOST_SET_VRING_KICK failed: No buffer space available
>>
>> Error on this scenario, and overide with the default one queue
>> configuration.
>
>I don't like the idea of overriding an explicitly given command line
>parameter (mq=2).
>So why do you provide mq=2 in the first place if you know that the
>kernel does not support it?

On the contrary, the help message doesn't explicitly state mq is not supported
when vhost=1, what's the insanity to supply with mq option?!

>I'd rather see the error message to be more descriptive in that case.
>That would help the user to understand what's going on, also it would
>still work should the kernel ever support multiple queues in the future.

I will check how it could be support, either in user land or kernel.
So let's drop this patch.

>Cheers,
>Andre.
>
>>
>> Signed-off-by: Fan Du <fan.du@intel.com>
>> ---
>>  virtio/net.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/virtio/net.c b/virtio/net.c
>> index d343615..21a80f3 100644
>> --- a/virtio/net.c
>> +++ b/virtio/net.c
>> @@ -730,6 +730,10 @@ static int set_net_param(struct kvm *kvm, struct
>virtio_net_params *p,
>>  		p->mq = atoi(val);
>>  	} else
>>  		die("Unknown network parameter %s", param);
>> +	if (p->vhost && p->mq > 1) {
>> +		p->mq = 1;
>> +		pr_err("vhost does not support mq yet, overide mq to 1.");
>> +	}
>>
>>  	return 0;
>>  }
>>