[PATCH 6/15] hypervisor console driver for Celleb

[PATCH 6/15] hypervisor console driver for Celleb

[Ishizaki Kou]

This patch adds hypervisor console driver for Celleb platform.

Signed-off-by: Kou Ishizaki <kou.ishizaki.co.jp>
---

Index: linux-powerpc-git/drivers/char/Kconfig
diff -u linux-powerpc-git/drivers/char/Kconfig:1.1.1.1 linux-powerpc-git/drivers/char/Kconfig:1.2
--- linux-powerpc-git/drivers/char/Kconfig:1.1.1.1	Wed Dec  6 08:24:08 2006
+++ linux-powerpc-git/drivers/char/Kconfig	Wed Dec  6 08:43:15 2006
@@ -595,6 +595,13 @@
 	help
 	  IBM Console device driver which makes use of RTAS
 
+config HVC_BEAT
+	bool "Toshiba's Beat Hypervisor Console support"
+	depends on PPC_CELLEB
+	select HVC_DRIVER
+	help
+	  Toshiba's Cell Reference Set Beat Console device driver
+
 config HVCS
 	tristate "IBM Hypervisor Virtual Console Server support"
 	depends on PPC_PSERIES
Index: linux-powerpc-git/drivers/char/Makefile
diff -u linux-powerpc-git/drivers/char/Makefile:1.1.1.1 linux-powerpc-git/drivers/char/Makefile:1.2
--- linux-powerpc-git/drivers/char/Makefile:1.1.1.1	Wed Dec  6 08:24:08 2006
+++ linux-powerpc-git/drivers/char/Makefile	Wed Dec  6 08:43:15 2006
@@ -44,6 +44,7 @@
 obj-$(CONFIG_HVC_CONSOLE)	+= hvc_vio.o hvsi.o
 obj-$(CONFIG_HVC_ISERIES)	+= hvc_iseries.o
 obj-$(CONFIG_HVC_RTAS)		+= hvc_rtas.o
+obj-$(CONFIG_HVC_BEAT)		+= hvc_beat.o
 obj-$(CONFIG_HVC_DRIVER)	+= hvc_console.o
 obj-$(CONFIG_RAW_DRIVER)	+= raw.o
 obj-$(CONFIG_SGI_SNSC)		+= snsc.o snsc_event.o
Index: linux-powerpc-git/drivers/char/hvc_beat.c
diff -u /dev/null linux-powerpc-git/drivers/char/hvc_beat.c:1.1
--- /dev/null	Mon Dec 11 20:37:34 2006
+++ linux-powerpc-git/drivers/char/hvc_beat.c	Wed Dec  6 08:43:15 2006
@@ -0,0 +1,106 @@
+/*
+ * Beat hypervisor console driver
+ *
+ * (C) Copyright 2006 TOSHIBA CORPORATION
+ *
+ * This code is based on drivers/char/hvc_rtas.c:
+ * (C) Copyright IBM Corporation 2001-2005
+ * (C) Copyright Red Hat, Inc. 2005
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/err.h>
+#include <linux/string.h>
+#include <linux/console.h>
+#include <asm/prom.h>
+#include <asm/hvconsole.h>
+
+#include "hvc_console.h"
+
+extern int64_t beat_get_term_char(uint64_t, uint64_t *, uint64_t *, uint64_t *);
+extern int64_t beat_put_term_char(uint64_t, uint64_t, uint64_t, uint64_t);
+
+struct hvc_struct *hvc_beat_dev = NULL;
+
+static int hvc_beat_get_chars(uint32_t vtermno, char *buf, int cnt)
+{
+	unsigned long kb[2];
+	unsigned long got;
+
+	if (beat_get_term_char(vtermno, &got, &kb[0], &kb[1]) == 0) {
+		memcpy(buf, kb, got);
+		return got;
+	} else {
+		return 0;
+	}
+}
+
+static int hvc_beat_put_chars(uint32_t vtermno, const char *buf, int cnt)
+{
+	unsigned long kb[2];
+
+	memcpy(kb, buf, sizeof(kb));
+	beat_put_term_char(vtermno, cnt, kb[0], kb[1]);
+	return cnt;
+}
+
+static struct hv_ops hvc_beat_get_put_ops = {
+	.get_chars = hvc_beat_get_chars,
+	.put_chars = hvc_beat_put_chars,
+};
+
+static int hvc_beat_useit = 1;
+
+static int hvc_beat_config(char *p)
+{
+	hvc_beat_useit = simple_strtoul(p, NULL, 0);
+	return 0;
+}
+
+static int hvc_beat_console_init(void)
+{
+	if (hvc_beat_useit && machine_is_compatible("Beat")) {
+		hvc_instantiate(0, 0, &hvc_beat_get_put_ops);
+	}
+	return 0;
+}
+
+/* temp */
+static int hvc_beat_init(void)
+{
+	struct hvc_struct *hp;
+
+	hp = hvc_alloc(0, NO_IRQ, &hvc_beat_get_put_ops, 16);
+	if (IS_ERR(hp))
+		return PTR_ERR(hp);
+	hvc_beat_dev = hp;
+	return 0;
+}
+
+static void __exit hvc_beat_exit(void)
+{
+	if (hvc_beat_dev)
+		hvc_remove(hvc_beat_dev);
+}
+
+module_init(hvc_beat_init);
+module_exit(hvc_beat_exit);
+
+__setup("hvc_beat=", hvc_beat_config);
+
+console_initcall(hvc_beat_console_init);

