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
