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

[Julien Grall <julien@xxxxxxx>]

On 20/12/2022 09:44, Michal Orzel wrote:
> Hi Ayan,
>
> On 17/12/2022 20:38, 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. Also, it checks that the load address and entry address
> > are the same. The reason being we currently support non compressed images
> > for uImage header. And as seen in uboot sources(image_decomp(), case
> > IH_COMP_NONE), load address and entry address can be the same.
> >
> > This behavior is applicable for both arm and arm64 platforms.
> >
> > Earlier for arm32 and arm64 platforms, Xen was ignoring the entry point
> > address set in the uImage header. With this commit, Xen will use the
> > kernel entry point address as specified in the header. This makes the
> > behavior of Xen consistent with uboot for uimage headers.
> >
> > Users who want to use Xen with statically partitioned domains, can
> > provide the fixed non zero load address for the dom0/domU kernel.
> >
> > 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>
> > ---
> >
> > Changes from v1 :-
> > 1. Added a check to ensure load address and entry address are the same.
> > 2. Considered load address == 0x0 as position independent execution.
> > 3. Ensured that the uImage header interpretation is consistent across
> > arm32 and arm64.
> >
> > v2 :-
> > 1. Mentioned the change in existing behavior in booting.txt.
> > 2. Updated booting.txt with a new section to document "Booting Guests".
> >
> >   docs/misc/arm/booting.txt         | 21 +++++++++++++++++++++
> >   xen/arch/arm/include/asm/kernel.h |  2 +-
> >   xen/arch/arm/kernel.c             | 27 ++++++++++++++++++++++++++-
> >   3 files changed, 48 insertions(+), 2 deletions(-)
> >
> > diff --git a/docs/misc/arm/booting.txt b/docs/misc/arm/booting.txt
> > index 3e0c03e065..01b12b49a5 100644
> > --- a/docs/misc/arm/booting.txt
> > +++ b/docs/misc/arm/booting.txt
> > @@ -23,6 +23,24 @@ The exceptions to this on 32-bit ARM are as follows:
> >   
> >   There are no exception on 64-bit ARM.
> >   
> > +Booting Guests
> > +--------------
> > +
> > +Xen supports the legacy image protocol[3], zImage protocol for 32-bit ARM
> uImage is not a protocol. It is just a header with no other information \wrt
> boot requirements.
>
> > +Linux[1] and Image protocol defined for ARM64[2].
> > +
> > +Earlier for legacy image protocol, Xen ignored the load address and entry
> > +point specified in the header. This has now changed.
> > +
> > +Now, it loads the image at the load address provided in the header.
> > +For now, it supports images where load address is same as entry point.
> Would be beneficial to add explanation why load address must be equal to start 
> address.
>
> > +
> > +A deviation from uboot is that, Xen treats "load address == 0x0" as
> > +position independent execution. Thus, Xen will load such an image at an
> > +address it considers appropriate.
> > +
> > +Users who want to use Xen with statically partitioned domains, can provide
> > +the fixed non zero load address for the dom0/domU kernel.
> I think this section is missing a note that in case of not PIE, a load/start 
> address
> specified by the user in a header must be within the memory region allocated by 
> Xen.
>
> >   
> >   Firmware/bootloader requirements
> >   --------------------------------
> > @@ -39,3 +57,6 @@ Latest version: 
> > http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/t
> >   
> >   [2] linux/Documentation/arm64/booting.rst
> >   Latest version: 
> > http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/arm64/booting.rst
> > +
> > +[3] legacy format header
> > +Latest version: 
> > https://source.denx.de/u-boot/u-boot/-/blob/master/include/image.h#L315
> > diff --git a/xen/arch/arm/include/asm/kernel.h 
> > b/xen/arch/arm/include/asm/kernel.h
> > index 5bb30c3f2f..4617cdc83b 100644
> > --- a/xen/arch/arm/include/asm/kernel.h
> > +++ b/xen/arch/arm/include/asm/kernel.h
> > @@ -72,7 +72,7 @@ struct kernel_info {
> >   #ifdef CONFIG_ARM_64
> >               paddr_t text_offset; /* 64-bit Image only */
> >   #endif
> > -            paddr_t start; /* 32-bit zImage only */
> > +            paddr_t start; /* Must be 0 for 64-bit Image */
> >           } zimage;
> >       };
> >   };
> > diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
> > index 23b840ea9e..e9c18993ef 100644
> > --- a/xen/arch/arm/kernel.c
> > +++ b/xen/arch/arm/kernel.c
> > @@ -127,7 +127,7 @@ static paddr_t __init kernel_zimage_place(struct 
> > kernel_info *info)
> >       paddr_t load_addr;
> >   
> >   #ifdef CONFIG_ARM_64
> > -    if ( info->type == DOMAIN_64BIT )
> > +    if ( (info->type == DOMAIN_64BIT) && (info->zimage.start == 0) )
> >           return info->mem.bank[0].start + info->zimage.text_offset;
> >   #endif
> >   
> > @@ -223,6 +223,31 @@ 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);
> > +
> > +    /*
> > +     * Currently, we support uImage headers for uncompressed images only.
> > +     * Thus, it is valid for the load address and start address to be the
> > +     * same. This is consistent with the uboot behavior (Refer
> > +     * "case IH_COMP_NONE" in image_decomp().
> Please make it clear that you are referring to uboot function.
Could we avoid to mention the u-boot function? The reason I am asking is 
the code is in a different repo and the function name can easily change 
without us noticing (so the comment would rot).
Is the behavior documented somewhere in U-boot (or online)? If not, 
what's the guarantee that it will not change?
Cheers,

--
Julien Grall