Re: [PATCH v1 03/18] x86: adopt new boot info structures

[Jan Beulich <jbeulich@suse.com>]

On 06.07.2022 23:04, Daniel P. Smith wrote:
> This commit replaces the use of the multiboot v1 structures starting
> at __start_xen(). The majority of this commit is converting the fields
> being accessed for the startup calculations. While adapting the ucode
> boot module location logic, this code was refactored to reduce some
> of the unnecessary complexity.

Things like this or ...

> --- a/xen/arch/x86/bzimage.c
> +++ b/xen/arch/x86/bzimage.c
> @@ -69,10 +69,8 @@ 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)
> +unsigned long __init bzimage_headroom(
> +    void *image_start, unsigned long image_length)
>  {
>      struct setup_header *hdr = (struct setup_header *)image_start;
>      int err;
> @@ -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 int 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;
> @@ -125,7 +125,7 @@ int __init bzimage_parse(void *image_base, void **image_start,
>  
>      BUG_ON(!(image_base < *image_start));
>  
> -    output_len = output_length(*image_start, orig_image_len);
> +    output_len = output_length(*image_start, *image_len);
>  
>      if ( (err = perform_gunzip(image_base, *image_start, orig_image_len)) > 0 )
>          err = decompress(*image_start, orig_image_len, image_base);

... whatever the deal is here want factoring out. Also you want to avoid
making formatting changes (like in the function headers here) in an
already large patch, when you don't otherwise touch the functions. I'm
not even convinced the formatting changes are desirable here, so I'd
like to ask that even on code you do touch for other reasons you do so
only if the existing layout ends up really awkward.

I have not looked in any further detail at this patch, sorry. Together
with my comment on the earlier patch I conclude that it might be best
if you moved things to the new representation field by field (or set of
related fields), introducing the new fields in the abstraction struct
as they are being made use of.

Jan