[PATCH 0/6] virtio: console: Fixes, abi update

[PATCH 0/6] virtio: console: Fixes, abi update

[Amit Shah]

Hello,

These patches fix a couple of small issues: 
 - generate a kobject change event so that udev is woken up on name
   changes
 - fix a crash after hot-unplug of the first console port and a
   subsequent config update

But majorly, it reworks how ports are discovered: instead of numbering
the ports individually in the host and the guest by just incrementing
a number, we now switch to a bitmap in the config space exposed by the
host to identify active ports. This lets us maintain the same
numbering used by the host and also allows for hot-unplug via the
config space. This is needed for proper migration support after
several hot-plug/unplug operations.

I've tested these patches on my testsuite to catch any regression or
correctness issues. I've also tested all the hotplug-related changes
here.

These should go to 2.6.34, so that we don't push out a stable release
with the older interface. Michael, please forward these to Linus if
everyone is OK with these. I also have a git repo at

git://git.kernel.org/pub/scm/linux/kernel/git/amit/vs-kernel.git master

if you prefer to pull the patches.

Amit Shah (6):
  virtio: console: Generate a kobject CHANGE event on adding 'name'
    attribute
  virtio: console: Check if port is valid in resize_console
  virtio: console: Switch to using a port bitmap for port discovery
  virtio: console: Separate out get_config in a separate function
  virtio: console: Handle hot-plug/unplug config actions
  virtio: console: Remove hot-unplug control message

 drivers/char/virtio_console.c  |  238 ++++++++++++++++++++++++----------------
 include/linux/virtio_console.h |   15 ++-
 2 files changed, 156 insertions(+), 97 deletions(-)

[PATCH 1/6] virtio: console: Generate a kobject CHANGE event on adding 'name' attribute

[Amit Shah]

When the host lets us know what 'name' a port is assigned, we create the
sysfs 'name' attribute. Generate a 'change' event after this so that
udev wakes up and acts on the rules for virtio-ports (currently there's
only one rule that creates a symlink from the 'name' to the actual char
device).

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/char/virtio_console.c |   11 +++++++++--
 1 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index f404ccf..67b474b 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -947,11 +947,18 @@ static void handle_control_message(struct ports_device *portdev,
 		 */
 		err = sysfs_create_group(&port->dev->kobj,
 					 &port_attribute_group);
-		if (err)
+		if (err) {
 			dev_err(port->dev,
 				"Error %d creating sysfs device attributes\n",
 				err);
-
+		} else {
+			/*
+			 * Generate a udev event so that appropriate
+			 * symlinks can be created based on udev
+			 * rules.
+			 */
+			kobject_uevent(&port->dev->kobj, KOBJ_CHANGE);
+		}
 		break;
 	case VIRTIO_CONSOLE_PORT_REMOVE:
 		/*
-- 
1.6.2.5

[PATCH 2/6] virtio: console: Check if port is valid in resize_console

[Amit Shah]

The console port could have been hot-unplugged. Check if it is valid
before working on it.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/char/virtio_console.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 67b474b..44288ce 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -681,6 +681,10 @@ static void resize_console(struct port *port)
 	struct virtio_device *vdev;
 	struct winsize ws;
 
+	/* The port could have been hot-unplugged */
+	if (!port)
+		return;
+
 	vdev = port->portdev->vdev;
 	if (virtio_has_feature(vdev, VIRTIO_CONSOLE_F_SIZE)) {
 		vdev->config->get(vdev,
-- 
1.6.2.5

[PATCH 3/6] virtio: console: Switch to using a port bitmap for port discovery

[Amit Shah]

Ports are now discovered by their location as given by host instead of
just incrementing a number and expecting the host and guest numbers stay
in sync. They are needed to be the same because the control messages
identify ports using the port id.

This is most beneficial to management software to properly place ports
at known ids so that the ids after hot-plug/unplug operations can be
controlled. This helps migration of guests after such hot-plug/unplug
operations.

The support for port hot-plug is removed in this commit, will be added
in the following commits.

The config space is now a variable-sized array.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/char/virtio_console.c  |  139 ++++++++++++++++++++++------------------
 include/linux/virtio_console.h |   14 +++-
 2 files changed, 87 insertions(+), 66 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 44288ce..dc15c92 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -120,7 +120,7 @@ struct ports_device {
 	spinlock_t cvq_lock;
 
 	/* The current config space is stored here */
-	struct virtio_console_config config;
+	struct virtio_console_config *config;
 
 	/* The virtio device we're associated with */
 	struct virtio_device *vdev;
@@ -1210,6 +1210,23 @@ fail:
 	return err;
 }
 
+static u32 find_next_bit_in_map(u32 *map)
+{
+	u32 port_bit;
+
+	port_bit = ffs(*map);
+	port_bit--; /* ffs returns bit location */
+
+	*map &= ~(1U << port_bit);
+
+	return port_bit;
+}
+
+static u32 get_ports_map_size(u32 max_nr_ports)
+{
+	return sizeof(u32) * ((max_nr_ports + 31)/ 32);
+}
+
 /*
  * The workhandler for config-space updates.
  *
@@ -1225,45 +1242,8 @@ static void config_work_handler(struct work_struct *work)
 	portdev = container_of(work, struct ports_device, config_work);
 
 	vdev = portdev->vdev;
-	vdev->config->get(vdev,
-			  offsetof(struct virtio_console_config, nr_ports),
-			  &virtconconf.nr_ports,
-			  sizeof(virtconconf.nr_ports));
 
-	if (portdev->config.nr_ports == virtconconf.nr_ports) {
-		/*
-		 * Port 0 got hot-added.  Since we already did all the
-		 * other initialisation for it, just tell the Host
-		 * that the port is ready if we find the port.  In
-		 * case the port was hot-removed earlier, we call
-		 * add_port to add the port.
-		 */
-		struct port *port;
-
-		port = find_port_by_id(portdev, 0);
-		if (!port)
-			add_port(portdev, 0);
-		else
-			send_control_msg(port, VIRTIO_CONSOLE_PORT_READY, 1);
-		return;
-	}
-	if (virtconconf.nr_ports > portdev->config.max_nr_ports) {
-		dev_warn(&vdev->dev,
-			 "More ports specified (%u) than allowed (%u)",
-			 portdev->config.nr_ports + 1,
-			 portdev->config.max_nr_ports);
-		return;
-	}
-	if (virtconconf.nr_ports < portdev->config.nr_ports)
-		return;
-
-	/* Hot-add ports */
-	while (virtconconf.nr_ports - portdev->config.nr_ports) {
-		err = add_port(portdev, portdev->config.nr_ports);
-		if (err)
-			break;
-		portdev->config.nr_ports++;
-	}
+	/* FIXME: Handle port hotplug/unplug */
 }
 
 static int init_vqs(struct ports_device *portdev)
@@ -1274,7 +1254,7 @@ static int init_vqs(struct ports_device *portdev)
 	u32 i, j, nr_ports, nr_queues;
 	int err;
 
-	nr_ports = portdev->config.max_nr_ports;
+	nr_ports = portdev->config->max_nr_ports;
 	nr_queues = use_multiport(portdev) ? (nr_ports + 1) * 2 : 2;
 
 	vqs = kmalloc(nr_queues * sizeof(struct virtqueue *), GFP_KERNEL);
@@ -1387,7 +1367,6 @@ static const struct file_operations portdev_fops = {
 static int __devinit virtcons_probe(struct virtio_device *vdev)
 {
 	struct ports_device *portdev;
-	u32 i;
 	int err;
 	bool multiport;
 
@@ -1415,30 +1394,45 @@ static int __devinit virtcons_probe(struct virtio_device *vdev)
 		goto free;
 	}
 
-	multiport = false;
-	portdev->config.nr_ports = 1;
-	portdev->config.max_nr_ports = 1;
 	if (virtio_has_feature(vdev, VIRTIO_CONSOLE_F_MULTIPORT)) {
+		u32 max_nr_ports;
+
 		multiport = true;
 		vdev->features[0] |= 1 << VIRTIO_CONSOLE_F_MULTIPORT;
 
 		vdev->config->get(vdev, offsetof(struct virtio_console_config,
-						 nr_ports),
-				  &portdev->config.nr_ports,
-				  sizeof(portdev->config.nr_ports));
-		vdev->config->get(vdev, offsetof(struct virtio_console_config,
 						 max_nr_ports),
-				  &portdev->config.max_nr_ports,
-				  sizeof(portdev->config.max_nr_ports));
-		if (portdev->config.nr_ports > portdev->config.max_nr_ports) {
-			dev_warn(&vdev->dev,
-				 "More ports (%u) specified than allowed (%u). Will init %u ports.",
-				 portdev->config.nr_ports,
-				 portdev->config.max_nr_ports,
-				 portdev->config.max_nr_ports);
-
-			portdev->config.nr_ports = portdev->config.max_nr_ports;
+				  &max_nr_ports, sizeof(max_nr_ports));
+		/*
+		 * We have a variable-sized config space that's
+		 * dependent on the maximum number of ports a guest
+		 * can have.  So we first get the max number of ports
+		 * we can have and then allocate the config space
+		 */
+		portdev->config = kmalloc(sizeof(struct virtio_console_config)
+					  + get_ports_map_size(max_nr_ports),
+					  GFP_KERNEL);
+		if (!portdev->config) {
+			err = -ENOMEM;
+			goto free_chrdev;
+		}
+
+		portdev->config->max_nr_ports = max_nr_ports;
+		vdev->config->get(vdev, offsetof(struct virtio_console_config,
+						 ports_map),
+				  portdev->config->ports_map,
+				  get_ports_map_size(max_nr_ports));
+	} else {
+		multiport = false;
+
+		portdev->config = kmalloc(sizeof(struct virtio_console_config),
+					  GFP_KERNEL);
+		if (!portdev->config) {
+			err = -ENOMEM;
+			goto free_chrdev;
 		}
+
+		portdev->config->max_nr_ports = 1;
 	}
 
 	/* Let the Host know we support multiple ports.*/
@@ -1447,14 +1441,14 @@ static int __devinit virtcons_probe(struct virtio_device *vdev)
 	err = init_vqs(portdev);
 	if (err < 0) {
 		dev_err(&vdev->dev, "Error %d initializing vqs\n", err);
-		goto free_chrdev;
+		goto free_config;
 	}
 
 	spin_lock_init(&portdev->ports_lock);
 	INIT_LIST_HEAD(&portdev->ports);
 
 	if (multiport) {
-		unsigned int nr_added_bufs;
+		unsigned int nr_added_bufs, i;
 
 		spin_lock_init(&portdev->cvq_lock);
 		INIT_WORK(&portdev->control_work, &control_work_handler);
@@ -1467,10 +1461,26 @@ static int __devinit virtcons_probe(struct virtio_device *vdev)
 			err = -ENOMEM;
 			goto free_vqs;
 		}
-	}
 
-	for (i = 0; i < portdev->config.nr_ports; i++)
-		add_port(portdev, i);
+		for (i = 0; i < (portdev->config->max_nr_ports + 31) / 32; i++) {
+			u32 map, port_nr;
+
+			map = portdev->config->ports_map[i];
+			while (map) {
+				port_nr = find_next_bit_in_map(&map) + i * 32;
+
+				add_port(portdev, port_nr);
+				/*
+				 * FIXME: Send a notification to the
+				 * host about failed port add
+				 */
+			}
+		}
+	} else {
+		err = add_port(portdev, 0);
+		if (err)
+			goto free_vqs;
+	}
 
 	/* Start using the new console output. */
 	early_put_chars = NULL;
