[Qemu-trivial] [Qemu-devel][PATCH v3] hw/i386/pc: prompt not multboot or morden kernel image

[Qemu-trivial] [Qemu-devel][PATCH v3] hw/i386/pc: prompt not multboot or morden kernel image

[liguang]

if head magic is missing or wrong unexpectedly, we'd
better to prompt memssage for this.
e.g.
I make a mistake to boot a vmlinuz for MIPS(which
I think it's for x86) like this:
qemu-system-x86_64 -kernel vmlinuz -initrd demord
then qemu report:
"qemu: linux kernel too old to load a ram disk"
that's misleading.

Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
---
 hw/i386/pc.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index ebbf059..6b29c3f 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -686,6 +686,8 @@ static void load_linux(void *fw_cfg,
         if (load_multiboot(fw_cfg, f, kernel_filename, initrd_filename,
                            kernel_cmdline, kernel_size, header)) {
             return;
+        } else {
+            fprintf(stderr, "warn: invalid multiboot or modern kernel image\n");
         }
         protocol = 0;
     }
-- 
1.7.2.5

[Qemu-devel] [PATCH v3] hw/i386/pc: prompt not multboot or morden kernel image

[liguang]

if head magic is missing or wrong unexpectedly, we'd
better to prompt memssage for this.
e.g.
I make a mistake to boot a vmlinuz for MIPS(which
I think it's for x86) like this:
qemu-system-x86_64 -kernel vmlinuz -initrd demord
then qemu report:
"qemu: linux kernel too old to load a ram disk"
that's misleading.

Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
---
 hw/i386/pc.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index ebbf059..6b29c3f 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -686,6 +686,8 @@ static void load_linux(void *fw_cfg,
         if (load_multiboot(fw_cfg, f, kernel_filename, initrd_filename,
                            kernel_cmdline, kernel_size, header)) {
             return;
+        } else {
+            fprintf(stderr, "warn: invalid multiboot or modern kernel image\n");
         }
         protocol = 0;
     }
-- 
1.7.2.5

Re: [Qemu-trivial] [Qemu-devel] [PATCH v3] hw/i386/pc: prompt not multboot or morden kernel image

[Jesse Larrew]

On 04/08/2013 09:27 PM, liguang wrote:
> if head magic is missing or wrong unexpectedly, we'd
> better to prompt memssage for this.
> e.g.
> I make a mistake to boot a vmlinuz for MIPS(which
> I think it's for x86) like this:
> qemu-system-x86_64 -kernel vmlinuz -initrd demord
> then qemu report:
> "qemu: linux kernel too old to load a ram disk"
> that's misleading.
> 

Yes, that message would definitely be misleading in this case.

> Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
> ---
>  hw/i386/pc.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index ebbf059..6b29c3f 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -686,6 +686,8 @@ static void load_linux(void *fw_cfg,
>          if (load_multiboot(fw_cfg, f, kernel_filename, initrd_filename,
>                             kernel_cmdline, kernel_size, header)) {
>              return;
> +        } else {
> +            fprintf(stderr, "warn: invalid multiboot or modern kernel image\n");
>          }
>          protocol = 0;
>      }
> 

My impression from reading through the code is that this branch can be
legitimately triggered by an older kernel (protocol < 0x200). In that case,
the error message above would also be misleading.

I think a better solution might be to simply validate that the arch
specified in the ELF header matches the arch that qemu is emulating. I'll
look into this more tomorrow morning. :)

Jesse Larrew
Software Engineer, KVM Team
IBM Linux Technology Center
Phone: (512) 973-2052 (T/L: 363-2052)
jlarrew@linux.vnet.ibm.com

Re: [Qemu-devel] [PATCH v3] hw/i386/pc: prompt not multboot or morden kernel image

[Jesse Larrew]

On 04/08/2013 09:27 PM, liguang wrote:
> if head magic is missing or wrong unexpectedly, we'd
> better to prompt memssage for this.
> e.g.
> I make a mistake to boot a vmlinuz for MIPS(which
> I think it's for x86) like this:
> qemu-system-x86_64 -kernel vmlinuz -initrd demord
> then qemu report:
> "qemu: linux kernel too old to load a ram disk"
> that's misleading.
> 

Yes, that message would definitely be misleading in this case.

> Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
> ---
>  hw/i386/pc.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index ebbf059..6b29c3f 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -686,6 +686,8 @@ static void load_linux(void *fw_cfg,
>          if (load_multiboot(fw_cfg, f, kernel_filename, initrd_filename,
>                             kernel_cmdline, kernel_size, header)) {
>              return;
> +        } else {
> +            fprintf(stderr, "warn: invalid multiboot or modern kernel image\n");
>          }
>          protocol = 0;
>      }
> 

My impression from reading through the code is that this branch can be
legitimately triggered by an older kernel (protocol < 0x200). In that case,
the error message above would also be misleading.

I think a better solution might be to simply validate that the arch
specified in the ELF header matches the arch that qemu is emulating. I'll
look into this more tomorrow morning. :)

Jesse Larrew
Software Engineer, KVM Team
IBM Linux Technology Center
Phone: (512) 973-2052 (T/L: 363-2052)
jlarrew@linux.vnet.ibm.com

Re: [Qemu-trivial] [Qemu-devel] [PATCH v3] hw/i386/pc: prompt not multboot or morden kernel image

[li guang]

在 2013-04-08一的 23:36 -0500，Jesse Larrew写道：
> On 04/08/2013 09:27 PM, liguang wrote:
> > if head magic is missing or wrong unexpectedly, we'd
> > better to prompt memssage for this.
> > e.g.
> > I make a mistake to boot a vmlinuz for MIPS(which
> > I think it's for x86) like this:
> > qemu-system-x86_64 -kernel vmlinuz -initrd demord
> > then qemu report:
> > "qemu: linux kernel too old to load a ram disk"
> > that's misleading.
> > 
> 
> Yes, that message would definitely be misleading in this case.
> 
> > Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
> > ---
> >  hw/i386/pc.c |    2 ++
> >  1 files changed, 2 insertions(+), 0 deletions(-)
> > 
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index ebbf059..6b29c3f 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -686,6 +686,8 @@ static void load_linux(void *fw_cfg,
> >          if (load_multiboot(fw_cfg, f, kernel_filename, initrd_filename,
> >                             kernel_cmdline, kernel_size, header)) {
> >              return;
> > +        } else {
> > +            fprintf(stderr, "warn: invalid multiboot or modern kernel image\n");
> >          }
> >          protocol = 0;
> >      }
> > 
> 
> My impression from reading through the code is that this branch can be
> legitimately triggered by an older kernel (protocol < 0x200). 

right.

> In that case,the error message above would also be misleading.

the warning message is saying: hey, we prefer modern kernel image
or multiboot, but you specify neither, what are you doing?
do you want to boot an old(obsolete) kernel?

or, add a line like fprintf(stderr, "do you boot an old kernel?\n") :-)

> 
> I think a better solution might be to simply validate that the arch
> specified in the ELF header matches the arch that qemu is emulating.
>  I'll look into this more tomorrow morning. :)

that's fine

> 
> Jesse Larrew
> Software Engineer, KVM Team
> IBM Linux Technology Center
> Phone: (512) 973-2052 (T/L: 363-2052)
> jlarrew@linux.vnet.ibm.com
> 

Re: [Qemu-devel] [PATCH v3] hw/i386/pc: prompt not multboot or morden kernel image

[li guang]

在 2013-04-08一的 23:36 -0500，Jesse Larrew写道：
> On 04/08/2013 09:27 PM, liguang wrote:
> > if head magic is missing or wrong unexpectedly, we'd
> > better to prompt memssage for this.
> > e.g.
> > I make a mistake to boot a vmlinuz for MIPS(which
> > I think it's for x86) like this:
> > qemu-system-x86_64 -kernel vmlinuz -initrd demord
> > then qemu report:
> > "qemu: linux kernel too old to load a ram disk"
> > that's misleading.
> > 
> 
> Yes, that message would definitely be misleading in this case.
> 
> > Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
> > ---
> >  hw/i386/pc.c |    2 ++
> >  1 files changed, 2 insertions(+), 0 deletions(-)
> > 
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index ebbf059..6b29c3f 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -686,6 +686,8 @@ static void load_linux(void *fw_cfg,
> >          if (load_multiboot(fw_cfg, f, kernel_filename, initrd_filename,
> >                             kernel_cmdline, kernel_size, header)) {
> >              return;
> > +        } else {
> > +            fprintf(stderr, "warn: invalid multiboot or modern kernel image\n");
> >          }
> >          protocol = 0;
> >      }
> > 
> 
> My impression from reading through the code is that this branch can be
> legitimately triggered by an older kernel (protocol < 0x200). 

right.

> In that case,the error message above would also be misleading.

the warning message is saying: hey, we prefer modern kernel image
or multiboot, but you specify neither, what are you doing?
do you want to boot an old(obsolete) kernel?

or, add a line like fprintf(stderr, "do you boot an old kernel?\n") :-)

> 
> I think a better solution might be to simply validate that the arch
> specified in the ELF header matches the arch that qemu is emulating.
>  I'll look into this more tomorrow morning. :)

that's fine

> 
> Jesse Larrew
> Software Engineer, KVM Team
> IBM Linux Technology Center
> Phone: (512) 973-2052 (T/L: 363-2052)
> jlarrew@linux.vnet.ibm.com
> 

Re: [Qemu-trivial] [Qemu-devel] [PATCH v3] hw/i386/pc: prompt not multboot or morden kernel image

[Jesse Larrew]

On 04/09/2013 12:05 AM, li guang wrote:
> 在 2013-04-08一的 23:36 -0500，Jesse Larrew写道：
>> On 04/08/2013 09:27 PM, liguang wrote:
>>> if head magic is missing or wrong unexpectedly, we'd
>>> better to prompt memssage for this.
>>> e.g.
>>> I make a mistake to boot a vmlinuz for MIPS(which
>>> I think it's for x86) like this:
>>> qemu-system-x86_64 -kernel vmlinuz -initrd demord
>>> then qemu report:
>>> "qemu: linux kernel too old to load a ram disk"
>>> that's misleading.
>>>
>>
>> Yes, that message would definitely be misleading in this case.
>>
>>> Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
>>> ---
>>>  hw/i386/pc.c |    2 ++
>>>  1 files changed, 2 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>>> index ebbf059..6b29c3f 100644
>>> --- a/hw/i386/pc.c
>>> +++ b/hw/i386/pc.c
>>> @@ -686,6 +686,8 @@ static void load_linux(void *fw_cfg,
>>>          if (load_multiboot(fw_cfg, f, kernel_filename, initrd_filename,
>>>                             kernel_cmdline, kernel_size, header)) {
>>>              return;
>>> +        } else {
>>> +            fprintf(stderr, "warn: invalid multiboot or modern kernel image\n");
>>>          }
>>>          protocol = 0;
>>>      }
>>>
>>
>> My impression from reading through the code is that this branch can be
>> legitimately triggered by an older kernel (protocol < 0x200). 
> 
> right.
> 
>> In that case,the error message above would also be misleading.
> 
> the warning message is saying: hey, we prefer modern kernel image
> or multiboot, but you specify neither, what are you doing?
> do you want to boot an old(obsolete) kernel?
> 
> or, add a line like fprintf(stderr, "do you boot an old kernel?\n") :-)
> 

Ah, I understand now. Thanks for explaining that. :)

>>
>> I think a better solution might be to simply validate that the arch
>> specified in the ELF header matches the arch that qemu is emulating.
>>  I'll look into this more tomorrow morning. :)
> 
> that's fine
> 

Looking at Documentation/x86/boot.txt in the kernel source, it looks
like there is a separate word of header magic that all x86 kernels should
have -- even the oldest images. Adding a separate check for that magic
might be a more complete solution:

---
 hw/i386/pc.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 8d75b34..e120427 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -686,6 +686,14 @@ static void load_linux(void *fw_cfg,
         if (load_multiboot(fw_cfg, f, kernel_filename, initrd_filename,
                            kernel_cmdline, kernel_size, header)) {
             return;
+        } else {
+            /* This is the closest thing old Linux kernels have to a
+               magic number. */
+            if (lduw_p(header+0x01FE) != 0xAA55) {
+                fprintf(stderr, "qemu: '%s' is not an x86 kernel image.\n",
+                        kernel_filename);
+                exit(1);
+            }
         }
         protocol = 0;
     }
-- 
1.7.11.7

Jesse Larrew
Software Engineer, KVM Team
IBM Linux Technology Center
Phone: (512) 973-2052 (T/L: 363-2052)
jlarrew@linux.vnet.ibm.com

Re: [Qemu-devel] [PATCH v3] hw/i386/pc: prompt not multboot or morden kernel image

[Jesse Larrew]

On 04/09/2013 12:05 AM, li guang wrote:
> 在 2013-04-08一的 23:36 -0500，Jesse Larrew写道：
>> On 04/08/2013 09:27 PM, liguang wrote:
>>> if head magic is missing or wrong unexpectedly, we'd
>>> better to prompt memssage for this.
>>> e.g.
>>> I make a mistake to boot a vmlinuz for MIPS(which
>>> I think it's for x86) like this:
>>> qemu-system-x86_64 -kernel vmlinuz -initrd demord
>>> then qemu report:
>>> "qemu: linux kernel too old to load a ram disk"
>>> that's misleading.
>>>
>>
>> Yes, that message would definitely be misleading in this case.
>>
>>> Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
>>> ---
>>>  hw/i386/pc.c |    2 ++
>>>  1 files changed, 2 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>>> index ebbf059..6b29c3f 100644
>>> --- a/hw/i386/pc.c
>>> +++ b/hw/i386/pc.c
>>> @@ -686,6 +686,8 @@ static void load_linux(void *fw_cfg,
>>>          if (load_multiboot(fw_cfg, f, kernel_filename, initrd_filename,
>>>                             kernel_cmdline, kernel_size, header)) {
>>>              return;
>>> +        } else {
>>> +            fprintf(stderr, "warn: invalid multiboot or modern kernel image\n");
>>>          }
>>>          protocol = 0;
>>>      }
>>>
>>
>> My impression from reading through the code is that this branch can be
>> legitimately triggered by an older kernel (protocol < 0x200). 
> 
> right.
> 
>> In that case,the error message above would also be misleading.
> 
> the warning message is saying: hey, we prefer modern kernel image
> or multiboot, but you specify neither, what are you doing?
> do you want to boot an old(obsolete) kernel?
> 
> or, add a line like fprintf(stderr, "do you boot an old kernel?\n") :-)
> 

Ah, I understand now. Thanks for explaining that. :)

