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

[Julien Grall <julien@xxxxxxx>]

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

Cheers,

--
Julien Grall