Re: [Qemu-devel] [PATCH] Add multi-boot kernel loading support

[Rene Rebe]

Anthony Liguori wrote:
...
> > +        memcpy(p, gdt, sizeof(gdt));
> > +        p+=sizeof(gdt);
> > +        *pgdt++ = gdtr;
> > +        *pgdt++ = gdtr >> 8;
> > +        *pgdt++ = gdtr >> 16;
> > +        *pgdt++ = gdtr >> 24;
> > +    }
>
> It was already pushing the limit to embed the linux loader.  This is a 
> bit too much.  Perhaps we should split it to a separate .S file?  I 
> guess we could apply it as is but I'd like to see this code (along with 
> the Linux code) moved to another file at least.

How do you want to assemble the .S file on non x86 builds, or just
ship a pre-built object like the BIOS?

> > +    fprintf(stderr, "qemu: multiboot loader code is %d bytes 
> > long.\n", (int)(p-rom));
> > +
> > +    /* sign rom */
> > +    sum = 0;
> > +    for (i = 0; i < (sizeof(rom) - 1); i++)
> > +        sum += rom[i];
> > +    rom[sizeof(rom) - 1] = -sum;
> > +
> > +    memcpy(option_rom, rom, sizeof(rom));
>
> This too could be common code.

Ok.
+    } else {
> > +        /* Valid if mh_flags sets MULTIBOOT_HEADER_HAS_ADDR. */
> > +        uint32_t mh_header_addr = ldl_p(header+i+12);
> > +        mh_load_addr = ldl_p(header+i+16);
>
> Mixing variable definitions and code.

Yuck.

> > +        fprintf(stderr, "multiboot: mh_header_addr = %#x\n", 
> > mh_header_addr);
> > +        fprintf(stderr, "multiboot: mh_load_addr = %#x\n", 
> > mh_load_addr);
> > +        fprintf(stderr, "multiboot: mh_load_end_addr = %#x\n", 
> > mh_load_end_addr);
> > +        fprintf(stderr, "multiboot: mh_bss_end_addr = %#x\n", 
> > mh_bss_end_addr);
>
> It's a bit chatty by default.

Yes, indeed - though quite handy for a yet new  feature. I could
just drop those (maybe after testing booting some other multi-boot
kernel as well, ...).

> Weird indent.
>
> What all uses multiboot at this point?  I know Xen does.

Maybe other homebrew desktop kernels. I remeber there also
went support into NetBSD some time ago.

> Some additional documentation would be nice too.

Greetings,
  Rene

--
  René Rebe - ExactCODE GmbH - Europe, Germany, Berlin
  http://exactcode.de | http://t2-project.org | http://rene.rebe.name