@@ -1480,6 +1490,8 @@ free_vqs:
 	vdev->config->del_vqs(vdev);
 	kfree(portdev->in_vqs);
 	kfree(portdev->out_vqs);
+free_config:
+	kfree(portdev->config);
 free_chrdev:
 	unregister_chrdev(portdev->chr_major, "virtio-portsdev");
 free:
@@ -1514,6 +1526,7 @@ static void virtcons_remove(struct virtio_device *vdev)
 	vdev->config->del_vqs(vdev);
 	kfree(portdev->in_vqs);
 	kfree(portdev->out_vqs);
+	kfree(portdev->config);
 
 	kfree(portdev);
 }
diff --git a/include/linux/virtio_console.h b/include/linux/virtio_console.h
index ae4f039..287ee44 100644
--- a/include/linux/virtio_console.h
+++ b/include/linux/virtio_console.h
@@ -14,15 +14,23 @@
 #define VIRTIO_CONSOLE_F_SIZE	0	/* Does host provide console size? */
 #define VIRTIO_CONSOLE_F_MULTIPORT 1	/* Does host provide multiple ports? */
 
+/*
+ * This is the config space shared between the Host and the Guest.
+ * The Host indicates to us the maximum number of ports this device
+ * can hold and a port map indicating which ports are active.
+ *
+ * The maximum number of ports is not a round number to prevent
+ * wastage of MSI vectors in case MSI is enabled for this device.
+ */
 struct virtio_console_config {
 	/* colums of the screens */
 	__u16 cols;
 	/* rows of the screens */
 	__u16 rows;
-	/* max. number of ports this device can hold */
+	/* max. number of ports this device can hold. */
 	__u32 max_nr_ports;
-	/* number of ports added so far */
-	__u32 nr_ports;
+	/* locations of ports in use; variable-sized array */
+	__u32 ports_map[0 /* (max_nr_ports + 31) / 2 */];
 } __attribute__((packed));
 
 /*
-- 
1.6.2.5

[PATCH 4/6] virtio: console: Separate out get_config in a separate function

[Amit Shah]

Getting config information is done in probe() as well as
config_work_handler().

Separate it out so that we only code it once.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/char/virtio_console.c |   85 +++++++++++++++++++++++------------------
 1 files changed, 48 insertions(+), 37 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index dc15c92..9f2cbf0 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1227,6 +1227,46 @@ static u32 get_ports_map_size(u32 max_nr_ports)
 	return sizeof(u32) * ((max_nr_ports + 31)/ 32);
 }
 
+static struct virtio_console_config *get_config(struct virtio_device *vdev,
+						bool multiport)
+{
+	struct virtio_console_config *config;
+	u32 max_nr_ports;
+
+	if (!multiport) {
+		config = kmalloc(sizeof(struct virtio_console_config),
+				 GFP_KERNEL);
+		if (!config)
+			return NULL;
+
+		config->max_nr_ports = 1;
+		return config;
+	}
+
+	vdev->config->get(vdev, offsetof(struct virtio_console_config,
+					 max_nr_ports),
+			  &max_nr_ports, sizeof(max_nr_ports));
+
+	/*
+	 * We have a variable-sized config space that's dependent on
+	 * the maximum number of ports a guest can have.  So we first
+	 * get the max number of ports we can have and then allocate
+	 * the config space
+	 */
+	config = kmalloc(sizeof(struct virtio_console_config)
+			 + get_ports_map_size(max_nr_ports),
+			 GFP_KERNEL);
+	if (!config)
+		return NULL;
+
+	config->max_nr_ports = max_nr_ports;
+	vdev->config->get(vdev, offsetof(struct virtio_console_config,
+					 ports_map),
+			  config->ports_map,
+			  get_ports_map_size(max_nr_ports));
+	return config;
+}
+
 /*
  * The workhandler for config-space updates.
  *
@@ -1394,49 +1434,20 @@ static int __devinit virtcons_probe(struct virtio_device *vdev)
 		goto free;
 	}
 
+	multiport = false;
 	if (virtio_has_feature(vdev, VIRTIO_CONSOLE_F_MULTIPORT)) {
-		u32 max_nr_ports;
-
 		multiport = true;
 		vdev->features[0] |= 1 << VIRTIO_CONSOLE_F_MULTIPORT;
 
-		vdev->config->get(vdev, offsetof(struct virtio_console_config,
-						 max_nr_ports),
-				  &max_nr_ports, sizeof(max_nr_ports));
-		/*
-		 * We have a variable-sized config space that's
-		 * dependent on the maximum number of ports a guest
-		 * can have.  So we first get the max number of ports
-		 * we can have and then allocate the config space
-		 */
-		portdev->config = kmalloc(sizeof(struct virtio_console_config)
-					  + get_ports_map_size(max_nr_ports),
-					  GFP_KERNEL);
-		if (!portdev->config) {
-			err = -ENOMEM;
-			goto free_chrdev;
-		}
-
-		portdev->config->max_nr_ports = max_nr_ports;
-		vdev->config->get(vdev, offsetof(struct virtio_console_config,
-						 ports_map),
-				  portdev->config->ports_map,
-				  get_ports_map_size(max_nr_ports));
-	} else {
-		multiport = false;
-
-		portdev->config = kmalloc(sizeof(struct virtio_console_config),
-					  GFP_KERNEL);
-		if (!portdev->config) {
-			err = -ENOMEM;
-			goto free_chrdev;
-		}
-
-		portdev->config->max_nr_ports = 1;
+		/* Let the Host know we support multiple ports.*/
+		vdev->config->finalize_features(vdev);
 	}
 
