[PATCH] export -boot parameter as qemu_boot{0,1,2}

[PATCH] export -boot parameter as qemu_boot{0,1,2}

[Robert Millan]

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


This patch makes GRUB gather the -boot parameter from CMOS and
export it as a set of variables (qemu_boot{0,1,2}), which can
be observed in grub.cfg scripts.

-- 
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: qemu_boot.diff --]
[-- Type: text/x-diff, Size: 2018 bytes --]

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

	* kern/i386/coreboot/init.c (QEMU_CMOS_BOOT_DEVICE_01)
	(QEMU_CMOS_BOOT_DEVICE_2, QEMU_CMOS_BOOT_FLOPPY)
	(QEMU_CMOS_BOOT_HDD, QEMU_CMOS_BOOT_CDROM)
	(QEMU_CMOS_BOOT_NETWORK): New macros.
	[GRUB_MACHINE_QEMU] (grub_machine_init): Initialize
	`qemu_bootX' variables using the CMOS values corresponding
	to `-boot' parameter in QEMU command-line.

Index: kern/i386/coreboot/init.c
===================================================================
--- kern/i386/coreboot/init.c	(revision 2367)
+++ kern/i386/coreboot/init.c	(working copy)
@@ -36,6 +36,15 @@
 #include <grub/cpu/io.h>
 #include <grub/cpu/kernel.h>
 #include <grub/cpu/tsc.h>
+#include <grub/cpu/cmos.h>
+
+#define QEMU_CMOS_BOOT_DEVICE_01	0x3d
+#define QEMU_CMOS_BOOT_DEVICE_2		0x38
+
+#define QEMU_CMOS_BOOT_FLOPPY		0x01
+#define QEMU_CMOS_BOOT_HDD		0x02
+#define QEMU_CMOS_BOOT_CDROM		0x03
+#define QEMU_CMOS_BOOT_NETWORK		0x04
 
 #define GRUB_FLOPPY_REG_DIGITAL_OUTPUT		0x3f2
 
@@ -125,6 +134,45 @@ grub_machine_init (void)
   grub_machine_mmap_iterate (heap_init);
 
   grub_tsc_init ();
+
+#ifdef GRUB_MACHINE_QEMU
+  {
+    grub_uint8_t device[3];
+    unsigned int i, j;
+
+    device[0] = grub_cmos_read (QEMU_CMOS_BOOT_DEVICE_01) & 0x0f;
+    device[1] = grub_cmos_read (QEMU_CMOS_BOOT_DEVICE_01) >> 4;
+    device[2] = grub_cmos_read (QEMU_CMOS_BOOT_DEVICE_2) >> 4;
+
+    auto void qemu_boot_ata (int n, int ata);
+    void qemu_boot_ata (int n, int ata)
+      {
+	char ata_device[] = "ataX";
+	char qemu_boot[] = "qemu_bootX";
+
+	qemu_boot[9] = n + '0';
+	ata_device[3] = ata + '0';
+
+	grub_env_set (qemu_boot, ata_device);
+      }
+
+    j = 0;
+    for (i = 0; i < sizeof (device) / sizeof (device[0]); i++)
+      switch (device[i])
+	{
+	case QEMU_CMOS_BOOT_HDD:
+	  qemu_boot_ata (j++, 0);
+	  break;
+	case QEMU_CMOS_BOOT_CDROM:
+	  qemu_boot_ata (j++, 2);
+	  break;
+	case QEMU_CMOS_BOOT_FLOPPY:
+	case QEMU_CMOS_BOOT_NETWORK:
+	default:
+	  break;
+	}
+  }
+#endif
 }
 
 void

Re: [PATCH] export -boot parameter as qemu_boot{0,1,2}

[Pavel Roskin]

On Sat, 2009-06-27 at 14:12 +0200, Robert Millan wrote:
> This patch makes GRUB gather the -boot parameter from CMOS and
> export it as a set of variables (qemu_boot{0,1,2}), which can
> be observed in grub.cfg scripts.

I think qemu constants belong to a separate header.

Use of a nested function seems totally unwarranted.  We actually agreed
a while ago that they are bad, but nobody had time to remove them.

Please use ARRAY_SIZE to calculate the number of elements.

-- 
Regards,
Pavel Roskin

Re: [PATCH] export -boot parameter as qemu_boot{0,1,2}

[Robert Millan]

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

