[PATCH] Fail gracefuly when attempting to load 64-bit kFreeBSD on IA32 CPU

[PATCH] Fail gracefuly when attempting to load 64-bit kFreeBSD on IA32 CPU

[Robert Millan]

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


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

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

	Fail gracefuly when attempting to load 64-bit kFreeBSD on IA32 CPU.

	* include/grub/i386/cpuid.h: New file.
	* commands/i386/cpuid.c: Include `<grub/i386/cpuid.h>'.
	(has_longmode): Rename to ...
	(grub_cpuid_has_longmode): ... this.  Update all users.  Remove
	`unsigned' attribute.
	* loader/i386/bsd.c: Include `<grub/i386/cpuid.h>'.
	(grub_bsd_load_elf): Fail if load of 64-bit kernel was requested
	on a CPU that doesn't implement AMD64 instruction set.

Index: include/grub/i386/cpuid.h
===================================================================
--- include/grub/i386/cpuid.h	(revision 0)
+++ include/grub/i386/cpuid.h	(revision 0)
@@ -0,0 +1,19 @@
+/*
+ *  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/>.
+ */
+
+extern unsigned char grub_cpuid_has_longmode;
Index: commands/i386/cpuid.c
===================================================================
--- commands/i386/cpuid.c	(revision 2626)
+++ commands/i386/cpuid.c	(working copy)
@@ -1,7 +1,7 @@
 /* cpuid.c - test for CPU features */
 /*
  *  GRUB  --  GRand Unified Bootloader
- *  Copyright (C) 2006, 2007  Free Software Foundation, Inc.
+ *  Copyright (C) 2006, 2007, 2009  Free Software Foundation, Inc.
  *  Based on gcc/gcc/config/i386/driver-i386.c
  *
  *  GRUB is free software: you can redistribute it and/or modify
@@ -24,6 +24,7 @@
 #include <grub/env.h>
 #include <grub/command.h>
 #include <grub/extcmd.h>
+#include <grub/i386/cpuid.h>
 
 #define cpuid(num,a,b,c,d) \
   asm volatile ("xchgl %%ebx, %1; cpuid; xchgl %%ebx, %1" \
@@ -38,14 +39,14 @@ static const struct grub_arg_option options[] =
 
 #define bit_LM (1 << 29)
 
-static unsigned char has_longmode = 0;
+unsigned char grub_cpuid_has_longmode = 0;
 
 static grub_err_t
 grub_cmd_cpuid (grub_extcmd_t cmd __attribute__ ((unused)),
 		int argc __attribute__ ((unused)),
 		char **args __attribute__ ((unused)))
 {
-  return has_longmode ? GRUB_ERR_NONE
+  return grub_cpuid_has_longmode ? GRUB_ERR_NONE
     : grub_error (GRUB_ERR_TEST_FAILURE, "false");
 }
 
@@ -55,7 +56,7 @@ GRUB_MOD_INIT(cpuid)
 {
 #ifdef __x86_64__
   /* grub-emu */
-  has_longmode = 1;
+  grub_cpuid_has_longmode = 1;
 #else
   unsigned int eax, ebx, ecx, edx;
   unsigned int max_level;
@@ -82,7 +83,7 @@ GRUB_MOD_INIT(cpuid)
     goto done;
 
   cpuid (0x80000001, eax, ebx, ecx, edx);
-  has_longmode = !!(edx & bit_LM);
+  grub_cpuid_has_longmode = !!(edx & bit_LM);
 done:
 #endif
 
Index: loader/i386/bsd.c
===================================================================
--- loader/i386/bsd.c	(revision 2626)
+++ loader/i386/bsd.c	(working copy)
@@ -19,6 +19,7 @@
 #include <grub/loader.h>
 #include <grub/cpu/loader.h>
 #include <grub/cpu/bsd.h>
+#include <grub/i386/cpuid.h>
 #include <grub/machine/init.h>
 #include <grub/machine/memory.h>
 #include <grub/memory.h>
@@ -871,6 +872,9 @@ grub_bsd_load_elf (grub_elf_t elf)
     {
       is_64bit = 1;
 
+      if (! grub_cpuid_has_longmode)
+	return grub_error (GRUB_ERR_BAD_OS, "Your CPU does not implement AMD64 architecture.");
+
       /* FreeBSD has 64-bit entry point.  */
       if (kernel_type == KERNEL_TYPE_FREEBSD)
 	{

Re: [PATCH] Fail gracefuly when attempting to load 64-bit kFreeBSD on IA32 CPU

[Felix Zielcke]

Am Mittwoch, den 07.10.2009, 23:13 +0200 schrieb Robert Millan:

>         (grub_cpuid_has_longmode): ... this.  Update all users.  Remove
>         `unsigned' attribute.

I think that should be `static' not `unsigned':

-static unsigned char has_longmode = 0;
+unsigned char grub_cpuid_has_longmode = 0;

