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

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

On 16/12/2022 09:34, Julien Grall wrote:
> Hi,
Hi Julien,
> On 15/12/2022 12: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.
> At the cost of breaking existing users. I think this want to be spelt 
> out clearly.
Do you mean I should document this in "docs/misc/arm/booting.txt" ?
> > 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.
> You want to explain why users want to use "0x0". Or maybe the other 
> way around, that only a very limited set of user (e.g. in static 
> environment) should provide a fixed address.
Does the following explaination sound fine ?

"Users who want to use Xen with statically partitioned domains, should 
provide the fixed non zero load address for the dom0/domU kernel."
> > 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.
> >
> > Some unanswered queries from v1 :-
> >
> > Q1. "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."
> > A. I am not sure the best way to handle this. Can we make the uImage 
> > probe
> > dependent on STATIC_MEMORY ? Currently, I have mentioned that user 
> > should
> > use load address = 0x0 if they want Xen to ignore it.
> See above.
>
> > Q2. Zephyr or any other RTOS have different requirements, then we may 
> > need
> > to modify Xen and document it.
> > A. I will take it in a separate email/patch where we can list the
> > requirements for booting Zephyr.
> >
> >   docs/misc/arm/booting.txt         | 11 ++++++++++-
> >   xen/arch/arm/include/asm/kernel.h |  2 +-
> >   xen/arch/arm/kernel.c             | 26 +++++++++++++++++++++++++-
> >   3 files changed, 36 insertions(+), 3 deletions(-)
> >
> > diff --git a/docs/misc/arm/booting.txt b/docs/misc/arm/booting.txt
> > index 3e0c03e065..872262686a 100644
> > --- a/docs/misc/arm/booting.txt
> > +++ b/docs/misc/arm/booting.txt
> > @@ -4,7 +4,13 @@ Booting Xen
> >   Xen follows the zImage protocol defined for 32-bit ARM Linux[1] and 
> > the
> >   Image protocol defined for ARM64 Linux[2].
> >   -In both cases the recommendation to boot in HYP/EL2 mode is a strict
> > +Xen also supports the legacy image protocol[3] for 32-bit ARM and 
> > ARM64.
> > +For now, it supports images where load address is same as entry 
> > 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.
> Hmmmm... The section above is related to Xen itself and not the guest 
> Image. So I think this is misplaced.
I don't see a booting section for the guest. Should I create one in this 
file ?
> > +
> > +In all cases the recommendation to boot in HYP/EL2 mode is a strict
> >   requirement for Xen.
> >     The exceptions to this on 32-bit ARM are as follows:
> > @@ -39,3 +45,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..ee69a47052 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;
> I think the comment should be replaced by "Must be 0 for 64-bit Image".
But, 64 bit image can also have a fixed load address.

For zImage(64), start is always 0.

For uImage(64), start can be a fixed load address (if it is not 0).

> >           } zimage;
> >       };
> >   };
> > diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
> > index 23b840ea9e..81ac945f5b 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,30 @@ 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()).
> > +     */
> > +    if ( info->zimage.start != be32_to_cpu(uimage.load) )
> > +    {
> > +        panic("Unable to support mismatching load address and entry 
> > address\n");
> > +        return -EINVAL;
> > +    }
> > +
> > +    /*
> > +     * While uboot considers 0x0 to be a valid load/start address, 
> > for Xen
> > +     * to mantain parity with zimage, we consider 0x0 to denote 
> > position
> > +     * independent image. That means Xen is free to load such an 
> > image at
> > +     * any valid address.
> > +     * Thus, we will print an appropriate warning.
> > +     */
> > +    if ( info->zimage.start == 0 )
> > +        printk(XENLOG_WARNING "No valid load address provided\n");
> This is likely the way user will want to use it in a non-static 
> environment. So why is this a warning?
Sorry, it should be.

printk(XENLOG_INFO, "No load address provided. Assuming domain is 
dynamically allocated\n");
I just wanted to give user a heads-up in case if they are using in a 
static environment.
I can drop the message if it is not much useful.

- Ayan

> Cheers,