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

[Rene Rebe <rene@exactcode.de>]

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