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