-	/* Let the Host know we support multiple ports.*/
-	vdev->config->finalize_features(vdev);
+	portdev->config = get_config(vdev, multiport);
+	if (!portdev->config) {
+		err = -ENOMEM;
+		goto free_chrdev;
+	}
 
 	err = init_vqs(portdev);
 	if (err < 0) {
-- 
1.6.2.5

[PATCH 5/6] virtio: console: Handle hot-plug/unplug config actions

[Amit Shah]

The new way of handling hot-plug as well as unplug is via a
config interrupt.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/char/virtio_console.c |   53 +++++++++++++++++++++++++++++++++++++----
 1 files changed, 48 insertions(+), 5 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 9f2cbf0..06ca95c 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1268,22 +1268,65 @@ static struct virtio_console_config *get_config(struct virtio_device *vdev,
 }
 
 /*
- * The workhandler for config-space updates.
+ * The workhandler for config-space updates.  Only called when
+ * multiport is supported.
  *
- * This is called when ports are hot-added.
+ * This is called when ports are hot-added and hot-removed.
  */
 static void config_work_handler(struct work_struct *work)
 {
-	struct virtio_console_config virtconconf;
+	struct virtio_console_config *config;
 	struct ports_device *portdev;
 	struct virtio_device *vdev;
-	int err;
+	unsigned int i, changed;
 
 	portdev = container_of(work, struct ports_device, config_work);
 
 	vdev = portdev->vdev;
 
-	/* FIXME: Handle port hotplug/unplug */
+	config = get_config(vdev, true);
+	if (!config) {
+		dev_warn(&vdev->dev,
+			 "Running out of memory on config change events\n");
+		return;
+	}
+
+	if (config->max_nr_ports != portdev->config->max_nr_ports) {
+		dev_warn(&vdev->dev,
+			 "Host updated max_nr_ports, can't handle that now.\n");
+		goto free;
+	}
+
+	changed = 0;
+	for (i = 0; i < (config->max_nr_ports + 31) / 32; i++) {
+		u32 map;
+
+		map = config->ports_map[i] ^ portdev->config->ports_map[i];
+		while (map) {
+			struct port *port;
+			u32 port_nr;
+
+			changed++;
+			port_nr = find_next_bit_in_map(&map) + i * 32;
+			port = find_port_by_id(portdev, port_nr);
+			if (port)
+				remove_port(port);
+			else
+				add_port(portdev, port_nr);
+		}
+		portdev->config->ports_map[i] = config->ports_map[i];
+	}
+	if (!changed && find_port_by_id(portdev, 0)) {
+		/*
+		 * No hot-plug / unplug activity. Port 0 might have
+		 * been hot-added.  Just send a 'ready' message in
+		 * that case, since we already have it added.
+		 */
+		send_control_msg(find_port_by_id(portdev, 0),
+				 VIRTIO_CONSOLE_PORT_READY, 1);
+	}
+free:
+	kfree(config);
 }
 
 static int init_vqs(struct ports_device *portdev)
-- 
1.6.2.5

[PATCH 6/6] virtio: console: Remove hot-unplug control message

[Amit Shah]

Now that we handle port hot-unplug via config updates, remove the
control message that was created.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/char/virtio_console.c  |   26 --------------------------
 include/linux/virtio_console.h |    1 -
 2 files changed, 0 insertions(+), 27 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 06ca95c..a810099 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -964,32 +964,6 @@ static void handle_control_message(struct ports_device *portdev,
 			kobject_uevent(&port->dev->kobj, KOBJ_CHANGE);
 		}
 		break;
-	case VIRTIO_CONSOLE_PORT_REMOVE:
-		/*
-		 * Hot unplug the port.  We don't decrement nr_ports
-		 * since we don't want to deal with extra complexities
-		 * of using the lowest-available port id: We can just
-		 * pick up the nr_ports number as the id and not have
-		 * userspace send it to us.  This helps us in two
-		 * ways:
-		 *
-		 * - We don't need to have a 'port_id' field in the
-		 *   config space when a port is hot-added.  This is a
-		 *   good thing as we might queue up multiple hotplug
-		 *   requests issued in our workqueue.
-		 *
-		 * - Another way to deal with this would have been to
-		 *   use a bitmap of the active ports and select the
-		 *   lowest non-active port from that map.  That
-		 *   bloats the already tight config space and we
-		 *   would end up artificially limiting the
-		 *   max. number of ports to sizeof(bitmap).  Right
-		 *   now we can support 2^32 ports (as the port id is
-		 *   stored in a u32 type).
-		 *
-		 */
-		remove_port(port);
-		break;
 	}
 }
 
