[PATCH] hvc_xen: introduce HVC_XEN_FRONTEND

[PATCH] hvc_xen: introduce HVC_XEN_FRONTEND

[Stefano Stabellini]

Introduce a new config option HVC_XEN_FRONTEND to enable/disable the
xenbus based pv console frontend.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 drivers/tty/hvc/Kconfig   |    8 +++
 drivers/tty/hvc/hvc_xen.c |  116 ++++++++++++++++++++++++---------------------
 2 files changed, 70 insertions(+), 54 deletions(-)

diff --git a/drivers/tty/hvc/Kconfig b/drivers/tty/hvc/Kconfig
index 4222035..192e21e 100644
--- a/drivers/tty/hvc/Kconfig
+++ b/drivers/tty/hvc/Kconfig
@@ -76,6 +76,14 @@ config HVC_XEN
 	help
 	  Xen virtual console device driver
 
+config HVC_XEN_FRONTEND
+	bool "Xen Hypervisor Multiple Consoles support"
+	depends on HVC_XEN
+	select XEN_XENBUS_FRONTEND
+	default y
+	help
+	  Xen driver for secondary virtual consoles
+
 config HVC_UDBG
        bool "udbg based fake hypervisor console"
        depends on PPC && EXPERIMENTAL
diff --git a/drivers/tty/hvc/hvc_xen.c b/drivers/tty/hvc/hvc_xen.c
index 26090c7..83d5c88 100644
--- a/drivers/tty/hvc/hvc_xen.c
+++ b/drivers/tty/hvc/hvc_xen.c
@@ -55,7 +55,6 @@ struct xencons_info {
 
 static LIST_HEAD(xenconsoles);
 static DEFINE_SPINLOCK(xencons_lock);
-static struct xenbus_driver xencons_driver;
 
 /* ------------------------------------------------------------------ */
 
@@ -298,53 +297,6 @@ static int xen_initial_domain_console_init(void)
 	return 0;
 }
 
-static int __init xen_hvc_init(void)
-{
-	int r;
-	struct xencons_info *info;
-	const struct hv_ops *ops;
-
-	if (!xen_domain())
-		return -ENODEV;
-
-	if (xen_initial_domain()) {
-		ops = &dom0_hvc_ops;
-		r = xen_initial_domain_console_init();
-		if (r < 0)
-			return r;
-		info = vtermno_to_xencons(HVC_COOKIE);
-	} else {
-		ops = &domU_hvc_ops;
-		if (xen_hvm_domain())
-			r = xen_hvm_console_init();
-		else
-			r = xen_pv_console_init();
-		if (r < 0)
-			return r;
-
-		info = vtermno_to_xencons(HVC_COOKIE);
-		info->irq = bind_evtchn_to_irq(info->evtchn);
-	}
-	if (info->irq < 0)
-		info->irq = 0; /* NO_IRQ */
-	else
-		irq_set_noprobe(info->irq);
-
-	info->hvc = hvc_alloc(HVC_COOKIE, info->irq, ops, 256);
-	if (IS_ERR(info->hvc)) {
-		r = PTR_ERR(info->hvc);
-		spin_lock(&xencons_lock);
-		list_del(&info->list);
-		spin_unlock(&xencons_lock);
-		if (info->irq)
-			unbind_from_irqhandler(info->irq, NULL);
-		kfree(info);
-		return r;
-	}
-
-	return xenbus_register_frontend(&xencons_driver);
-}
-
 void xen_console_resume(void)
 {
 	struct xencons_info *info = vtermno_to_xencons(HVC_COOKIE);
@@ -392,6 +344,9 @@ static int xen_console_remove(struct xencons_info *info)
 	return 0;
 }
 
