Re: [PATCH] kvm tools: Support multiple net devices

[Asias He <asias.hejun@gmail.com>]

On 09/24/2011 05:17 PM, Sasha Levin wrote:
> This patch adds support for multiple network devices. The command line syntax
> changes to the following:
> 
> 	--network/-n [mode=[tap/user/none]] [guest_ip=[guest ip]] [host_ip=
> [host_ip]] [guest_mac=[guest_mac]] [script=[script]]
> 
> Each of the parameters is optional, and the config defaults to a TAP based
> networking with a random MAC.
> 
> Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
> ---
>  tools/kvm/builtin-run.c |  118 ++++++++++++++++++++++++++++++------
>  tools/kvm/virtio/net.c  |  150 +++++++++++++++++++++++++++--------------------
>  2 files changed, 185 insertions(+), 83 deletions(-)
> 
> diff --git a/tools/kvm/builtin-run.c b/tools/kvm/builtin-run.c
> index 28dc95a..6e5ae5f 100644
> --- a/tools/kvm/builtin-run.c
> +++ b/tools/kvm/builtin-run.c
> @@ -87,9 +87,12 @@ static bool sdl;
>  static bool balloon;
>  static bool using_rootfs;
>  static bool custom_rootfs;
> +static bool no_net;
>  extern bool ioport_debug;
>  extern int  active_console;
>  extern int  debug_iodelay;
> +struct virtio_net_parameters *net_params;
> +int num_net_devices;
>  
>  bool do_debug_print = false;
>  
> @@ -182,6 +185,89 @@ static int tty_parser(const struct option *opt, const char *arg, int unset)
>  	return 0;
>  }
>  
> +static int set_net_param(struct virtio_net_parameters *p, const char *param,
> +				const char *val)
> +{
> +	if (strcmp(param, "guest_mac") == 0) {
> +		sscanf(val, "%hhx:%hhx:%hhx:%hhx:%hhx:%hhx",
> +			p->guest_mac,
> +			p->guest_mac+1,
> +			p->guest_mac+2,
> +			p->guest_mac+3,
> +			p->guest_mac+4,
> +			p->guest_mac+5);

A lot of place are using this long sscanf code, a helper is better.

> +	} else if (strcmp(param, "mode") == 0) {
> +		if (!strncmp(val, "user", 4)) {
> +			int i;
> +
> +			for (i = 0; i < num_net_devices; i++)
> +				if (net_params[i].mode == NET_MODE_USER)
> +					die("Only one usermode network device allowed at a time");
> +			p->mode = NET_MODE_USER;
> +		} else if (!strncmp(val, "tap", 3)) {
> +			p->mode = NET_MODE_TAP;
> +		} else if (!strncmp(val, "none", 4)) {
> +			no_net = 1;
> +			return -1;
> +		} else
> +			die("Unkown network mode %s, please use user, tap or none", network);
> +	} else if (strcmp(param, "script") == 0) {
> +		p->script = val;
> +	} else if (strcmp(param, "guest_ip") == 0) {
> +		p->guest_ip = val;
> +	} else if (strcmp(param, "host_ip") == 0) {
> +		p->host_ip = val;
> +	}
> +
> +	return 0;
> +}
> +
> +static int netdev_parser(const struct option *opt, const char *arg, int unset)
> +{
> +	struct virtio_net_parameters p;
> +	char *buf, *cmd = NULL, *cur = NULL;
> +	bool on_cmd = true;
> +	int i;
> +
> +	if (arg) {
> +		buf = strdup(arg);
> +		if (buf == NULL)
> +			die("Failed allocating new net buffer");
> +		cur = strtok(buf, ",=");
> +	}
> +
> +	p.guest_ip = DEFAULT_GUEST_ADDR;
> +	p.host_ip = DEFAULT_HOST_ADDR;
> +	p.script = DEFAULT_SCRIPT;
> +	p.mode = NET_MODE_TAP;
> +	p.guest_mac[0] = 0x02;

This block deserve a vertical alignment.

> +	for (i = 1; i < 6; i++)
> +		p.guest_mac[i] = rand();


I don't think a random  guest mac address is the best choice.
This way, the guest is gonna have random eth* instance name.

I prefer:

 default_guest_mac + 0  for guest net device  0
 default_guest_mac + 1  for guest net device  1
 default_guest_mac + 2  for guest net device  2


> +	while (cur) {
> +		if (on_cmd) {
> +			cmd = cur;
> +		} else {
> +			if (set_net_param(&p, cmd, cur) < 0)
> +				goto done;
> +		}
> +		on_cmd = !on_cmd;
> +
> +		cur = strtok(NULL, ",=");
> +	};
> +
> +	num_net_devices++;
> +
> +	net_params = realloc(net_params, num_net_devices * sizeof(*net_params));
> +	if (net_params == NULL)
> +		die("Failed adding new network device");
> +
> +	net_params[num_net_devices - 1] = p;
> +
> +done:
> +	return 0;
> +}
> +
>  static int shmem_parser(const struct option *opt, const char *arg, int unset)
>  {
>  	const u64 default_size = SHMEM_DEFAULT_SIZE;
> @@ -339,18 +425,9 @@ static const struct option options[] = {
>  			"Kernel command line arguments"),
>  
>  	OPT_GROUP("Networking options:"),
> -	OPT_STRING('n', "network", &network, "user, tap, none",
> -			"Network to use"),
> -	OPT_STRING('\0', "host-ip", &host_ip, "a.b.c.d",
> -			"Assign this address to the host side networking"),
> -	OPT_STRING('\0', "guest-ip", &guest_ip, "a.b.c.d",
> -			"Assign this address to the guest side networking"),
> -	OPT_STRING('\0', "host-mac", &host_mac, "aa:bb:cc:dd:ee:ff",
> -			"Assign this address to the host side NIC"),
> -	OPT_STRING('\0', "guest-mac", &guest_mac, "aa:bb:cc:dd:ee:ff",
> -			"Assign this address to the guest side NIC"),
> -	OPT_STRING('\0', "tapscript", &script, "Script path",
> -			 "Assign a script to process created tap device"),
> +	OPT_CALLBACK_DEFAULT('n', "network", NULL, "network params",
> +		     "Create a new guest NIC",
> +		     netdev_parser, NULL),
>  
>  	OPT_GROUP("BIOS options:"),
>  	OPT_INTEGER('\0', "vidmode", &vidmode,
> @@ -615,7 +692,6 @@ void kvm_run_help(void)
>  
>  int kvm_cmd_run(int argc, const char **argv, const char *prefix)
>  {
> -	struct virtio_net_parameters net_params;
>  	static char real_cmdline[2048], default_name[20];
>  	struct framebuffer *fb = NULL;
>  	unsigned int nr_online_cpus;
> @@ -823,11 +899,19 @@ int kvm_cmd_run(int argc, const char **argv, const char *prefix)
>  
>  	virtio_9p__init(kvm);
>  
> -	if (strncmp(network, "none", 4)) {
> +	for (i = 0; i < num_net_devices; i++) {
> +		net_params[i].kvm = kvm;
> +		virtio_net__init(&net_params[i]);
> +	}
> +
> +	if (num_net_devices == 0 && no_net == 0) {
> +		struct virtio_net_parameters net_params;
> +
>  		net_params.guest_ip = guest_ip;
>  		net_params.host_ip = host_ip;
>  		net_params.kvm = kvm;
>  		net_params.script = script;
> +		net_params.mode = NET_MODE_USER;

vertical alignment, no?

>  		sscanf(guest_mac, "%hhx:%hhx:%hhx:%hhx:%hhx:%hhx",
>  			net_params.guest_mac,
>  			net_params.guest_mac+1,
> @@ -843,12 +927,6 @@ int kvm_cmd_run(int argc, const char **argv, const char *prefix)
>  			net_params.host_mac+4,
>  			net_params.host_mac+5);
>  
> -		if (!strncmp(network, "user", 4))
> -			net_params.mode = NET_MODE_USER;
> -		else if (!strncmp(network, "tap", 3))
> -			net_params.mode = NET_MODE_TAP;
> -		else
> -			die("Unkown network mode %s, please use -network user, tap, none", network);
>  		virtio_net__init(&net_params);
>  	}
>  
> diff --git a/tools/kvm/virtio/net.c b/tools/kvm/virtio/net.c
> index 4700483..26fed2b 100644
> --- a/tools/kvm/virtio/net.c
> +++ b/tools/kvm/virtio/net.c
> @@ -43,6 +43,7 @@ struct net_dev_operations {
>  struct net_dev {
>  	pthread_mutex_t			mutex;
>  	struct virtio_pci		vpci;
> +	struct list_head		list;
>  
>  	struct virt_queue		vqs[VIRTIO_NET_NUM_QUEUES];
>  	struct virtio_net_config	config;
> @@ -64,9 +65,12 @@ struct net_dev {
>  
>  	struct uip_info			info;
>  	struct net_dev_operations	*ops;
> +	struct kvm			*kvm;
>  };
>  
> -static struct net_dev ndev = {
> +static LIST_HEAD(ndevs);
> +
> +/*static struct net_dev ndev = {
>  	.mutex	= PTHREAD_MUTEX_INITIALIZER,
>  
>  	.config = {
> @@ -75,37 +79,38 @@ static struct net_dev ndev = {
>  	.info = {
>  		.buf_nr			= 20,
>  	}
> -};
> +};*/

Please remove this code which is commented out.

>  static void *virtio_net_rx_thread(void *p)
>  {
>  	struct iovec iov[VIRTIO_NET_QUEUE_SIZE];
>  	struct virt_queue *vq;
>  	struct kvm *kvm;
> +	struct net_dev *ndev = p;
>  	u16 out, in;
>  	u16 head;
>  	int len;
>  
> -	kvm	= p;
> -	vq	= &ndev.vqs[VIRTIO_NET_RX_QUEUE];
> +	kvm	= ndev->kvm;
> +	vq	= &ndev->vqs[VIRTIO_NET_RX_QUEUE];
>  
>  	while (1) {
>  
> -		mutex_lock(&ndev.io_rx_lock);
> +		mutex_lock(&ndev->io_rx_lock);
>  		if (!virt_queue__available(vq))
> -			pthread_cond_wait(&ndev.io_rx_cond, &ndev.io_rx_lock);
> -		mutex_unlock(&ndev.io_rx_lock);
> +			pthread_cond_wait(&ndev->io_rx_cond, &ndev->io_rx_lock);
> +		mutex_unlock(&ndev->io_rx_lock);
>  
>  		while (virt_queue__available(vq)) {
>  
>  			head = virt_queue__get_iov(vq, iov, &out, &in, kvm);
>  
> -			len = ndev.ops->rx(iov, in, &ndev);
> +			len = ndev->ops->rx(iov, in, ndev);
>  
>  			virt_queue__set_used_elem(vq, head, len);
>  
>  			/* We should interrupt guest right now, otherwise latency is huge. */
> -			virtio_pci__signal_vq(kvm, &ndev.vpci, VIRTIO_NET_RX_QUEUE);
> +			virtio_pci__signal_vq(kvm, &ndev->vpci, VIRTIO_NET_RX_QUEUE);
>  		}
>  
>  	}
> @@ -120,29 +125,30 @@ static void *virtio_net_tx_thread(void *p)
>  	struct iovec iov[VIRTIO_NET_QUEUE_SIZE];
>  	struct virt_queue *vq;
>  	struct kvm *kvm;
> +	struct net_dev *ndev = p;
>  	u16 out, in;
>  	u16 head;
>  	int len;
>  
> -	kvm	= p;
> -	vq	= &ndev.vqs[VIRTIO_NET_TX_QUEUE];
> +	kvm	= ndev->kvm;
> +	vq	= &ndev->vqs[VIRTIO_NET_TX_QUEUE];
>  
>  	while (1) {
> -		mutex_lock(&ndev.io_tx_lock);
> +		mutex_lock(&ndev->io_tx_lock);
>  		if (!virt_queue__available(vq))
> -			pthread_cond_wait(&ndev.io_tx_cond, &ndev.io_tx_lock);
> -		mutex_unlock(&ndev.io_tx_lock);
> +			pthread_cond_wait(&ndev->io_tx_cond, &ndev->io_tx_lock);
> +		mutex_unlock(&ndev->io_tx_lock);
>  
>  		while (virt_queue__available(vq)) {
>  
>  			head = virt_queue__get_iov(vq, iov, &out, &in, kvm);
>  
> -			len = ndev.ops->tx(iov, out, &ndev);
> +			len = ndev->ops->tx(iov, out, ndev);
>  
>  			virt_queue__set_used_elem(vq, head, len);
>  		}
>  
> -		virtio_pci__signal_vq(kvm, &ndev.vpci, VIRTIO_NET_TX_QUEUE);
> +		virtio_pci__signal_vq(kvm, &ndev->vpci, VIRTIO_NET_TX_QUEUE);
>  	}
>  
>  	pthread_exit(NULL);
> @@ -151,58 +157,58 @@ static void *virtio_net_tx_thread(void *p)
>  
>  }
>  
> -static void virtio_net_handle_callback(struct kvm *kvm, u16 queue_index)
> +static void virtio_net_handle_callback(struct kvm *kvm, struct net_dev *ndev, int queue)
>  {
> -	switch (queue_index) {
> +	switch (queue) {
>  	case VIRTIO_NET_TX_QUEUE:
> -		mutex_lock(&ndev.io_tx_lock);
> -		pthread_cond_signal(&ndev.io_tx_cond);
> -		mutex_unlock(&ndev.io_tx_lock);
> +		mutex_lock(&ndev->io_tx_lock);
> +		pthread_cond_signal(&ndev->io_tx_cond);
> +		mutex_unlock(&ndev->io_tx_lock);
>  		break;
>  	case VIRTIO_NET_RX_QUEUE:
> -		mutex_lock(&ndev.io_rx_lock);
> -		pthread_cond_signal(&ndev.io_rx_cond);
> -		mutex_unlock(&ndev.io_rx_lock);
> +		mutex_lock(&ndev->io_rx_lock);
> +		pthread_cond_signal(&ndev->io_rx_cond);
> +		mutex_unlock(&ndev->io_rx_lock);
>  		break;
>  	default:
> -		pr_warning("Unknown queue index %u", queue_index);
> +		pr_warning("Unknown queue index %u", queue);
>  	}
>  }
>  
> -static bool virtio_net__tap_init(const struct virtio_net_parameters *params)
> +static bool virtio_net__tap_init(const struct virtio_net_parameters *params,
> +					struct net_dev *ndev)
>  {
>  	int sock = socket(AF_INET, SOCK_STREAM, 0);
>  	int pid, status, offload, hdr_len;
>  	struct sockaddr_in sin = {0};
>  	struct ifreq ifr;
>  
> -	ndev.tap_fd = open("/dev/net/tun", O_RDWR);
> -	if (ndev.tap_fd < 0) {
> +	ndev->tap_fd = open("/dev/net/tun", O_RDWR);
> +	if (ndev->tap_fd < 0) {
>  		pr_warning("Unable to open /dev/net/tun");
>  		goto fail;
>  	}
>  
>  	memset(&ifr, 0, sizeof(ifr));
>  	ifr.ifr_flags = IFF_TAP | IFF_NO_PI | IFF_VNET_HDR;
> -	if (ioctl(ndev.tap_fd, TUNSETIFF, &ifr) < 0) {
> +	if (ioctl(ndev->tap_fd, TUNSETIFF, &ifr) < 0) {
>  		pr_warning("Config tap device error. Are you root?");
>  		goto fail;
>  	}
>  
> -	strncpy(ndev.tap_name, ifr.ifr_name, sizeof(ndev.tap_name));
> +	strncpy(ndev->tap_name, ifr.ifr_name, sizeof(ndev->tap_name));
>  
> -	if (ioctl(ndev.tap_fd, TUNSETNOCSUM, 1) < 0) {
> +	if (ioctl(ndev->tap_fd, TUNSETNOCSUM, 1) < 0) {
>  		pr_warning("Config tap device TUNSETNOCSUM error");
>  		goto fail;
>  	}
>  
>  	hdr_len = sizeof(struct virtio_net_hdr);
> -	if (ioctl(ndev.tap_fd, TUNSETVNETHDRSZ, &hdr_len) < 0) {
> +	if (ioctl(ndev->tap_fd, TUNSETVNETHDRSZ, &hdr_len) < 0)
>  		pr_warning("Config tap device TUNSETVNETHDRSZ error");
> -	}
>  
>  	offload = TUN_F_CSUM | TUN_F_TSO4 | TUN_F_TSO6 | TUN_F_UFO;
> -	if (ioctl(ndev.tap_fd, TUNSETOFFLOAD, offload) < 0) {
> +	if (ioctl(ndev->tap_fd, TUNSETOFFLOAD, offload) < 0) {
>  		pr_warning("Config tap device TUNSETOFFLOAD error");
>  		goto fail;
>  	}
> @@ -210,7 +216,7 @@ static bool virtio_net__tap_init(const struct virtio_net_parameters *params)
>  	if (strcmp(params->script, "none")) {
>  		pid = fork();
>  		if (pid == 0) {
> -			execl(params->script, params->script, ndev.tap_name, NULL);
> +			execl(params->script, params->script, ndev->tap_name, NULL);
>  			_exit(1);
>  		} else {
>  			waitpid(pid, &status, 0);
> @@ -221,7 +227,7 @@ static bool virtio_net__tap_init(const struct virtio_net_parameters *params)
>  		}
>  	} else {
>  		memset(&ifr, 0, sizeof(ifr));
> -		strncpy(ifr.ifr_name, ndev.tap_name, sizeof(ndev.tap_name));
> +		strncpy(ifr.ifr_name, ndev->tap_name, sizeof(ndev->tap_name));
>  		sin.sin_addr.s_addr = inet_addr(params->host_ip);
>  		memcpy(&(ifr.ifr_addr), &sin, sizeof(ifr.ifr_addr));
>  		ifr.ifr_addr.sa_family = AF_INET;
> @@ -232,7 +238,7 @@ static bool virtio_net__tap_init(const struct virtio_net_parameters *params)
>  	}
>  
>  	memset(&ifr, 0, sizeof(ifr));
> -	strncpy(ifr.ifr_name, ndev.tap_name, sizeof(ndev.tap_name));
> +	strncpy(ifr.ifr_name, ndev->tap_name, sizeof(ndev->tap_name));
>  	ioctl(sock, SIOCGIFFLAGS, &ifr);
>  	ifr.ifr_flags |= IFF_UP | IFF_RUNNING;
>  	if (ioctl(sock, SIOCSIFFLAGS, &ifr) < 0)
> @@ -245,22 +251,22 @@ static bool virtio_net__tap_init(const struct virtio_net_parameters *params)
>  fail:
>  	if (sock >= 0)
>  		close(sock);
> -	if (ndev.tap_fd >= 0)
> -		close(ndev.tap_fd);
> +	if (ndev->tap_fd >= 0)
> +		close(ndev->tap_fd);
>  
>  	return 0;
>  }
>  
> -static void virtio_net__io_thread_init(struct kvm *kvm)
> +static void virtio_net__io_thread_init(struct kvm *kvm, struct net_dev *ndev)
>  {
> -	pthread_mutex_init(&ndev.io_rx_lock, NULL);
> -	pthread_cond_init(&ndev.io_tx_cond, NULL);
> +	pthread_mutex_init(&ndev->io_rx_lock, NULL);
> +	pthread_cond_init(&ndev->io_tx_cond, NULL);
>  
> -	pthread_mutex_init(&ndev.io_rx_lock, NULL);
> -	pthread_cond_init(&ndev.io_tx_cond, NULL);
> +	pthread_mutex_init(&ndev->io_rx_lock, NULL);
> +	pthread_cond_init(&ndev->io_tx_cond, NULL);
>  
> -	pthread_create(&ndev.io_rx_thread, NULL, virtio_net_rx_thread, (void *)kvm);
> -	pthread_create(&ndev.io_tx_thread, NULL, virtio_net_tx_thread, (void *)kvm);
> +	pthread_create(&ndev->io_rx_thread, NULL, virtio_net_rx_thread, ndev);
> +	pthread_create(&ndev->io_tx_thread, NULL, virtio_net_tx_thread, ndev);
>  }
>  
>  static inline int tap_ops_tx(struct iovec *iov, u16 out, struct net_dev *ndev)
> @@ -345,7 +351,9 @@ static int init_vq(struct kvm *kvm, void *dev, u32 vq, u32 pfn)
>  
>  static int notify_vq(struct kvm *kvm, void *dev, u32 vq)
>  {
> -	virtio_net_handle_callback(kvm, vq);
> +	struct net_dev *ndev = dev;
> +
> +	virtio_net_handle_callback(kvm, ndev, vq);
>  
>  	return 0;
>  }
> @@ -365,27 +373,43 @@ static int get_size_vq(struct kvm *kvm, void *dev, u32 vq)
>  void virtio_net__init(const struct virtio_net_parameters *params)
>  {
>  	int i;
> +	struct net_dev *ndev;
> +
> +	if (!params)
> +		return;
> +
> +	ndev = calloc(1, sizeof(struct net_dev));
> +	if (ndev == NULL)
> +		die("Failed allocating ndev");
> +
> +	list_add_tail(&ndev->list, &ndevs);
> +
> +	ndev->kvm = params->kvm;
> +
> +	mutex_init(&ndev->mutex);
> +	ndev->config.status				= VIRTIO_NET_S_LINK_UP;

This indentation is wrong.

>  	for (i = 0 ; i < 6 ; i++) {
> -		ndev.config.mac[i]		= params->guest_mac[i];
> -		ndev.info.guest_mac.addr[i]	= params->guest_mac[i];
> -		ndev.info.host_mac.addr[i]	= params->host_mac[i];
> +		ndev->config.mac[i]		= params->guest_mac[i];
> +		ndev->info.guest_mac.addr[i]	= params->guest_mac[i];
> +		ndev->info.host_mac.addr[i]	= params->host_mac[i];
>  	}
>  
> -	ndev.mode = params->mode;
> -	if (ndev.mode == NET_MODE_TAP) {
> -		virtio_net__tap_init(params);
> -		ndev.ops = &tap_ops;
> +	ndev->mode = params->mode;
> +	if (ndev->mode == NET_MODE_TAP) {
> +		virtio_net__tap_init(params, ndev);
> +		ndev->ops = &tap_ops;
>  	} else {
> -		ndev.info.host_ip		= ntohl(inet_addr(params->host_ip));
> -		ndev.info.guest_ip		= ntohl(inet_addr(params->guest_ip));
> -		ndev.info.guest_netmask		= ntohl(inet_addr("255.255.255.0"));
> -		uip_init(&ndev.info);
> -		ndev.ops = &uip_ops;
> +		ndev->info.host_ip		= ntohl(inet_addr(params->host_ip));
> +		ndev->info.guest_ip		= ntohl(inet_addr(params->guest_ip));
> +		ndev->info.guest_netmask	= ntohl(inet_addr("255.255.255.0"));
> +		ndev->info.buf_nr = 20,

Vertical indentation is needed for info.buf_nr as well.

> +		uip_init(&ndev->info);
> +		ndev->ops = &uip_ops;
>  	}
>  
> -	virtio_pci__init(kvm, &ndev.vpci, &ndev, PCI_DEVICE_ID_VIRTIO_NET, VIRTIO_ID_NET);
> -	ndev.vpci.ops = (struct virtio_pci_ops) {
> +	virtio_pci__init(kvm, &ndev->vpci, ndev, PCI_DEVICE_ID_VIRTIO_NET, VIRTIO_ID_NET);
> +	ndev->vpci.ops = (struct virtio_pci_ops) {
>  		.set_config		= set_config,
>  		.get_config		= get_config,
>  		.get_host_features	= get_host_features,
> @@ -396,9 +420,9 @@ void virtio_net__init(const struct virtio_net_parameters *params)
>  		.get_size_vq		= get_size_vq,
>  	};
>  
> -	virtio_net__io_thread_init(params->kvm);
> +	virtio_net__io_thread_init(params->kvm, ndev);
>  
> -	ndev.compat_id = compat__add_message("virtio-net device was not detected",
> +	ndev->compat_id = compat__add_message("virtio-net device was not detected",
>  						"While you have requested a virtio-net device, "
>  						"the guest kernel didn't seem to detect it.\n"
>  						"Please make sure that the kernel was compiled "

Other changes look good to me.

-- 
Asias He