diff --git a/include/linux/virtio_console.h b/include/linux/virtio_console.h
index 287ee44..eaaf04b 100644
--- a/include/linux/virtio_console.h
+++ b/include/linux/virtio_console.h
@@ -49,7 +49,6 @@ struct virtio_console_control {
 #define VIRTIO_CONSOLE_RESIZE		2
 #define VIRTIO_CONSOLE_PORT_OPEN	3
 #define VIRTIO_CONSOLE_PORT_NAME	4
-#define VIRTIO_CONSOLE_PORT_REMOVE	5
 
 #ifdef __KERNEL__
 int __init virtio_cons_early_init(int (*put_chars)(u32, const char *, int));
-- 
1.6.2.5

Re: [PATCH 3/6] virtio: console: Switch to using a port bitmap for port discovery

[Michael S. Tsirkin]

On Fri, Mar 19, 2010 at 05:36:45PM +0530, Amit Shah wrote:
> Ports are now discovered by their location as given by host instead of
> just incrementing a number and expecting the host and guest numbers stay
> in sync. They are needed to be the same because the control messages
> identify ports using the port id.
> 
> This is most beneficial to management software to properly place ports
> at known ids so that the ids after hot-plug/unplug operations can be
> controlled. This helps migration of guests after such hot-plug/unplug
> operations.
> 
> The support for port hot-plug is removed in this commit, will be added
> in the following commits.

It might be cleaner not to split it this way, merge the following
commits into this one or split it in a different way.

> The config space is now a variable-sized array.

I think this last bit is problematic: we won't be able to add any more data if
we have to, without a lot of complexity.  Further, in the past we have
also had problems running out of config space: see
3225beaba05d4f06087593f5e903ce867b6e118a. There's also
a comment in handle_control_message which suggests we
might want a very large number of ports at some point,
if we do using config space would be a mistake.

It might be better to use a control vq for this, like virtio block ended
up doing.  The comment in handle_control_message hints we don't want to
pass port id in config space, but I am not sure why we can't pass it in
message buffer.


> Signed-off-by: Amit Shah <amit.shah@redhat.com>
> ---
>  drivers/char/virtio_console.c  |  139 ++++++++++++++++++++++------------------
>  include/linux/virtio_console.h |   14 +++-
>  2 files changed, 87 insertions(+), 66 deletions(-)
> 
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index 44288ce..dc15c92 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -120,7 +120,7 @@ struct ports_device {
>  	spinlock_t cvq_lock;
>  
>  	/* The current config space is stored here */
> -	struct virtio_console_config config;
> +	struct virtio_console_config *config;
>  
>  	/* The virtio device we're associated with */
>  	struct virtio_device *vdev;
> @@ -1210,6 +1210,23 @@ fail:
>  	return err;
>  }
>  
> +static u32 find_next_bit_in_map(u32 *map)
> +{
> +	u32 port_bit;
> +
> +	port_bit = ffs(*map);
> +	port_bit--; /* ffs returns bit location */
> +
> +	*map &= ~(1U << port_bit);

The above only works well if map is non-zero.  This happens to be the
case the way we call it, but since this means the function is not
generic, it might be better to opencode it to make it obvious.

> +
> +	return port_bit;
> +}
> +
> +static u32 get_ports_map_size(u32 max_nr_ports)
> +{
> +	return sizeof(u32) * ((max_nr_ports + 31)/ 32);

DIV_ROUND_UP here and elsewhere?

> +}
> +
>  /*
>   * The workhandler for config-space updates.
>   *
> @@ -1225,45 +1242,8 @@ static void config_work_handler(struct work_struct *work)
>  	portdev = container_of(work, struct ports_device, config_work);
>  
>  	vdev = portdev->vdev;
> -	vdev->config->get(vdev,
> -			  offsetof(struct virtio_console_config, nr_ports),
> -			  &virtconconf.nr_ports,
> -			  sizeof(virtconconf.nr_ports));
>  
> -	if (portdev->config.nr_ports == virtconconf.nr_ports) {
> -		/*
> -		 * Port 0 got hot-added.  Since we already did all the
> -		 * other initialisation for it, just tell the Host
> -		 * that the port is ready if we find the port.  In
> -		 * case the port was hot-removed earlier, we call
> -		 * add_port to add the port.
> -		 */
> -		struct port *port;
> -
> -		port = find_port_by_id(portdev, 0);
> -		if (!port)
> -			add_port(portdev, 0);
> -		else
> -			send_control_msg(port, VIRTIO_CONSOLE_PORT_READY, 1);
> -		return;
> -	}
> -	if (virtconconf.nr_ports > portdev->config.max_nr_ports) {
> -		dev_warn(&vdev->dev,
> -			 "More ports specified (%u) than allowed (%u)",
> -			 portdev->config.nr_ports + 1,
> -			 portdev->config.max_nr_ports);
> -		return;
> -	}
> -	if (virtconconf.nr_ports < portdev->config.nr_ports)
> -		return;
> -
> -	/* Hot-add ports */
> -	while (virtconconf.nr_ports - portdev->config.nr_ports) {
> -		err = add_port(portdev, portdev->config.nr_ports);
> -		if (err)
> -			break;
> -		portdev->config.nr_ports++;
> -	}
> +	/* FIXME: Handle port hotplug/unplug */
>  }
>  
>  static int init_vqs(struct ports_device *portdev)
> @@ -1274,7 +1254,7 @@ static int init_vqs(struct ports_device *portdev)
>  	u32 i, j, nr_ports, nr_queues;
>  	int err;
>  
> -	nr_ports = portdev->config.max_nr_ports;
> +	nr_ports = portdev->config->max_nr_ports;
>  	nr_queues = use_multiport(portdev) ? (nr_ports + 1) * 2 : 2;
>  
>  	vqs = kmalloc(nr_queues * sizeof(struct virtqueue *), GFP_KERNEL);
> @@ -1387,7 +1367,6 @@ static const struct file_operations portdev_fops = {
>  static int __devinit virtcons_probe(struct virtio_device *vdev)
>  {
>  	struct ports_device *portdev;
> -	u32 i;
>  	int err;
>  	bool multiport;
>  
> @@ -1415,30 +1394,45 @@ static int __devinit virtcons_probe(struct virtio_device *vdev)
>  		goto free;
>  	}
>  
> -	multiport = false;
> -	portdev->config.nr_ports = 1;
> -	portdev->config.max_nr_ports = 1;
>  	if (virtio_has_feature(vdev, VIRTIO_CONSOLE_F_MULTIPORT)) {
> +		u32 max_nr_ports;
> +
>  		multiport = true;
>  		vdev->features[0] |= 1 << VIRTIO_CONSOLE_F_MULTIPORT;
>  
>  		vdev->config->get(vdev, offsetof(struct virtio_console_config,
> -						 nr_ports),
> -				  &portdev->config.nr_ports,
> -				  sizeof(portdev->config.nr_ports));
> -		vdev->config->get(vdev, offsetof(struct virtio_console_config,
>  						 max_nr_ports),
> -				  &portdev->config.max_nr_ports,
> -				  sizeof(portdev->config.max_nr_ports));
> -		if (portdev->config.nr_ports > portdev->config.max_nr_ports) {
> -			dev_warn(&vdev->dev,
> -				 "More ports (%u) specified than allowed (%u). Will init %u ports.",
> -				 portdev->config.nr_ports,
> -				 portdev->config.max_nr_ports,
> -				 portdev->config.max_nr_ports);
> -
> -			portdev->config.nr_ports = portdev->config.max_nr_ports;
> +				  &max_nr_ports, sizeof(max_nr_ports));
> +		/*
> +		 * We have a variable-sized config space that's
> +		 * dependent on the maximum number of ports a guest
> +		 * can have.  So we first get the max number of ports
> +		 * we can have and then allocate the config space
> +		 */
> +		portdev->config = kmalloc(sizeof(struct virtio_console_config)
> +					  + get_ports_map_size(max_nr_ports),
> +					  GFP_KERNEL);
> +		if (!portdev->config) {
> +			err = -ENOMEM;
> +			goto free_chrdev;
> +		}
> +
> +		portdev->config->max_nr_ports = max_nr_ports;
> +		vdev->config->get(vdev, offsetof(struct virtio_console_config,
> +						 ports_map),
> +				  portdev->config->ports_map,
> +				  get_ports_map_size(max_nr_ports));
> +	} else {
> +		multiport = false;
> +
> +		portdev->config = kmalloc(sizeof(struct virtio_console_config),
> +					  GFP_KERNEL);
> +		if (!portdev->config) {
> +			err = -ENOMEM;
> +			goto free_chrdev;
>  		}
> +
> +		portdev->config->max_nr_ports = 1;
>  	}
>  
>  	/* Let the Host know we support multiple ports.*/
> @@ -1447,14 +1441,14 @@ static int __devinit virtcons_probe(struct virtio_device *vdev)
>  	err = init_vqs(portdev);
>  	if (err < 0) {
>  		dev_err(&vdev->dev, "Error %d initializing vqs\n", err);
> -		goto free_chrdev;
> +		goto free_config;
>  	}
>  
>  	spin_lock_init(&portdev->ports_lock);
>  	INIT_LIST_HEAD(&portdev->ports);
>  
>  	if (multiport) {
> -		unsigned int nr_added_bufs;
> +		unsigned int nr_added_bufs, i;
>  
>  		spin_lock_init(&portdev->cvq_lock);
>  		INIT_WORK(&portdev->control_work, &control_work_handler);
> @@ -1467,10 +1461,26 @@ static int __devinit virtcons_probe(struct virtio_device *vdev)
>  			err = -ENOMEM;
>  			goto free_vqs;
>  		}
> -	}
>  
> -	for (i = 0; i < portdev->config.nr_ports; i++)
> -		add_port(portdev, i);
> +		for (i = 0; i < (portdev->config->max_nr_ports + 31) / 32; i++) {
> +			u32 map, port_nr;
> +
> +			map = portdev->config->ports_map[i];
> +			while (map) {
> +				port_nr = find_next_bit_in_map(&map) + i * 32;
> +
> +				add_port(portdev, port_nr);
> +				/*
> +				 * FIXME: Send a notification to the
> +				 * host about failed port add
> +				 */

This FIXME is just pointing out an existing bug, correct?

> +			}
> +		}
> +	} else {
> +		err = add_port(portdev, 0);
> +		if (err)
> +			goto free_vqs;
> +	}
>  
>  	/* Start using the new console output. */
>  	early_put_chars = NULL;
> @@ -1480,6 +1490,8 @@ free_vqs:
>  	vdev->config->del_vqs(vdev);
>  	kfree(portdev->in_vqs);
>  	kfree(portdev->out_vqs);
> +free_config:
> +	kfree(portdev->config);
>  free_chrdev:
>  	unregister_chrdev(portdev->chr_major, "virtio-portsdev");
>  free:
> @@ -1514,6 +1526,7 @@ static void virtcons_remove(struct virtio_device *vdev)
>  	vdev->config->del_vqs(vdev);
>  	kfree(portdev->in_vqs);
>  	kfree(portdev->out_vqs);
> +	kfree(portdev->config);
>  
>  	kfree(portdev);
>  }
> diff --git a/include/linux/virtio_console.h b/include/linux/virtio_console.h
> index ae4f039..287ee44 100644
> --- a/include/linux/virtio_console.h
> +++ b/include/linux/virtio_console.h
> @@ -14,15 +14,23 @@
>  #define VIRTIO_CONSOLE_F_SIZE	0	/* Does host provide console size? */
>  #define VIRTIO_CONSOLE_F_MULTIPORT 1	/* Does host provide multiple ports? */
>  
> +/*
> + * This is the config space shared between the Host and the Guest.
> + * The Host indicates to us the maximum number of ports this device
> + * can hold and a port map indicating which ports are active.
> + *
> + * The maximum number of ports is not a round number to prevent
> + * wastage of MSI vectors in case MSI is enabled for this device.

What does 'round number' mean in this context?

> + */
>  struct virtio_console_config {
>  	/* colums of the screens */
>  	__u16 cols;
>  	/* rows of the screens */
>  	__u16 rows;
> -	/* max. number of ports this device can hold */
> +	/* max. number of ports this device can hold. */
>  	__u32 max_nr_ports;
> -	/* number of ports added so far */
> -	__u32 nr_ports;
> +	/* locations of ports in use; variable-sized array */

It's in fact a bitmap, not an array, is it?

> +	__u32 ports_map[0 /* (max_nr_ports + 31) / 2 */];

The array is in the packed structure. So I am not sure
we can legally take a pointer to ports_map like this patch does in
multiple places above: if we do compiler might assume
alignment?


>  } __attribute__((packed));
>
Note that sizeof for variable sized structures is as far as I know a gcc
extension. You are supposed to use offsetof. And since packed  structure
is also an extension, we should be very careful about combining them
together.

