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

[Julien Grall <julien@xxxxxxx>]

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.

>   
>       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