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 <dpsm...@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