In fact, virtio seems to overuse packed structures: we probably can
save a ton of code by carefully padding everything without using
packed attribute.

>  /*
> -- 
> 1.6.2.5

Re: [PATCH 0/6] virtio: console: Fixes, abi update

[Michael S. Tsirkin]

On Fri, Mar 19, 2010 at 05:36:42PM +0530, Amit Shah wrote:
> Hello,
> 
> These patches fix a couple of small issues: 
>  - generate a kobject change event so that udev is woken up on name
>    changes
>  - fix a crash after hot-unplug of the first console port and a
>    subsequent config update
> 
> But majorly, it reworks how ports are discovered: instead of numbering
> the ports individually in the host and the guest by just incrementing
> a number, we now switch to a bitmap in the config space exposed by the
> host to identify active ports. This lets us maintain the same
> numbering used by the host and also allows for hot-unplug via the
> config space. This is needed for proper migration support after
> several hot-plug/unplug operations.
> 
> I've tested these patches on my testsuite to catch any regression or
> correctness issues. I've also tested all the hotplug-related changes
> here.
> 
> These should go to 2.6.34, so that we don't push out a stable release
> with the older interface.
> 
> Michael, please forward these to Linus if
> everyone is OK with these.

I have some concerns with the new ABI.  I also expect Rusty to be back
in a couple of days, maybe he'll find some time to review the patches.
We'll also have to update the spec ...

Would you like me to queue up the first 2 patches meanwhile?

> I also have a git repo at
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/amit/vs-kernel.git master
> 
> if you prefer to pull the patches.
> Amit Shah (6):
>   virtio: console: Generate a kobject CHANGE event on adding 'name'
>     attribute
>   virtio: console: Check if port is valid in resize_console
>   virtio: console: Switch to using a port bitmap for port discovery
>   virtio: console: Separate out get_config in a separate function
>   virtio: console: Handle hot-plug/unplug config actions
>   virtio: console: Remove hot-unplug control message
> 
>  drivers/char/virtio_console.c  |  238 ++++++++++++++++++++++++----------------
>  include/linux/virtio_console.h |   15 ++-
>  2 files changed, 156 insertions(+), 97 deletions(-)

Re: [PATCH 3/6] virtio: console: Switch to using a port bitmap for port discovery

[Amit Shah]

On (Sun) Mar 21 2010 [13:29:45], Michael S. Tsirkin wrote:
> On Fri, Mar 19, 2010 at 05:36:45PM +0530, Amit Shah wrote:
> > Ports are now discovered by their location as given by host instead of
> > just incrementing a number and expecting the host and guest numbers stay
> > in sync. They are needed to be the same because the control messages
> > identify ports using the port id.
> > 
> > This is most beneficial to management software to properly place ports
> > at known ids so that the ids after hot-plug/unplug operations can be
> > controlled. This helps migration of guests after such hot-plug/unplug
> > operations.
> > 
> > The support for port hot-plug is removed in this commit, will be added
> > in the following commits.
> 
> It might be cleaner not to split it this way, merge the following
> commits into this one or split it in a different way.

Rusty in the past indicated he was OK with such a split since it
simplifies things and makes for easier review.

I'm fine with merging the next two patches in here too.

> > The config space is now a variable-sized array.
> 
> I think this last bit is problematic: we won't be able to add any more data if
> we have to, without a lot of complexity.

Adding new fields before the bitmap should be fine as long as we
discover features and fetch the config is the right order. If, however,
another bitmap has to be added, that'll surely be painful.

However, I don't see the need to add another bitmap for sure, and I
don't think we need more config variables too. However, we have the
control channel in case this has to be expanded.

>  Further, in the past we have
> also had problems running out of config space: see
> 3225beaba05d4f06087593f5e903ce867b6e118a.

How much config space is available? I guess there's enough for a ports
bitmap: without the ports, we're using 64 bits. And each port takes up
one bit. I guess we easily have 256 bits of space, so we can have 192
ports represented (at least) via the config space.

> There's also
> a comment in handle_control_message which suggests we
> might want a very large number of ports at some point,
> if we do using config space would be a mistake.

[Which comment?]

I can't certainly predict how many ports might be needed, but I think
we'll have other ways of communication if we need > 200 ports.

> It might be better to use a control vq for this, like virtio block ended
> up doing.  The comment in handle_control_message hints we don't want to
> pass port id in config space, but I am not sure why we can't pass it in
> message buffer.

I'm not sure which comment you're referring to really.

> > +static u32 find_next_bit_in_map(u32 *map)
> > +{
> > +	u32 port_bit;
> > +
> > +	port_bit = ffs(*map);
> > +	port_bit--; /* ffs returns bit location */
> > +
> > +	*map &= ~(1U << port_bit);
> 
> The above only works well if map is non-zero.  This happens to be the
> case the way we call it, but since this means the function is not
> generic, it might be better to opencode it to make it obvious.

You're right; when I first had a bitmap-based approach, I had a
find_next_active_port() and returned VIRTIO_CONSOLE_BAD_ID in case ffs
returned 0. This is a valid case and should be fixed. I'll send out a
v2.