But don't we need to add now an `insmod cpuid' in grub-mkconfig so that
this actually works / is useful?
Or does the cpuid command get loaded automatically somewhere already?

-- 
Felix Zielcke
Proud Debian Maintainer and GNU GRUB developer

Re: [PATCH] Fail gracefuly when attempting to load 64-bit kFreeBSD on IA32 CPU

[Vladimir 'phcoder' Serbinenko]

Felix Zielcke wrote:
> Am Mittwoch, den 07.10.2009, 23:13 +0200 schrieb Robert Millan:
>
>   
>>         (grub_cpuid_has_longmode): ... this.  Update all users.  Remove
>>         `unsigned' attribute.
>>     
>
> I think that should be `static' not `unsigned':
>
> -static unsigned char has_longmode = 0;
> +unsigned char grub_cpuid_has_longmode = 0;
>
> But don't we need to add now an `insmod cpuid' in grub-mkconfig so that
> this actually works / is useful?
> Or does the cpuid command get loaded automatically somewhere already?
>
>   
It gets loaded because of symbol dependency of bsd.mod on

grub_cpuid_has_longmode



-- 
Regards
Vladimir 'phcoder' Serbinenko
Personal git repository: http://repo.or.cz/w/grub2/phcoder.git 

Re: [PATCH] Fail gracefuly when attempting to load 64-bit kFreeBSD on IA32 CPU

[Robert Millan]

On Thu, Oct 08, 2009 at 12:12:11PM +0200, Felix Zielcke wrote:
> Am Mittwoch, den 07.10.2009, 23:13 +0200 schrieb Robert Millan:
> 
> >         (grub_cpuid_has_longmode): ... this.  Update all users.  Remove
> >         `unsigned' attribute.
> 
> I think that should be `static' not `unsigned':
> 
> -static unsigned char has_longmode = 0;
> +unsigned char grub_cpuid_has_longmode = 0;

Oh, right.

-- 
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: [PATCH] Fail gracefuly when attempting to load 64-bit kFreeBSD on IA32 CPU

[Vladimir 'phcoder' Serbinenko]

>Index: include/grub/i386/cpuid.h
>===================================================================
>--- include/grub/i386/cpuid.h	(revision 0)
>+++ include/grub/i386/cpuid.h	(revision 0)

This file doesn't follow convention of having an ifdef around the file

>+extern unsigned char grub_cpuid_has_longmode;
I don't like it being declared as a variable: user may inadvertently assign a value to it. I would be more confortable with a function

Other than that patch looks good


-- 
Regards
Vladimir 'phcoder' Serbinenko
Personal git repository: http://repo.or.cz/w/grub2/phcoder.git 

Re: [PATCH] Fail gracefuly when attempting to load 64-bit kFreeBSD on IA32 CPU

[Robert Millan]

Hi,

I didn't notice this mail untill a few minutes ago (was still in fetch
queue), so my first patch was already checked in.

On Fri, Oct 09, 2009 at 01:10:12AM +0200, Vladimir 'phcoder' Serbinenko wrote:
> >Index: include/grub/i386/cpuid.h
> >===================================================================
> >--- include/grub/i386/cpuid.h	(revision 0)
> >+++ include/grub/i386/cpuid.h	(revision 0)
> 
> This file doesn't follow convention of having an ifdef around the file

Thanks, I've added it now.

> >+extern unsigned char grub_cpuid_has_longmode;
> I don't like it being declared as a variable: user may inadvertently assign a value to it. I would be more confortable with a function

I tend to prefer a (const) function too, but this required some restructuring
in cpuid.c so for 1.97 I opted for keeping the changes minimal.

-- 
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: [PATCH] Fail gracefuly when attempting to load 64-bit kFreeBSD on IA32 CPU

[Vladimir 'phcoder' Serbinenko]

Robert Millan wrote:
> Hi,
>
> I didn't notice this mail untill a few minutes ago (was still in fetch
> queue), so my first patch was already checked in.
>
> On Fri, Oct 09, 2009 at 01:10:12AM +0200, Vladimir 'phcoder' Serbinenko wrote:
>   
>>> Index: include/grub/i386/cpuid.h
>>> ===================================================================
>>> --- include/grub/i386/cpuid.h	(revision 0)
>>> +++ include/grub/i386/cpuid.h	(revision 0)
>>>       
>> This file doesn't follow convention of having an ifdef around the file
>>     
>
> Thanks, I've added it now.
>
>   
>>> +extern unsigned char grub_cpuid_has_longmode;
>>>       
>> I don't like it being declared as a variable: user may inadvertently assign a value to it. I would be more confortable with a function
>>     
>
> I tend to prefer a (const) function too, but this required some restructuring
> in cpuid.c so for 1.97 I opted for keeping the changes minimal.
>
>   
Why would these changes be intrusive? It's just a function returning  a
static value or did I miss something?

-- 
Regards
Vladimir 'phcoder' Serbinenko
Personal git repository: http://repo.or.cz/w/grub2/phcoder.git