Re: [PATCH 6/15] hypervisor console driver for Celleb

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 4675 bytes --]

On Tue, 2006-12-12 at 12:31 +0900, Ishizaki Kou wrote:
> This patch adds hypervisor console driver for Celleb platform.
> 
> Signed-off-by: Kou Ishizaki <kou.ishizaki.co.jp>
> ---
> 
> Index: linux-powerpc-git/drivers/char/Kconfig
> diff -u linux-powerpc-git/drivers/char/Kconfig:1.1.1.1 linux-powerpc-git/drivers/char/Kconfig:1.2
> --- linux-powerpc-git/drivers/char/Kconfig:1.1.1.1	Wed Dec  6 08:24:08 2006
> +++ linux-powerpc-git/drivers/char/Kconfig	Wed Dec  6 08:43:15 2006
> @@ -595,6 +595,13 @@
>  	help
>  	  IBM Console device driver which makes use of RTAS
>  
> +config HVC_BEAT
> +	bool "Toshiba's Beat Hypervisor Console support"
> +	depends on PPC_CELLEB
> +	select HVC_DRIVER
> +	help
> +	  Toshiba's Cell Reference Set Beat Console device driver
> +
>  config HVCS
>  	tristate "IBM Hypervisor Virtual Console Server support"
>  	depends on PPC_PSERIES
> Index: linux-powerpc-git/drivers/char/Makefile
> diff -u linux-powerpc-git/drivers/char/Makefile:1.1.1.1 linux-powerpc-git/drivers/char/Makefile:1.2
> --- linux-powerpc-git/drivers/char/Makefile:1.1.1.1	Wed Dec  6 08:24:08 2006
> +++ linux-powerpc-git/drivers/char/Makefile	Wed Dec  6 08:43:15 2006
> @@ -44,6 +44,7 @@
>  obj-$(CONFIG_HVC_CONSOLE)	+= hvc_vio.o hvsi.o
>  obj-$(CONFIG_HVC_ISERIES)	+= hvc_iseries.o
>  obj-$(CONFIG_HVC_RTAS)		+= hvc_rtas.o
> +obj-$(CONFIG_HVC_BEAT)		+= hvc_beat.o
>  obj-$(CONFIG_HVC_DRIVER)	+= hvc_console.o
>  obj-$(CONFIG_RAW_DRIVER)	+= raw.o
>  obj-$(CONFIG_SGI_SNSC)		+= snsc.o snsc_event.o
> Index: linux-powerpc-git/drivers/char/hvc_beat.c
> diff -u /dev/null linux-powerpc-git/drivers/char/hvc_beat.c:1.1
> --- /dev/null	Mon Dec 11 20:37:34 2006
> +++ linux-powerpc-git/drivers/char/hvc_beat.c	Wed Dec  6 08:43:15 2006
> @@ -0,0 +1,106 @@
> +/*
> + * Beat hypervisor console driver
> + *
> + * (C) Copyright 2006 TOSHIBA CORPORATION
> + *
> + * This code is based on drivers/char/hvc_rtas.c:
> + * (C) Copyright IBM Corporation 2001-2005
> + * (C) Copyright Red Hat, Inc. 2005
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, write to the Free Software Foundation, Inc.,
> + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/err.h>
> +#include <linux/string.h>
> +#include <linux/console.h>
> +#include <asm/prom.h>
> +#include <asm/hvconsole.h>
> +
> +#include "hvc_console.h"
> +
> +extern int64_t beat_get_term_char(uint64_t, uint64_t *, uint64_t *, uint64_t *);
> +extern int64_t beat_put_term_char(uint64_t, uint64_t, uint64_t, uint64_t);
> +
> +struct hvc_struct *hvc_beat_dev = NULL;
> +
> +static int hvc_beat_get_chars(uint32_t vtermno, char *buf, int cnt)
> +{
> +	unsigned long kb[2];
> +	unsigned long got;
> +
> +	if (beat_get_term_char(vtermno, &got, &kb[0], &kb[1]) == 0) {
> +		memcpy(buf, kb, got);
> +		return got;
> +	} else {
> +		return 0;
> +	}
> +}
> +
> +static int hvc_beat_put_chars(uint32_t vtermno, const char *buf, int cnt)
> +{
> +	unsigned long kb[2];
> +
> +	memcpy(kb, buf, sizeof(kb));
> +	beat_put_term_char(vtermno, cnt, kb[0], kb[1]);
> +	return cnt;
> +}
> +
> +static struct hv_ops hvc_beat_get_put_ops = {
> +	.get_chars = hvc_beat_get_chars,
> +	.put_chars = hvc_beat_put_chars,
> +};
> +
> +static int hvc_beat_useit = 1;
> +
> +static int hvc_beat_config(char *p)
> +{
> +	hvc_beat_useit = simple_strtoul(p, NULL, 0);
> +	return 0;
> +}
> +
> +static int hvc_beat_console_init(void)
> +{
> +	if (hvc_beat_useit && machine_is_compatible("Beat")) {
> +		hvc_instantiate(0, 0, &hvc_beat_get_put_ops);
> +	}
> +	return 0;
> +}

Why do you need hvc_beat_useit? You don't want the driver loaded at all
sometimes? Is console=blah not enough?

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 6/15] hypervisor console driver for Celleb

[Ishizaki Kou]

Michael-san,

Thank you for your comment.

From: Michael Ellerman <michael@ellerman.id.au>
Subject: Re: [PATCH 6/15] hypervisor console driver for Celleb
Date: Tue, 12 Dec 2006 15:09:26 +1100

> Why do you need hvc_beat_useit? You don't want the driver loaded at all
> sometimes? Is console=blah not enough?

Mainly it is used to stop polling "beat" console temporary without
changing code. Because the "beat" console does not use interrupts
and/or buffers, polling the console and printing to the console
impact kernel performance deeply. Adding to that, the code was written
when the serial port driver is not implemented.

In short, we'd like to stop working the "beat" driver in some situation;
I don't know that console=blah is enough to do that.

Best regards,
Kou Ishizaki

Re: [PATCH 6/15] hypervisor console driver for Celleb

[Ishizaki Kou]

> On Tue, 2006-12-12 at 15:53 +1100, Benjamin Herrenschmidt wrote:
> > On Tue, 2006-12-12 at 15:13 +1100, Michael Ellerman wrote:
> > > On Tue, 2006-12-12 at 13:02 +0900, Ishizaki Kou wrote:
> > > > op_model_cell supports native Cell processor. Under LPAR environment,
> > > > it cannot work.
> > > > 
> > > > Signed-off-by: Kou Ishizaki <kou.ishizaki.co.jp>
> > > > ---
> > > > 
> > > > Index: linux-powerpc-git/arch/powerpc/oprofile/common.c
> > > > diff -u linux-powerpc-git/arch/powerpc/oprofile/common.c:1.1.1.1 linux-powerpc-git/arch/powerpc/oprofile/common.c:1.2
> > > > --- linux-powerpc-git/arch/powerpc/oprofile/common.c:1.1.1.1	 Wed Dec  6 08:25:43 2006
> > > > +++ linux-powerpc-git/arch/powerpc/oprofile/common.c		 Wed Dec  6 08:43:15 2006
> > > > @@ -149,6 +149,8 @@
> > > >  #ifdef CONFIG_PPC64
> > > >  #ifdef CONFIG_PPC_CELL_NATIVE
> > > >			case PPC_OPROFILE_CELL:
> > > > +			if (firmware_has_feature(FW_FEATURE_LPAR))
> > > > +					return -ENODEV;
> > > >							model = &op_model_cell;
> > > >								break;
> > > >  #endif
> > > 
> > > Is FW_FEATURE_LPAR is the right switch to use here, I'm not sure.
> > 
> > For now, it probably is as none of the LPAR env. for cell work with that
> > op_model.
> 
> Sure, there's probably twenty different things we could use at the
> moment, I'm just wondering out loud if LPAR is the best thing to reduce
> the chance we have to change it in the future.

We will implement Cell LPAR support in the future. The code doesn't
exist now, and it tries to use oprofile_timer by returning -ENODEV
here.
				   
Thank you,
Kou Ishizaki
Toshiba

Re: [PATCH 6/15] hypervisor console driver for Celleb

[Ishizaki Kou]

Sorry. My previous mail is a reply to "Re: [PATCH 12/15] powerpc: Do not use op_model_cell under LPAR".

Kou Ishizaki

Re: [PATCH 6/15] hypervisor console driver for Celleb

[Linas Vepstas]

On Tue, Dec 12, 2006 at 12:31:29PM +0900, Ishizaki Kou wrote:
> +
> +static int hvc_beat_get_chars(uint32_t vtermno, char *buf, int cnt)
> +{
> +	unsigned long kb[2];
> +	unsigned long got;
> +
> +	if (beat_get_term_char(vtermno, &got, &kb[0], &kb[1]) == 0) {
> +		memcpy(buf, kb, got);
> +		return got;

This seems to completely ignore "cnt". Thus, I presume that
beat_get_term_char might return more chars than there is room for in buf,
thus corrupting something, somewhere.


> +static int hvc_beat_put_chars(uint32_t vtermno, const char *buf, int cnt)
> +{
> +	unsigned long kb[2];
> +
> +	memcpy(kb, buf, sizeof(kb));
> +	beat_put_term_char(vtermno, cnt, kb[0], kb[1]);
> +	return cnt;
> +}

I can't imagine how this can possibly work. 
What if "cnt" is greater than 8?

--linas

Re: [PATCH 6/15] hypervisor console driver for Celleb

[Ishizaki Kou]

Linas-san,

Thanks for your comment.

> On Tue, Dec 12, 2006 at 12:31:29PM +0900, Ishizaki Kou wrote:
> > +
> > +static int hvc_beat_get_chars(uint32_t vtermno, char *buf, int cnt)
> > +{
> > +	unsigned long kb[2];
> > +	unsigned long got;
> > +
> > +	if (beat_get_term_char(vtermno, &got, &kb[0], &kb[1]) == 0) {
> > +	   memcpy(buf, kb, got);
> > +		       return got;

> This seems to completely ignore "cnt". Thus, I presume that
> beat_get_term_char might return more chars than there is room for in buf,
> thus corrupting something, somewhere.

This depends "beat_get_term_char" returns only one character at once
(for now), and assumes cnt > 0. This assumption will reduce code for
now.

> > +static int hvc_beat_put_chars(uint32_t vtermno, const char *buf, int cnt)
> > +{
> > +	unsigned long kb[2];
> > +
> > +	memcpy(kb, buf, sizeof(kb));
> > +	beat_put_term_char(vtermno, cnt, kb[0], kb[1]);
> > +	return cnt;
> > +}

> I can't imagine how this can possibly work. 
> What if "cnt" is greater than 8?

This routine assumes that 0 <= cnt <= 16, that is already checked by
caller. (Note that "unsigned long" is 8 bytes long at ppc64)

Best regards,
Kou Ishizaki

Re: [PATCH 6/15] hypervisor console driver for Celleb

[Linas Vepstas]

On Thu, Dec 14, 2006 at 10:42:16AM +0900, Ishizaki Kou wrote:
> Linas-san,
> 
> Thanks for your comment.
> 
> > On Tue, Dec 12, 2006 at 12:31:29PM +0900, Ishizaki Kou wrote:
> > > +
> > > +static int hvc_beat_get_chars(uint32_t vtermno, char *buf, int cnt)
> > > +{
> > > +	unsigned long kb[2];
> > > +	unsigned long got;
> > > +
> > > +	if (beat_get_term_char(vtermno, &got, &kb[0], &kb[1]) == 0) {
> > > +	   memcpy(buf, kb, got);
> > > +		       return got;
> 
> > This seems to completely ignore "cnt". Thus, I presume that
> > beat_get_term_char might return more chars than there is room for in buf,
> > thus corrupting something, somewhere.
> 
> This depends "beat_get_term_char" returns only one character at once
> (for now), and assumes cnt > 0. This assumption will reduce code for
> now.

But it will break in the future. If new firmware is released for beat,
that returns more than one char, then it will silently corrupt old kernels
on rare occasions, making the problem hard to find. What's more, the 
firmware people might forget to tell you about thier change, and
so you won't submit a matching update to the linux kernel. (Or you may
have a new job by then, or have lost interest/got bored, etc.)
Years later, someone will finally debug the problem, after a long and
difficult search, and they'll curse this code.  Better do it right, now.

> > > +static int hvc_beat_put_chars(uint32_t vtermno, const char *buf, int cnt)
> > > +{
> > > +	unsigned long kb[2];
> > > +
> > > +	memcpy(kb, buf, sizeof(kb));
> > > +	beat_put_term_char(vtermno, cnt, kb[0], kb[1]);
> > > +	return cnt;
> > > +}
> 
> > I can't imagine how this can possibly work. 
> > What if "cnt" is greater than 8?
> 
> This routine assumes that 0 <= cnt <= 16, that is already checked by
> caller. (Note that "unsigned long" is 8 bytes long at ppc64)

Actually, its N_OUTBUF which is currently defined to be 16. However,
that may someday change, in which case you'd have a bug, again.

--linas

Re: [PATCH 6/15] hypervisor console driver for Celleb

[Ishizaki Kou]

Linas-san,

> On Thu, Dec 14, 2006 at 10:42:16AM +0900, Ishizaki Kou wrote:
> > Linas-san,
> > 
> > Thanks for your comment.
> > 
> > > On Tue, Dec 12, 2006 at 12:31:29PM +0900, Ishizaki Kou wrote:
> > > > +
> > > > +static int hvc_beat_get_chars(uint32_t vtermno, char *buf, int cnt)
> > > > +{
> > > > +	unsigned long kb[2];
> > > > +	unsigned long got;
> > > > +
> > > > +	if (beat_get_term_char(vtermno, &got, &kb[0], &kb[1]) == 0) {
> > > > +	   memcpy(buf, kb, got);
> > > > +		              return got;
> > 
> > > This seems to completely ignore "cnt". Thus, I presume that
> > > beat_get_term_char might return more chars than there is room for in buf,
> > > thus corrupting something, somewhere.

> > This depends "beat_get_term_char" returns only one character at once
> > (for now), and assumes cnt > 0. This assumption will reduce code for
> > now.

> But it will break in the future. If new firmware is released for beat,
> that returns more than one char, then it will silently corrupt old kernels
> on rare occasions, making the problem hard to find. What's more, the 
> firmware people might forget to tell you about thier change, and
> so you won't submit a matching update to the linux kernel. (Or you may
> have a new job by then, or have lost interest/got bored, etc.)
> Years later, someone will finally debug the problem, after a long and
> difficult search, and they'll curse this code.  Better do it right, now.

Okay, we will do.

We have to remark 'this is NOT FOR PRODUCT,' since console I/O of Beat
is HORRIBLY slow (putting characters to console waits till
the characters actually putted to the serial port. No queue is available.)


> > > > +static int hvc_beat_put_chars(uint32_t vtermno, const char *buf, int cnt)
> > > > +{
> > > > +	unsigned long kb[2];
> > > > +
> > > > +	memcpy(kb, buf, sizeof(kb));
> > > > +	beat_put_term_char(vtermno, cnt, kb[0], kb[1]);
> > > > +	return cnt;
> > > > +}
> > 
> > > I can't imagine how this can possibly work. 
> > > What if "cnt" is greater than 8?
> > 
> > This routine assumes that 0 <= cnt <= 16, that is already checked by
> > caller. (Note that "unsigned long" is 8 bytes long at ppc64)
> 
> Actually, its N_OUTBUF which is currently defined to be 16. However,
> that may someday change, in which case you'd have a bug, again.

Okay, we understand your opinion, and will do so.

Best regards,
Kou Ishizaki.