Xen devel - Mar 2012 - [PATCH] xen/xenbus: Add quirk to deal with misconfigured backends.

A rather annoying and common case is when booting a PVonHVM guest
and exposing the PV KBD and PV VFB - as both of those do not
make any sense. The HVM guest is using the VGA driver and the emulated
keyboard for this. So we provide a very basic quirk framework
(can be expanded in the future) to not wait for 6 minutes for those devices
to initialize - they never wont.

To trigger this, put this in your guest config:

vfb = [ ''vnc=1, vnclisten=0.0.0.0 ,vncunused=1'']

instead of this:
vnc=1
vnclisten="0.0.0.0"

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/xen/xenbus/xenbus_probe_frontend.c |   30 ++++++++++++++++++++++++++++
 1 files changed, 30 insertions(+), 0 deletions(-)

diff --git a/drivers/xen/xenbus/xenbus_probe_frontend.c
b/drivers/xen/xenbus/xenbus_probe_frontend.c
index f20c5f1..80325cf 100644
--- a/drivers/xen/xenbus/xenbus_probe_frontend.c
+++ b/drivers/xen/xenbus/xenbus_probe_frontend.c
@@ -134,7 +134,31 @@ static int read_backend_details(struct xenbus_device
*xendev)
 {
 	return xenbus_read_otherend_details(xendev, "backend-id",
"backend");
 }
