Re: [XEN v1] xen/Arm: Probe the entry point address of an uImage correctly

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 8 Dec 2022, Ayan Kumar Halder wrote:
> On 08/12/2022 16:53, Julien Grall wrote:
> > Hi,
> Hi,
> > 
> > On 08/12/2022 15:24, Michal Orzel wrote:
> > > On 08/12/2022 14:51, Julien Grall wrote:
> > > > Caution: This message originated from an External Source. Use proper
> > > > caution when opening attachments, clicking links, or responding.
> > > > 
> > > > 
> > > > Hi,
> > > > 
> > > > Title extra NIT: I have seen it multiple time and so far refrain to say
> > > > it. Please use 'arm' rather than 'Arm'. This is for consistency in the
> > > > way we name the subsystem in the title.
> > > > 
> > > > On 08/12/2022 12:49, Ayan Kumar Halder wrote:
> > > > > Currently, kernel_uimage_probe() does not set info->zimage.start. As a
> > > > > result, it contains the default value (ie 0). 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.ep" and set
> > > > > info->zimage.start. This will ensure that the binary is loaded at the
> > > > > correct address.
> > > > 
> > > > In non-statically allocated setup, a user doesn't know where the memory
> > > > for dom0/domU will be allocated.
> > > > 
> > > > So I think this was correct to ignore the address. In fact, I am worry
> > > > that...
> > > > 
> > > > > 
> > > > > Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>
> > > > > ---
> > > > > 
> > > > > I uncovered this issue while loading Zephyr as a dom0less domU with
> > > > > Xen on
> > > > > R52 FVP. Zephyr builds with static device tree. Thus, the load address
> > > > > is
> > > > > always fixed.
> > > > > 
> > > > >    xen/arch/arm/kernel.c | 2 ++
> > > > >    1 file changed, 2 insertions(+)
> > > > > 
> > > > > diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
> > > > > index 2556a45c38..e4e8c67669 100644
> > > > > --- a/xen/arch/arm/kernel.c
> > > > > +++ b/xen/arch/arm/kernel.c
> > > > > @@ -222,6 +222,8 @@ static int __init kernel_uimage_probe(struct
> > > > > kernel_info *info,
> > > > >        if ( len > size - sizeof(uimage) )
> > > > >            return -EINVAL;
> > > > > 
> > > > > +    info->zimage.start = be32_to_cpu(uimage.ep);
> > > > ... this will now ended up to break anyone that may have set an address
> > > > but didn't care where it should be loaded.
> > > > 
> > > > I also understand your use case but now, we have contradictory
> > > > approaches. I am not entirely sure how we can solve it. We may have to
> > > > break those users (Cc some folks that may use it). But we should figure
> > > > out what is the alternative for them.
> > > > 
> > > > If we decide to break those users, then this should be documented in the
> > > > commit message and in docs/misc/arm/booting.txt (which interestingly
> > > > didn't mention uImage).
> > > > 
> > > So the first issue with Zephyr is that it does not support zImage protocol
> > > for arm32.
> > > Volodymyr added support only for Image header for arm64 Zephyr.
> > > I guess this is why Ayan, willing to boot it on Xen (arm32), decided to
> > > add u-boot header.
> > 
> > If that's the only reason, then I would rather prefer if we go with zImage
> > for a few reasons:
> >  - The zImage protocol has at least some documentation (not perfect) of the
> > expected state of the memory/registers when jumping to the image.
> >  - AFAICT libxl is not (yet) supporting uImage. So this means zephyr cannot
> > be loaded on older Xen releases (not great).
> 
> I am exploring for a similar option as Volodymyr ie support zimage protocol
> for arm32.
> 
> But for that I need some public documentation that explains the zimage header
> format for arm32.
> 
> Refer xen/arch/arm/kernel.c
> 
> #define ZIMAGE32_MAGIC_OFFSET 0x24
> #define ZIMAGE32_START_OFFSET 0x28
> #define ZIMAGE32_END_OFFSET   0x2c
> #define ZIMAGE32_HEADER_LEN   0x30
> 
> #define ZIMAGE32_MAGIC 0x016f2818
> 
> Is this documented anywhere ?
> 
> I had a look at
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/arm/booting.rst
> , but there is nothing that explains the header format.

I think this is the closest to documentation of it:

http://www.simtec.co.uk/products/SWLINUX/files/booting_article.html