The bzimage logic uses the unit global orig_image_len to hold the original
module length for the kernel when the headroom is calculated. It then uses
orig_image_len to locate the start of the bzimage when the expansion is done.
This is an issue when more than one bzimage is processed by the headroom
calculation logic, as it will leave orig_image_len set to the length of the
last bzimage it processed.
The boot module work introduced storing the headroom size on a per module
basis. By passing in the headroom from the boot module, orig_image_len is no
longer needed to locate the beginning of the bzimage after the allocated
headroom. The bzimage functions are reworked as such, allowing the removal of
orig_image_len and enabling them to be reused by multiple kernel boot modules.
Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
---
xen/arch/x86/bzimage.c | 38 ++++++++++++++++++------------
xen/arch/x86/hvm/dom_build.c | 3 ++-
xen/arch/x86/include/asm/bzimage.h | 5 ++--
xen/arch/x86/pv/dom0_build.c | 3 ++-
4 files changed, 30 insertions(+), 19 deletions(-)
diff --git a/xen/arch/x86/bzimage.c b/xen/arch/x86/bzimage.c
index 66f648f311e4..32f0360d25b4 100644
--- a/xen/arch/x86/bzimage.c
+++ b/xen/arch/x86/bzimage.c
@@ -68,8 +68,6 @@ static __init int bzimage_check(struct setup_header *hdr, unsigned long len)
return 1;
}
-static unsigned long __initdata orig_image_len;
-
unsigned long __init bzimage_headroom(void *image_start,
unsigned long image_length)
{
@@ -90,7 +88,6 @@ unsigned long __init bzimage_headroom(void *image_start,
if ( elf_is_elfbinary(image_start, image_length) )
return 0;
- orig_image_len = image_length;
headroom = output_length(image_start, image_length);
if (gzip_check(image_start, image_length))
{
@@ -103,13 +100,20 @@ unsigned long __init bzimage_headroom(void *image_start,
return headroom;
}
-int __init bzimage_parse(void *image_base, void **image_start,
- unsigned long *image_len)
+int __init bzimage_parse(
+ void *image_base, void **image_start, unsigned long headroom,
+ unsigned long *image_len)
{
struct setup_header *hdr = (struct setup_header *)(*image_start);
int err = bzimage_check(hdr, *image_len);
- unsigned long output_len;
-
+ unsigned long module_len = *image_len;
+
+ /*
+ * Variable err will have one of three values:
+ * - < 0: a error occurred trying to inspect the contents
+ * - > 0: the image is a bzImage
+ * - == 0: not a bzImage, could be raw elf or elf.gz (vmlinuz.gz)
+ */
if ( err < 0 )
return err;
@@ -118,21 +122,25 @@ int __init bzimage_parse(void *image_base, void **image_start,
*image_start += (hdr->setup_sects + 1) * 512 + hdr->payload_offset;
*image_len = hdr->payload_length;
}
-
- if ( elf_is_elfbinary(*image_start, *image_len) )
- return 0;
+ else
+ {
+ if ( elf_is_elfbinary(*image_start, *image_len) )
+ return 0;
+ else
+ *image_len = *image_len - headroom;
+ }
BUG_ON(!(image_base < *image_start));
- output_len = output_length(*image_start, orig_image_len);
-
- if ( (err = perform_gunzip(image_base, *image_start, orig_image_len)) > 0 )
- err = decompress(*image_start, orig_image_len, image_base);
+ if ( (err = perform_gunzip(image_base, *image_start, *image_len)) > 0 )
+ err = decompress(*image_start, *image_len, image_base);
if ( !err )
{
+ printk(XENLOG_ERR "%s(%d): decompression failed, reseting image start and len\n",
+ __func__, err);
*image_start = image_base;
- *image_len = output_len;
+ *image_len = module_len;
}
return err > 0 ? 0 : err;
diff --git a/xen/arch/x86/hvm/dom_build.c b/xen/arch/x86/hvm/dom_build.c
index a847c2cb16d9..4f614aea34c3 100644
--- a/xen/arch/x86/hvm/dom_build.c
+++ b/xen/arch/x86/hvm/dom_build.c
@@ -743,7 +743,8 @@ static int __init pvh_load_kernel(
struct vcpu *v = d->vcpu[0];
int rc;
- if ( (rc = bzimage_parse(image_base, &image_start, &image_len)) != 0 )
+ if ( (rc = bzimage_parse(image_base, &image_start, image->headroom,
+ &image_len)) != 0 )
{
printk("Error trying to detect bz compressed kernel\n");
return rc;
diff --git a/xen/arch/x86/include/asm/bzimage.h b/xen/arch/x86/include/asm/bzimage.h
index 7ed69d39103d..24c7d4b8eb68 100644
--- a/xen/arch/x86/include/asm/bzimage.h
+++ b/xen/arch/x86/include/asm/bzimage.h
@@ -5,7 +5,8 @@
unsigned long bzimage_headroom(void *image_start, unsigned long image_length);
-int bzimage_parse(void *image_base, void **image_start,
- unsigned long *image_len);
+int bzimage_parse(
+ void *image_base, void **image_start, unsigned long headroom,
+ unsigned long *image_len);
#endif /* __X86_BZIMAGE_H__ */
diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c
index 8d941ff4486e..8b02f62892d4 100644
--- a/xen/arch/x86/pv/dom0_build.c
+++ b/xen/arch/x86/pv/dom0_build.c
@@ -427,7 +427,8 @@ static int __init dom0_construct(struct boot_domain *bd)
d->max_pages = ~0U;
- if ( (rc = bzimage_parse(image_base, &image_start, &image_len)) != 0 )
+ if ( (rc = bzimage_parse(image_base, &image_start, bd->kernel->headroom,
+ &image_len)) != 0 )
return rc;
if ( (rc = elf_init(&elf, image_start, image_len)) != 0 )
--
2.30.2On 2025-04-19 18:08, Daniel P. Smith wrote:
> The bzimage logic uses the unit global orig_image_len to hold the original
> module length for the kernel when the headroom is calculated. It then uses
> orig_image_len to locate the start of the bzimage when the expansion is done.
> This is an issue when more than one bzimage is processed by the headroom
> calculation logic, as it will leave orig_image_len set to the length of the
> last bzimage it processed.
>
> The boot module work introduced storing the headroom size on a per module
> basis. By passing in the headroom from the boot module, orig_image_len is no
> longer needed to locate the beginning of the bzimage after the allocated
> headroom. The bzimage functions are reworked as such, allowing the removal of
> orig_image_len and enabling them to be reused by multiple kernel boot modules.
>
> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> ---
> xen/arch/x86/bzimage.c | 38 ++++++++++++++++++------------
> xen/arch/x86/hvm/dom_build.c | 3 ++-
> xen/arch/x86/include/asm/bzimage.h | 5 ++--
> xen/arch/x86/pv/dom0_build.c | 3 ++-
> 4 files changed, 30 insertions(+), 19 deletions(-)
>
> diff --git a/xen/arch/x86/bzimage.c b/xen/arch/x86/bzimage.c
> index 66f648f311e4..32f0360d25b4 100644
> @@ -103,13 +100,20 @@ unsigned long __init bzimage_headroom(void *image_start,
> return headroom;
> }
>
> -int __init bzimage_parse(void *image_base, void **image_start,
> - unsigned long *image_len)
> +int __init bzimage_parse(
> + void *image_base, void **image_start, unsigned long headroom,
> + unsigned long *image_len)
> {
> struct setup_header *hdr = (struct setup_header *)(*image_start);
> int err = bzimage_check(hdr, *image_len);
> - unsigned long output_len;
> -
> + unsigned long module_len = *image_len;
> +
> + /*
> + * Variable err will have one of three values:
> + * - < 0: a error occurred trying to inspect the contents
> + * - > 0: the image is a bzImage
> + * - == 0: not a bzImage, could be raw elf or elf.gz (vmlinuz.gz)
> + */
This comment seems a little independent of this change, so maybe it
should be submitted separately. Also, I think a better placement would
be next to bzimage_check().
> if ( err < 0 )
> return err;
>
> @@ -118,21 +122,25 @@ int __init bzimage_parse(void *image_base, void **image_start,
> *image_start += (hdr->setup_sects + 1) * 512 + hdr->payload_offset;
> *image_len = hdr->payload_length;
@here
> }
> -
> - if ( elf_is_elfbinary(*image_start, *image_len) )
> - return 0;
> + else
> + {
> + if ( elf_is_elfbinary(*image_start, *image_len) )
> + return 0;
> + else
> + *image_len = *image_len - headroom;
> + }
I don't like this extra indention which includes the return. If you
retain orig_image_len as a local variable, and set it above at "@here",
you can have a smaller diff and leave cleaner logic.
orig_image_len previously was set as a static variable, so the correct
value was set after bzimage_headroom. Now that it is no longer static,
we need to grab the hdr->payload_length value when we have a bzImage.
Otherwise output_length will read past the end of the module.
Below is the diff for bzimage.c with the change I suggest.
Regards,
Jason
diff --git a/xen/arch/x86/bzimage.c b/xen/arch/x86/bzimage.c
index ac4fd428be..775a8b45ff 100644
--- a/xen/arch/x86/bzimage.c
+++ b/xen/arch/x86/bzimage.c
@@ -69,8 +69,6 @@ static __init int bzimage_check(struct setup_header
*hdr, unsigned long len)
return 1;
}
-static unsigned long __initdata orig_image_len;
-
unsigned long __init bzimage_headroom(void *image_start,
unsigned long image_length)
{
@@ -91,7 +89,6 @@ unsigned long __init bzimage_headroom(void *image_start,
if ( elf_is_elfbinary(image_start, image_length) )
return 0;
- orig_image_len = image_length;
headroom = output_length(image_start, image_length);
if (gzip_check(image_start, image_length))
{
@@ -104,12 +101,15 @@ unsigned long __init bzimage_headroom(void
*image_start,
return headroom;
}
-int __init bzimage_parse(void *image_base, void **image_start,
- unsigned long *image_len)
+int __init bzimage_parse(
+ void *image_base, void **image_start, unsigned long headroom,
+ unsigned long *image_len)
{
struct setup_header *hdr = (struct setup_header *)(*image_start);
int err = bzimage_check(hdr, *image_len);
- unsigned long output_len;
+ unsigned long output_len, orig_image_len;
+
+ orig_image_len = *image_len - headroom;
if ( err < 0 )
return err;
@@ -118,6 +118,7 @@ int __init bzimage_parse(void *image_base, void
**image_start,
{
*image_start += (hdr->setup_sects + 1) * 512 +
hdr->payload_offset;
*image_len = hdr->payload_length;
+ orig_image_len = hdr->payload_length;
}
if ( elf_is_elfbinary(*image_start, *image_len) )
On 4/23/25 15:27, Jason Andryuk wrote:
> On 2025-04-19 18:08, Daniel P. Smith wrote:
>> The bzimage logic uses the unit global orig_image_len to hold the
>> original
>> module length for the kernel when the headroom is calculated. It then
>> uses
>> orig_image_len to locate the start of the bzimage when the expansion
>> is done.
>> This is an issue when more than one bzimage is processed by the headroom
>> calculation logic, as it will leave orig_image_len set to the length
>> of the
>> last bzimage it processed.
>>
>> The boot module work introduced storing the headroom size on a per module
>> basis. By passing in the headroom from the boot module, orig_image_len
>> is no
>> longer needed to locate the beginning of the bzimage after the allocated
>> headroom. The bzimage functions are reworked as such, allowing the
>> removal of
>> orig_image_len and enabling them to be reused by multiple kernel boot
>> modules.
>>
>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
>> ---
>> xen/arch/x86/bzimage.c | 38 ++++++++++++++++++------------
>> xen/arch/x86/hvm/dom_build.c | 3 ++-
>> xen/arch/x86/include/asm/bzimage.h | 5 ++--
>> xen/arch/x86/pv/dom0_build.c | 3 ++-
>> 4 files changed, 30 insertions(+), 19 deletions(-)
>>
>> diff --git a/xen/arch/x86/bzimage.c b/xen/arch/x86/bzimage.c
>> index 66f648f311e4..32f0360d25b4 100644
>
>> @@ -103,13 +100,20 @@ unsigned long __init bzimage_headroom(void
>> *image_start,
>> return headroom;
>> }
>> -int __init bzimage_parse(void *image_base, void **image_start,
>> - unsigned long *image_len)
>> +int __init bzimage_parse(
>> + void *image_base, void **image_start, unsigned long headroom,
>> + unsigned long *image_len)
>> {
>> struct setup_header *hdr = (struct setup_header *)(*image_start);
>> int err = bzimage_check(hdr, *image_len);
>> - unsigned long output_len;
>> -
>> + unsigned long module_len = *image_len;
>> +
>> + /*
>> + * Variable err will have one of three values:
>> + * - < 0: a error occurred trying to inspect the contents
>> + * - > 0: the image is a bzImage
>> + * - == 0: not a bzImage, could be raw elf or elf.gz (vmlinuz.gz)
>> + */
>
> This comment seems a little independent of this change, so maybe it
> should be submitted separately. Also, I think a better placement would
> be next to bzimage_check().
>
>> if ( err < 0 )
>> return err;
>> @@ -118,21 +122,25 @@ int __init bzimage_parse(void *image_base, void
>> **image_start,
>> *image_start += (hdr->setup_sects + 1) * 512 + hdr-
>> >payload_offset;
>> *image_len = hdr->payload_length;
>
> @here
>
>> }
>> -
>> - if ( elf_is_elfbinary(*image_start, *image_len) )
>> - return 0;
>> + else
>> + {
>> + if ( elf_is_elfbinary(*image_start, *image_len) )
>> + return 0;
>> + else
>> + *image_len = *image_len - headroom;
>> + }
>
> I don't like this extra indention which includes the return. If you
> retain orig_image_len as a local variable, and set it above at "@here",
> you can have a smaller diff and leave cleaner logic.
Right, but I find it sillier to be checking every kernel for elf when we
know it's a bzImage. While the elf check is fairly simplistic, it is
still multiple value checks.
> orig_image_len previously was set as a static variable, so the correct
> value was set after bzimage_headroom. Now that it is no longer static,
> we need to grab the hdr->payload_length value when we have a bzImage.
> Otherwise output_length will read past the end of the module.
Yes, I am fully aware.
> Below is the diff for bzimage.c with the change I suggest.
I will provide an alternate version.
v/r,
dps
On 26.04.2025 03:53, Daniel P. Smith wrote:
> On 4/23/25 15:27, Jason Andryuk wrote:
>> On 2025-04-19 18:08, Daniel P. Smith wrote:
>>> The bzimage logic uses the unit global orig_image_len to hold the
>>> original
>>> module length for the kernel when the headroom is calculated. It then
>>> uses
>>> orig_image_len to locate the start of the bzimage when the expansion
>>> is done.
>>> This is an issue when more than one bzimage is processed by the headroom
>>> calculation logic, as it will leave orig_image_len set to the length
>>> of the
>>> last bzimage it processed.
>>>
>>> The boot module work introduced storing the headroom size on a per module
>>> basis. By passing in the headroom from the boot module, orig_image_len
>>> is no
>>> longer needed to locate the beginning of the bzimage after the allocated
>>> headroom. The bzimage functions are reworked as such, allowing the
>>> removal of
>>> orig_image_len and enabling them to be reused by multiple kernel boot
>>> modules.
>>>
>>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
>>> ---
>>> xen/arch/x86/bzimage.c | 38 ++++++++++++++++++------------
>>> xen/arch/x86/hvm/dom_build.c | 3 ++-
>>> xen/arch/x86/include/asm/bzimage.h | 5 ++--
>>> xen/arch/x86/pv/dom0_build.c | 3 ++-
>>> 4 files changed, 30 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/xen/arch/x86/bzimage.c b/xen/arch/x86/bzimage.c
>>> index 66f648f311e4..32f0360d25b4 100644
>>
>>> @@ -103,13 +100,20 @@ unsigned long __init bzimage_headroom(void
>>> *image_start,
>>> return headroom;
>>> }
>>> -int __init bzimage_parse(void *image_base, void **image_start,
>>> - unsigned long *image_len)
>>> +int __init bzimage_parse(
>>> + void *image_base, void **image_start, unsigned long headroom,
>>> + unsigned long *image_len)
>>> {
>>> struct setup_header *hdr = (struct setup_header *)(*image_start);
>>> int err = bzimage_check(hdr, *image_len);
>>> - unsigned long output_len;
>>> -
>>> + unsigned long module_len = *image_len;
>>> +
>>> + /*
>>> + * Variable err will have one of three values:
>>> + * - < 0: a error occurred trying to inspect the contents
>>> + * - > 0: the image is a bzImage
>>> + * - == 0: not a bzImage, could be raw elf or elf.gz (vmlinuz.gz)
>>> + */
>>
>> This comment seems a little independent of this change, so maybe it
>> should be submitted separately. Also, I think a better placement would
>> be next to bzimage_check().
>>
>>> if ( err < 0 )
>>> return err;
>>> @@ -118,21 +122,25 @@ int __init bzimage_parse(void *image_base, void
>>> **image_start,
>>> *image_start += (hdr->setup_sects + 1) * 512 + hdr-
>>>> payload_offset;
>>> *image_len = hdr->payload_length;
>>
>> @here
>>
>>> }
>>> -
>>> - if ( elf_is_elfbinary(*image_start, *image_len) )
>>> - return 0;
>>> + else
>>> + {
>>> + if ( elf_is_elfbinary(*image_start, *image_len) )
>>> + return 0;
>>> + else
>>> + *image_len = *image_len - headroom;
>>> + }
>>
>> I don't like this extra indention which includes the return. If you
>> retain orig_image_len as a local variable, and set it above at "@here",
>> you can have a smaller diff and leave cleaner logic.
>
> Right, but I find it sillier to be checking every kernel for elf when we
> know it's a bzImage. While the elf check is fairly simplistic, it is
> still multiple value checks.
Even without any extra local vars the above can be
else if ( elf_is_elfbinary(*image_start, *image_len) )
return 0;
else
*image_len = *image_len - headroom;
which would already address the "extra indentation" aspect.
Jan
On 2025-04-28 02:41, Jan Beulich wrote:
> On 26.04.2025 03:53, Daniel P. Smith wrote:
>> On 4/23/25 15:27, Jason Andryuk wrote:
>>> On 2025-04-19 18:08, Daniel P. Smith wrote:
>>>> The bzimage logic uses the unit global orig_image_len to hold the
>>>> original
>>>> module length for the kernel when the headroom is calculated. It then
>>>> uses
>>>> orig_image_len to locate the start of the bzimage when the expansion
>>>> is done.
>>>> This is an issue when more than one bzimage is processed by the headroom
>>>> calculation logic, as it will leave orig_image_len set to the length
>>>> of the
>>>> last bzimage it processed.
>>>>
>>>> The boot module work introduced storing the headroom size on a per module
>>>> basis. By passing in the headroom from the boot module, orig_image_len
>>>> is no
>>>> longer needed to locate the beginning of the bzimage after the allocated
>>>> headroom. The bzimage functions are reworked as such, allowing the
>>>> removal of
>>>> orig_image_len and enabling them to be reused by multiple kernel boot
>>>> modules.
>>>>
>>>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
>>>> ---
>>>> xen/arch/x86/bzimage.c | 38 ++++++++++++++++++------------
>>>> xen/arch/x86/hvm/dom_build.c | 3 ++-
>>>> xen/arch/x86/include/asm/bzimage.h | 5 ++--
>>>> xen/arch/x86/pv/dom0_build.c | 3 ++-
>>>> 4 files changed, 30 insertions(+), 19 deletions(-)
>>>>
>>>> diff --git a/xen/arch/x86/bzimage.c b/xen/arch/x86/bzimage.c
>>>> index 66f648f311e4..32f0360d25b4 100644
>>>
>>>> @@ -103,13 +100,20 @@ unsigned long __init bzimage_headroom(void
>>>> *image_start,
>>>> return headroom;
>>>> }
>>>> -int __init bzimage_parse(void *image_base, void **image_start,
>>>> - unsigned long *image_len)
>>>> +int __init bzimage_parse(
>>>> + void *image_base, void **image_start, unsigned long headroom,
>>>> + unsigned long *image_len)
>>>> {
>>>> struct setup_header *hdr = (struct setup_header *)(*image_start);
>>>> int err = bzimage_check(hdr, *image_len);
>>>> - unsigned long output_len;
>>>> -
>>>> + unsigned long module_len = *image_len;
>>>> +
>>>> + /*
>>>> + * Variable err will have one of three values:
>>>> + * - < 0: a error occurred trying to inspect the contents
>>>> + * - > 0: the image is a bzImage
>>>> + * - == 0: not a bzImage, could be raw elf or elf.gz (vmlinuz.gz)
>>>> + */
>>>
>>> This comment seems a little independent of this change, so maybe it
>>> should be submitted separately. Also, I think a better placement would
>>> be next to bzimage_check().
>>>
>>>> if ( err < 0 )
>>>> return err;
>>>> @@ -118,21 +122,25 @@ int __init bzimage_parse(void *image_base, void
>>>> **image_start,
>>>> *image_start += (hdr->setup_sects + 1) * 512 + hdr-
>>>>> payload_offset;
>>>> *image_len = hdr->payload_length;
>>>
>>> @here
>>>
>>>> }
>>>> -
>>>> - if ( elf_is_elfbinary(*image_start, *image_len) )
>>>> - return 0;
>>>> + else
>>>> + {
>>>> + if ( elf_is_elfbinary(*image_start, *image_len) )
>>>> + return 0;
>>>> + else
>>>> + *image_len = *image_len - headroom;
>>>> + }
>>>
>>> I don't like this extra indention which includes the return. If you
>>> retain orig_image_len as a local variable, and set it above at "@here",
>>> you can have a smaller diff and leave cleaner logic.
>>
>> Right, but I find it sillier to be checking every kernel for elf when we
>> know it's a bzImage. While the elf check is fairly simplistic, it is
>> still multiple value checks.
bzimage_parse() needs to handle:
ELF
compressed ELF
bzImage
bzimage_check just returns
+ * Variable err will have one of three values:
+ * - < 0: a error occurred trying to inspect the contents
+ * - > 0: the image is a bzImage
+ * - == 0: not a bzImage, could be raw elf or elf.gz (vmlinuz.gz)
The elf_is_elfbinary() check is needed to detect plain ELF files and
skip decompression.
> Even without any extra local vars the above can be
>
> else if ( elf_is_elfbinary(*image_start, *image_len) )
> return 0;
> else
> *image_len = *image_len - headroom;
>
> which would already address the "extra indentation" aspect.
Yes. Thanks, but this actually helps illustrate how Dan's change is
incorrect. Well, Dan's elimination of orig_image_len makes *image_len
correct later, but I think there are further issues.
if ( err > 0 ) <--- bzImage case
{
*image_start += (hdr->setup_sects + 1) * 512 + hdr->payload_offset;
*image_len = hdr->payload_length;
}
else if ( elf_is_elfbinary(*image_start, *image_len) ) <--- ELF
return 0;
else <--- compressed ELF
*image_len = *image_len - headroom;
My addition is to the bzImage case, since that is what I saw breaking
and fixed. Setting orig_image_len in the else of the bzImage case would
not fix it.
Later changes:
> - output_len = output_length(*image_start, orig_image_len);
You drop output_len...
> -
> - if ( (err = perform_gunzip(image_base, *image_start,
orig_image_len)) > 0 )
> - err = decompress(*image_start, orig_image_len, image_base);
> + if ( (err = perform_gunzip(image_base, *image_start,
*image_len)) > 0 )
> + err = decompress(*image_start, *image_len, image_base);
>
> if ( !err )
> {
> + printk(XENLOG_ERR "%s(%d): decompression failed, reseting
image start and len\n",
> + __func__, err);
... but this is actually the success case...
> *image_start = image_base;
> - *image_len = output_len;
> + *image_len = module_len;
... it should be writing output_len.
> }
Again, please just use the minimal change from the other email. It is
what I've been using, so tested and working. That is your original
conversion with the one line fixup and much easier to review.
Regards,
Jason
On 4/23/25 15:27, Jason Andryuk wrote:
> On 2025-04-19 18:08, Daniel P. Smith wrote:
>> The bzimage logic uses the unit global orig_image_len to hold the
>> original
>> module length for the kernel when the headroom is calculated. It then
>> uses
>> orig_image_len to locate the start of the bzimage when the expansion
>> is done.
>> This is an issue when more than one bzimage is processed by the headroom
>> calculation logic, as it will leave orig_image_len set to the length
>> of the
>> last bzimage it processed.
>>
>> The boot module work introduced storing the headroom size on a per module
>> basis. By passing in the headroom from the boot module, orig_image_len
>> is no
>> longer needed to locate the beginning of the bzimage after the allocated
>> headroom. The bzimage functions are reworked as such, allowing the
>> removal of
>> orig_image_len and enabling them to be reused by multiple kernel boot
>> modules.
>>
>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
>> ---
>> xen/arch/x86/bzimage.c | 38 ++++++++++++++++++------------
>> xen/arch/x86/hvm/dom_build.c | 3 ++-
>> xen/arch/x86/include/asm/bzimage.h | 5 ++--
>> xen/arch/x86/pv/dom0_build.c | 3 ++-
>> 4 files changed, 30 insertions(+), 19 deletions(-)
>>
>> diff --git a/xen/arch/x86/bzimage.c b/xen/arch/x86/bzimage.c
>> index 66f648f311e4..32f0360d25b4 100644
>
>> @@ -103,13 +100,20 @@ unsigned long __init bzimage_headroom(void
>> *image_start,
>> return headroom;
>> }
>> -int __init bzimage_parse(void *image_base, void **image_start,
>> - unsigned long *image_len)
>> +int __init bzimage_parse(
>> + void *image_base, void **image_start, unsigned long headroom,
>> + unsigned long *image_len)
>> {
>> struct setup_header *hdr = (struct setup_header *)(*image_start);
>> int err = bzimage_check(hdr, *image_len);
>> - unsigned long output_len;
>> -
>> + unsigned long module_len = *image_len;
>> +
>> + /*
>> + * Variable err will have one of three values:
>> + * - < 0: a error occurred trying to inspect the contents
>> + * - > 0: the image is a bzImage
>> + * - == 0: not a bzImage, could be raw elf or elf.gz (vmlinuz.gz)
>> + */
>
> This comment seems a little independent of this change, so maybe it
> should be submitted separately. Also, I think a better placement would
> be next to bzimage_check().
>
>> if ( err < 0 )
>> return err;
>> @@ -118,21 +122,25 @@ int __init bzimage_parse(void *image_base, void
>> **image_start,
>> *image_start += (hdr->setup_sects + 1) * 512 + hdr-
>> >payload_offset;
>> *image_len = hdr->payload_length;
>
> @here
>
>> }
>> -
>> - if ( elf_is_elfbinary(*image_start, *image_len) )
>> - return 0;
>> + else
>> + {
>> + if ( elf_is_elfbinary(*image_start, *image_len) )
>> + return 0;
>> + else
>> + *image_len = *image_len - headroom;
>> + }
>
> I don't like this extra indention which includes the return. If you
> retain orig_image_len as a local variable, and set it above at "@here",
> you can have a smaller diff and leave cleaner logic.
>
> orig_image_len previously was set as a static variable, so the correct
> value was set after bzimage_headroom. Now that it is no longer static,
> we need to grab the hdr->payload_length value when we have a bzImage.
> Otherwise output_length will read past the end of the module.
>
> Below is the diff for bzimage.c with the change I suggest.
>
I will review it again when as I rebase it.
v/r,
dps
© 2016 - 2025 Red Hat, Inc.