+static bool skip_if_quirk(struct device *dev, void *data)
+{
+	struct xenbus_device *xendev = to_xenbus_device(dev);
+	struct device_driver *drv = data;
+
+	/* Is this operation limited to a particular driver? */
+	if (drv && (dev->driver != drv))
+		return false;
+
+	if (xen_pv_domain())
+		return false;
+
+	/* With older QEMU, for PVonHVM guests the guest config files could
+	 * contain: vfb = [ ''vnc=1, vnclisten=0.0.0.0 ,vncunused=1'']
+	 * which is nonsensical as there is no PV FB (or PV KBD) when
+	 * running as HVM guest. */
 
+	if ((strncmp(xendev->nodename, "device/vkbd", 11) == 0))
+		return true;
+
+	if ((strncmp(xendev->nodename, "device/vfb", 10) == 0))
+		return true;
+
+	return false;
+}
 static int is_device_connecting(struct device *dev, void *data)
 {
 	struct xenbus_device *xendev = to_xenbus_device(dev);
@@ -153,6 +177,12 @@ static int is_device_connecting(struct device *dev, void
*data)
 		return 0;
 
 	xendrv = to_xenbus_driver(dev->driver);
+
+	if (skip_if_quirk(dev, data)) {
+		printk(KERN_INFO "XENBUS: (quirk) Skipping : %s\n",
+		       xendev->nodename);
+		return 0;
+	}
 	return (xendev->state < XenbusStateConnected ||
 		(xendev->state == XenbusStateConnected &&
 		 xendrv->is_ready && !xendrv->is_ready(xendev)));
-- 
1.7.7.5

On Thu, 22 Mar 2012, Konrad Rzeszutek Wilk wrote:
> A rather annoying and common case is when booting a PVonHVM guest
> and exposing the PV KBD and PV VFB - as both of those do not
> make any sense. The HVM guest is using the VGA driver and the emulated
> keyboard for this. So we provide a very basic quirk framework
> (can be expanded in the future) to not wait for 6 minutes for those devices
> to initialize - they never wont.
> 
> To trigger this, put this in your guest config:
> 
> vfb = [ ''vnc=1, vnclisten=0.0.0.0 ,vncunused=1'']
> 
> instead of this:
> vnc=1
> vnclisten="0.0.0.0"
While I do understand the issue you are trying to solve, it actually
makes sense to have PV KBD (and PV VFB maybe in the future) in a PVonHVM
guest. In particular PV KVB is already enabled in upstream QEMU for
PVonHVM guests because it allows users to have a keyboard and mouse
without USB emulation, that requires lots of wakes up in QEMU.

Maybe we could just reduce the timeout in general for all the PV
devices? After all, why are we waiting 6 minutes? I could understand 6
seconds, but 6 minutes seem really too much.

On Thu, 2012-03-22 at 10:44 +0000, Stefano Stabellini
wrote:
> On Thu, 22 Mar 2012, Konrad Rzeszutek Wilk wrote:
> > A rather annoying and common case is when booting a PVonHVM guest
> > and exposing the PV KBD and PV VFB - as both of those do not
> > make any sense. The HVM guest is using the VGA driver and the emulated
> > keyboard for this. So we provide a very basic quirk framework
> > (can be expanded in the future) to not wait for 6 minutes for those
devices
> > to initialize - they never wont.
> > 
> > To trigger this, put this in your guest config:
> > 
> > vfb = [ ''vnc=1, vnclisten=0.0.0.0 ,vncunused=1'']
> > 
> > instead of this:
> > vnc=1
> > vnclisten="0.0.0.0"
> 
> While I do understand the issue you are trying to solve, it actually
> makes sense to have PV KBD (and PV VFB maybe in the future) in a PVonHVM
> guest. In particular PV KVB is already enabled in upstream QEMU for
> PVonHVM guests because it allows users to have a keyboard and mouse
> without USB emulation, that requires lots of wakes up in QEMU.
> 
> Maybe we could just reduce the timeout in general for all the PV
> devices? After all, why are we waiting 6 minutes? I could understand 6
> seconds, but 6 minutes seem really too much.
This was increased based on empirical evidence, way back (circa
150:09c88868e344 in linux-2.6.18-xen.hg)

It really can happen when starting lots of guests on a heavily loaded
dom0 that you timeout when connecting devices, at which point the guest
fails to boot if it happened to contain the root filesystem.

Maybe a halfway house would be to wait a the longer time for more
critical devices (like disks and nics)?

Ian.

On Thu, Mar 22, 2012 at 10:52:26AM +0000, Ian Campbell
wrote:
> On Thu, 2012-03-22 at 10:44 +0000, Stefano Stabellini wrote:
> > On Thu, 22 Mar 2012, Konrad Rzeszutek Wilk wrote:
> > > A rather annoying and common case is when booting a PVonHVM guest
> > > and exposing the PV KBD and PV VFB - as both of those do not
> > > make any sense. The HVM guest is using the VGA driver and the
emulated
> > > keyboard for this. So we provide a very basic quirk framework
> > > (can be expanded in the future) to not wait for 6 minutes for
those devices
> > > to initialize - they never wont.
> > > 
> > > To trigger this, put this in your guest config:
> > > 
> > > vfb = [ ''vnc=1, vnclisten=0.0.0.0
,vncunused=1'']
> > > 
> > > instead of this:
> > > vnc=1
> > > vnclisten="0.0.0.0"
> > 
> > While I do understand the issue you are trying to solve, it actually
> > makes sense to have PV KBD (and PV VFB maybe in the future) in a
PVonHVM
> > guest. In particular PV KVB is already enabled in upstream QEMU for
> > PVonHVM guests because it allows users to have a keyboard and mouse
How about looking for a particular Xen version? The patch
could check for anything less than 4.2 (does 4.2 use that version of
QEMU that has this implemented?). I can''t find any way to get the QEMU
version from within the guest - DMI reports:

DMI: Xen HVM domU, BIOS 4.1-120322 03/22/2012
> > without USB emulation, that requires lots of wakes up in QEMU.
> > 
> > Maybe we could just reduce the timeout in general for all the PV
> > devices? After all, why are we waiting 6 minutes? I could understand 6
> > seconds, but 6 minutes seem really too much.
> 
> This was increased based on empirical evidence, way back (circa
> 150:09c88868e344 in linux-2.6.18-xen.hg)
> 
> It really can happen when starting lots of guests on a heavily loaded
> dom0 that you timeout when connecting devices, at which point the guest
> fails to boot if it happened to contain the root filesystem.
> 
> Maybe a halfway house would be to wait a the longer time for more
> critical devices (like disks and nics)?
> 
> Ian.
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

On Thu, 22 Mar 2012, Konrad Rzeszutek Wilk wrote:
> On Thu, Mar 22, 2012 at 10:52:26AM +0000, Ian Campbell wrote:
> > On Thu, 2012-03-22 at 10:44 +0000, Stefano Stabellini wrote:
> > > On Thu, 22 Mar 2012, Konrad Rzeszutek Wilk wrote:
> > > > A rather annoying and common case is when booting a PVonHVM
guest
> > > > and exposing the PV KBD and PV VFB - as both of those do not
> > > > make any sense. The HVM guest is using the VGA driver and
the emulated
> > > > keyboard for this. So we provide a very basic quirk
framework
> > > > (can be expanded in the future) to not wait for 6 minutes
for those devices
> > > > to initialize - they never wont.
> > > > 
> > > > To trigger this, put this in your guest config:
> > > > 
> > > > vfb = [ ''vnc=1, vnclisten=0.0.0.0
,vncunused=1'']
> > > > 
> > > > instead of this:
> > > > vnc=1
> > > > vnclisten="0.0.0.0"
> > > 
> > > While I do understand the issue you are trying to solve, it
actually
> > > makes sense to have PV KBD (and PV VFB maybe in the future) in a
PVonHVM
> > > guest. In particular PV KVB is already enabled in upstream QEMU
for
> > > PVonHVM guests because it allows users to have a keyboard and
mouse
> 
> How about looking for a particular Xen version? The patch
> could check for anything less than 4.2 (does 4.2 use that version of
> QEMU that has this implemented?). I can''t find any way to get the
QEMU
> version from within the guest - DMI reports:
> 
> DMI: Xen HVM domU, BIOS 4.1-120322 03/22/2012
I am not a great fun of enabling/disabling features depending on the Xen
version. What happens for example if a Linux distro decided to patch
QEMU 4.1 to enable PV KBD for PVonHVM guests?
I realize that it is unlikely but it is possible.
Maybe we can reduce the timeout for non essential devices (everything
expect disk, network and console) like Ian suggested?

On Thu, Mar 22, 2012 at 03:16:43PM +0000, Stefano Stabellini
wrote:
> On Thu, 22 Mar 2012, Konrad Rzeszutek Wilk wrote:
> > On Thu, Mar 22, 2012 at 10:52:26AM +0000, Ian Campbell wrote:
> > > On Thu, 2012-03-22 at 10:44 +0000, Stefano Stabellini wrote:
> > > > On Thu, 22 Mar 2012, Konrad Rzeszutek Wilk wrote:
> > > > > A rather annoying and common case is when booting a
PVonHVM guest
> > > > > and exposing the PV KBD and PV VFB - as both of those
do not
> > > > > make any sense. The HVM guest is using the VGA driver
and the emulated
> > > > > keyboard for this. So we provide a very basic quirk
framework
> > > > > (can be expanded in the future) to not wait for 6
minutes for those devices
> > > > > to initialize - they never wont.
> > > > > 
> > > > > To trigger this, put this in your guest config:
> > > > > 
> > > > > vfb = [ ''vnc=1, vnclisten=0.0.0.0
,vncunused=1'']
> > > > > 
> > > > > instead of this:
> > > > > vnc=1
> > > > > vnclisten="0.0.0.0"
> > > > 
> > > > While I do understand the issue you are trying to solve, it
actually
> > > > makes sense to have PV KBD (and PV VFB maybe in the future)
in a PVonHVM
> > > > guest. In particular PV KVB is already enabled in upstream
QEMU for
> > > > PVonHVM guests because it allows users to have a keyboard
and mouse
> > 
> > How about looking for a particular Xen version? The patch
> > could check for anything less than 4.2 (does 4.2 use that version of
> > QEMU that has this implemented?). I can''t find any way to get
the QEMU
> > version from within the guest - DMI reports:
> > 
> > DMI: Xen HVM domU, BIOS 4.1-120322 03/22/2012
> 
> I am not a great fun of enabling/disabling features depending on the Xen
> version. What happens for example if a Linux distro decided to patch
> QEMU 4.1 to enable PV KBD for PVonHVM guests?
> I realize that it is unlikely but it is possible.
> Maybe we can reduce the timeout for non essential devices (everything
> expect disk, network and console) like Ian suggested?
Please take a look:

From 85c9c09c69e3b4a277b2c930ca0a295f5bacc696 Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date: Wed, 28 Mar 2012 14:55:45 -0400
Subject: [PATCH] xen/xenbus: Add quirk to deal with misconfigured backends.

A rather annoying and common case is when booting a PVonHVM guest
and exposing the PV KBD and PV VFB - as both of those do not
make any sense. The HVM guest is using the VGA driver and the emulated
keyboard for this (in Xen 4.2 and lower). We provide a very basic quirk
framework to not wait for 6 minutes for those devices to initialize.

To trigger this, put this in your guest config:

vfb = [ ''vnc=1, vnclisten=0.0.0.0 ,vncunused=1'']

instead of this:
vnc=1
vnclisten="0.0.0.0"

Note: The upstream QEMU can do PV KBD, so we can''t just outright ignore
the keyboard driver. Instead we change the timeout for that particular
device to be much less.

[v1: Redo with timeout per Ian and Stefano suggestion]
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/xen/xenbus/xenbus_probe_frontend.c |   39 ++++++++++++++++++++++++++-
 1 files changed, 37 insertions(+), 2 deletions(-)

diff --git a/drivers/xen/xenbus/xenbus_probe_frontend.c
b/drivers/xen/xenbus/xenbus_probe_frontend.c
index f20c5f1..09da9cf 100644
--- a/drivers/xen/xenbus/xenbus_probe_frontend.c
+++ b/drivers/xen/xenbus/xenbus_probe_frontend.c
@@ -157,7 +157,40 @@ static int is_device_connecting(struct device *dev, void
*data)
 		(xendev->state == XenbusStateConnected &&
 		 xendrv->is_ready && !xendrv->is_ready(xendev)));
 }
