at_keyboard flush on i386-qemu

at_keyboard flush on i386-qemu

[Robert Millan]

Hi,

Pavel pointed out earlier that sometimes when starting on i386-qemu GRUB
receives spurious events from AT keyboard.  It seems that it is the role
of the firmware to flush this buffer at startup.

Unless someone has a better idea, I would fix this with:

  - Moving at_keyboard to kernel on i386-qemu.

  - [ifdef GRUB_MACHINE_QEMU]: flush the input buffer at at_keyboard
    startup by reading and discarding events for a fixed amount of time.

I don't like that we have a race here.  Suggestions welcome on how to
improve that, but TTBOMK if there's more than one event you can't tell
when you're processing the last one.

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

Re: at_keyboard flush on i386-qemu

[Pavel Roskin]

On Sat, 2009-06-27 at 13:25 +0200, Robert Millan wrote:
> Hi,
> 
> Pavel pointed out earlier that sometimes when starting on i386-qemu GRUB
> receives spurious events from AT keyboard.

Yes, it's still happening, but not always.  Approximately half of the
time I'm getting "2" at the prompt.

>   It seems that it is the role
> of the firmware to flush this buffer at startup.
> 
> Unless someone has a better idea, I would fix this with:
> 
>   - Moving at_keyboard to kernel on i386-qemu.
> 
>   - [ifdef GRUB_MACHINE_QEMU]: flush the input buffer at at_keyboard
>     startup by reading and discarding events for a fixed amount of time.

I would just read and discard the keyboard data from the port at startup
without embedding at_keyboard.

> I don't like that we have a race here.  Suggestions welcome on how to
> improve that, but TTBOMK if there's more than one event you can't tell
> when you're processing the last one.

I think there is at most one event.  And I suspect it's due to a qemu
bug.  Or at least qemu could do better by starting in the same state
every time.

-- 
Regards,
Pavel Roskin

Re: at_keyboard flush on i386-qemu

[Robert Millan]

On Mon, Jun 29, 2009 at 12:02:55AM -0400, Pavel Roskin wrote:
> 
> I would just read and discard the keyboard data from the port at startup
> without embedding at_keyboard.

Seems fine.

> > I don't like that we have a race here.  Suggestions welcome on how to
> > improve that, but TTBOMK if there's more than one event you can't tell
> > when you're processing the last one.
> 
> I think there is at most one event.

Actually, you need two events for a single key press (MAKE + BREAK), and
I've seen it print two keys sometimes.

> And I suspect it's due to a qemu
> bug.

Well, other firmwares (coreboot, bios.bin) don't have this problem.  It's
probably something that you need to do on real hardware anyway (which we
might want to support someday).

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

Re: at_keyboard flush on i386-qemu

[Pavel Roskin]

Quoting Robert Millan <rmh@aybabtu.com>:

> On Mon, Jun 29, 2009 at 12:02:55AM -0400, Pavel Roskin wrote:
>>
>> I would just read and discard the keyboard data from the port at startup
>> without embedding at_keyboard.
>
> Seems fine.
>
>> > I don't like that we have a race here.  Suggestions welcome on how to
>> > improve that, but TTBOMK if there's more than one event you can't tell
>> > when you're processing the last one.
>>
>> I think there is at most one event.
>
> Actually, you need two events for a single key press (MAKE + BREAK), and
> I've seen it print two keys sometimes.

OK, let's read e.g. 8 events to be safe.

>> And I suspect it's due to a qemu
>> bug.
>
> Well, other firmwares (coreboot, bios.bin) don't have this problem.  It's
> probably something that you need to do on real hardware anyway (which we
> might want to support someday).

That's what coreboot is for.  Otherwise we would need to reimplement coreboot.

-- 
Regards,
Pavel Roskin

[PATCH] use ljmp to reboot (Re: at_keyboard flush on i386-qemu)

[Robert Millan]

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

On Sat, Jun 27, 2009 at 01:25:21PM +0200, Robert Millan wrote:
> 
> Hi,
> 
> Pavel pointed out earlier that sometimes when starting on i386-qemu GRUB
> receives spurious events from AT keyboard.  It seems that it is the role
> of the firmware to flush this buffer at startup.
> 
> Unless someone has a better idea, I would fix this with:
> 
>   - Moving at_keyboard to kernel on i386-qemu.
> 
>   - [ifdef GRUB_MACHINE_QEMU]: flush the input buffer at at_keyboard
>     startup by reading and discarding events for a fixed amount of time.
> 
> I don't like that we have a race here.  Suggestions welcome on how to
> improve that, but TTBOMK if there's more than one event you can't tell
> when you're processing the last one.

Actually I diagnosed this completely wrong.  The real problem is we're
using the keyboard to reboot the machine, which (at least on QEMU) results
in spurious keys being put in the buffer.

I think we should just jump to 0xffff0 to reboot.  I didn't know back when I
wrote reboot.c, but this is supposedly more reliable since we're jumping to
code in ROM which hasn't been overwritten (kern/i386/pc/startup.S uses the
same trick for its own grub_reboot()).

See attached patch.

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

