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

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

On 08/12/2022 15:24, Michal Orzel wrote:
> Hi,
Hi,
> 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.
>
> 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.
IIUC, at present arm64 and arm32 are not in sync.

For Arm32, we do not consider "info->zimage.text_offset".

- Ayan

> > Cheers,
> >
> > --
> > Julien Grall
> ~Michal