* [Qemu-devel] [PATCH v3] hw/i386/pc: prompt not multboot or morden kernel image
@ 2013-04-09 2:27 liguang
2013-04-09 4:36 ` Jesse Larrew
0 siblings, 1 reply; 6+ messages in thread
From: liguang @ 2013-04-09 2:27 UTC (permalink / raw)
To: Stefan Hajnoczi, qemu-devel, qemu-trivial; +Cc: 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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v3] hw/i386/pc: prompt not multboot or morden kernel image
2013-04-09 2:27 [Qemu-devel] [PATCH v3] hw/i386/pc: prompt not multboot or morden kernel image liguang
@ 2013-04-09 4:36 ` Jesse Larrew
2013-04-09 5:05 ` li guang
0 siblings, 1 reply; 6+ messages in thread
From: Jesse Larrew @ 2013-04-09 4:36 UTC (permalink / raw)
To: liguang; +Cc: qemu-trivial, Stefan Hajnoczi, Anthony Liguori, qemu-devel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v3] hw/i386/pc: prompt not multboot or morden kernel image
2013-04-09 4:36 ` Jesse Larrew
@ 2013-04-09 5:05 ` li guang
2013-04-09 19:15 ` Jesse Larrew
0 siblings, 1 reply; 6+ messages in thread
From: li guang @ 2013-04-09 5:05 UTC (permalink / raw)
To: Jesse Larrew; +Cc: qemu-trivial, Stefan Hajnoczi, Anthony Liguori, qemu-devel
在 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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v3] hw/i386/pc: prompt not multboot or morden kernel image
2013-04-09 5:05 ` li guang
@ 2013-04-09 19:15 ` Jesse Larrew
2013-04-11 2:18 ` li guang
0 siblings, 1 reply; 6+ messages in thread
From: Jesse Larrew @ 2013-04-09 19:15 UTC (permalink / raw)
To: li guang; +Cc: qemu-trivial, Stefan Hajnoczi, Anthony Liguori, qemu-devel
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v3] hw/i386/pc: prompt not multboot or morden kernel image
2013-04-09 19:15 ` Jesse Larrew
@ 2013-04-11 2:18 ` li guang
2013-04-12 11:40 ` Stefan Hajnoczi
0 siblings, 1 reply; 6+ messages in thread
From: li guang @ 2013-04-11 2:18 UTC (permalink / raw)
To: Jesse Larrew; +Cc: qemu-trivial, Stefan Hajnoczi, Anthony Liguori, qemu-devel
在 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;
> }
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v3] hw/i386/pc: prompt not multboot or morden kernel image
2013-04-11 2:18 ` li guang
@ 2013-04-12 11:40 ` Stefan Hajnoczi
0 siblings, 0 replies; 6+ messages in thread
From: Stefan Hajnoczi @ 2013-04-12 11:40 UTC (permalink / raw)
To: li guang; +Cc: qemu-trivial, Anthony Liguori, Jesse Larrew, qemu-devel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-04-12 11:40 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-04-09 2:27 [Qemu-devel] [PATCH v3] hw/i386/pc: prompt not multboot or morden kernel image liguang
2013-04-09 4:36 ` Jesse Larrew
2013-04-09 5:05 ` li guang
2013-04-09 19:15 ` Jesse Larrew
2013-04-11 2:18 ` li guang
2013-04-12 11:40 ` Stefan Hajnoczi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).