+static int check_for_quirks(struct device *dev, void *data)
+{
+	struct xenbus_device *xendev = to_xenbus_device(dev);
+	struct device_driver *drv = data;
+
+	/* Is this operation limited to a particular driver? */
+	if (drv && (dev->driver != drv))
+		return false;
+
+	if (xen_pv_domain())
+		return false;
 
+	/* With older QEMU, for PVonHVM guests the guest config files could
+	 * contain: vfb = [ ''vnc=1, vnclisten=0.0.0.0 ,vncunused=1'']
+	 * which is nonsensical as there is no PV FB (or PV KBD) when
+	 * running as HVM guest - at least with Xen 4.1 and Xen 4.2. */
+
+	if ((strncmp(xendev->nodename, "device/vkbd", 11) == 0))
+		return true;
+
+	if ((strncmp(xendev->nodename, "device/vfb", 10) == 0))
+		return true;
+
+	return false;
+}
+#define QUIRKS_TIMEOUT		30
+#define DEFAULT_TIMEOUT		300
+static unsigned int check_for_timeout_quirks(struct device_driver *drv,
unsigned int timeout)
+{
+	if (bus_for_each_dev(&xenbus_frontend.bus, NULL, drv,
+				check_for_quirks))
+		return QUIRKS_TIMEOUT;
+	return timeout;
+}
 static int exists_connecting_device(struct device_driver *drv)
 {
 	return bus_for_each_dev(&xenbus_frontend.bus, NULL, drv,
@@ -211,18 +244,20 @@ static void wait_for_devices(struct xenbus_driver *xendrv)
 	unsigned long start = jiffies;
 	struct device_driver *drv = xendrv ? &xendrv->driver : NULL;
 	unsigned int seconds_waited = 0;
+	unsigned int max_delay = DEFAULT_TIMEOUT;
 
 	if (!ready_to_wait_for_devices || !xen_domain())
 		return;
 
+	max_delay = min(check_for_timeout_quirks(drv, max_delay), max_delay);
 	while (exists_connecting_device(drv)) {
 		if (time_after(jiffies, start + (seconds_waited+5)*HZ)) {
 			if (!seconds_waited)
 				printk(KERN_WARNING "XENBUS: Waiting for "
 				       "devices to initialise: ");
 			seconds_waited += 5;
-			printk("%us...", 300 - seconds_waited);
-			if (seconds_waited == 300)
+			printk("%us...", max_delay - seconds_waited);
+			if (seconds_waited == max_delay)
 				break;
 		}
 
-- 
1.7.7.5

On Wed, 28 Mar 2012, Konrad Rzeszutek Wilk wrote:
> Please take a look:
> 
> >From 85c9c09c69e3b4a277b2c930ca0a295f5bacc696 Mon Sep 17 00:00:00 2001
> From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Date: Wed, 28 Mar 2012 14:55:45 -0400
> Subject: [PATCH] xen/xenbus: Add quirk to deal with misconfigured backends.
> 
> A rather annoying and common case is when booting a PVonHVM guest
> and exposing the PV KBD and PV VFB - as both of those do not
> make any sense. The HVM guest is using the VGA driver and the emulated
> keyboard for this (in Xen 4.2 and lower). We provide a very basic quirk
> framework to not wait for 6 minutes for those devices to initialize.
> 
> To trigger this, put this in your guest config:
> 
> vfb = [ ''vnc=1, vnclisten=0.0.0.0 ,vncunused=1'']
> 
> instead of this:
> vnc=1
> vnclisten="0.0.0.0"
> 
> Note: The upstream QEMU can do PV KBD, so we can''t just outright
ignore
> the keyboard driver. Instead we change the timeout for that particular
> device to be much less.
> 
> [v1: Redo with timeout per Ian and Stefano suggestion]
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  drivers/xen/xenbus/xenbus_probe_frontend.c |   39
++++++++++++++++++++++++++-
>  1 files changed, 37 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/xen/xenbus/xenbus_probe_frontend.c
b/drivers/xen/xenbus/xenbus_probe_frontend.c
> index f20c5f1..09da9cf 100644
> --- a/drivers/xen/xenbus/xenbus_probe_frontend.c
> +++ b/drivers/xen/xenbus/xenbus_probe_frontend.c
> @@ -157,7 +157,40 @@ static int is_device_connecting(struct device *dev,
void *data)
>  		(xendev->state == XenbusStateConnected &&
>  		 xendrv->is_ready && !xendrv->is_ready(xendev)));
>  }
> +static int check_for_quirks(struct device *dev, void *data)
> +{
> +	struct xenbus_device *xendev = to_xenbus_device(dev);
> +	struct device_driver *drv = data;
> +
> +	/* Is this operation limited to a particular driver? */
> +	if (drv && (dev->driver != drv))
> +		return false;
> +
> +	if (xen_pv_domain())
> +		return false;
>  
> +	/* With older QEMU, for PVonHVM guests the guest config files could
> +	 * contain: vfb = [ ''vnc=1, vnclisten=0.0.0.0
,vncunused=1'']
> +	 * which is nonsensical as there is no PV FB (or PV KBD) when
> +	 * running as HVM guest - at least with Xen 4.1 and Xen 4.2. */
> +
> +	if ((strncmp(xendev->nodename, "device/vkbd", 11) == 0))
> +		return true;
> +
> +	if ((strncmp(xendev->nodename, "device/vfb", 10) == 0))
> +		return true;
> +
> +	return false;
> +}
> +#define QUIRKS_TIMEOUT		30
> +#define DEFAULT_TIMEOUT		300
> +static unsigned int check_for_timeout_quirks(struct device_driver *drv,
unsigned int timeout)
> +{
> +	if (bus_for_each_dev(&xenbus_frontend.bus, NULL, drv,
> +				check_for_quirks))
> +		return QUIRKS_TIMEOUT;
> +	return timeout;
> +}
>  static int exists_connecting_device(struct device_driver *drv)
>  {
>  	return bus_for_each_dev(&xenbus_frontend.bus, NULL, drv,
> @@ -211,18 +244,20 @@ static void wait_for_devices(struct xenbus_driver
*xendrv)
>  	unsigned long start = jiffies;
>  	struct device_driver *drv = xendrv ? &xendrv->driver : NULL;
>  	unsigned int seconds_waited = 0;
> +	unsigned int max_delay = DEFAULT_TIMEOUT;
>  
>  	if (!ready_to_wait_for_devices || !xen_domain())
>  		return;
>  
> +	max_delay = min(check_for_timeout_quirks(drv, max_delay), max_delay);
>  	while (exists_connecting_device(drv)) {
>  		if (time_after(jiffies, start + (seconds_waited+5)*HZ)) {
>  			if (!seconds_waited)
>  				printk(KERN_WARNING "XENBUS: Waiting for "
>  				       "devices to initialise: ");
>  			seconds_waited += 5;
> -			printk("%us...", 300 - seconds_waited);
> -			if (seconds_waited == 300)
> +			printk("%us...", max_delay - seconds_waited);
> +			if (seconds_waited == max_delay)
>  				break;
>  		}
This patch would reduce the timeout to 30 in case the guest is PV on HVM
and vkbd and/or vfb are present.
It is an improvement over the current situation but it doesn''t directly
address the fact that critical PV devices, like PV disk and network,
could be slow and we actually need to wait for them.  Could we just have
two wait loops, one for critical devices and one for non-critical
devices?
First, with a timeout of 300, we would wait for disk and network, then
we would checkout if everything else is connected. If some other PV
devices, like VFB/VKBD, are not yet connected, we could decide whether
to continue anyway or wait for them some more.

From: konrad <konrad@localhost.localdomain>

A rather annoying and common case is when booting a PVonHVM guest
and exposing the PV KBD and PV VFB - as both of those do not
make any sense. The HVM guest is using the VGA driver and the emulated
keyboard for this. So we provide a very basic quirk framework
(can be expanded in the future) to not wait for 6 minutes for those devices
to initialize - they never wont.

To trigger this, put this in your guest config:

vfb = [ ''vnc=1, vnclisten=0.0.0.0 ,vncunused=1'']

instead of this:
vnc=1
vnclisten="0.0.0.0"

[v3: Split delay in non-essential (30 seconds) and essential
 devices per Ian and Stefano suggestion]
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>quirk
---
 drivers/xen/xenbus/xenbus_probe_frontend.c |   69 +++++++++++++++++++++------
 1 files changed, 53 insertions(+), 16 deletions(-)

diff --git a/drivers/xen/xenbus/xenbus_probe_frontend.c
b/drivers/xen/xenbus/xenbus_probe_frontend.c
index f20c5f1..7422913 100644
--- a/drivers/xen/xenbus/xenbus_probe_frontend.c
+++ b/drivers/xen/xenbus/xenbus_probe_frontend.c
@@ -135,7 +135,7 @@ static int read_backend_details(struct xenbus_device
*xendev)
 	return xenbus_read_otherend_details(xendev, "backend-id",
"backend");
 }
 
-static int is_device_connecting(struct device *dev, void *data)
+static int is_device_connecting(struct device *dev, void *data, bool
ignore_vkbd)
 {
 	struct xenbus_device *xendev = to_xenbus_device(dev);
 	struct device_driver *drv = data;
@@ -152,16 +152,41 @@ static int is_device_connecting(struct device *dev, void
*data)
 	if (drv && (dev->driver != drv))
 		return 0;
 
+	if (ignore_vkbd) {
+		/* With older QEMU, for PVonHVM guests the guest config files
+		 * could contain: vfb = [ ''vnc=1, vnclisten=0.0.0.0'']
+		 * which is nonsensical as there is no PV FB (there can be
+		 * a PVKB) running as HVM guest. */
+
+		if ((strncmp(xendev->nodename, "device/vkbd", 11) == 0))
+			return 0;
+
+		if ((strncmp(xendev->nodename, "device/vfb", 10) == 0))
+			return 0;
+	}
 	xendrv = to_xenbus_driver(dev->driver);
 	return (xendev->state < XenbusStateConnected ||
 		(xendev->state == XenbusStateConnected &&
 		 xendrv->is_ready && !xendrv->is_ready(xendev)));
 }
+static int essential_device_connecting(struct device *dev, void *data)
+{
+	return is_device_connecting(dev, data, true /* ignore PVKB+PVDB */);
+}
+static int non_essential_device_connecting(struct device *dev, void *data)
+{
+	return is_device_connecting(dev, data, false);
+}
 
-static int exists_connecting_device(struct device_driver *drv)
+static int exists_essential_connecting_device(struct device_driver *drv)
 {
 	return bus_for_each_dev(&xenbus_frontend.bus, NULL, drv,
-				is_device_connecting);
+				essential_device_connecting);
+}
+static int exists_non_essential_connecting_device(struct device_driver *drv)
+{
+	return bus_for_each_dev(&xenbus_frontend.bus, NULL, drv,
+				non_essential_device_connecting);
 }
 
 static int print_device_status(struct device *dev, void *data)
@@ -192,6 +217,23 @@ static int print_device_status(struct device *dev, void
*data)
 /* We only wait for device setup after most initcalls have run. */
 static int ready_to_wait_for_devices;
 
+static bool wait_loop(unsigned long start, unsigned int max_delay,
+		     unsigned int *seconds_waited)
+{
+	if (time_after(jiffies, start + (*seconds_waited+5)*HZ)) {
+		if (!*seconds_waited)
+			printk(KERN_WARNING "XENBUS: Waiting for "
+			       "devices to initialise: ");
+		*seconds_waited += 5;
+		printk("%us...", max_delay - *seconds_waited);
+		if (*seconds_waited == max_delay)
+			return true;
+	}
+
+	schedule_timeout_interruptible(HZ/10);
+
+	return false;
+}
 /*
  * On a 5-minute timeout, wait for all devices currently configured.  We need
  * to do this to guarantee that the filesystems and / or network devices
@@ -215,19 +257,14 @@ static void wait_for_devices(struct xenbus_driver *xendrv)
 	if (!ready_to_wait_for_devices || !xen_domain())
 		return;
 
-	while (exists_connecting_device(drv)) {
-		if (time_after(jiffies, start + (seconds_waited+5)*HZ)) {
-			if (!seconds_waited)
-				printk(KERN_WARNING "XENBUS: Waiting for "
-				       "devices to initialise: ");
-			seconds_waited += 5;
-			printk("%us...", 300 - seconds_waited);
-			if (seconds_waited == 300)
-				break;
-		}
-
-		schedule_timeout_interruptible(HZ/10);
-	}
+	while (exists_non_essential_connecting_device(drv))
+		if (wait_loop(start, 30, &seconds_waited))
+			break;
+
+	/* Skips PVKB and PVFB check.*/
+	while (exists_essential_connecting_device(drv))
+		if (wait_loop(start, 270, &seconds_waited))
+			break;
 
 	if (seconds_waited)
 		printk("\n");
-- 
1.7.7.5

On Wed, 18 Apr 2012, Konrad Rzeszutek Wilk wrote:
> From: konrad <konrad@localhost.localdomain>
wrong email address

> A rather annoying and common case is when booting a PVonHVM guest
> and exposing the PV KBD and PV VFB - as both of those do not
> make any sense.
I would rather write: "as broken toolstacks don''t always
initialize the
backends correctly."

> The HVM guest is using the VGA driver and the emulated
> keyboard for this. So we provide a very basic quirk framework
> (can be expanded in the future) to not wait for 6 minutes for those devices
> to initialize - they never wont.
> 
> To trigger this, put this in your guest config:
> 
> vfb = [ ''vnc=1, vnclisten=0.0.0.0 ,vncunused=1'']
> 
> instead of this:
> vnc=1
> vnclisten="0.0.0.0"
> 
> [v3: Split delay in non-essential (30 seconds) and essential
>  devices per Ian and Stefano suggestion]
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>quirk
> ---
>  drivers/xen/xenbus/xenbus_probe_frontend.c |   69
+++++++++++++++++++++------
>  1 files changed, 53 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/xen/xenbus/xenbus_probe_frontend.c
b/drivers/xen/xenbus/xenbus_probe_frontend.c
> index f20c5f1..7422913 100644
> --- a/drivers/xen/xenbus/xenbus_probe_frontend.c
> +++ b/drivers/xen/xenbus/xenbus_probe_frontend.c
> @@ -135,7 +135,7 @@ static int read_backend_details(struct xenbus_device
*xendev)
>  	return xenbus_read_otherend_details(xendev, "backend-id",
"backend");
>  }
>  
> -static int is_device_connecting(struct device *dev, void *data)
> +static int is_device_connecting(struct device *dev, void *data, bool
ignore_vkbd)
>  {
>  	struct xenbus_device *xendev = to_xenbus_device(dev);
>  	struct device_driver *drv = data;
> @@ -152,16 +152,41 @@ static int is_device_connecting(struct device *dev,
void *data)
>  	if (drv && (dev->driver != drv))
>  		return 0;
>  
> +	if (ignore_vkbd) {
I would name the parameter ignore_nonessential, just to make it clearer
that is not just about vkbd

> +		/* With older QEMU, for PVonHVM guests the guest config files
> +		 * could contain: vfb = [ ''vnc=1, vnclisten=0.0.0.0'']
> +		 * which is nonsensical as there is no PV FB (there can be
> +		 * a PVKB) running as HVM guest. */
> +
> +		if ((strncmp(xendev->nodename, "device/vkbd", 11) == 0))
> +			return 0;
> +
> +		if ((strncmp(xendev->nodename, "device/vfb", 10) == 0))
> +			return 0;
> +	}
>  	xendrv = to_xenbus_driver(dev->driver);
>  	return (xendev->state < XenbusStateConnected ||
>  		(xendev->state == XenbusStateConnected &&
>  		 xendrv->is_ready && !xendrv->is_ready(xendev)));
>  }
> +static int essential_device_connecting(struct device *dev, void *data)
> +{
> +	return is_device_connecting(dev, data, true /* ignore PVKB+PVDB */);
> +}
the comment is wrong

> +static int non_essential_device_connecting(struct device *dev, void *data)
> +{
> +	return is_device_connecting(dev, data, false);
> +}
>  
> -static int exists_connecting_device(struct device_driver *drv)
> +static int exists_essential_connecting_device(struct device_driver *drv)
>  {
>  	return bus_for_each_dev(&xenbus_frontend.bus, NULL, drv,
> -				is_device_connecting);
> +				essential_device_connecting);
> +}
> +static int exists_non_essential_connecting_device(struct device_driver
*drv)
> +{
> +	return bus_for_each_dev(&xenbus_frontend.bus, NULL, drv,
> +				non_essential_device_connecting);
>  }
>  
>  static int print_device_status(struct device *dev, void *data)
> @@ -192,6 +217,23 @@ static int print_device_status(struct device *dev,
void *data)
>  /* We only wait for device setup after most initcalls have run. */
>  static int ready_to_wait_for_devices;
>  
> +static bool wait_loop(unsigned long start, unsigned int max_delay,
> +		     unsigned int *seconds_waited)
> +{
> +	if (time_after(jiffies, start + (*seconds_waited+5)*HZ)) {
> +		if (!*seconds_waited)
> +			printk(KERN_WARNING "XENBUS: Waiting for "
> +			       "devices to initialise: ");
> +		*seconds_waited += 5;
> +		printk("%us...", max_delay - *seconds_waited);
> +		if (*seconds_waited == max_delay)
> +			return true;
> +	}
> +
> +	schedule_timeout_interruptible(HZ/10);
> +
> +	return false;
> +}
>  /*
>   * On a 5-minute timeout, wait for all devices currently configured.  We
need
>   * to do this to guarantee that the filesystems and / or network devices
> @@ -215,19 +257,14 @@ static void wait_for_devices(struct xenbus_driver
*xendrv)
>  	if (!ready_to_wait_for_devices || !xen_domain())
>  		return;
>  
> -	while (exists_connecting_device(drv)) {
> -		if (time_after(jiffies, start + (seconds_waited+5)*HZ)) {
> -			if (!seconds_waited)
> -				printk(KERN_WARNING "XENBUS: Waiting for "
> -				       "devices to initialise: ");
> -			seconds_waited += 5;
> -			printk("%us...", 300 - seconds_waited);
> -			if (seconds_waited == 300)
> -				break;
> -		}
> -
> -		schedule_timeout_interruptible(HZ/10);
> -	}
> +	while (exists_non_essential_connecting_device(drv))
> +		if (wait_loop(start, 30, &seconds_waited))
> +			break;
> +
> +	/* Skips PVKB and PVFB check.*/
> +	while (exists_essential_connecting_device(drv))
> +		if (wait_loop(start, 270, &seconds_waited))
> +			break;
>  
>  	if (seconds_waited)
>  		printk("\n");
> -- 
> 1.7.7.5
> 
Other than the comments I wrote above, I think the patch is good

