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
Michael-san,
Thank you for your comment.
From: Michael Ellerman <redacted>
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
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
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
On Tue, Dec 12, 2006 at 12:31:29PM +0900, Ishizaki Kou wrote:
quoted
+
+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.
quoted
+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
On Thu, Dec 14, 2006 at 10:42:16AM +0900, Ishizaki Kou wrote:
Linas-san,
Thanks for your comment.
quoted
On Tue, Dec 12, 2006 at 12:31:29PM +0900, Ishizaki Kou wrote:
quoted
+
+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;
quoted
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.
quoted
quoted
+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;
+}
quoted
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
On Thu, Dec 14, 2006 at 10:42:16AM +0900, Ishizaki Kou wrote:
quoted
Linas-san,
Thanks for your comment.
quoted
On Tue, Dec 12, 2006 at 12:31:29PM +0900, Ishizaki Kou wrote:
quoted
+
+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;
quoted
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.
quoted
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.)
quoted
quoted
quoted
+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;
+}
quoted
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.