> > +static u32 get_ports_map_size(u32 max_nr_ports)
> > +{
> > +	return sizeof(u32) * ((max_nr_ports + 31)/ 32);
> 
> DIV_ROUND_UP here and elsewhere?

Ah, yes, I'll use it.

> > -	for (i = 0; i < portdev->config.nr_ports; i++)
> > -		add_port(portdev, i);
> > +		for (i = 0; i < (portdev->config->max_nr_ports + 31) / 32; i++) {
> > +			u32 map, port_nr;
> > +
> > +			map = portdev->config->ports_map[i];
> > +			while (map) {
> > +				port_nr = find_next_bit_in_map(&map) + i * 32;
> > +
> > +				add_port(portdev, port_nr);
> > +				/*
> > +				 * FIXME: Send a notification to the
> > +				 * host about failed port add
> > +				 */
> 
> This FIXME is just pointing out an existing bug, correct?

Yes, a control message indicating to the host something went wrong would
be helpful for mgmt.

> > diff --git a/include/linux/virtio_console.h b/include/linux/virtio_console.h
> > index ae4f039..287ee44 100644
> > --- a/include/linux/virtio_console.h
> > +++ b/include/linux/virtio_console.h
> > @@ -14,15 +14,23 @@
> >  #define VIRTIO_CONSOLE_F_SIZE	0	/* Does host provide console size? */
> >  #define VIRTIO_CONSOLE_F_MULTIPORT 1	/* Does host provide multiple ports? */
> >  
> > +/*
> > + * This is the config space shared between the Host and the Guest.
> > + * The Host indicates to us the maximum number of ports this device
> > + * can hold and a port map indicating which ports are active.
> > + *
> > + * The maximum number of ports is not a round number to prevent
> > + * wastage of MSI vectors in case MSI is enabled for this device.
> 
> What does 'round number' mean in this context?

Meaning restricting the 'max_nr_ports' to multiples of 32.

> > + */
> >  struct virtio_console_config {
> >  	/* colums of the screens */
> >  	__u16 cols;
> >  	/* rows of the screens */
> >  	__u16 rows;
> > -	/* max. number of ports this device can hold */
> > +	/* max. number of ports this device can hold. */
> >  	__u32 max_nr_ports;
> > -	/* number of ports added so far */
> > -	__u32 nr_ports;
> > +	/* locations of ports in use; variable-sized array */
> 
> It's in fact a bitmap, not an array, is it?

hm, yeah; I'll update.

> > +	__u32 ports_map[0 /* (max_nr_ports + 31) / 2 */];
> 
> The array is in the packed structure. So I am not sure
> we can legally take a pointer to ports_map like this patch does in
> multiple places above: if we do compiler might assume
> alignment?

We always reference the elements in the ports_map[] in u32-sized
increments. Is there a problem?

> >  } __attribute__((packed));
> >
> Note that sizeof for variable sized structures is as far as I know a gcc
> extension. You are supposed to use offsetof. And since packed  structure
> is also an extension, we should be very careful about combining them
> together.

OK, I'll switch to offsetof(ports_map) where I use
sizeof(virtio_console_config) -- in the get_config() function.

> In fact, virtio seems to overuse packed structures: we probably can
> save a ton of code by carefully padding everything without using
> packed attribute.

Yeah; packing could be avoided. Maybe Rusty has an explanation for why
it was done this way.

		Amit

Re: [PATCH 3/6] virtio: console: Switch to using a port bitmap for port discovery

[Michael S. Tsirkin]

On Mon, Mar 22, 2010 at 09:34:01AM +0530, Amit Shah wrote:
> On (Sun) Mar 21 2010 [13:29:45], Michael S. Tsirkin wrote:
> > On Fri, Mar 19, 2010 at 05:36:45PM +0530, Amit Shah wrote:
> > > Ports are now discovered by their location as given by host instead of
> > > just incrementing a number and expecting the host and guest numbers stay
> > > in sync. They are needed to be the same because the control messages
> > > identify ports using the port id.
> > > 
> > > This is most beneficial to management software to properly place ports
> > > at known ids so that the ids after hot-plug/unplug operations can be
> > > controlled. This helps migration of guests after such hot-plug/unplug
> > > operations.
> > > 
> > > The support for port hot-plug is removed in this commit, will be added
> > > in the following commits.
> > 
> > It might be cleaner not to split it this way, merge the following
> > commits into this one or split it in a different way.
> 
> Rusty in the past indicated he was OK with such a split since it
> simplifies things and makes for easier review.
> 
> I'm fine with merging the next two patches in here too.
> 
> > > The config space is now a variable-sized array.
> > 
> > I think this last bit is problematic: we won't be able to add any more data if
> > we have to, without a lot of complexity.
> 
> Adding new fields before the bitmap should be fine as long as we
> discover features and fetch the config is the right order. If, however,
> another bitmap has to be added, that'll surely be painful.

Well, we'll need two structures old_config and new_config,
as opposed to simply extending the existing one.

> However, I don't see the need to add another bitmap for sure, and I
> don't think we need more config variables too. However, we have the
> control channel in case this has to be expanded.

How about using it right now?

> >  Further, in the past we have
> > also had problems running out of config space: see
> > 3225beaba05d4f06087593f5e903ce867b6e118a.
> 
> How much config space is available? I guess there's enough for a ports
> bitmap: without the ports, we're using 64 bits. And each port takes up
> one bit. I guess we easily have 256 bits of space, so we can have 192
> ports represented (at least) via the config space.

256 bytes but we are using config space for other things as well.

> > There's also
> > a comment in handle_control_message which suggests we
> > might want a very large number of ports at some point,
> > if we do using config space would be a mistake.
> 
> [Which comment?]
> 
> I can't certainly predict how many ports might be needed, but I think
> we'll have other ways of communication if we need > 200 ports.

If, say, 32 bytes are sufficient, let's just reserve a fixed size
array and everything will be simpler?

> > It might be better to use a control vq for this, like virtio block ended
> > up doing.  The comment in handle_control_message hints we don't want to
> > pass port id in config space, but I am not sure why we can't pass it in
> > message buffer.
> 
> I'm not sure which comment you're referring to really.

We have this:

+ * Hot unplug the port. We don't decrement nr_ports
+ * since we don't want to deal with extra complexities
+ * of using the lowest-available port id: We can just
+ * pick up the nr_ports number as the id and not have
+ * userspace send it to us. This helps us in two
+ * ways:
+ *
+ * - We don't need to have a 'port_id' field in the
+ * config space when a port is hot-added. This is a
+ * good thing as we might queue up multiple hotplug
+ * requests issued in our workqueue.
+ *
+ * - Another way to deal with this would have been to
+ * use a bitmap of the active ports and select the
+ * lowest non-active port from that map. That
+ * bloats the already tight config space and we
+ * would end up artificially limiting the
+ * max. number of ports to sizeof(bitmap). Right
+ * now we can support 2^32 ports (as the port id is
+ * stored in a u32 type).
+ *

Did you change your mind then?

> > > +static u32 find_next_bit_in_map(u32 *map)
> > > +{
> > > +	u32 port_bit;
> > > +
> > > +	port_bit = ffs(*map);
> > > +	port_bit--; /* ffs returns bit location */
> > > +
> > > +	*map &= ~(1U << port_bit);
> > 
> > The above only works well if map is non-zero.  This happens to be the
> > case the way we call it, but since this means the function is not
> > generic, it might be better to opencode it to make it obvious.
> 
> You're right; when I first had a bitmap-based approach, I had a
> find_next_active_port() and returned VIRTIO_CONSOLE_BAD_ID in case ffs
> returned 0. This is a valid case and should be fixed. I'll send out a
> v2.
> 
> > > +static u32 get_ports_map_size(u32 max_nr_ports)
> > > +{
> > > +	return sizeof(u32) * ((max_nr_ports + 31)/ 32);
> > 
> > DIV_ROUND_UP here and elsewhere?
> 
> Ah, yes, I'll use it.
> 
> > > -	for (i = 0; i < portdev->config.nr_ports; i++)
> > > -		add_port(portdev, i);
> > > +		for (i = 0; i < (portdev->config->max_nr_ports + 31) / 32; i++) {
> > > +			u32 map, port_nr;
> > > +
> > > +			map = portdev->config->ports_map[i];
> > > +			while (map) {
> > > +				port_nr = find_next_bit_in_map(&map) + i * 32;
> > > +
> > > +				add_port(portdev, port_nr);
> > > +				/*
> > > +				 * FIXME: Send a notification to the
> > > +				 * host about failed port add
> > > +				 */
> > 
> > This FIXME is just pointing out an existing bug, correct?
> 
> Yes, a control message indicating to the host something went wrong would
> be helpful for mgmt.
> 
> > > diff --git a/include/linux/virtio_console.h b/include/linux/virtio_console.h
> > > index ae4f039..287ee44 100644
> > > --- a/include/linux/virtio_console.h
> > > +++ b/include/linux/virtio_console.h
> > > @@ -14,15 +14,23 @@
> > >  #define VIRTIO_CONSOLE_F_SIZE	0	/* Does host provide console size? */
> > >  #define VIRTIO_CONSOLE_F_MULTIPORT 1	/* Does host provide multiple ports? */
> > >  
> > > +/*
> > > + * This is the config space shared between the Host and the Guest.
> > > + * The Host indicates to us the maximum number of ports this device
> > > + * can hold and a port map indicating which ports are active.
> > > + *
> > > + * The maximum number of ports is not a round number to prevent
> > > + * wastage of MSI vectors in case MSI is enabled for this device.
> > 
> > What does 'round number' mean in this context?
> 
> Meaning restricting the 'max_nr_ports' to multiples of 32.

It's an unusual use of the word :)

> > > + */
> > >  struct virtio_console_config {
> > >  	/* colums of the screens */
> > >  	__u16 cols;
> > >  	/* rows of the screens */
> > >  	__u16 rows;
> > > -	/* max. number of ports this device can hold */
> > > +	/* max. number of ports this device can hold. */
> > >  	__u32 max_nr_ports;
> > > -	/* number of ports added so far */
> > > -	__u32 nr_ports;
> > > +	/* locations of ports in use; variable-sized array */
> > 
> > It's in fact a bitmap, not an array, is it?
> 
> hm, yeah; I'll update.
> 
> > > +	__u32 ports_map[0 /* (max_nr_ports + 31) / 2 */];
> > 
> > The array is in the packed structure. So I am not sure
> > we can legally take a pointer to ports_map like this patch does in
> > multiple places above: if we do compiler might assume
> > alignment?
> 
> We always reference the elements in the ports_map[] in u32-sized
> increments. Is there a problem?

Hmm I'm not sure. Maybe not.

> > >  } __attribute__((packed));
> > >
> > Note that sizeof for variable sized structures is as far as I know a gcc
> > extension. You are supposed to use offsetof. And since packed  structure
> > is also an extension, we should be very careful about combining them
> > together.
> 
> OK, I'll switch to offsetof(ports_map) where I use
> sizeof(virtio_console_config) -- in the get_config() function.
> 
> > In fact, virtio seems to overuse packed structures: we probably can
> > save a ton of code by carefully padding everything without using
> > packed attribute.
> 
> Yeah; packing could be avoided. Maybe Rusty has an explanation for why
> it was done this way.
> 
> 		Amit

Re: [PATCH 3/6] virtio: console: Switch to using a port bitmap for port discovery

[Amit Shah]

On (Mon) Mar 22 2010 [10:53:36], Michael S. Tsirkin wrote:
> On Mon, Mar 22, 2010 at 09:34:01AM +0530, Amit Shah wrote:
> > On (Sun) Mar 21 2010 [13:29:45], Michael S. Tsirkin wrote:
> > > On Fri, Mar 19, 2010 at 05:36:45PM +0530, Amit Shah wrote:
> > > > Ports are now discovered by their location as given by host instead of
> > > > just incrementing a number and expecting the host and guest numbers stay
> > > > in sync. They are needed to be the same because the control messages
> > > > identify ports using the port id.
> > > > 
> > > > This is most beneficial to management software to properly place ports
> > > > at known ids so that the ids after hot-plug/unplug operations can be
> > > > controlled. This helps migration of guests after such hot-plug/unplug
> > > > operations.
> > > > 
> > > > The support for port hot-plug is removed in this commit, will be added
> > > > in the following commits.
> > > 
> > > It might be cleaner not to split it this way, merge the following
> > > commits into this one or split it in a different way.
> > 
> > Rusty in the past indicated he was OK with such a split since it
> > simplifies things and makes for easier review.
> > 
> > I'm fine with merging the next two patches in here too.
> > 
> > > > The config space is now a variable-sized array.
> > > 
> > > I think this last bit is problematic: we won't be able to add any more data if
> > > we have to, without a lot of complexity.
> > 
> > Adding new fields before the bitmap should be fine as long as we
> > discover features and fetch the config is the right order. If, however,
> > another bitmap has to be added, that'll surely be painful.
> 
> Well, we'll need two structures old_config and new_config,
> as opposed to simply extending the existing one.

True; we'd be adding fields before the ports_map. We anyway have to keep
older fields around for backward compat, so not much changes.

> > However, I don't see the need to add another bitmap for sure, and I
> > don't think we need more config variables too. However, we have the
> > control channel in case this has to be expanded.
> 
> How about using it right now?

Just that doing hot-plug/unplug via config updates seems better for some
reason.

> > >  Further, in the past we have
> > > also had problems running out of config space: see
> > > 3225beaba05d4f06087593f5e903ce867b6e118a.
> > 
> > How much config space is available? I guess there's enough for a ports
> > bitmap: without the ports, we're using 64 bits. And each port takes up
> > one bit. I guess we easily have 256 bits of space, so we can have 192
> > ports represented (at least) via the config space.
> 
> 256 bytes but we are using config space for other things as well.
> 
> > > There's also
> > > a comment in handle_control_message which suggests we
> > > might want a very large number of ports at some point,
> > > if we do using config space would be a mistake.
> > 
> > [Which comment?]
> > 
> > I can't certainly predict how many ports might be needed, but I think
> > we'll have other ways of communication if we need > 200 ports.
> 
> If, say, 32 bytes are sufficient, let's just reserve a fixed size
> array and everything will be simpler?

Yes, 32 bytes means 256 ports. Should be OK; if not, one could add more
such devices and get more ports. But if I have to choose between fixing
the number of ports in the config space vs using the control queue for
the bitmap, I'll go for the latter. That's assuming we really really
don't want to use the config space for storing the bitmap.

> > > It might be better to use a control vq for this, like virtio block ended
> > > up doing.  The comment in handle_control_message hints we don't want to
> > > pass port id in config space, but I am not sure why we can't pass it in
> > > message buffer.
> > 
> > I'm not sure which comment you're referring to really.
> 
> We have this:
> 
> + * Hot unplug the port. We don't decrement nr_ports
> + * since we don't want to deal with extra complexities
> + * of using the lowest-available port id: We can just
> + * pick up the nr_ports number as the id and not have
> + * userspace send it to us. This helps us in two
> + * ways:
> + *
> + * - We don't need to have a 'port_id' field in the
> + * config space when a port is hot-added. This is a
> + * good thing as we might queue up multiple hotplug
> + * requests issued in our workqueue.
> + *
> + * - Another way to deal with this would have been to
> + * use a bitmap of the active ports and select the
> + * lowest non-active port from that map. That
> + * bloats the already tight config space and we
> + * would end up artificially limiting the
> + * max. number of ports to sizeof(bitmap). Right
> + * now we can support 2^32 ports (as the port id is
> + * stored in a u32 type).
> + *
> 
> Did you change your mind then?

Ah, ok. This comment got removed by the last patch in this series.

Well right now the kernel can support 2^32 ports; but the downside of
doing it w/o bitmaps was a race in port number allocation between the
guest and the host. Without a bitmap, we could use a port id only once,
so after unplugging a port, a new port had to be on a new id.

With bitmaps, any free location can be re-used. So it's not all that bad
as that comment makes it sound. Also, as you suggest using the control
queue for bitmaps, we could have large bitmaps irrespective of the
config space limit.

> > > > +static u32 find_next_bit_in_map(u32 *map)
> > > > +{
> > > > +	u32 port_bit;
> > > > +
> > > > +	port_bit = ffs(*map);
> > > > +	port_bit--; /* ffs returns bit location */
> > > > +
> > > > +	*map &= ~(1U << port_bit);
> > > 
> > > The above only works well if map is non-zero.  This happens to be the
> > > case the way we call it, but since this means the function is not
> > > generic, it might be better to opencode it to make it obvious.
> > 
> > You're right; when I first had a bitmap-based approach, I had a
> > find_next_active_port() and returned VIRTIO_CONSOLE_BAD_ID in case ffs
> > returned 0. This is a valid case and should be fixed. I'll send out a
> > v2.

BTW I just added a comment to the function mentioning it's to be called
with 'map' non-zero. Should be sufficient for the current usage.

		Amit

Re: [PATCH 0/6] virtio: console: Fixes, abi update

[Amit Shah]

On (Sun) Mar 21 2010 [13:44:04], Michael S. Tsirkin wrote:
> 
> Would you like me to queue up the first 2 patches meanwhile?

Yes, that'll be great.

Thanks,

		Amit

Re: [PATCH 3/6] virtio: console: Switch to using a port bitmap for port discovery

[Michael S. Tsirkin]

On Mon, Mar 22, 2010 at 03:15:24PM +0530, Amit Shah wrote:
> On (Mon) Mar 22 2010 [10:53:36], Michael S. Tsirkin wrote:
> > On Mon, Mar 22, 2010 at 09:34:01AM +0530, Amit Shah wrote:
> > > On (Sun) Mar 21 2010 [13:29:45], Michael S. Tsirkin wrote:
> > > > On Fri, Mar 19, 2010 at 05:36:45PM +0530, Amit Shah wrote:
> > > > > Ports are now discovered by their location as given by host instead of
> > > > > just incrementing a number and expecting the host and guest numbers stay
> > > > > in sync. They are needed to be the same because the control messages
> > > > > identify ports using the port id.
> > > > > 
> > > > > This is most beneficial to management software to properly place ports
> > > > > at known ids so that the ids after hot-plug/unplug operations can be
> > > > > controlled. This helps migration of guests after such hot-plug/unplug
> > > > > operations.
> > > > > 
> > > > > The support for port hot-plug is removed in this commit, will be added
> > > > > in the following commits.
> > > > 
> > > > It might be cleaner not to split it this way, merge the following
> > > > commits into this one or split it in a different way.
> > > 
> > > Rusty in the past indicated he was OK with such a split since it
> > > simplifies things and makes for easier review.
> > > 
> > > I'm fine with merging the next two patches in here too.
> > > 
> > > > > The config space is now a variable-sized array.
> > > > 
> > > > I think this last bit is problematic: we won't be able to add any more data if
> > > > we have to, without a lot of complexity.
> > > 
> > > Adding new fields before the bitmap should be fine as long as we
> > > discover features and fetch the config is the right order. If, however,
> > > another bitmap has to be added, that'll surely be painful.
> > 
> > Well, we'll need two structures old_config and new_config,
> > as opposed to simply extending the existing one.
> 
> True; we'd be adding fields before the ports_map. We anyway have to keep
> older fields around for backward compat, so not much changes.
> 
> > > However, I don't see the need to add another bitmap for sure, and I
> > > don't think we need more config variables too. However, we have the
> > > control channel in case this has to be expanded.
> > 
> > How about using it right now?
> 
> Just that doing hot-plug/unplug via config updates seems better for some
> reason.
> 
> > > >  Further, in the past we have
> > > > also had problems running out of config space: see
> > > > 3225beaba05d4f06087593f5e903ce867b6e118a.
> > > 
> > > How much config space is available? I guess there's enough for a ports
> > > bitmap: without the ports, we're using 64 bits. And each port takes up
> > > one bit. I guess we easily have 256 bits of space, so we can have 192
> > > ports represented (at least) via the config space.
> > 
> > 256 bytes but we are using config space for other things as well.
> > 
> > > > There's also
> > > > a comment in handle_control_message which suggests we
> > > > might want a very large number of ports at some point,
> > > > if we do using config space would be a mistake.
> > > 
> > > [Which comment?]
> > > 
> > > I can't certainly predict how many ports might be needed, but I think
> > > we'll have other ways of communication if we need > 200 ports.
> > 
> > If, say, 32 bytes are sufficient, let's just reserve a fixed size
> > array and everything will be simpler?
> 
> Yes, 32 bytes means 256 ports. Should be OK; if not, one could add more
> such devices and get more ports. But if I have to choose between fixing
> the number of ports in the config space vs using the control queue for
> the bitmap, I'll go for the latter. That's assuming we really really
> don't want to use the config space for storing the bitmap.

I guess with control queue we also simply use message per port
state change - would that be simpler than a bitmap?

> > > > It might be better to use a control vq for this, like virtio block ended
> > > > up doing.  The comment in handle_control_message hints we don't want to
> > > > pass port id in config space, but I am not sure why we can't pass it in
> > > > message buffer.
> > > 
> > > I'm not sure which comment you're referring to really.
> > 
> > We have this:
> > 
> > + * Hot unplug the port. We don't decrement nr_ports
> > + * since we don't want to deal with extra complexities
> > + * of using the lowest-available port id: We can just
> > + * pick up the nr_ports number as the id and not have
> > + * userspace send it to us. This helps us in two
> > + * ways:
> > + *
> > + * - We don't need to have a 'port_id' field in the
> > + * config space when a port is hot-added. This is a
> > + * good thing as we might queue up multiple hotplug
> > + * requests issued in our workqueue.
> > + *
> > + * - Another way to deal with this would have been to
> > + * use a bitmap of the active ports and select the
> > + * lowest non-active port from that map. That
> > + * bloats the already tight config space and we
> > + * would end up artificially limiting the
> > + * max. number of ports to sizeof(bitmap). Right
> > + * now we can support 2^32 ports (as the port id is
> > + * stored in a u32 type).
> > + *
> > 
> > Did you change your mind then?
> 
> Ah, ok. This comment got removed by the last patch in this series.
> 
> Well right now the kernel can support 2^32 ports; but the downside of
> doing it w/o bitmaps was a race in port number allocation between the
> guest and the host. Without a bitmap, we could use a port id only once,
> so after unplugging a port, a new port had to be on a new id.
> 
> With bitmaps, any free location can be re-used. So it's not all that bad
> as that comment makes it sound. Also, as you suggest using the control
> queue for bitmaps, we could have large bitmaps irrespective of the
> config space limit.
> 
> > > > > +static u32 find_next_bit_in_map(u32 *map)
> > > > > +{
> > > > > +	u32 port_bit;
> > > > > +
> > > > > +	port_bit = ffs(*map);
> > > > > +	port_bit--; /* ffs returns bit location */


simple u32 port_bit = ffs(*map) - 1; not clear enough?

> > > > > +
> > > > > +	*map &= ~(1U << port_bit);
> > > > 
> > > > The above only works well if map is non-zero.  This happens to be the
> > > > case the way we call it, but since this means the function is not
> > > > generic, it might be better to opencode it to make it obvious.
> > > 
> > > You're right; when I first had a bitmap-based approach, I had a
> > > find_next_active_port() and returned VIRTIO_CONSOLE_BAD_ID in case ffs
> > > returned 0. This is a valid case and should be fixed. I'll send out a
> > > v2.
> 
> BTW I just added a comment to the function mentioning it's to be called
> with 'map' non-zero. Should be sufficient for the current usage.
> 
> 		Amit

Or just opencode it, it's two lines.

-- 
MST

Re: [PATCH 3/6] virtio: console: Switch to using a port bitmap for port discovery

[Amit Shah]

On (Mon) Mar 22 2010 [14:16:32], Michael S. Tsirkin wrote:
> > > > I can't certainly predict how many ports might be needed, but I think
> > > > we'll have other ways of communication if we need > 200 ports.
> > > 
> > > If, say, 32 bytes are sufficient, let's just reserve a fixed size
> > > array and everything will be simpler?
> > 
> > Yes, 32 bytes means 256 ports. Should be OK; if not, one could add more
> > such devices and get more ports. But if I have to choose between fixing
> > the number of ports in the config space vs using the control queue for
> > the bitmap, I'll go for the latter. That's assuming we really really
> > don't want to use the config space for storing the bitmap.
> 
> I guess with control queue we also simply use message per port
> state change - would that be simpler than a bitmap?

Yes, that's how hot-unplug is done currently (before this patchset).
I'll cook up a similar version like you suggest for adding ports as
well.

> > > > > > +	port_bit = ffs(*map);
> > > > > > +	port_bit--; /* ffs returns bit location */
> 
> simple u32 port_bit = ffs(*map) - 1; not clear enough?

Well yeah; I'll just open-code it.

		Amit
-- 
http://log.amitshah.net/