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

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

Hi Julien/Michal,

On 20/12/2022 10:05, Julien Grall wrote:
> 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.
Answered below.
> > > +
> > > +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?
I could not find any documentation which states this. I found this from 
the following in uboot source code.
https://source.denx.de/u-boot/u-boot/-/blob/master/boot/image.c#L458

AFAIU when kernel_uimage_probe() get invoked, the image has already been 
decompressed. So, I added this as a limitation.
I will remove the limitation if it does not look correct.

- Ayan

> Cheers,