Linux Virtualization - Nov 2008 - [RFC/PATCH 0/2] virtio console resize support

Hello Rusty,

this patch set adds virtio console resize support.
Patch 1 implements the necessary changes in virtio_console
Patch 2 implements the config_changed callback for the s390 virtio transport

Patch 1 needs an additional patch against hvc_console 
( http://lkml.org/lkml/2008/11/18/121 )
to properly set the initial size.

If you are ok with these patches, can they go into linux-next via your tree?

An lguest implementation for the initial console size should be trivial. 
SIGWINCH would require the config_changed implementation which is currently 
in your tree.

Thanks

From: Christian Borntraeger <borntraeger at de.ibm.com>

this patch uses the new hvc callback hvc_resize to set the window site 
which allows to change the tty size of hvc_console via a hvc_resize 
function.

I have added a new feature bit VIRTIO_CONSOLE_F_SIZE. The driver will 
change the window size on tty open and via the config_changed callback
of the transport. Currently lguest and kvm_s390 have not implemented this
callback, but the callback can be implemented at a later point in time.

Signed-off-by: Christian Borntraeger <borntraeger at de.ibm.com>
---
 drivers/char/virtio_console.c  |   30 +++++++++++++++++++++++++++++-
 include/linux/virtio_console.h |   11 +++++++++++
 2 files changed, 40 insertions(+), 1 deletion(-)

Index: kvm/drivers/char/virtio_console.c
==================================================================---
kvm.orig/drivers/char/virtio_console.c
+++ kvm/drivers/char/virtio_console.c
@@ -138,12 +138,33 @@ int __init virtio_cons_early_init(int (*
 }
 
 /*
+ * virtio console configuration. This supports:
+ * - console resize
+ */
+static void virtcons_apply_config(struct virtio_device *dev)
+{
+	struct winsize ws;
+
+	if (virtio_has_feature(dev, VIRTIO_CONSOLE_F_SIZE)) {
+		dev->config->get(dev,
+				 offsetof(struct virtio_console_config, cols),
+				 &ws.ws_col, sizeof(u16));
+		dev->config->get(dev,
+				 offsetof(struct virtio_console_config, rows),
+				 &ws.ws_row, sizeof(u16));
+		hvc_resize(hvc, ws);
+	}
+}
+
+/*
  * we support only one console, the hvc struct is a global var
- * There is no need to do anything
+ * We set the configuration at this point, since we now have a tty
  */
 static int notifier_add_vio(struct hvc_struct *hp, int data)
 {
 	hp->irq_requested = 1;
+	virtcons_apply_config(vdev);
+
 	return 0;
 }
 
@@ -234,11 +255,18 @@ static struct virtio_device_id id_table[
 	{ 0 },
 };
 
+static unsigned int features[] = {
+	VIRTIO_CONSOLE_F_SIZE,
+};
+
 static struct virtio_driver virtio_console = {
+	.feature_table = features,
+	.feature_table_size = ARRAY_SIZE(features),
 	.driver.name =	KBUILD_MODNAME,
 	.driver.owner =	THIS_MODULE,
 	.id_table =	id_table,
 	.probe =	virtcons_probe,
+	.config_changed = virtcons_apply_config,
 };
 
 static int __init init(void)
Index: kvm/include/linux/virtio_console.h
==================================================================---
kvm.orig/include/linux/virtio_console.h
+++ kvm/include/linux/virtio_console.h
@@ -7,6 +7,17 @@
 /* The ID for virtio console */
 #define VIRTIO_ID_CONSOLE	3
 
+/* Feature bits */
+#define VIRTIO_CONSOLE_F_SIZE	0	/* Does host provide console size? */
+
+struct virtio_console_config {
+	/* colums of the screens */
+	__u16 cols;
+	/* rows of the screens */
+	__u16 rows;
+} __attribute__((packed));
+
+
 #ifdef __KERNEL__
 int __init virtio_cons_early_init(int (*put_chars)(u32, const char *, int));
 #endif /* __KERNEL__ */

From: Christian Borntraeger <borntraeger at de.ibm.com>

This patch implements config_changed for the s390 virtio transport.

We use the least significant bit of the interrupt parameter field
to decide, if this interrupt should call the virtio virtqueue callback
or the config_changed callback.

This method is compatible with old host and guest code. Old 64 bit guests
will not check the bit and trigger a harmless additional vring_interrupt
call. Old host code will never set this bit, this is also safe.

I dont know if we will implement 31bit guest support, but this interface 
is also 31-bit safe

Signed-off-by: Christian Borntraeger <borntraeger at de.ibm.com>
---
 drivers/s390/kvm/kvm_virtio.c |   22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

Index: kvm/drivers/s390/kvm/kvm_virtio.c
==================================================================---
kvm.orig/drivers/s390/kvm/kvm_virtio.c
+++ kvm/drivers/s390/kvm/kvm_virtio.c
@@ -295,13 +295,29 @@ static void scan_devices(void)
  */
 static void kvm_extint_handler(u16 code)
 {
-	void *data = (void *) *(long *) __LC_PFAULT_INTPARM;
-	u16 subcode = S390_lowcore.cpu_addr;
+	struct virtqueue *vq;
+	u16 subcode;
+	int config_changed;
 
+ 	subcode = S390_lowcore.cpu_addr;
 	if ((subcode & 0xff00) != VIRTIO_SUBCODE_64)
 		return;
 
-	vring_interrupt(0, data);
+	/* The LSB is overloaded, we have to mask it */
+	vq = (struct virtqueue *) ((*(long *) __LC_PFAULT_INTPARM) & ~1UL);
+
+	/* We use the LSB of extparam, to decide, if this interrupt is a config
+	 * change or a "standard" interrupt */
+	config_changed =  (*(long *)  __LC_PFAULT_INTPARM & 1);
+
+	if (config_changed) {
+		struct virtio_driver *drv;
+		drv = container_of(vq->vdev->dev.driver,
+				   struct virtio_driver, driver);
+		if (drv->config_changed)
+			drv->config_changed(vq->vdev);
+	} else
+		vring_interrupt(0, vq);
 }
 
 /*

On Tuesday 18 November 2008 22:17:41 Christian Borntraeger
wrote:
> From: Christian Borntraeger <borntraeger at de.ibm.com>
>
> This patch implements config_changed for the s390 virtio transport.
Thanks, both applied.

Rusty.

Hmpf. There was a thinko. Overloading the LSB of the token address does not 
work with old kernels and new userspace on 64bit. My test did not trigger 
because I only tested with virtio_console and my userspace checks for the 
feature bit. 

Can you replace the kvm-s390 patch with this one?
It basically moves the "is a config change bit" from LC_PFAULT_INTPARM
to
LC_EXT_PARAMS. The content of both defined locations can be defined by the 
transport. On 31bit both locations are identical.

This gives us:
64bit:
LC_PFAULT_INTPARM	0x11b8	(8Byte)	token
LC_EXT_PARAMS		0x80	(4Byte) unused->changebit
LSB of token is not overloaded, change is compatible.

31bit:
LC_PFAULT_INTPARM	0x80	(4Byte) token + changebit
LC_EXT_PARAMS		0x80	(4byte) token + changebit
LSB of token is overloaded, but 31bit transport does not exist yet.
No need to worry about compatibility.

-

[PATCH 2/2] kvm-s390: implement config_changed for virtio on s390

From: Christian Borntraeger <borntraeger at de.ibm.com>

This patch implements config_changed for the s390 virtio transport.

We use the least significant bit of the interrupt parameter field
to decide, if this interrupt should call the virtio virtqueue callback
or the config_changed callback.

This method is compatible with old host and guest code. Old 64 bit guests
will not check the bit and trigger a harmless additional vring_interrupt
call. Old host code will never set this bit, this is also safe.

This patch also takes care of a potential future 31 bit virtio transport
for s390. On 31 bit _LC_PFAULT_INTPARM and __LC_EXT_PARAMS are identical.
We exploit the alignment of the token and fold the change bit into the
lsb of the token itself.


Signed-off-by: Christian Borntraeger <borntraeger at de.ibm.com>
---
 drivers/s390/kvm/kvm_virtio.c |   22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

Index: kvm/drivers/s390/kvm/kvm_virtio.c
==================================================================---
kvm.orig/drivers/s390/kvm/kvm_virtio.c
+++ kvm/drivers/s390/kvm/kvm_virtio.c
@@ -295,13 +295,29 @@ static void scan_devices(void)
  */
 static void kvm_extint_handler(u16 code)
 {
-	void *data = (void *) *(long *) __LC_PFAULT_INTPARM;
-	u16 subcode = S390_lowcore.cpu_addr;
+	struct virtqueue *vq;
+	u16 subcode;
+	int config_changed;
 
+ 	subcode = S390_lowcore.cpu_addr;
 	if ((subcode & 0xff00) != VIRTIO_SUBCODE_64)
 		return;
 
-	vring_interrupt(0, data);
+	/* The LSB might be overloaded, we have to mask it */
+	vq = (struct virtqueue *) ((*(long *) __LC_PFAULT_INTPARM) & ~1UL);
+
+	/* We use the LSB of extparam, to decide, if this interrupt is a config
+	 * change or a "standard" interrupt */
+	config_changed =  (*(int *)  __LC_EXT_PARAMS & 1);
+
+	if (config_changed) {
+		struct virtio_driver *drv;
+		drv = container_of(vq->vdev->dev.driver,
+				   struct virtio_driver, driver);
+		if (drv->config_changed)
+			drv->config_changed(vq->vdev);
+	} else
+		vring_interrupt(0, vq);
 }
 
 /*