>>
>> I think a better solution might be to simply validate that the arch
>> specified in the ELF header matches the arch that qemu is emulating.
>>  I'll look into this more tomorrow morning. :)
> 
> that's fine
> 

Looking at Documentation/x86/boot.txt in the kernel source, it looks
like there is a separate word of header magic that all x86 kernels should
have -- even the oldest images. Adding a separate check for that magic
might be a more complete solution:

---
 hw/i386/pc.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 8d75b34..e120427 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -686,6 +686,14 @@ static void load_linux(void *fw_cfg,
         if (load_multiboot(fw_cfg, f, kernel_filename, initrd_filename,
                            kernel_cmdline, kernel_size, header)) {
             return;
+        } else {
+            /* This is the closest thing old Linux kernels have to a
+               magic number. */
+            if (lduw_p(header+0x01FE) != 0xAA55) {
+                fprintf(stderr, "qemu: '%s' is not an x86 kernel image.\n",
+                        kernel_filename);
+                exit(1);
+            }
         }
         protocol = 0;
     }
-- 
1.7.11.7

Jesse Larrew
Software Engineer, KVM Team
IBM Linux Technology Center
Phone: (512) 973-2052 (T/L: 363-2052)
jlarrew@linux.vnet.ibm.com

Re: [Qemu-trivial] [Qemu-devel] [PATCH v3] hw/i386/pc: prompt not multboot or morden kernel image

