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

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

On 08/12/2022 13:51, Julien Grall wrote:
> Hi,
Hi Julien,
> 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.
I will take care of it henceforth.
> 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.
Can we use a config option (STATIC_MEMORY or any option you may suggest) 
to distinguish between the two ?
Or using some magic number (eg 0xdeadbeef) for uimage.ep which will 
denote position independent ? This needs to be documented in 
docs/misc/arm/booting.txt.
Or ...

> 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.
I am open to any alternative suggestion.
> 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).
I agree.

- Ayan

> Cheers,