[-- Attachment #2: reboot.diff --]
[-- Type: text/x-diff, Size: 2585 bytes --]

2009-07-07  Robert Millan  <rmh.grub@aybabtu.com>

	* conf/i386-coreboot.rmk (reboot_mod_SOURCES): Remove
	`kern/i386/reboot.c'.
	* kern/i386/qemu/startup.S: Include `"../realmode.S"'.
	(grub_reboot): New function.
	* kern/i386/realmode.S: Include `<grub/machine/machine.h>'.
	(prot_to_real): Only enable i8086 interrupts on i386-pc.
	* include/grub/i386/coreboot/init.h (grub_reboot): New function
	prototype.

Index: conf/i386-coreboot.rmk
===================================================================
--- conf/i386-coreboot.rmk	(revision 2398)
+++ conf/i386-coreboot.rmk	(working copy)
@@ -174,7 +174,7 @@ linux_mod_CFLAGS = $(COMMON_CFLAGS)
 linux_mod_LDFLAGS = $(COMMON_LDFLAGS)
 
 # For reboot.mod.
-reboot_mod_SOURCES = commands/reboot.c kern/i386/reboot.c
+reboot_mod_SOURCES = commands/reboot.c
 reboot_mod_CFLAGS = $(COMMON_CFLAGS)
 reboot_mod_LDFLAGS = $(COMMON_LDFLAGS)
 
Index: kern/i386/qemu/startup.S
===================================================================
--- kern/i386/qemu/startup.S	(revision 2398)
+++ kern/i386/qemu/startup.S	(working copy)
@@ -95,3 +95,11 @@ codestart:
 
 	/* This should never happen.  */
 	jmp	EXT_C(grub_stop)
+
+#include "../realmode.S"
+
+FUNCTION(grub_reboot)
+	call    prot_to_real
+	.code16
+	ljmp	$0xf000, $0xfff0
+	.code32
Index: kern/i386/realmode.S
===================================================================
--- kern/i386/realmode.S	(revision 2398)
+++ kern/i386/realmode.S	(working copy)
@@ -41,6 +41,8 @@
  *       with "ret $N" where N is ((the number of arguments) - 3) * 4.
  */
 
+#include <grub/machine/machine.h>
+
 /*
  *  This is the area for all of the special variables.
  */
@@ -215,8 +217,10 @@ realcseg:
 	movw	%ax, %gs
 	movw	%ax, %ss
 
+#ifdef GRUB_MACHINE_PCBIOS
 	/* restore interrupts */
 	sti
+#endif
 
 	/* return on new stack! */
 	DATA32	ret
Index: include/grub/i386/coreboot/init.h
===================================================================
--- include/grub/i386/coreboot/init.h	(revision 2398)
+++ include/grub/i386/coreboot/init.h	(working copy)
@@ -1,6 +1,6 @@
 /*
  *  GRUB  --  GRand Unified Bootloader
- *  Copyright (C) 2007  Free Software Foundation, Inc.
+ *  Copyright (C) 2007,2009  Free Software Foundation, Inc.
  *
  *  GRUB is free software: you can redistribute it and/or modify
  *  it under the terms of the GNU General Public License as published by
@@ -24,5 +24,6 @@
 
 void EXPORT_FUNC(grub_stop) (void) __attribute__ ((noreturn));
 void EXPORT_FUNC(grub_stop_floppy) (void);
+void EXPORT_FUNC (grub_reboot) (void) __attribute__ ((noreturn));
 
 #endif

Re: [PATCH] use ljmp to reboot (Re: at_keyboard flush on i386-qemu)

[Pavel Roskin]

On Tue, 2009-07-07 at 21:51 +0200, Robert Millan wrote:

> I think we should just jump to 0xffff0 to reboot.  I didn't know back when I
> wrote reboot.c, but this is supposedly more reliable since we're jumping to
> code in ROM which hasn't been overwritten (kern/i386/pc/startup.S uses the
> same trick for its own grub_reboot()).

I have no objections about the patch except that it breaks compilation
for i386-coreboot:

kernel_img-symlist.o:(.data+0x3b4): undefined reference to `grub_reboot'

Perhaps kern/i386/reboot.c should be kept, but the code should only be
enabled for coreboot.

By the way, GRUB_MACHINE_LINUXBIOS should be renamed to
GRUB_MACHINE_COREBOOT.  Its use in loader/multiboot_loader.c is dubious.

-- 
Regards,
Pavel Roskin

Re: [PATCH] use ljmp to reboot (Re: at_keyboard flush on i386-qemu)

[Robert Millan]

On Tue, Jul 07, 2009 at 04:51:47PM -0400, Pavel Roskin wrote:
> On Tue, 2009-07-07 at 21:51 +0200, Robert Millan wrote:
> 
> > I think we should just jump to 0xffff0 to reboot.  I didn't know back when I
> > wrote reboot.c, but this is supposedly more reliable since we're jumping to
> > code in ROM which hasn't been overwritten (kern/i386/pc/startup.S uses the
> > same trick for its own grub_reboot()).
> 
> I have no objections about the patch except that it breaks compilation
> for i386-coreboot:
> 
> kernel_img-symlist.o:(.data+0x3b4): undefined reference to `grub_reboot'
> 
> Perhaps kern/i386/reboot.c should be kept, but the code should only be
> enabled for coreboot.

Actually, coreboot has the same problem and should use the same approach
(these two ports are almost identical).

> By the way, GRUB_MACHINE_LINUXBIOS should be renamed to
> GRUB_MACHINE_COREBOOT.

Fine with me.  Only lack of time prevents this :-)

> Its use in loader/multiboot_loader.c is dubious.

This should become "ifdef __i386__" someday.  Unfortunately some things
(e.g. mmap) associated with the multiboot loader are not portable.

Ah, but GRUB_MACHINE_QEMU should be there too.  This can be added already.

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."