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 <dpsm...@apertussolutions.com>
>>> ---
>>> 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