On Thu, Apr 19, 2012 at 12:37:42PM +0100, Stefano Stabellini
wrote:
> On Wed, 18 Apr 2012, Konrad Rzeszutek Wilk wrote:
> > From: konrad <konrad@localhost.localdomain>
> 
> wrong email address
Yeah :-)
> 
> 
> > A rather annoying and common case is when booting a PVonHVM guest
> > and exposing the PV KBD and PV VFB - as both of those do not
> > make any sense.
> 
> I would rather write: "as broken toolstacks don''t always
initialize the
> backends correctly."
OK.
> 
> 
> > The HVM guest is using the VGA driver and the emulated
> > keyboard for this. So we provide a very basic quirk framework
> > (can be expanded in the future) to not wait for 6 minutes for those
devices
> > to initialize - they never wont.
> > 
> > To trigger this, put this in your guest config:
> > 
> > vfb = [ ''vnc=1, vnclisten=0.0.0.0 ,vncunused=1'']
> > 
> > instead of this:
> > vnc=1
> > vnclisten="0.0.0.0"
> > 
> > [v3: Split delay in non-essential (30 seconds) and essential
> >  devices per Ian and Stefano suggestion]
> > Signed-off-by: Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com>quirk
> > ---
> >  drivers/xen/xenbus/xenbus_probe_frontend.c |   69
+++++++++++++++++++++------
> >  1 files changed, 53 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/xen/xenbus/xenbus_probe_frontend.c
b/drivers/xen/xenbus/xenbus_probe_frontend.c
> > index f20c5f1..7422913 100644
> > --- a/drivers/xen/xenbus/xenbus_probe_frontend.c
> > +++ b/drivers/xen/xenbus/xenbus_probe_frontend.c
> > @@ -135,7 +135,7 @@ static int read_backend_details(struct
xenbus_device *xendev)
> >  	return xenbus_read_otherend_details(xendev, "backend-id",
"backend");
> >  }
> >  
> > -static int is_device_connecting(struct device *dev, void *data)
> > +static int is_device_connecting(struct device *dev, void *data, bool
ignore_vkbd)
> >  {
> >  	struct xenbus_device *xendev = to_xenbus_device(dev);
> >  	struct device_driver *drv = data;
> > @@ -152,16 +152,41 @@ static int is_device_connecting(struct device
*dev, void *data)
> >  	if (drv && (dev->driver != drv))
> >  		return 0;
> >  
> > +	if (ignore_vkbd) {
> 
> I would name the parameter ignore_nonessential, just to make it clearer
> that is not just about vkbd
OK.
> 
> 
> > +		/* With older QEMU, for PVonHVM guests the guest config files
> > +		 * could contain: vfb = [ ''vnc=1,
vnclisten=0.0.0.0'']
> > +		 * which is nonsensical as there is no PV FB (there can be
> > +		 * a PVKB) running as HVM guest. */
> > +
> > +		if ((strncmp(xendev->nodename, "device/vkbd", 11) ==
0))
> > +			return 0;
> > +
> > +		if ((strncmp(xendev->nodename, "device/vfb", 10) ==
0))
> > +			return 0;
> > +	}
> >  	xendrv = to_xenbus_driver(dev->driver);
> >  	return (xendev->state < XenbusStateConnected ||
> >  		(xendev->state == XenbusStateConnected &&
> >  		 xendrv->is_ready && !xendrv->is_ready(xendev)));
> >  }
> > +static int essential_device_connecting(struct device *dev, void
*data)
> > +{
> > +	return is_device_connecting(dev, data, true /* ignore PVKB+PVDB */);
> > +}
> 
> the comment is wrong
Oh, PVKBD + PVFB. Right.
> 
> 
> > +static int non_essential_device_connecting(struct device *dev, void
*data)
> > +{
> > +	return is_device_connecting(dev, data, false);
> > +}
> >  
> > -static int exists_connecting_device(struct device_driver *drv)
> > +static int exists_essential_connecting_device(struct device_driver
*drv)
> >  {
> >  	return bus_for_each_dev(&xenbus_frontend.bus, NULL, drv,
> > -				is_device_connecting);
> > +				essential_device_connecting);
> > +}
> > +static int exists_non_essential_connecting_device(struct
device_driver *drv)
> > +{
> > +	return bus_for_each_dev(&xenbus_frontend.bus, NULL, drv,
> > +				non_essential_device_connecting);
> >  }
> >  
> >  static int print_device_status(struct device *dev, void *data)
> > @@ -192,6 +217,23 @@ static int print_device_status(struct device
*dev, void *data)
> >  /* We only wait for device setup after most initcalls have run. */
> >  static int ready_to_wait_for_devices;
> >  
> > +static bool wait_loop(unsigned long start, unsigned int max_delay,
> > +		     unsigned int *seconds_waited)
> > +{
> > +	if (time_after(jiffies, start + (*seconds_waited+5)*HZ)) {
> > +		if (!*seconds_waited)
> > +			printk(KERN_WARNING "XENBUS: Waiting for "
> > +			       "devices to initialise: ");
> > +		*seconds_waited += 5;
> > +		printk("%us...", max_delay - *seconds_waited);
> > +		if (*seconds_waited == max_delay)
> > +			return true;
> > +	}
> > +
> > +	schedule_timeout_interruptible(HZ/10);
> > +
> > +	return false;
> > +}
> >  /*
> >   * On a 5-minute timeout, wait for all devices currently configured. 
We need
> >   * to do this to guarantee that the filesystems and / or network
devices
> > @@ -215,19 +257,14 @@ static void wait_for_devices(struct
xenbus_driver *xendrv)
> >  	if (!ready_to_wait_for_devices || !xen_domain())
> >  		return;
> >  
> > -	while (exists_connecting_device(drv)) {
> > -		if (time_after(jiffies, start + (seconds_waited+5)*HZ)) {
> > -			if (!seconds_waited)
> > -				printk(KERN_WARNING "XENBUS: Waiting for "
> > -				       "devices to initialise: ");
> > -			seconds_waited += 5;
> > -			printk("%us...", 300 - seconds_waited);
> > -			if (seconds_waited == 300)
> > -				break;
> > -		}
> > -
> > -		schedule_timeout_interruptible(HZ/10);
> > -	}
> > +	while (exists_non_essential_connecting_device(drv))
> > +		if (wait_loop(start, 30, &seconds_waited))
> > +			break;
> > +
> > +	/* Skips PVKB and PVFB check.*/
> > +	while (exists_essential_connecting_device(drv))
> > +		if (wait_loop(start, 270, &seconds_waited))
> > +			break;
> >  
> >  	if (seconds_waited)
> >  		printk("\n");
> > -- 
> > 1.7.7.5
> > 
> 
> Other than the comments I wrote above, I think the patch is good
Sweet. thx!