Re: [RFC 33/38] x86/boot: refactor bzimage parser to be re-enterant

[Jason Andryuk <jason.andryuk@xxxxxxx>]

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@xxxxxxxxxxxxxxxxxxxx>
> ---
>   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) )