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

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Julien,

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:

-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
+     * gzip header.
+    */
+    paddr_t addr = mod->start + offset;
+    paddr_t size = mod->size - offset;
 
     if ( size < 2 )
         return -EINVAL;
@@ -250,6 +256,13 @@ static __init int kernel_decompress(struct bootmodule *mod)
     for ( ; i < (1 << kernel_order_out); i++ )
         free_domheap_page(pages + i);
 
+    /*
+     * When freeing the kernel, we need to pass the module start address and
+     * size as they were before taking an offset to gzip header into account.
+     */
+    addr -= offset;
+    size += offset;
+
     /*
      * Free the original kernel, update the pointers to the
      * decompressed kernel

> 
> Cheers,
> 
> --
> Julien Grall

~Michal