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

["Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>]

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