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

[Jason Andryuk <jason.andryuk@xxxxxxx>]

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