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

[Stefan Hajnoczi <stefanha@gmail.com>]

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