[li guang]

在 2013-04-09二的 14:15 -0500，Jesse Larrew写道：
> On 04/09/2013 12:05 AM, li guang wrote:
> > 在 2013-04-08一的 23:36 -0500，Jesse Larrew写道：
> >> On 04/08/2013 09:27 PM, liguang wrote:
> >>> if head magic is missing or wrong unexpectedly, we'd
> >>> better to prompt memssage for this.
> >>> e.g.
> >>> I make a mistake to boot a vmlinuz for MIPS(which
> >>> I think it's for x86) like this:
> >>> qemu-system-x86_64 -kernel vmlinuz -initrd demord
> >>> then qemu report:
> >>> "qemu: linux kernel too old to load a ram disk"
> >>> that's misleading.
> >>>
> >>
> >> Yes, that message would definitely be misleading in this case.
> >>
> >>> Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
> >>> ---
> >>>  hw/i386/pc.c |    2 ++
> >>>  1 files changed, 2 insertions(+), 0 deletions(-)
> >>>
> >>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> >>> index ebbf059..6b29c3f 100644
> >>> --- a/hw/i386/pc.c
> >>> +++ b/hw/i386/pc.c
> >>> @@ -686,6 +686,8 @@ static void load_linux(void *fw_cfg,
> >>>          if (load_multiboot(fw_cfg, f, kernel_filename, initrd_filename,
> >>>                             kernel_cmdline, kernel_size, header)) {
> >>>              return;
> >>> +        } else {
> >>> +            fprintf(stderr, "warn: invalid multiboot or modern kernel image\n");
> >>>          }
> >>>          protocol = 0;
> >>>      }
> >>>
> >>
> >> My impression from reading through the code is that this branch can be
> >> legitimately triggered by an older kernel (protocol < 0x200). 
> > 
> > right.
> > 
> >> In that case,the error message above would also be misleading.
> > 
> > the warning message is saying: hey, we prefer modern kernel image
> > or multiboot, but you specify neither, what are you doing?
> > do you want to boot an old(obsolete) kernel?
> > 
> > or, add a line like fprintf(stderr, "do you boot an old kernel?\n") :-)
> > 
> 
> Ah, I understand now. Thanks for explaining that. :)
> 
> >>
> >> I think a better solution might be to simply validate that the arch
> >> specified in the ELF header matches the arch that qemu is emulating.
> >>  I'll look into this more tomorrow morning. :)
> > 
> > that's fine
> > 
> 
> Looking at Documentation/x86/boot.txt in the kernel source, it looks
> like there is a separate word of header magic that all x86 kernels should
> have -- even the oldest images. Adding a separate check for that magic
> might be a more complete solution:
> 
> ---
>  hw/i386/pc.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 8d75b34..e120427 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -686,6 +686,14 @@ static void load_linux(void *fw_cfg,
>          if (load_multiboot(fw_cfg, f, kernel_filename, initrd_filename,
>                             kernel_cmdline, kernel_size, header)) {
>              return;
> +        } else {
> +            /* This is the closest thing old Linux kernels have to a
> +               magic number. */
> +            if (lduw_p(header+0x01FE) != 0xAA55) {

maybe a good candidate, 
but, AFAIK, 0xAA55 can hardly be a magic for
it's commonly used by many cases.
 
> +                fprintf(stderr, "qemu: '%s' is not an x86 kernel image.\n",
> +                        kernel_filename);
> +                exit(1);
> +            }
>          }
>          protocol = 0;
>      }

