[Qemu-devel] [PATCH v2] multiboot: Fix bss segment support

[Qemu-devel] [PATCH v2] multiboot: Fix bss segment support

[Göran Weinholt]

Multiboot images can specify a bss segment. The boot loader must clear
the memory of the bss and ensure that no modules or structures are
allocated inside it. Several fields are provided in the Multiboot
header that were previously not used properly. The header is now used
to determine how much data should be read from the image and how much
memory should be reserved to the bss segment.

Signed-off-by: Göran Weinholt <goran@weinholt.se>
---
 hw/multiboot.c |   14 +++++++++-----
 1 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/hw/multiboot.c b/hw/multiboot.c
index 2426e84..a1d3f41 100644
--- a/hw/multiboot.c
+++ b/hw/multiboot.c
@@ -198,11 +198,14 @@ int load_multiboot(void *fw_cfg,
     } else {
         /* Valid if mh_flags sets MULTIBOOT_HEADER_HAS_ADDR. */
         uint32_t mh_header_addr = ldl_p(header+i+12);
+        uint32_t mh_load_end_addr = ldl_p(header+i+20);
+        uint32_t mh_bss_end_addr = ldl_p(header+i+24);
         mh_load_addr = ldl_p(header+i+16);
         uint32_t mb_kernel_text_offset = i - (mh_header_addr - mh_load_addr);
+        uint32_t mb_load_size = mh_load_end_addr - mh_load_addr;
 
         mh_entry_addr = ldl_p(header+i+28);
-        mb_kernel_size = kernel_file_size - mb_kernel_text_offset;
+        mb_kernel_size = mh_bss_end_addr - mh_load_addr;
 
         /* Valid if mh_flags sets MULTIBOOT_HEADER_HAS_VBE.
         uint32_t mh_mode_type = ldl_p(header+i+32);
@@ -212,17 +215,18 @@ int load_multiboot(void *fw_cfg,
 
         mb_debug("multiboot: mh_header_addr = %#x\n", mh_header_addr);
         mb_debug("multiboot: mh_load_addr = %#x\n", mh_load_addr);
-        mb_debug("multiboot: mh_load_end_addr = %#x\n", ldl_p(header+i+20));
-        mb_debug("multiboot: mh_bss_end_addr = %#x\n", ldl_p(header+i+24));
+        mb_debug("multiboot: mh_load_end_addr = %#x\n", mh_load_end_addr);
+        mb_debug("multiboot: mh_bss_end_addr = %#x\n", mh_bss_end_addr);
         mb_debug("qemu: loading multiboot kernel (%#x bytes) at %#x\n",
-                 mb_kernel_size, mh_load_addr);
+                 mb_load_size, mh_load_addr);
 
         mbs.mb_buf = qemu_malloc(mb_kernel_size);
         fseek(f, mb_kernel_text_offset, SEEK_SET);
-        if (fread(mbs.mb_buf, 1, mb_kernel_size, f) != mb_kernel_size) {
+        if (fread(mbs.mb_buf, 1, mb_load_size, f) != mb_load_size) {
             fprintf(stderr, "fread() failed\n");
             exit(1);
         }
+        memset(mbs.mb_buf + mb_load_size, 0, mb_kernel_size - mb_load_size);
         fclose(f);
     }
 
-- 
1.7.2.5

Re: [Qemu-devel] [PATCH v2] multiboot: Fix bss segment support

[malc]

On Sun, 24 Jul 2011, G?ran Weinholt wrote:

> Multiboot images can specify a bss segment. The boot loader must clear
> the memory of the bss and ensure that no modules or structures are
> allocated inside it. Several fields are provided in the Multiboot
> header that were previously not used properly. The header is now used
> to determine how much data should be read from the image and how much
> memory should be reserved to the bss segment.
> 
> Signed-off-by: G?ran Weinholt <goran@weinholt.se>
> ---
>  hw/multiboot.c |   14 +++++++++-----
>  1 files changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/multiboot.c b/hw/multiboot.c
> index 2426e84..a1d3f41 100644
> --- a/hw/multiboot.c
> +++ b/hw/multiboot.c
> @@ -198,11 +198,14 @@ int load_multiboot(void *fw_cfg,
>      } else {
>          /* Valid if mh_flags sets MULTIBOOT_HEADER_HAS_ADDR. */
>          uint32_t mh_header_addr = ldl_p(header+i+12);
> +        uint32_t mh_load_end_addr = ldl_p(header+i+20);
> +        uint32_t mh_bss_end_addr = ldl_p(header+i+24);
>          mh_load_addr = ldl_p(header+i+16);
>          uint32_t mb_kernel_text_offset = i - (mh_header_addr - mh_load_addr);
> +        uint32_t mb_load_size = mh_load_end_addr - mh_load_addr;
>  
>          mh_entry_addr = ldl_p(header+i+28);
> -        mb_kernel_size = kernel_file_size - mb_kernel_text_offset;
> +        mb_kernel_size = mh_bss_end_addr - mh_load_addr;
>  
>          /* Valid if mh_flags sets MULTIBOOT_HEADER_HAS_VBE.
>          uint32_t mh_mode_type = ldl_p(header+i+32);
> @@ -212,17 +215,18 @@ int load_multiboot(void *fw_cfg,
>  
>          mb_debug("multiboot: mh_header_addr = %#x\n", mh_header_addr);
>          mb_debug("multiboot: mh_load_addr = %#x\n", mh_load_addr);
> -        mb_debug("multiboot: mh_load_end_addr = %#x\n", ldl_p(header+i+20));
> -        mb_debug("multiboot: mh_bss_end_addr = %#x\n", ldl_p(header+i+24));
> +        mb_debug("multiboot: mh_load_end_addr = %#x\n", mh_load_end_addr);
> +        mb_debug("multiboot: mh_bss_end_addr = %#x\n", mh_bss_end_addr);
>          mb_debug("qemu: loading multiboot kernel (%#x bytes) at %#x\n",
> -                 mb_kernel_size, mh_load_addr);
> +                 mb_load_size, mh_load_addr);
>  
>          mbs.mb_buf = qemu_malloc(mb_kernel_size);
>          fseek(f, mb_kernel_text_offset, SEEK_SET);
> -        if (fread(mbs.mb_buf, 1, mb_kernel_size, f) != mb_kernel_size) {
> +        if (fread(mbs.mb_buf, 1, mb_load_size, f) != mb_load_size) {

Not that it matters, but.. you are asking to read mb_load_size records of
1 byte each, it's simple to ask for one record of mb_load_size as a bonus
check becomes != 1 thus saving 11 bytes making the earth that much
greener.

>              fprintf(stderr, "fread() failed\n");
>              exit(1);
>          }
> +        memset(mbs.mb_buf + mb_load_size, 0, mb_kernel_size - mb_load_size);
>          fclose(f);
>      }
>  
> 

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] [PATCH v2] multiboot: Fix bss segment support

[Anthony Liguori]

On 07/24/2011 10:55 AM, Göran Weinholt wrote:
> Multiboot images can specify a bss segment. The boot loader must clear
> the memory of the bss and ensure that no modules or structures are
> allocated inside it. Several fields are provided in the Multiboot
> header that were previously not used properly. The header is now used
> to determine how much data should be read from the image and how much
> memory should be reserved to the bss segment.
>
> Signed-off-by: Göran Weinholt<goran@weinholt.se>

Applied.  Thanks.

Regards,

Anthony Liguori

> ---
>   hw/multiboot.c |   14 +++++++++-----
>   1 files changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/hw/multiboot.c b/hw/multiboot.c
> index 2426e84..a1d3f41 100644
> --- a/hw/multiboot.c
> +++ b/hw/multiboot.c
> @@ -198,11 +198,14 @@ int load_multiboot(void *fw_cfg,
>       } else {
>           /* Valid if mh_flags sets MULTIBOOT_HEADER_HAS_ADDR. */
>           uint32_t mh_header_addr = ldl_p(header+i+12);
> +        uint32_t mh_load_end_addr = ldl_p(header+i+20);
> +        uint32_t mh_bss_end_addr = ldl_p(header+i+24);
>           mh_load_addr = ldl_p(header+i+16);
>           uint32_t mb_kernel_text_offset = i - (mh_header_addr - mh_load_addr);
> +        uint32_t mb_load_size = mh_load_end_addr - mh_load_addr;
>
>           mh_entry_addr = ldl_p(header+i+28);
> -        mb_kernel_size = kernel_file_size - mb_kernel_text_offset;
> +        mb_kernel_size = mh_bss_end_addr - mh_load_addr;
>
>           /* Valid if mh_flags sets MULTIBOOT_HEADER_HAS_VBE.
>           uint32_t mh_mode_type = ldl_p(header+i+32);
> @@ -212,17 +215,18 @@ int load_multiboot(void *fw_cfg,
>
>           mb_debug("multiboot: mh_header_addr = %#x\n", mh_header_addr);
>           mb_debug("multiboot: mh_load_addr = %#x\n", mh_load_addr);
> -        mb_debug("multiboot: mh_load_end_addr = %#x\n", ldl_p(header+i+20));
> -        mb_debug("multiboot: mh_bss_end_addr = %#x\n", ldl_p(header+i+24));
> +        mb_debug("multiboot: mh_load_end_addr = %#x\n", mh_load_end_addr);
> +        mb_debug("multiboot: mh_bss_end_addr = %#x\n", mh_bss_end_addr);
>           mb_debug("qemu: loading multiboot kernel (%#x bytes) at %#x\n",
> -                 mb_kernel_size, mh_load_addr);
> +                 mb_load_size, mh_load_addr);
>
>           mbs.mb_buf = qemu_malloc(mb_kernel_size);
>           fseek(f, mb_kernel_text_offset, SEEK_SET);
> -        if (fread(mbs.mb_buf, 1, mb_kernel_size, f) != mb_kernel_size) {
> +        if (fread(mbs.mb_buf, 1, mb_load_size, f) != mb_load_size) {
>               fprintf(stderr, "fread() failed\n");
>               exit(1);
>           }
> +        memset(mbs.mb_buf + mb_load_size, 0, mb_kernel_size - mb_load_size);
>           fclose(f);
>       }
>

Re: [Qemu-devel] [PATCH v2] multiboot: Fix bss segment support

[Alexander Graf]

On 24.07.2011, at 17:55, Göran Weinholt wrote:

> Multiboot images can specify a bss segment. The boot loader must clear
> the memory of the bss and ensure that no modules or structures are
> allocated inside it. Several fields are provided in the Multiboot
> header that were previously not used properly. The header is now used
> to determine how much data should be read from the image and how much
> memory should be reserved to the bss segment.

This patch breaks the OSX booter:

  http://people.exactcode.de/~rene/mac/boot

It now fails in fread(). Please revert this change for 1.0.1 and/or provide a timely fix.


Alex

> 
> Signed-off-by: Göran Weinholt <goran@weinholt.se>
> ---
> hw/multiboot.c |   14 +++++++++-----
> 1 files changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/multiboot.c b/hw/multiboot.c
> index 2426e84..a1d3f41 100644
> --- a/hw/multiboot.c
> +++ b/hw/multiboot.c
> @@ -198,11 +198,14 @@ int load_multiboot(void *fw_cfg,
>     } else {
>         /* Valid if mh_flags sets MULTIBOOT_HEADER_HAS_ADDR. */
>         uint32_t mh_header_addr = ldl_p(header+i+12);
> +        uint32_t mh_load_end_addr = ldl_p(header+i+20);
> +        uint32_t mh_bss_end_addr = ldl_p(header+i+24);
>         mh_load_addr = ldl_p(header+i+16);
>         uint32_t mb_kernel_text_offset = i - (mh_header_addr - mh_load_addr);
> +        uint32_t mb_load_size = mh_load_end_addr - mh_load_addr;
> 
>         mh_entry_addr = ldl_p(header+i+28);
> -        mb_kernel_size = kernel_file_size - mb_kernel_text_offset;
> +        mb_kernel_size = mh_bss_end_addr - mh_load_addr;
> 
>         /* Valid if mh_flags sets MULTIBOOT_HEADER_HAS_VBE.
>         uint32_t mh_mode_type = ldl_p(header+i+32);
> @@ -212,17 +215,18 @@ int load_multiboot(void *fw_cfg,
> 
>         mb_debug("multiboot: mh_header_addr = %#x\n", mh_header_addr);
>         mb_debug("multiboot: mh_load_addr = %#x\n", mh_load_addr);
> -        mb_debug("multiboot: mh_load_end_addr = %#x\n", ldl_p(header+i+20));
> -        mb_debug("multiboot: mh_bss_end_addr = %#x\n", ldl_p(header+i+24));
> +        mb_debug("multiboot: mh_load_end_addr = %#x\n", mh_load_end_addr);
> +        mb_debug("multiboot: mh_bss_end_addr = %#x\n", mh_bss_end_addr);
>         mb_debug("qemu: loading multiboot kernel (%#x bytes) at %#x\n",
> -                 mb_kernel_size, mh_load_addr);
> +                 mb_load_size, mh_load_addr);
> 
>         mbs.mb_buf = qemu_malloc(mb_kernel_size);
>         fseek(f, mb_kernel_text_offset, SEEK_SET);
> -        if (fread(mbs.mb_buf, 1, mb_kernel_size, f) != mb_kernel_size) {
> +        if (fread(mbs.mb_buf, 1, mb_load_size, f) != mb_load_size) {
>             fprintf(stderr, "fread() failed\n");
>             exit(1);
>         }
> +        memset(mbs.mb_buf + mb_load_size, 0, mb_kernel_size - mb_load_size);
>         fclose(f);
>     }
> 
> -- 
> 1.7.2.5
> 
> 

Re: [Qemu-devel] [PATCH v2] multiboot: Fix bss segment support

[Anthony Liguori]

On 12/19/2011 11:35 AM, Alexander Graf wrote:
>
> On 24.07.2011, at 17:55, Göran Weinholt wrote:
>
>> Multiboot images can specify a bss segment. The boot loader must clear
>> the memory of the bss and ensure that no modules or structures are
>> allocated inside it. Several fields are provided in the Multiboot
>> header that were previously not used properly. The header is now used
>> to determine how much data should be read from the image and how much
>> memory should be reserved to the bss segment.
>
> This patch breaks the OSX booter:
>
>    http://people.exactcode.de/~rene/mac/boot

How is this licensed?  Is there source available?

>
> It now fails in fread(). Please revert this change for 1.0.1 and/or provide a timely fix.

Is the patch incorrect in some way?  I don't see how it's reasonable to expect 
someone to fix a guest that cannot be legally run under QEMU.

If the patch is obviously incorrect, I'm all for reverting it, but I don't think 
we can reasonably ask people to debug OS X guest failures since OS X is clearly 
not allowed to run under QEMU.

Regards,

Anthony Liguori

>
> Alex
>
>>
>> Signed-off-by: Göran Weinholt<goran@weinholt.se>
>> ---
>> hw/multiboot.c |   14 +++++++++-----
>> 1 files changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/multiboot.c b/hw/multiboot.c
>> index 2426e84..a1d3f41 100644
>> --- a/hw/multiboot.c
>> +++ b/hw/multiboot.c
>> @@ -198,11 +198,14 @@ int load_multiboot(void *fw_cfg,
>>      } else {
>>          /* Valid if mh_flags sets MULTIBOOT_HEADER_HAS_ADDR. */
>>          uint32_t mh_header_addr = ldl_p(header+i+12);
>> +        uint32_t mh_load_end_addr = ldl_p(header+i+20);
>> +        uint32_t mh_bss_end_addr = ldl_p(header+i+24);
>>          mh_load_addr = ldl_p(header+i+16);
>>          uint32_t mb_kernel_text_offset = i - (mh_header_addr - mh_load_addr);
>> +        uint32_t mb_load_size = mh_load_end_addr - mh_load_addr;
>>
>>          mh_entry_addr = ldl_p(header+i+28);
>> -        mb_kernel_size = kernel_file_size - mb_kernel_text_offset;
>> +        mb_kernel_size = mh_bss_end_addr - mh_load_addr;
>>
>>          /* Valid if mh_flags sets MULTIBOOT_HEADER_HAS_VBE.
>>          uint32_t mh_mode_type = ldl_p(header+i+32);
>> @@ -212,17 +215,18 @@ int load_multiboot(void *fw_cfg,
>>
>>          mb_debug("multiboot: mh_header_addr = %#x\n", mh_header_addr);
>>          mb_debug("multiboot: mh_load_addr = %#x\n", mh_load_addr);
>> -        mb_debug("multiboot: mh_load_end_addr = %#x\n", ldl_p(header+i+20));
>> -        mb_debug("multiboot: mh_bss_end_addr = %#x\n", ldl_p(header+i+24));
>> +        mb_debug("multiboot: mh_load_end_addr = %#x\n", mh_load_end_addr);
>> +        mb_debug("multiboot: mh_bss_end_addr = %#x\n", mh_bss_end_addr);
>>          mb_debug("qemu: loading multiboot kernel (%#x bytes) at %#x\n",
>> -                 mb_kernel_size, mh_load_addr);
>> +                 mb_load_size, mh_load_addr);
>>
>>          mbs.mb_buf = qemu_malloc(mb_kernel_size);
>>          fseek(f, mb_kernel_text_offset, SEEK_SET);
>> -        if (fread(mbs.mb_buf, 1, mb_kernel_size, f) != mb_kernel_size) {
>> +        if (fread(mbs.mb_buf, 1, mb_load_size, f) != mb_load_size) {
>>              fprintf(stderr, "fread() failed\n");
>>              exit(1);
>>          }
>> +        memset(mbs.mb_buf + mb_load_size, 0, mb_kernel_size - mb_load_size);
>>          fclose(f);
>>      }
>>
>> --
>> 1.7.2.5
>>
>>
>
>

Re: [Qemu-devel] [PATCH v2] multiboot: Fix bss segment support

[Alexander Graf]

[-- Attachment #1: Type: text/plain, Size: 2266 bytes --]


On 19.12.2011, at 23:01, Anthony Liguori wrote:

> On 12/19/2011 11:35 AM, Alexander Graf wrote:
>> 
>> On 24.07.2011, at 17:55, Göran Weinholt wrote:
>> 
>>> Multiboot images can specify a bss segment. The boot loader must clear
>>> the memory of the bss and ensure that no modules or structures are
>>> allocated inside it. Several fields are provided in the Multiboot
>>> header that were previously not used properly. The header is now used
>>> to determine how much data should be read from the image and how much
>>> memory should be reserved to the bss segment.
>> 
>> This patch breaks the OSX booter:
>> 
>>   http://people.exactcode.de/~rene/mac/boot
> 
> How is this licensed?  Is there source available?

Yes, it's the XNU booter, but binaries are easier to debug when it comes to reading out the bootloader info:

  http://tgwbd.org/darwin/boot.html

I don't know if the binary above is 1:1 built from those sources, but it's definitely close enough.

> 
>> 
>> It now fails in fread(). Please revert this change for 1.0.1 and/or provide a timely fix.
> 
> Is the patch incorrect in some way?  I don't see how it's reasonable to expect someone to fix a guest that cannot be legally run under QEMU.

The guest is the bootloader which is an open source multiboot binary. The fact that it bootstraps OSX is a detail. It's a kernel that worked with -kernel before and is now broken. It also works with grub. I'd go as far as guessing that there are more kernels experiencing the same breakage.

> If the patch is obviously incorrect, I'm all for reverting it, but I don't think we can reasonably ask people to debug OS X guest failures since OS X is clearly not allowed to run under QEMU.

I think we can reasonably argue that multiboot kernels (especially the one kernel this whole thing was written for) that worked before shouldn't be broken, as that's clearly a regression. And usually regressions warrant reverts IMHO.

But either way, I'd rather have this fixed. And once I'm through my pile of other patches to review and commit I'll gladly take a look at it. Until then, I'd rather have a 1.0.1 that works with what worked before, so behaves the same as 0.15 rather than a version that regresses.


Alex


[-- Attachment #2: Type: text/html, Size: 3668 bytes --]

Re: [Qemu-devel] [PATCH v2] multiboot: Fix bss segment support

[Göran Weinholt]

[-- Attachment #1: Type: text/plain, Size: 3094 bytes --]

Alexander Graf <agraf@suse.de> writes:

> On 19.12.2011, at 23:01, Anthony Liguori wrote:
>
>     On 12/19/2011 11:35 AM, Alexander Graf wrote:
>
>         On 24.07.2011, at 17:55, Göran Weinholt wrote:
>
>             Multiboot images can specify a bss segment. The boot loader must
>             clear
>        
>             the memory of the bss and ensure that no modules or structures are
>        
>             allocated inside it. Several fields are provided in the Multiboot
>        
>             header that were previously not used properly. The header is now
>             used
>        
>             to determine how much data should be read from the image and how
>             much
>        
>             memory should be reserved to the bss segment.
>
>         This patch breaks the OSX booter:
>
>           http://people.exactcode.de/~rene/mac/boot
>
>     How is this licensed?  Is there source available?
>
> Yes, it's the XNU booter, but binaries are easier to debug when it comes to
> reading out the bootloader info:
>
>   http://tgwbd.org/darwin/boot.html
>
> I don't know if the binary above is 1:1 built from those sources, but it's
> definitely close enough.

I was unable to build that program from source, but I found a pre-built
binary that triggered the same bug. Please test the following patch.


Subject: [PATCH] multiboot: mh_load_end_addr and mh_bss_end_addr may be zero

There are two special cases in the address fields of the multiboot
format. If mh_load_end_addr is zero then the whole image file should
be loaded and if mh_bss_end_addr is zero then there is no bss segment.
With this change it is again possible to boot kernels where these
fields are zero.

Signed-off-by: Göran Weinholt <goran@weinholt.se>
---
 hw/multiboot.c |   13 ++++++++++++-
 1 files changed, 12 insertions(+), 1 deletions(-)

diff --git a/hw/multiboot.c b/hw/multiboot.c
index b4484a3..a43d56c 100644
--- a/hw/multiboot.c
+++ b/hw/multiboot.c
@@ -202,10 +202,21 @@ int load_multiboot(void *fw_cfg,
         uint32_t mh_bss_end_addr = ldl_p(header+i+24);
         mh_load_addr = ldl_p(header+i+16);
         uint32_t mb_kernel_text_offset = i - (mh_header_addr - mh_load_addr);
-        uint32_t mb_load_size = mh_load_end_addr - mh_load_addr;
+        uint32_t mb_load_size;
+
+        /* A load end address of zero indicates that the whole file
+         * should be loaded. */
+        if (!mh_load_end_addr)
+            mh_load_end_addr = kernel_file_size + mh_load_addr;
+
+        /* A bss end address of zero indicates that there is no bss
+         * segment. */
+        if (!mh_bss_end_addr)
+            mh_bss_end_addr = mh_load_end_addr;
 
         mh_entry_addr = ldl_p(header+i+28);
         mb_kernel_size = mh_bss_end_addr - mh_load_addr;
+        mb_load_size = mh_load_end_addr - mh_load_addr;
 
         /* Valid if mh_flags sets MULTIBOOT_HEADER_HAS_VBE.
         uint32_t mh_mode_type = ldl_p(header+i+32);
-- 
1.7.2.5


-- 
Göran Weinholt <goran@weinholt.se>

[-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --]

Re: [Qemu-devel] [PATCH v2] multiboot: Fix bss segment support

[Alexander Graf]

On 20.12.2011, at 12:53, Göran Weinholt wrote:

> Alexander Graf <agraf@suse.de> writes:
> 
>> On 19.12.2011, at 23:01, Anthony Liguori wrote:
>> 
>>    On 12/19/2011 11:35 AM, Alexander Graf wrote:
>> 
>>        On 24.07.2011, at 17:55, Göran Weinholt wrote:
>> 
>>            Multiboot images can specify a bss segment. The boot loader must
>>            clear
>> 
>>            the memory of the bss and ensure that no modules or structures are
>> 
>>            allocated inside it. Several fields are provided in the Multiboot
>> 
>>            header that were previously not used properly. The header is now
>>            used
>> 
>>            to determine how much data should be read from the image and how
>>            much
>> 
>>            memory should be reserved to the bss segment.
>> 
>>        This patch breaks the OSX booter:
>> 
>>          http://people.exactcode.de/~rene/mac/boot
>> 
>>    How is this licensed?  Is there source available?
>> 
>> Yes, it's the XNU booter, but binaries are easier to debug when it comes to
>> reading out the bootloader info:
>> 
>>  http://tgwbd.org/darwin/boot.html
>> 
>> I don't know if the binary above is 1:1 built from those sources, but it's
>> definitely close enough.
> 
> I was unable to build that program from source, but I found a pre-built
> binary that triggered the same bug. Please test the following patch.
> 
> 
> Subject: [PATCH] multiboot: mh_load_end_addr and mh_bss_end_addr may be zero
> 
> There are two special cases in the address fields of the multiboot
> format. If mh_load_end_addr is zero then the whole image file should
> be loaded and if mh_bss_end_addr is zero then there is no bss segment.
> With this change it is again possible to boot kernels where these
> fields are zero.
> 
> Signed-off-by: Göran Weinholt <goran@weinholt.se>

Yes, this patch makes things work again :). Thanks a lot!

The only thing I could nitpick on would be the coding style - checkpatch.pl complains :). Could you please resend with braces?
Justin, Please also queue this for 1.0-stable when it comes in its final form.

Tested-by: Alexander Graf <agraf@suse.de>


Alex

> ---
> hw/multiboot.c |   13 ++++++++++++-
> 1 files changed, 12 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/multiboot.c b/hw/multiboot.c
> index b4484a3..a43d56c 100644
> --- a/hw/multiboot.c
> +++ b/hw/multiboot.c
> @@ -202,10 +202,21 @@ int load_multiboot(void *fw_cfg,
>         uint32_t mh_bss_end_addr = ldl_p(header+i+24);
>         mh_load_addr = ldl_p(header+i+16);
>         uint32_t mb_kernel_text_offset = i - (mh_header_addr - mh_load_addr);
> -        uint32_t mb_load_size = mh_load_end_addr - mh_load_addr;
> +        uint32_t mb_load_size;
> +
> +        /* A load end address of zero indicates that the whole file
> +         * should be loaded. */
> +        if (!mh_load_end_addr)
> +            mh_load_end_addr = kernel_file_size + mh_load_addr;
> +
> +        /* A bss end address of zero indicates that there is no bss
> +         * segment. */
> +        if (!mh_bss_end_addr)
> +            mh_bss_end_addr = mh_load_end_addr;

WARNING: braces {} are necessary for all arms of this statement
#83: FILE: hw/multiboot.c:209:
+        if (!mh_load_end_addr)
[...]

WARNING: braces {} are necessary for all arms of this statement
#88: FILE: hw/multiboot.c:214:
+        if (!mh_bss_end_addr)
[...]

Re: [Qemu-devel] [PATCH v2] multiboot: Fix bss segment support

[Göran Weinholt]

[-- Attachment #1: Type: text/plain, Size: 2485 bytes --]

Alexander Graf <agraf@suse.de> writes:

> Yes, this patch makes things work again :). Thanks a lot!
>
> The only thing I could nitpick on would be the coding style - checkpatch.pl complains :). Could you please resend with braces?
> Justin, Please also queue this for 1.0-stable when it comes in its final form.
>
> Tested-by: Alexander Graf <agraf@suse.de>

