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

[Julien Grall <julien@xxxxxxx>]

Hi,

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.
> 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.
> 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.
> +
> +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".

>           } 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?
Cheers,

--
Julien Grall