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

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

On 7/19/22 09:19, Jan Beulich wrote:
> 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.

Ack. As I mentioned, I tried dropping these based on past reviews. I
will do another pass to try to catch just formatting and drop them.

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

I am not sure whether it is possible to do this field by field. This is
why I was asking on IRC on about dealing with this kind of situation. As
soon as multiboot_info_t/module_t are replaced with struct
boot_info{}/struct boot_module{} a wholesale replacement must be done.

v/r,
dps