Thanks for testing it. Here it is, with braces this time.


From 2a2df801d68a76fb2210e556652fb2e17e0f6711 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?G=C3=B6ran=20Weinholt?= <goran@weinholt.se>
Date: Tue, 20 Dec 2011 19:36:10 +0100
Subject: [PATCH] multiboot: mh_load_end_addr and mh_bss_end_addr may be zero
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

There are two special cases in the address fields of the multiboot
format. If mh_load_end_addr is zero then the whole image file should
be loaded and if mh_bss_end_addr is zero then there is no bss segment.
With this change it is again possible to boot kernels where these
fields are zero.

Signed-off-by: Göran Weinholt <goran@weinholt.se>
Tested-by: Alexander Graf <agraf@suse.de>
---
 hw/multiboot.c |   15 ++++++++++++++-
 1 files changed, 14 insertions(+), 1 deletions(-)

diff --git a/hw/multiboot.c b/hw/multiboot.c
index b4484a3..db28328 100644
--- a/hw/multiboot.c
+++ b/hw/multiboot.c
@@ -202,10 +202,23 @@ int load_multiboot(void *fw_cfg,
         uint32_t mh_bss_end_addr = ldl_p(header+i+24);
         mh_load_addr = ldl_p(header+i+16);
         uint32_t mb_kernel_text_offset = i - (mh_header_addr - mh_load_addr);
-        uint32_t mb_load_size = mh_load_end_addr - mh_load_addr;
+        uint32_t mb_load_size;
+
+        /* A load end address of zero indicates that the whole file
+         * should be loaded. */
+        if (!mh_load_end_addr) {
+            mh_load_end_addr = kernel_file_size + mh_load_addr;
+        }
+
+        /* A bss end address of zero indicates that there is no bss
+         * segment. */
+        if (!mh_bss_end_addr) {
+            mh_bss_end_addr = mh_load_end_addr;
+        }
 
         mh_entry_addr = ldl_p(header+i+28);
         mb_kernel_size = mh_bss_end_addr - mh_load_addr;
+        mb_load_size = mh_load_end_addr - mh_load_addr;
 
         /* Valid if mh_flags sets MULTIBOOT_HEADER_HAS_VBE.
         uint32_t mh_mode_type = ldl_p(header+i+32);
-- 
1.7.2.5


-- 
Göran Weinholt <goran@weinholt.se>

[-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --]

Re: [Qemu-devel] [PATCH v2] multiboot: Fix bss segment support

[Alexander Graf]

On 20.12.2011, at 19:49, Göran Weinholt <goran@weinholt.se> wrote:

> Alexander Graf <agraf@suse.de> writes:
> 
>> Yes, this patch makes things work again :). Thanks a lot!
>> 
>> The only thing I could nitpick on would be the coding style - checkpatch.pl complains :). Could you please resend with braces?
>> Justin, Please also queue this for 1.0-stable when it comes in its final form.
>> 
>> Tested-by: Alexander Graf <agraf@suse.de>
> 
> Thanks for testing it. Here it is, with braces this time.
> 
> 
> From 2a2df801d68a76fb2210e556652fb2e17e0f6711 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?G=C3=B6ran=20Weinholt?= <goran@weinholt.se>
> Date: Tue, 20 Dec 2011 19:36:10 +0100
> Subject: [PATCH] multiboot: mh_load_end_addr and mh_bss_end_addr may be zero
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
> 
> There are two special cases in the address fields of the multiboot
> format. If mh_load_end_addr is zero then the whole image file should
> be loaded and if mh_bss_end_addr is zero then there is no bss segment.
> With this change it is again possible to boot kernels where these
> fields are zero.
> 
> Signed-off-by: Göran Weinholt <goran@weinholt.se>
> Tested-by: Alexander Graf <agraf@suse.de>

Acked-by: Alexander Graf <agraf@suse.de>

Alex

> ---
> hw/multiboot.c |   15 ++++++++++++++-
> 1 files changed, 14 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/multiboot.c b/hw/multiboot.c
> index b4484a3..db28328 100644
> --- a/hw/multiboot.c
> +++ b/hw/multiboot.c
> @@ -202,10 +202,23 @@ int load_multiboot(void *fw_cfg,
>         uint32_t mh_bss_end_addr = ldl_p(header+i+24);
>         mh_load_addr = ldl_p(header+i+16);
>         uint32_t mb_kernel_text_offset = i - (mh_header_addr - mh_load_addr);
> -        uint32_t mb_load_size = mh_load_end_addr - mh_load_addr;
> +        uint32_t mb_load_size;
> +
> +        /* A load end address of zero indicates that the whole file
> +         * should be loaded. */
> +        if (!mh_load_end_addr) {
> +            mh_load_end_addr = kernel_file_size + mh_load_addr;
> +        }
> +
> +        /* A bss end address of zero indicates that there is no bss
> +         * segment. */
> +        if (!mh_bss_end_addr) {
> +            mh_bss_end_addr = mh_load_end_addr;
> +        }
> 
>         mh_entry_addr = ldl_p(header+i+28);
>         mb_kernel_size = mh_bss_end_addr - mh_load_addr;
> +        mb_load_size = mh_load_end_addr - mh_load_addr;
> 
>         /* Valid if mh_flags sets MULTIBOOT_HEADER_HAS_VBE.
>         uint32_t mh_mode_type = ldl_p(header+i+32);
> -- 
> 1.7.2.5
> 
> 
> -- 
> Göran Weinholt <goran@weinholt.se>

Re: [Qemu-devel] [PATCH v2] multiboot: Fix bss segment support

[Kevin Wolf]

Am 20.12.2011 19:49, schrieb Göran Weinholt:
> Alexander Graf <agraf@suse.de> writes:
> 
>> Yes, this patch makes things work again :). Thanks a lot!
>>
>> The only thing I could nitpick on would be the coding style - checkpatch.pl complains :). Could you please resend with braces?
>> Justin, Please also queue this for 1.0-stable when it comes in its final form.
>>
>> Tested-by: Alexander Graf <agraf@suse.de>
> 
> Thanks for testing it. Here it is, with braces this time.
> 
> 
> From 2a2df801d68a76fb2210e556652fb2e17e0f6711 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?G=C3=B6ran=20Weinholt?= <goran@weinholt.se>
> Date: Tue, 20 Dec 2011 19:36:10 +0100
> Subject: [PATCH] multiboot: mh_load_end_addr and mh_bss_end_addr may be zero
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
> 
> There are two special cases in the address fields of the multiboot
> format. If mh_load_end_addr is zero then the whole image file should
> be loaded and if mh_bss_end_addr is zero then there is no bss segment.
> With this change it is again possible to boot kernels where these
> fields are zero.
> 
> Signed-off-by: Göran Weinholt <goran@weinholt.se>
> Tested-by: Alexander Graf <agraf@suse.de>

This doesn't seem to be applied yet. Can you resend it as a top-level
email instead of deep in some thread where it may be missed?

Kevin