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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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.

Jan