On Mon, Jun 29, 2009 at 12:09:16AM -0400, Pavel Roskin wrote:
> On Sat, 2009-06-27 at 14:12 +0200, Robert Millan wrote:
> > This patch makes GRUB gather the -boot parameter from CMOS and
> > export it as a set of variables (qemu_boot{0,1,2}), which can
> > be observed in grub.cfg scripts.
> 
> I think qemu constants belong to a separate header.

Ok, done.

> Please use ARRAY_SIZE to calculate the number of elements.

Done.

> Use of a nested function seems totally unwarranted.

I did it to make the code more readable.  Declaring functions that will
be used locally inside their scope improves code clarity and prevents
mistakes (like attempting to run them from somewhere else).

> We actually agreed
> a while ago that they are bad, but nobody had time to remove them.

I didn't participate in that discussion.  Did Marco and Okuji agree with this?

-- 
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: qemu_boot.diff --]
[-- Type: text/x-diff, Size: 3009 bytes --]

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

	* include/grub/i386/qemu/init.h: Include `<grub/cpu/cmos.h>'.
	(QEMU_CMOS_BOOT_DEVICE_01, QEMU_CMOS_BOOT_DEVICE_2)
	(QEMU_CMOS_BOOT_FLOPPY, QEMU_CMOS_BOOT_HDD)
	(QEMU_CMOS_BOOT_CDROM, QEMU_CMOS_BOOT_NETWORK): New macros.

	* kern/i386/coreboot/init.c [GRUB_MACHINE_QEMU]
	(grub_machine_init): Initialize `qemu_bootX' variables using
	the CMOS values corresponding to `-boot' parameter in QEMU
	command-line.

Index: kern/i386/coreboot/init.c
===================================================================
--- kern/i386/coreboot/init.c	(revision 2371)
+++ kern/i386/coreboot/init.c	(working copy)
@@ -125,6 +125,45 @@ grub_machine_init (void)
   grub_machine_mmap_iterate (heap_init);
 
   grub_tsc_init ();
+
+#ifdef GRUB_MACHINE_QEMU
+  {
+    grub_uint8_t device[3];
+    unsigned int i, j;
+
+    device[0] = grub_cmos_read (QEMU_CMOS_BOOT_DEVICE_01) & 0x0f;
+    device[1] = grub_cmos_read (QEMU_CMOS_BOOT_DEVICE_01) >> 4;
+    device[2] = grub_cmos_read (QEMU_CMOS_BOOT_DEVICE_2) >> 4;
+
+    auto void qemu_boot_ata (int n, int ata);
+    void qemu_boot_ata (int n, int ata)
+      {
+	char ata_device[] = "ataX";
+	char qemu_boot[] = "qemu_bootX";
+
+	qemu_boot[9] = n + '0';
+	ata_device[3] = ata + '0';
+
+	grub_env_set (qemu_boot, ata_device);
+      }
+
+    j = 0;
+    for (i = 0; i < ARRAY_SIZE (device); i++)
+      switch (device[i])
+	{
+	case QEMU_CMOS_BOOT_HDD:
+	  qemu_boot_ata (j++, 0);
+	  break;
+	case QEMU_CMOS_BOOT_CDROM:
+	  qemu_boot_ata (j++, 2);
+	  break;
+	case QEMU_CMOS_BOOT_FLOPPY:
+	case QEMU_CMOS_BOOT_NETWORK:
+	default:
+	  break;
+	}
+  }
+#endif
 }
 
 void
Index: include/grub/i386/qemu/init.h
===================================================================
--- include/grub/i386/qemu/init.h	(revision 2371)
+++ include/grub/i386/qemu/init.h	(working copy)
@@ -1 +1,33 @@
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 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
+ *  the Free Software Foundation, either version 3 of the License, or
+ *  (at your option) any later version.
+ *
+ *  GRUB 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 GRUB.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef GRUB_INIT_I386_QEMU_HEADER
+#define GRUB_INIT_I386_QEMU_HEADER 1
+
+#include <grub/cpu/cmos.h>
 #include <grub/i386/coreboot/init.h>
+
+#define QEMU_CMOS_BOOT_DEVICE_01	0x3d
+#define QEMU_CMOS_BOOT_DEVICE_2		0x38
+
+#define QEMU_CMOS_BOOT_FLOPPY		0x01
+#define QEMU_CMOS_BOOT_HDD		0x02
+#define QEMU_CMOS_BOOT_CDROM		0x03
+#define QEMU_CMOS_BOOT_NETWORK		0x04
+
+#endif

Re: [PATCH] export -boot parameter as qemu_boot{0,1,2}

[Vladimir 'phcoder' Serbinenko]

On Mon, Jun 29, 2009 at 3:49 PM, Robert Millan<rmh@aybabtu.com> wrote:
>> We actually agreed
>> a while ago that they are bad, but nobody had time to remove them.
>
> I didn't participate in that discussion.  Did Marco and Okuji agree with this?
No. It didn't really end in a clear consesus either (partially:
everybody agreed to remove trampolines). IMO there is nothing bad in
nested function as long as they don't generate trampolines.
>
> --
> 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."
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>
>



-- 
Regards
Vladimir 'phcoder' Serbinenko

Personal git repository: http://repo.or.cz/w/grub2/phcoder.git

Re: [PATCH] export -boot parameter as qemu_boot{0,1,2}

[Pavel Roskin]

On Mon, 2009-06-29 at 22:18 +0200, Vladimir 'phcoder' Serbinenko wrote:
> On Mon, Jun 29, 2009 at 3:49 PM, Robert Millan<rmh@aybabtu.com> wrote:
> >> We actually agreed
> >> a while ago that they are bad, but nobody had time to remove them.
> >
> > I didn't participate in that discussion.  Did Marco and Okuji agree with this?
> No. It didn't really end in a clear consesus either (partially:
> everybody agreed to remove trampolines). IMO there is nothing bad in
> nested function as long as they don't generate trampolines.

I guess it will be discussed when the patches will be posted.

In any case, there is no need for non-static nested functions in this
particular case.

-- 
Regards,
Pavel Roskin

Re: [PATCH] export -boot parameter as qemu_boot{0,1,2}

[Pavel Roskin]

On Mon, 2009-06-29 at 15:49 +0200, Robert Millan wrote:

> > Use of a nested function seems totally unwarranted.
> 
> I did it to make the code more readable.  Declaring functions that will
> be used locally inside their scope improves code clarity and prevents
> mistakes (like attempting to run them from somewhere else).

However, they are not widely used.  In GRUB, nested functions are used
to allow passing them as callbacks that can access local variables of
the containing function.

There is a problem associates with nested functions.  Some versions of
gcc miscompile them with -mregparm=3.  Arguments other than the first
one are not passed correctly.  There was a test in configure.ac, but it
turned out to be wrong, so we are always specifying
__attribute__((__regparm__(1))) for nested functions taking more than
one argument on i386 (kernel code only).

Just look for NESTED_FUNC_ATTR to see what I mean.  We had several bugs
because NESTED_FUNC_ATTR wasn't used consistently.

I don't know if NESTED_FUNC_ATTR is needed in your case, or it's only
needed when the function is passed as an argument.  In any case, I think
it's an overkill for the problem you are solving.

> > We actually agreed
> > a while ago that they are bad, but nobody had time to remove them.
> 
> I didn't participate in that discussion.  Did Marco and Okuji agree with this?

They didn't participate.

-- 
Regards,
Pavel Roskin

Re: [PATCH] export -boot parameter as qemu_boot{0,1,2}

[Robert Millan]

On Mon, Jun 29, 2009 at 06:59:22PM -0400, Pavel Roskin wrote:
> On Mon, 2009-06-29 at 15:49 +0200, Robert Millan wrote:
> 
> > > Use of a nested function seems totally unwarranted.
> > 
> > I did it to make the code more readable.  Declaring functions that will
> > be used locally inside their scope improves code clarity and prevents
> > mistakes (like attempting to run them from somewhere else).
> 
> However, they are not widely used.  In GRUB, nested functions are used
> to allow passing them as callbacks that can access local variables of
> the containing function.
> 
> There is a problem associates with nested functions.  Some versions of
> gcc miscompile them with -mregparm=3.  Arguments other than the first
> one are not passed correctly.  There was a test in configure.ac, but it
> turned out to be wrong, so we are always specifying
> __attribute__((__regparm__(1))) for nested functions taking more than
> one argument on i386 (kernel code only).
> 
> Just look for NESTED_FUNC_ATTR to see what I mean.  We had several bugs
> because NESTED_FUNC_ATTR wasn't used consistently.
> 
> I don't know if NESTED_FUNC_ATTR is needed in your case, or it's only
> needed when the function is passed as an argument.  In any case, I think
> it's an overkill for the problem you are solving.

Isn't this something that should be handled at GCC level?  When you declare a
local variable as "static", GCC knows it should be allocated statically.  Is
there nothing similar for local functions?

I'd rather rewrite the function differently than spreading it in different
parts of the file.  There's no need to make the code harder to figure out
for the sake of this implementation problem.

-- 
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."