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

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

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.
> Note this doesn't mean we should not fix Xen for uImage.
>
> > Now, there is also a question about supporting arm64 uImage kernels. 
> > In Xen kernel_zimage_place,
> > we do:
> > #ifdef CONFIG_ARM_64
> >      if ( info->type == DOMAIN_64BIT )
> >          return info->mem.bank[0].start + info->zimage.text_offset;
> > #endif
> >
> > So if we modify the uImage behavior for arm32, we will break 
> > consistency with arm64
> > (we would take uImage entry point address into account for arm32 but 
> > not for arm64).
> > At the moment at least they are in sync.
> That's a good point. It would be best if the behavior is consistent.
Currently, kernel_zimage_place() is called for both uImage and zImage.

Will it be sane if we write a different function for uImage ?

Something like this ...

static paddr_t __init kernel_uimage_place(struct kernel_info *info)

{

    /* Read and return uImage header's load address */

    return be32_to_cpu(uimage.load);

}

This will be consistent across arm32 and arm64

- Ayan

> Cheers,