Re: [Qemu-devel] [PATCH v3] hw/i386/pc: prompt not multboot or morden kernel image

[li guang]

在 2013-04-09二的 14:15 -0500，Jesse Larrew写道：
> On 04/09/2013 12:05 AM, li guang wrote:
> > 在 2013-04-08一的 23:36 -0500，Jesse Larrew写道：
> >> On 04/08/2013 09:27 PM, liguang wrote:
> >>> if head magic is missing or wrong unexpectedly, we'd
> >>> better to prompt memssage for this.
> >>> e.g.
> >>> I make a mistake to boot a vmlinuz for MIPS(which
> >>> I think it's for x86) like this:
> >>> qemu-system-x86_64 -kernel vmlinuz -initrd demord
> >>> then qemu report:
> >>> "qemu: linux kernel too old to load a ram disk"
> >>> that's misleading.
> >>>
> >>
> >> Yes, that message would definitely be misleading in this case.
> >>
> >>> Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
> >>> ---
> >>>  hw/i386/pc.c |    2 ++
> >>>  1 files changed, 2 insertions(+), 0 deletions(-)
> >>>
> >>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> >>> index ebbf059..6b29c3f 100644
> >>> --- a/hw/i386/pc.c
> >>> +++ b/hw/i386/pc.c
> >>> @@ -686,6 +686,8 @@ static void load_linux(void *fw_cfg,
> >>>          if (load_multiboot(fw_cfg, f, kernel_filename, initrd_filename,
> >>>                             kernel_cmdline, kernel_size, header)) {
> >>>              return;
> >>> +        } else {
> >>> +            fprintf(stderr, "warn: invalid multiboot or modern kernel image\n");
> >>>          }
> >>>          protocol = 0;
> >>>      }
> >>>
> >>
> >> My impression from reading through the code is that this branch can be
> >> legitimately triggered by an older kernel (protocol < 0x200). 
> > 
> > right.
> > 
> >> In that case,the error message above would also be misleading.
> > 
> > the warning message is saying: hey, we prefer modern kernel image
> > or multiboot, but you specify neither, what are you doing?
> > do you want to boot an old(obsolete) kernel?
> > 
> > or, add a line like fprintf(stderr, "do you boot an old kernel?\n") :-)
> > 
> 
> Ah, I understand now. Thanks for explaining that. :)
> 
> >>
> >> I think a better solution might be to simply validate that the arch
> >> specified in the ELF header matches the arch that qemu is emulating.
> >>  I'll look into this more tomorrow morning. :)
> > 
> > that's fine
> > 
> 
> Looking at Documentation/x86/boot.txt in the kernel source, it looks
> like there is a separate word of header magic that all x86 kernels should
> have -- even the oldest images. Adding a separate check for that magic
> might be a more complete solution:
> 
> ---
>  hw/i386/pc.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 8d75b34..e120427 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -686,6 +686,14 @@ static void load_linux(void *fw_cfg,
>          if (load_multiboot(fw_cfg, f, kernel_filename, initrd_filename,
>                             kernel_cmdline, kernel_size, header)) {
>              return;
> +        } else {
> +            /* This is the closest thing old Linux kernels have to a
> +               magic number. */
> +            if (lduw_p(header+0x01FE) != 0xAA55) {

maybe a good candidate, 
but, AFAIK, 0xAA55 can hardly be a magic for
it's commonly used by many cases.
 
> +                fprintf(stderr, "qemu: '%s' is not an x86 kernel image.\n",
> +                        kernel_filename);
> +                exit(1);
> +            }
>          }
>          protocol = 0;
>      }

