Re: [PATCH 2/2] xen/arm: Add support for booting gzip compressed uImages

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Julien,

On 01/02/2023 19:54, Julien Grall wrote:
> 
> 
> On 01/02/2023 12:56, Michal Orzel wrote:
>> Hi Julien,
> 
> Hi Michal,
> 
>> On 01/02/2023 12:20, Julien Grall wrote:
>>>
>>>
>>> On 01/02/2023 11:01, Michal Orzel wrote:
>>>> I would prefer not to do this in this series as the goals are different. 
>>>> This series is to remove
>>>> the known Xen limitation.
>>>
>>> The reason I am asking is it effectively change the way you would
>>> implement. If we were going to support zImage/Image within uImage, then
>>> we would need to fallthrough rather than calling kernel_decompress().
>>>
>>> I am not aware of any at the moment. Better asking now than realizing
>>> after the fact that there is a need...
>> We need uImage support as there is more and more need to support booting
>> raw images of some RTOSes (there is always additional concern \wrt booting 
>> requirements
>> but at least the header allows to try to boot them). We are not aware of any 
>> need
>> to do some special handling to parse more than one header + from what I said 
>> earlier,
>> this is an unusual approach not seen to be handled by anyone.
>>
>>>
>>>>>> This could be solved by doing (not harmful in my opinion for common case)
>>>>>> addr &= PAGE_MASK.
>>>>> I don't quite understand your argument here. We need a check that work
>>>>> for everyone (not only in the common case).
>>>> As we assume the kernel module is at page aligned address (otherwise the 
>>>> issue you observed
>>>> can happen not only in uImage compressed case)
>>>
>>> I am not aware of such assumption. It is allowed to use non page-aligned
>>> address and it is correct for Xen to not free the first part if it is
>>> not page aligned because the first part may contain data from another
>>> module (or else).
>>>
>>>> , having a check like
>>>> this is generic. I.e. for normal usecase (no uImage compressed), addr &= 
>>>> PAGE_MASK
>>>> does not modify the address. For uImage compressed usecase it causes the 
>>>> addr to get
>>>> back to the previous value (before we added header size to it).
>>>>
>>>> Apart from the addr, we need to pass the correct size (as we extracted 
>>>> header size from it).
>>>> We could have the following (with appropriate comment):
>>>> size += addr - (addr & PAGE_MASK);
>>>> addr &= PAGE_MASK;
>>>> fw_unreserved_regions(addr, addr + size, init_domheap_pages, 0);
>>>>
>>>> It does not look great but solves the uImage issue and does not modify
>>>> the previous behavior. Anyway, I'm open for suggestions.
>>>
>>> Two options:
>>>    1) Move the call to fw_unreserved_regions() outside of
>>> kernel_decompress().
>>>    2) Pass the offset of the gzip header to kernel_decompress().
>>> Something like where it would be  sizeof(uimage) in the uImage case or 0
>>> otherwise.
>>>
>>> I have a slight preference for the latter.
>> That is cool.
>> I'm in favor of this as well because it would allow not to set 
>> mod->start,size
>> from kernel_uimage_probe. Everything can be done in kernel_decompress:
>>
>> Diff:
> 
> A few comments because you resend the patch with it included.
> 
>>
>> -static __init int kernel_decompress(struct bootmodule *mod)
>> +static __init int kernel_decompress(struct bootmodule *mod, uint32_t offset)
>>   {
>>       char *output, *input;
>>       char magic[2];
>> @@ -201,8 +201,14 @@ static __init int kernel_decompress(struct bootmodule 
>> *mod)
>>       struct page_info *pages;
>>       mfn_t mfn;
>>       int i;
>> -    paddr_t addr = mod->start;
>> -    paddr_t size = mod->size;
>> +
>> +    /*
>> +     * It might be that gzip header does not appear at the start address
>> +     * (i.e. in case of compressed uImage) so take into account offset to
> 
> NIT: I would use 'e.g.' because in the future we may have multiple
> reason where the offset is not zero.
> 
>> +     * gzip header.
>> +    */ > +    paddr_t addr = mod->start + offset;
>> +    paddr_t size = mod->size - offset;
> 
> You want to check that mod->size is at least equal to offset.
Sounds reasonable. Thanks.

~Michal