Re: [XEN v5] xen/arm: Probe the load/entry point address of an uImage correctly

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 19 Jan 2023, Michal Orzel wrote:
> Hello all,
> 
> On 13/01/2023 13:24, Ayan Kumar Halder wrote:
> > Currently, kernel_uimage_probe() does not read the load/entry point address
> > set in the uImge header. Thus, info->zimage.start is 0 (default value). This
> > causes, kernel_zimage_place() to treat the binary (contained within uImage)
> > as position independent executable. Thus, it loads it at an incorrect
> > address.
> > 
> > The correct approach would be to read "uimage.load" and set
> > info->zimage.start. This will ensure that the binary is loaded at the
> > correct address. Also, read "uimage.ep" and set info->entry (ie kernel entry
> > address).
> > 
> > If user provides load address (ie "uimage.load") as 0x0, then the image is
> > treated as position independent executable. Xen can load such an image at
> > any address it considers appropriate. A position independent executable
> > cannot have a fixed entry point address.
> > 
> > This behavior is applicable for both arm32 and arm64 platforms.
> > 
> > Earlier for arm32 and arm64 platforms, Xen was ignoring the load and entry
> > point address set in the uImage header. With this commit, Xen will use them.
> > This makes the behavior of Xen consistent with uboot for uimage headers.
> > 
> > Users who want to use Xen with statically partitioned domains, can provide
> > non zero load address and entry address for the dom0/domU kernel. It is
> > required that the load and entry address provided must be within the memory
> > region allocated by Xen.
> > 
> > A deviation from uboot behaviour is that we consider load address == 0x0,
> > to denote that the image supports position independent execution. This
> > is to make the behavior consistent across uImage and zImage.
> > 
> > Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>
> 
> When looking at this patch, I spotted one incorrect Xen behavior related to 
> supporting uImage kernels.
> And if we want to document that we support such kernels, we should take a 
> look at it.
> 
> Xen supports gzip compressed kernels and it tries to decompress them before 
> kernel_XXX_probe.
> For zImage and Image, their respective headers are built into the kernel 
> itself and then such image is compressed.
> This results in a gzip header appearing right at the top of the image and the 
> workflow works smoothly.
> 
> However, uImage header is something that is always added as a last step in 
> the image preparation using mkimage utility
> and always appears right at the top of the image. That is why uImage header 
> has a field "ih_comp" used to inform about the compression type.
> So the uImage gzip compressed image will have the uImage header before gzip 
> header. Because Xen tries to decompress the image
> before probing its header, this will not work for uImage.

It looks like to solve both this problem and also the other one about
zimage64.text_offset we would need to move the kernel_uimage_probe()
call earlier, before kernel_decompress() and before
kernel_zimage64_probe().

However, I can see that this patch is already complex as is, so I would
also be OK if any additional changes (e.g. moving kernel_uimage_probe()
earlier) were done in a separate patch on top.