Re: [Qemu-trivial] [Qemu-devel] [PATCH v3] hw/i386/pc: prompt not multboot or morden kernel image

[Stefan Hajnoczi]

On Thu, Apr 11, 2013 at 10:18:56AM +0800, li guang wrote:
> 在 2013-04-09二的 14:15 -0500，Jesse Larrew写道：
> > On 04/09/2013 12:05 AM, li guang wrote:
> > > 在 2013-04-08一的 23:36 -0500，Jesse Larrew写道：
> > >> On 04/08/2013 09:27 PM, liguang wrote:
> > >>> if head magic is missing or wrong unexpectedly, we'd
> > >>> better to prompt memssage for this.
> > >>> e.g.
> > >>> I make a mistake to boot a vmlinuz for MIPS(which
> > >>> I think it's for x86) like this:
> > >>> qemu-system-x86_64 -kernel vmlinuz -initrd demord
> > >>> then qemu report:
> > >>> "qemu: linux kernel too old to load a ram disk"
> > >>> that's misleading.
> > >>>
> > >>
> > >> Yes, that message would definitely be misleading in this case.
> > >>
> > >>> Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
> > >>> ---
> > >>>  hw/i386/pc.c |    2 ++
> > >>>  1 files changed, 2 insertions(+), 0 deletions(-)
> > >>>
> > >>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > >>> index ebbf059..6b29c3f 100644
> > >>> --- a/hw/i386/pc.c
> > >>> +++ b/hw/i386/pc.c
> > >>> @@ -686,6 +686,8 @@ static void load_linux(void *fw_cfg,
> > >>>          if (load_multiboot(fw_cfg, f, kernel_filename, initrd_filename,
> > >>>                             kernel_cmdline, kernel_size, header)) {
> > >>>              return;
> > >>> +        } else {
> > >>> +            fprintf(stderr, "warn: invalid multiboot or modern kernel image\n");
> > >>>          }
> > >>>          protocol = 0;
> > >>>      }
> > >>>
> > >>
> > >> My impression from reading through the code is that this branch can be
> > >> legitimately triggered by an older kernel (protocol < 0x200). 
> > > 
> > > right.
> > > 
> > >> In that case,the error message above would also be misleading.
> > > 
> > > the warning message is saying: hey, we prefer modern kernel image
> > > or multiboot, but you specify neither, what are you doing?
> > > do you want to boot an old(obsolete) kernel?
> > > 
> > > or, add a line like fprintf(stderr, "do you boot an old kernel?\n") :-)
> > > 
> > 
> > Ah, I understand now. Thanks for explaining that. :)
> > 
> > >>
> > >> I think a better solution might be to simply validate that the arch
> > >> specified in the ELF header matches the arch that qemu is emulating.
> > >>  I'll look into this more tomorrow morning. :)
> > > 
> > > that's fine
> > > 
> > 
> > Looking at Documentation/x86/boot.txt in the kernel source, it looks
> > like there is a separate word of header magic that all x86 kernels should
> > have -- even the oldest images. Adding a separate check for that magic
> > might be a more complete solution:
> > 
> > ---
> >  hw/i386/pc.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index 8d75b34..e120427 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -686,6 +686,14 @@ static void load_linux(void *fw_cfg,
> >          if (load_multiboot(fw_cfg, f, kernel_filename, initrd_filename,
> >                             kernel_cmdline, kernel_size, header)) {
> >              return;
> > +        } else {
> > +            /* This is the closest thing old Linux kernels have to a
> > +               magic number. */
> > +            if (lduw_p(header+0x01FE) != 0xAA55) {
> 
> maybe a good candidate, 
> but, AFAIK, 0xAA55 can hardly be a magic for
> it's commonly used by many cases.

The problem is that older kernel images don't have a real magic number
to check.  We cannot break compatibility by rejecting them now.

Jesse's patch makes sense to me.  Feel free to discuss more, I'll merge
once there is consensus.

Stefan

Re: [Qemu-devel] [PATCH v3] hw/i386/pc: prompt not multboot or morden kernel image

[Stefan Hajnoczi]

On Thu, Apr 11, 2013 at 10:18:56AM +0800, li guang wrote:
> 在 2013-04-09二的 14:15 -0500，Jesse Larrew写道：
> > On 04/09/2013 12:05 AM, li guang wrote:
> > > 在 2013-04-08一的 23:36 -0500，Jesse Larrew写道：
> > >> On 04/08/2013 09:27 PM, liguang wrote:
> > >>> if head magic is missing or wrong unexpectedly, we'd
> > >>> better to prompt memssage for this.
> > >>> e.g.
> > >>> I make a mistake to boot a vmlinuz for MIPS(which
> > >>> I think it's for x86) like this:
> > >>> qemu-system-x86_64 -kernel vmlinuz -initrd demord
> > >>> then qemu report:
> > >>> "qemu: linux kernel too old to load a ram disk"
> > >>> that's misleading.
> > >>>
> > >>
> > >> Yes, that message would definitely be misleading in this case.
> > >>
> > >>> Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
> > >>> ---
> > >>>  hw/i386/pc.c |    2 ++
> > >>>  1 files changed, 2 insertions(+), 0 deletions(-)
> > >>>
> > >>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > >>> index ebbf059..6b29c3f 100644
> > >>> --- a/hw/i386/pc.c
> > >>> +++ b/hw/i386/pc.c
> > >>> @@ -686,6 +686,8 @@ static void load_linux(void *fw_cfg,
> > >>>          if (load_multiboot(fw_cfg, f, kernel_filename, initrd_filename,
> > >>>                             kernel_cmdline, kernel_size, header)) {
> > >>>              return;
> > >>> +        } else {
> > >>> +            fprintf(stderr, "warn: invalid multiboot or modern kernel image\n");
> > >>>          }
> > >>>          protocol = 0;
> > >>>      }
> > >>>
> > >>
> > >> My impression from reading through the code is that this branch can be
> > >> legitimately triggered by an older kernel (protocol < 0x200). 
> > > 
> > > right.
> > > 
> > >> In that case,the error message above would also be misleading.
> > > 
> > > the warning message is saying: hey, we prefer modern kernel image
> > > or multiboot, but you specify neither, what are you doing?
> > > do you want to boot an old(obsolete) kernel?
> > > 
> > > or, add a line like fprintf(stderr, "do you boot an old kernel?\n") :-)
> > > 
> > 
> > Ah, I understand now. Thanks for explaining that. :)
> > 
> > >>
> > >> I think a better solution might be to simply validate that the arch
> > >> specified in the ELF header matches the arch that qemu is emulating.
> > >>  I'll look into this more tomorrow morning. :)
> > > 
> > > that's fine
> > > 
> > 
> > Looking at Documentation/x86/boot.txt in the kernel source, it looks
> > like there is a separate word of header magic that all x86 kernels should
> > have -- even the oldest images. Adding a separate check for that magic
> > might be a more complete solution:
> > 
> > ---
> >  hw/i386/pc.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index 8d75b34..e120427 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -686,6 +686,14 @@ static void load_linux(void *fw_cfg,
> >          if (load_multiboot(fw_cfg, f, kernel_filename, initrd_filename,
> >                             kernel_cmdline, kernel_size, header)) {
> >              return;
> > +        } else {
> > +            /* This is the closest thing old Linux kernels have to a
> > +               magic number. */
> > +            if (lduw_p(header+0x01FE) != 0xAA55) {
> 
> maybe a good candidate, 
> but, AFAIK, 0xAA55 can hardly be a magic for
> it's commonly used by many cases.

The problem is that older kernel images don't have a real magic number
to check.  We cannot break compatibility by rejecting them now.

Jesse's patch makes sense to me.  Feel free to discuss more, I'll merge
once there is consensus.

Stefan