+#ifdef CONFIG_HVC_XEN_FRONTEND
+static struct xenbus_driver xencons_driver;
+
 static int xencons_remove(struct xenbus_device *dev)
 {
 	return xen_console_remove(dev_get_drvdata(&dev->dev));
@@ -543,6 +498,65 @@ static const struct xenbus_device_id xencons_ids[] = {
 };
 
 
+static DEFINE_XENBUS_DRIVER(xencons, "xenconsole",
+	.probe = xencons_probe,
+	.remove = xencons_remove,
+	.resume = xencons_resume,
+	.otherend_changed = xencons_backend_changed,
+);
+#endif /* CONFIG_HVC_XEN_FRONTEND */
+
+static int __init xen_hvc_init(void)
+{
+	int r;
+	struct xencons_info *info;
+	const struct hv_ops *ops;
+
+	if (!xen_domain())
+		return -ENODEV;
+
+	if (xen_initial_domain()) {
+		ops = &dom0_hvc_ops;
+		r = xen_initial_domain_console_init();
+		if (r < 0)
+			return r;
+		info = vtermno_to_xencons(HVC_COOKIE);
+	} else {
+		ops = &domU_hvc_ops;
+		if (xen_hvm_domain())
+			r = xen_hvm_console_init();
+		else
+			r = xen_pv_console_init();
+		if (r < 0)
+			return r;
+
+		info = vtermno_to_xencons(HVC_COOKIE);
+		info->irq = bind_evtchn_to_irq(info->evtchn);
+	}
+	if (info->irq < 0)
+		info->irq = 0; /* NO_IRQ */
+	else
+		irq_set_noprobe(info->irq);
+
+	info->hvc = hvc_alloc(HVC_COOKIE, info->irq, ops, 256);
+	if (IS_ERR(info->hvc)) {
+		r = PTR_ERR(info->hvc);
+		spin_lock(&xencons_lock);
+		list_del(&info->list);
+		spin_unlock(&xencons_lock);
+		if (info->irq)
+			unbind_from_irqhandler(info->irq, NULL);
+		kfree(info);
+		return r;
+	}
+
+	r = 0;
+#ifdef CONFIG_HVC_XEN_FRONTEND
+	r = xenbus_register_frontend(&xencons_driver);
+#endif
+	return r;
+}
+
 static void __exit xen_hvc_fini(void)
 {
 	struct xencons_info *entry, *next;
@@ -580,12 +594,6 @@ static int xen_cons_init(void)
 	return 0;
 }
 
-static DEFINE_XENBUS_DRIVER(xencons, "xenconsole",
-	.probe = xencons_probe,
-	.remove = xencons_remove,
-	.resume = xencons_resume,
-	.otherend_changed = xencons_backend_changed,
-);
 
 module_init(xen_hvc_init);
 module_exit(xen_hvc_fini);
-- 
1.7.2.5

Re: [PATCH] hvc_xen: introduce HVC_XEN_FRONTEND

[Konrad Rzeszutek Wilk]

On Tue, Feb 21, 2012 at 11:30:42AM +0000, Stefano Stabellini wrote:
> Introduce a new config option HVC_XEN_FRONTEND to enable/disable the
> xenbus based pv console frontend.

applied.

> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
>  drivers/tty/hvc/Kconfig   |    8 +++
>  drivers/tty/hvc/hvc_xen.c |  116 ++++++++++++++++++++++++---------------------
>  2 files changed, 70 insertions(+), 54 deletions(-)
> 
> diff --git a/drivers/tty/hvc/Kconfig b/drivers/tty/hvc/Kconfig
> index 4222035..192e21e 100644
> --- a/drivers/tty/hvc/Kconfig
> +++ b/drivers/tty/hvc/Kconfig
> @@ -76,6 +76,14 @@ config HVC_XEN
>  	help
>  	  Xen virtual console device driver
>  
> +config HVC_XEN_FRONTEND
> +	bool "Xen Hypervisor Multiple Consoles support"
> +	depends on HVC_XEN
> +	select XEN_XENBUS_FRONTEND
> +	default y
> +	help
> +	  Xen driver for secondary virtual consoles
> +
>  config HVC_UDBG
>         bool "udbg based fake hypervisor console"
>         depends on PPC && EXPERIMENTAL
> diff --git a/drivers/tty/hvc/hvc_xen.c b/drivers/tty/hvc/hvc_xen.c
> index 26090c7..83d5c88 100644
> --- a/drivers/tty/hvc/hvc_xen.c
> +++ b/drivers/tty/hvc/hvc_xen.c
> @@ -55,7 +55,6 @@ struct xencons_info {
>  
>  static LIST_HEAD(xenconsoles);
>  static DEFINE_SPINLOCK(xencons_lock);
> -static struct xenbus_driver xencons_driver;
>  
>  /* ------------------------------------------------------------------ */
>  
> @@ -298,53 +297,6 @@ static int xen_initial_domain_console_init(void)
>  	return 0;
>  }
>  
> -static int __init xen_hvc_init(void)
> -{
> -	int r;
> -	struct xencons_info *info;
> -	const struct hv_ops *ops;
> -
> -	if (!xen_domain())
> -		return -ENODEV;
> -
> -	if (xen_initial_domain()) {
> -		ops = &dom0_hvc_ops;
> -		r = xen_initial_domain_console_init();
> -		if (r < 0)
> -			return r;
> -		info = vtermno_to_xencons(HVC_COOKIE);
> -	} else {
> -		ops = &domU_hvc_ops;
> -		if (xen_hvm_domain())
> -			r = xen_hvm_console_init();
> -		else
> -			r = xen_pv_console_init();
> -		if (r < 0)
> -			return r;
> -
> -		info = vtermno_to_xencons(HVC_COOKIE);
> -		info->irq = bind_evtchn_to_irq(info->evtchn);
> -	}
> -	if (info->irq < 0)
> -		info->irq = 0; /* NO_IRQ */
> -	else
> -		irq_set_noprobe(info->irq);
> -
> -	info->hvc = hvc_alloc(HVC_COOKIE, info->irq, ops, 256);
> -	if (IS_ERR(info->hvc)) {
> -		r = PTR_ERR(info->hvc);
> -		spin_lock(&xencons_lock);
> -		list_del(&info->list);
> -		spin_unlock(&xencons_lock);
> -		if (info->irq)
> -			unbind_from_irqhandler(info->irq, NULL);
> -		kfree(info);
> -		return r;
> -	}
> -
> -	return xenbus_register_frontend(&xencons_driver);
> -}
> -
>  void xen_console_resume(void)
>  {
>  	struct xencons_info *info = vtermno_to_xencons(HVC_COOKIE);
> @@ -392,6 +344,9 @@ static int xen_console_remove(struct xencons_info *info)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_HVC_XEN_FRONTEND
> +static struct xenbus_driver xencons_driver;
> +
>  static int xencons_remove(struct xenbus_device *dev)
>  {
>  	return xen_console_remove(dev_get_drvdata(&dev->dev));
> @@ -543,6 +498,65 @@ static const struct xenbus_device_id xencons_ids[] = {
>  };
>  
>  
> +static DEFINE_XENBUS_DRIVER(xencons, "xenconsole",
> +	.probe = xencons_probe,
> +	.remove = xencons_remove,
> +	.resume = xencons_resume,
> +	.otherend_changed = xencons_backend_changed,
> +);
> +#endif /* CONFIG_HVC_XEN_FRONTEND */
> +
> +static int __init xen_hvc_init(void)
> +{
> +	int r;
> +	struct xencons_info *info;
> +	const struct hv_ops *ops;
> +
> +	if (!xen_domain())
> +		return -ENODEV;
> +
> +	if (xen_initial_domain()) {
> +		ops = &dom0_hvc_ops;
> +		r = xen_initial_domain_console_init();
> +		if (r < 0)
> +			return r;
> +		info = vtermno_to_xencons(HVC_COOKIE);
> +	} else {
> +		ops = &domU_hvc_ops;
> +		if (xen_hvm_domain())
> +			r = xen_hvm_console_init();
> +		else
> +			r = xen_pv_console_init();
> +		if (r < 0)
> +			return r;
> +
> +		info = vtermno_to_xencons(HVC_COOKIE);
> +		info->irq = bind_evtchn_to_irq(info->evtchn);
> +	}
> +	if (info->irq < 0)
> +		info->irq = 0; /* NO_IRQ */
> +	else
> +		irq_set_noprobe(info->irq);
> +
> +	info->hvc = hvc_alloc(HVC_COOKIE, info->irq, ops, 256);
> +	if (IS_ERR(info->hvc)) {
> +		r = PTR_ERR(info->hvc);
> +		spin_lock(&xencons_lock);
> +		list_del(&info->list);
> +		spin_unlock(&xencons_lock);
> +		if (info->irq)
> +			unbind_from_irqhandler(info->irq, NULL);
> +		kfree(info);
> +		return r;
> +	}
> +
> +	r = 0;
> +#ifdef CONFIG_HVC_XEN_FRONTEND
> +	r = xenbus_register_frontend(&xencons_driver);
> +#endif
> +	return r;
> +}
> +
>  static void __exit xen_hvc_fini(void)
>  {
>  	struct xencons_info *entry, *next;
> @@ -580,12 +594,6 @@ static int xen_cons_init(void)
>  	return 0;
>  }
>  
> -static DEFINE_XENBUS_DRIVER(xencons, "xenconsole",
> -	.probe = xencons_probe,
> -	.remove = xencons_remove,
> -	.resume = xencons_resume,
> -	.otherend_changed = xencons_backend_changed,
> -);
>  
>  module_init(xen_hvc_init);
>  module_exit(xen_hvc_fini);
> -- 
> 1.7.2.5
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xensource.com/xen-devel

Re: [PATCH] hvc_xen: introduce HVC_XEN_FRONTEND

[Konrad Rzeszutek Wilk]

On Tue, Feb 21, 2012 at 11:30:42AM +0000, Stefano Stabellini wrote:
> Introduce a new config option HVC_XEN_FRONTEND to enable/disable the
> xenbus based pv console frontend.

One concern I have - not with this patch - but rather with the whole
PV consoel functionality - is how it is going to work with older hypervisors/QEMU.

I am specifically thinking about Amazon EC2 or 3.4 Xen installations.
I recall that a patch was required to QEMU to make this work flawlessly - so
perhaps all of this code should be gated on checking fora version (so Xen 4.2?)
or by looking for a 'feature-pv-on-hvm-console' XenBus attribute?


> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
>  drivers/tty/hvc/Kconfig   |    8 +++
>  drivers/tty/hvc/hvc_xen.c |  116 ++++++++++++++++++++++++---------------------
>  2 files changed, 70 insertions(+), 54 deletions(-)
> 
> diff --git a/drivers/tty/hvc/Kconfig b/drivers/tty/hvc/Kconfig
> index 4222035..192e21e 100644
> --- a/drivers/tty/hvc/Kconfig
> +++ b/drivers/tty/hvc/Kconfig
> @@ -76,6 +76,14 @@ config HVC_XEN
>  	help
>  	  Xen virtual console device driver
>  
> +config HVC_XEN_FRONTEND
> +	bool "Xen Hypervisor Multiple Consoles support"
> +	depends on HVC_XEN
> +	select XEN_XENBUS_FRONTEND
> +	default y
> +	help
> +	  Xen driver for secondary virtual consoles
> +
>  config HVC_UDBG
>         bool "udbg based fake hypervisor console"
>         depends on PPC && EXPERIMENTAL
> diff --git a/drivers/tty/hvc/hvc_xen.c b/drivers/tty/hvc/hvc_xen.c
> index 26090c7..83d5c88 100644
> --- a/drivers/tty/hvc/hvc_xen.c
> +++ b/drivers/tty/hvc/hvc_xen.c
> @@ -55,7 +55,6 @@ struct xencons_info {
>  
>  static LIST_HEAD(xenconsoles);
>  static DEFINE_SPINLOCK(xencons_lock);
> -static struct xenbus_driver xencons_driver;
>  
>  /* ------------------------------------------------------------------ */
>  
> @@ -298,53 +297,6 @@ static int xen_initial_domain_console_init(void)
>  	return 0;
>  }
>  
> -static int __init xen_hvc_init(void)
> -{
> -	int r;
> -	struct xencons_info *info;
> -	const struct hv_ops *ops;
> -
> -	if (!xen_domain())
> -		return -ENODEV;
> -
> -	if (xen_initial_domain()) {
> -		ops = &dom0_hvc_ops;
> -		r = xen_initial_domain_console_init();
> -		if (r < 0)
> -			return r;
> -		info = vtermno_to_xencons(HVC_COOKIE);
> -	} else {
> -		ops = &domU_hvc_ops;
> -		if (xen_hvm_domain())
> -			r = xen_hvm_console_init();
> -		else
> -			r = xen_pv_console_init();
> -		if (r < 0)
> -			return r;
> -
> -		info = vtermno_to_xencons(HVC_COOKIE);
> -		info->irq = bind_evtchn_to_irq(info->evtchn);
> -	}
> -	if (info->irq < 0)
> -		info->irq = 0; /* NO_IRQ */
> -	else
> -		irq_set_noprobe(info->irq);
> -
> -	info->hvc = hvc_alloc(HVC_COOKIE, info->irq, ops, 256);
> -	if (IS_ERR(info->hvc)) {
> -		r = PTR_ERR(info->hvc);
> -		spin_lock(&xencons_lock);
> -		list_del(&info->list);
> -		spin_unlock(&xencons_lock);
> -		if (info->irq)
> -			unbind_from_irqhandler(info->irq, NULL);
> -		kfree(info);
> -		return r;
> -	}
> -
> -	return xenbus_register_frontend(&xencons_driver);
> -}
> -
>  void xen_console_resume(void)
>  {
>  	struct xencons_info *info = vtermno_to_xencons(HVC_COOKIE);
> @@ -392,6 +344,9 @@ static int xen_console_remove(struct xencons_info *info)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_HVC_XEN_FRONTEND
> +static struct xenbus_driver xencons_driver;
> +
>  static int xencons_remove(struct xenbus_device *dev)
>  {
>  	return xen_console_remove(dev_get_drvdata(&dev->dev));
> @@ -543,6 +498,65 @@ static const struct xenbus_device_id xencons_ids[] = {
>  };
>  
>  
> +static DEFINE_XENBUS_DRIVER(xencons, "xenconsole",
> +	.probe = xencons_probe,
> +	.remove = xencons_remove,
> +	.resume = xencons_resume,
> +	.otherend_changed = xencons_backend_changed,
> +);
> +#endif /* CONFIG_HVC_XEN_FRONTEND */
> +
> +static int __init xen_hvc_init(void)
> +{
> +	int r;
> +	struct xencons_info *info;
> +	const struct hv_ops *ops;
> +
> +	if (!xen_domain())
> +		return -ENODEV;
> +
> +	if (xen_initial_domain()) {
> +		ops = &dom0_hvc_ops;
> +		r = xen_initial_domain_console_init();
> +		if (r < 0)
> +			return r;
> +		info = vtermno_to_xencons(HVC_COOKIE);
> +	} else {
> +		ops = &domU_hvc_ops;
> +		if (xen_hvm_domain())
> +			r = xen_hvm_console_init();
> +		else
> +			r = xen_pv_console_init();
> +		if (r < 0)
> +			return r;
> +
> +		info = vtermno_to_xencons(HVC_COOKIE);
> +		info->irq = bind_evtchn_to_irq(info->evtchn);
> +	}
> +	if (info->irq < 0)
> +		info->irq = 0; /* NO_IRQ */
> +	else
> +		irq_set_noprobe(info->irq);
> +
> +	info->hvc = hvc_alloc(HVC_COOKIE, info->irq, ops, 256);
> +	if (IS_ERR(info->hvc)) {
> +		r = PTR_ERR(info->hvc);
> +		spin_lock(&xencons_lock);
> +		list_del(&info->list);
> +		spin_unlock(&xencons_lock);
> +		if (info->irq)
> +			unbind_from_irqhandler(info->irq, NULL);
> +		kfree(info);
> +		return r;
> +	}
> +
> +	r = 0;
> +#ifdef CONFIG_HVC_XEN_FRONTEND
> +	r = xenbus_register_frontend(&xencons_driver);
> +#endif
> +	return r;
> +}
> +
>  static void __exit xen_hvc_fini(void)
>  {
>  	struct xencons_info *entry, *next;
> @@ -580,12 +594,6 @@ static int xen_cons_init(void)
>  	return 0;
>  }
>  
> -static DEFINE_XENBUS_DRIVER(xencons, "xenconsole",
> -	.probe = xencons_probe,
> -	.remove = xencons_remove,
> -	.resume = xencons_resume,
> -	.otherend_changed = xencons_backend_changed,
> -);
>  
>  module_init(xen_hvc_init);
>  module_exit(xen_hvc_fini);
> -- 
> 1.7.2.5

Re: [PATCH] hvc_xen: introduce HVC_XEN_FRONTEND

[Stefano Stabellini]

On Sun, 4 Mar 2012, Konrad Rzeszutek Wilk wrote:
> On Tue, Feb 21, 2012 at 11:30:42AM +0000, Stefano Stabellini wrote:
> > Introduce a new config option HVC_XEN_FRONTEND to enable/disable the
> > xenbus based pv console frontend.
> 
> One concern I have - not with this patch - but rather with the whole
> PV consoel functionality - is how it is going to work with older hypervisors/QEMU.
> 
> I am specifically thinking about Amazon EC2 or 3.4 Xen installations.
> I recall that a patch was required to QEMU to make this work flawlessly - so
> perhaps all of this code should be gated on checking fora version (so Xen 4.2?)
> or by looking for a 'feature-pv-on-hvm-console' XenBus attribute?

I agree that this issue needs more thoughts about compatibility with old
xen installations, but if it is possible I would rather avoid
introducing a this-bug-is-now-fixed kind of flag.

Only xen installations supporting vfb and qemu as a console backend are
susceptible, so Amazon should be safe because I don't think they use any
of them.
Also looking through the code I have found that qemu-xen 3.4, 4.0 and
4.1 are all susceptible to this bug the same way and they can all be
fixed with the same patch.

>From the Linux side the best thing we could do is completely ignore
devices/console/0, the problem is that if we don't we are either going
to upset QEMU or xenconsoled.
If we return 0 from xencons_probe we are going to pretend everything is
normal so as a consequence the xenbus state is going to be set to 4,
upsetting xenconsoled.
If we return -ENODEV we are going to admit that the device shouldn't
even be there and in that case the xenbus drivers set the state to 6
causing the unpatched qemu to crash.
I don't think there is anything we can do within xencons_probe to avoid
the bug: what return value are we supposed to return so that the
xenbus drivers does not take any further actions?
Even a 'feature-pv-on-hvm-console' flag  wouldn't help.

Maybe we need to introduce an explicit check in xenbus_probe_device_type
to avoid calling bus->probe if type == "console" and dir[i] == "0", what
do you think?

Re: [PATCH] hvc_xen: introduce HVC_XEN_FRONTEND

[Konrad Rzeszutek Wilk]

On Mon, Mar 05, 2012 at 10:45:33PM +0000, Stefano Stabellini wrote:
> On Sun, 4 Mar 2012, Konrad Rzeszutek Wilk wrote:
> > On Tue, Feb 21, 2012 at 11:30:42AM +0000, Stefano Stabellini wrote:
> > > Introduce a new config option HVC_XEN_FRONTEND to enable/disable the
> > > xenbus based pv console frontend.
> > 
> > One concern I have - not with this patch - but rather with the whole
> > PV consoel functionality - is how it is going to work with older hypervisors/QEMU.
> > 
> > I am specifically thinking about Amazon EC2 or 3.4 Xen installations.
> > I recall that a patch was required to QEMU to make this work flawlessly - so
> > perhaps all of this code should be gated on checking fora version (so Xen 4.2?)
> > or by looking for a 'feature-pv-on-hvm-console' XenBus attribute?
> 
> I agree that this issue needs more thoughts about compatibility with old
> xen installations, but if it is possible I would rather avoid
> introducing a this-bug-is-now-fixed kind of flag.

Think of it as working around broken implementations. Like dealing with BIOSes
that aren't exactly working right.

> 
> Only xen installations supporting vfb and qemu as a console backend are
> susceptible, so Amazon should be safe because I don't think they use any
> of them.

I think they use the normal xenconsole .. but then the patch to return 0
would work .. but upset future version of QEMU (or is it the other way
around).

> Also looking through the code I have found that qemu-xen 3.4, 4.0 and
> 4.1 are all susceptible to this bug the same way and they can all be
> fixed with the same patch.
> 
> >From the Linux side the best thing we could do is completely ignore
> devices/console/0, the problem is that if we don't we are either going
> to upset QEMU or xenconsoled.
> If we return 0 from xencons_probe we are going to pretend everything is
> normal so as a consequence the xenbus state is going to be set to 4,
> upsetting xenconsoled.
> If we return -ENODEV we are going to admit that the device shouldn't
> even be there and in that case the xenbus drivers set the state to 6
> causing the unpatched qemu to crash.
> I don't think there is anything we can do within xencons_probe to avoid
> the bug: what return value are we supposed to return so that the
> xenbus drivers does not take any further actions?

Right. So I was thinking that finding out what hypervisor is present - 
if 4.2, then it is OK to assume QEMU has it fixed.

> Even a 'feature-pv-on-hvm-console' flag  wouldn't help.
> 
> Maybe we need to introduce an explicit check in xenbus_probe_device_type
> to avoid calling bus->probe if type == "console" and dir[i] == "0", what
> do you think?

If that works..?

> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH] hvc_xen: introduce HVC_XEN_FRONTEND

[Stefano Stabellini]

On Tue, 13 Mar 2012, Konrad Rzeszutek Wilk wrote:
> > Even a 'feature-pv-on-hvm-console' flag  wouldn't help.
> > 
> > Maybe we need to introduce an explicit check in xenbus_probe_device_type
> > to avoid calling bus->probe if type == "console" and dir[i] == "0", what
> > do you think?
> 
> If that works..?

Yes, it does: the appended patch might be ugly but fixes the problem for
me (tested xen 4.1, xm/xend, vfb enabled and disabled so both qemu and
xenconsoled as console backends).

---

xenbus: ignore console/0

Unfortunately xend creates a bogus console/0 frotend/backend entry pair
on xenstore that console backends cannot properly cope with.
Any guest behavior that is not completely ignoring console/0 is going
to either cause problems with xenconsoled or qemu.
Returning 0 or -ENODEV from xencons_probe is not enough because it is
going to cause the frontend state to become 4 or 6 respectively.
The best possible thing we can do here is just ignore the entry from
xenbus_probe_frontend.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

diff --git a/drivers/xen/xenbus/xenbus_probe_frontend.c b/drivers/xen/xenbus/xenbus_probe_frontend.c
index 9c57819..f20c5f1 100644
--- a/drivers/xen/xenbus/xenbus_probe_frontend.c
+++ b/drivers/xen/xenbus/xenbus_probe_frontend.c
@@ -53,6 +53,12 @@ static int xenbus_probe_frontend(struct xen_bus_type *bus, const char *type,
 	char *nodename;
 	int err;
 
+	/* ignore console/0 */
+	if (!strncmp(type, "console", 7) && !strncmp(name, "0", 1)) {
+		DPRINTK("Ignoring buggy device entry console/0");
+		return 0;
+	}
+
 	nodename = kasprintf(GFP_KERNEL, "%s/%s/%s", bus->root, type, name);
 	if (!nodename)
 		return -ENOMEM;

Re: [PATCH] hvc_xen: introduce HVC_XEN_FRONTEND

[Konrad Rzeszutek Wilk]

On Tue, Mar 13, 2012 at 06:30:44PM +0000, Stefano Stabellini wrote:
> On Tue, 13 Mar 2012, Konrad Rzeszutek Wilk wrote:
> > > Even a 'feature-pv-on-hvm-console' flag  wouldn't help.
> > > 
> > > Maybe we need to introduce an explicit check in xenbus_probe_device_type
> > > to avoid calling bus->probe if type == "console" and dir[i] == "0", what
> > > do you think?
> > 
> > If that works..?
> 
> Yes, it does: the appended patch might be ugly but fixes the problem for
> me (tested xen 4.1, xm/xend, vfb enabled and disabled so both qemu and
> xenconsoled as console backends).

Yup. Seems to work with PV and HVM in both Xen 4.1 (with patch) and Xen 4.0.

> 
> ---
> 
> xenbus: ignore console/0
> 
> Unfortunately xend creates a bogus console/0 frotend/backend entry pair
> on xenstore that console backends cannot properly cope with.
> Any guest behavior that is not completely ignoring console/0 is going
> to either cause problems with xenconsoled or qemu.
> Returning 0 or -ENODEV from xencons_probe is not enough because it is
> going to cause the frontend state to become 4 or 6 respectively.
> The best possible thing we can do here is just ignore the entry from
> xenbus_probe_frontend.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> 
> diff --git a/drivers/xen/xenbus/xenbus_probe_frontend.c b/drivers/xen/xenbus/xenbus_probe_frontend.c
> index 9c57819..f20c5f1 100644
> --- a/drivers/xen/xenbus/xenbus_probe_frontend.c
> +++ b/drivers/xen/xenbus/xenbus_probe_frontend.c
> @@ -53,6 +53,12 @@ static int xenbus_probe_frontend(struct xen_bus_type *bus, const char *type,
>  	char *nodename;
>  	int err;
>  
> +	/* ignore console/0 */
> +	if (!strncmp(type, "console", 7) && !strncmp(name, "0", 1)) {
> +		DPRINTK("Ignoring buggy device entry console/0");
> +		return 0;
> +	}
> +
>  	nodename = kasprintf(GFP_KERNEL, "%s/%s/%s", bus->root, type, name);
>  	if (!nodename)
>  		return -ENOMEM;

Re: [PATCH] hvc_xen: introduce HVC_XEN_FRONTEND

[Matt Wilson]

On Tue, Mar 13, 2012 at 09:29:44AM -0700, Konrad Rzeszutek Wilk wrote:
> On Mon, Mar 05, 2012 at 10:45:33PM +0000, Stefano Stabellini wrote:
> > Only xen installations supporting vfb and qemu as a console backend are
> > susceptible, so Amazon should be safe because I don't think they use any
> > of them.
> 
> I think they use the normal xenconsole .. but then the patch to return 0
> would work .. but upset future version of QEMU (or is it the other way
> around).

There are HVM instance types in EC2: Cluster Compute (HPC cc1.*, cg1.*
and cc2.* instances) and all instances running Windows. When booting
Linux on a Cluster Compute instance you may see:

[    1.887398] XENBUS: Device with no driver: device/vfb/0
[    1.890850] XENBUS: Device with no driver: device/console/0

It's not safe to assume anything in regard to breaking compatibility
with older versions of Xen or QEMU.

Matt