Currently, kernel_uimage_probe() does not read the load/entry point address
set in the uImge header. Thus, info->zimage.start is 0 (default value). 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.load" and set
info->zimage.start. This will ensure that the binary is loaded at the
correct address. Also, read "uimage.ep" and set info->entry (ie kernel entry
address).

If user provides load address (ie "uimage.load") as 0x0, then the image is
treated as position independent executable. Xen can load such an image at
any address it considers appropriate. A position independent executable
cannot have a fixed entry point address.

This behavior is applicable for both arm32 and arm64 platforms.

Earlier for arm32 and arm64 platforms, Xen was ignoring the load and entry
point address set in the uImage header. With this commit, Xen will use them.
This makes the behavior of Xen consistent with uboot for uimage headers.

Users who want to use Xen with statically partitioned domains, can provide
non zero load address and entry address for the dom0/domU kernel. It is
required that the load and entry address provided must be within the memory
region allocated by Xen.

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@amd.com>
---

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

v3 :-
1. Removed the constraint that the entry point should be same as the load
address. Thus, Xen uses both the load address and entry point to determine
where the image is to be copied and the start address.
2. Updated documentation to denote that load address and start address
should be within the memory region allocated by Xen.
3. Added constraint that user cannot provide entry point for a position
independent executable (PIE) image.

v4 :-
1. Explicitly mentioned the version in booting.txt from when the uImage
probing behavior has changed.
2. Logged the requested load address and entry point parsed from the uImage
header.
3. Some style issues.

 docs/misc/arm/booting.txt         | 26 ++++++++++++++++
 xen/arch/arm/include/asm/kernel.h |  2 +-
 xen/arch/arm/kernel.c             | 49 +++++++++++++++++++++++++++++--
 3 files changed, 73 insertions(+), 4 deletions(-)

diff --git a/docs/misc/arm/booting.txt b/docs/misc/arm/booting.txt
index 3e0c03e065..aeb0123e8d 100644
--- a/docs/misc/arm/booting.txt
+++ b/docs/misc/arm/booting.txt
@@ -23,6 +23,28 @@ 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 header[3], zImage protocol for 32-bit
+ARM Linux[1] and Image protocol defined for ARM64[2].
+
+Until Xen 4.17, in case of 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.
+And the entry point is used as the kernel start address.
+
+A deviation from uboot is that, Xen treats "load address == 0x0" as
+position independent execution (PIE). Thus, Xen will load such an image
+at an address it considers appropriate. Also, user cannot specify the
+entry point of a PIE image since the start address cennot be
+predetermined.
+
+Users who want to use Xen with statically partitioned domains, can provide
+the fixed non zero load address and start address for the dom0/domU kernel.
+The load address and start address specified by the user in the header must
+be within the memory region allocated by Xen.
 
 Firmware/bootloader requirements
 --------------------------------
@@ -39,3 +61,7 @@ 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
+https://linux.die.net/man/1/mkimage
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..0b7f591857 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
 
@@ -162,7 +162,12 @@ static void __init kernel_zimage_load(struct kernel_info *info)
     void *kernel;
     int rc;
 
-    info->entry = load_addr;
+    /*
+     * If the image does not have a fixed entry point, then use the load
+     * address as the entry point.
+     */
+    if ( info->entry == 0 )
+        info->entry = load_addr;
 
     place_modules(info, load_addr, load_addr + len);
 
@@ -223,10 +228,38 @@ static int __init kernel_uimage_probe(struct kernel_info *info,
     if ( len > size - sizeof(uimage) )
         return -EINVAL;
 
+    info->zimage.start = be32_to_cpu(uimage.load);
+    info->entry = be32_to_cpu(uimage.ep);
+
+    /*
+     * While uboot considers 0x0 to be a valid load/start address, for Xen
+     * to maintain 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.
+     */
+    if ( info->zimage.start == 0 )
+        printk(XENLOG_INFO
+               "No load address provided. Xen will decide where to load it.\n");
+    else
+        printk(XENLOG_INFO
+               "Provided load address: %"PRIpaddr" and entry address: %"PRIpaddr"\n",
+               info->zimage.start, info->entry);
+
+    /*
+     * If the image supports position independent execution, then user cannot
+     * provide an entry point as Xen will load such an image at any appropriate
+     * memory address. Thus, we need to return error.
+     */
+    if ( (info->zimage.start == 0) && (info->entry != 0) )
+    {
+        printk(XENLOG_ERR
+               "Entry point cannot be non zero for PIE image.\n");
+        return -EINVAL;
+    }
+
     info->zimage.kernel_addr = addr + sizeof(uimage);
     info->zimage.len = len;
 
-    info->entry = info->zimage.start;
     info->load = kernel_zimage_load;
 
 #ifdef CONFIG_ARM_64
@@ -366,6 +399,7 @@ static int __init kernel_zimage64_probe(struct kernel_info *info,
     info->zimage.kernel_addr = addr;
     info->zimage.len = end - start;
     info->zimage.text_offset = zimage.text_offset;
+    info->zimage.start = 0;
 
     info->load = kernel_zimage_load;
 
@@ -436,6 +470,15 @@ int __init kernel_probe(struct kernel_info *info,
     u64 kernel_addr, initrd_addr, dtb_addr, size;
     int rc;
 
+    /*
+     * We need to initialize start to 0. This field may be populated during
+     * kernel_xxx_probe() if the image has a fixed entry point (for e.g.
+     * uimage.ep).
+     * We will use this to determine if the image has a fixed entry point or
+     * the load address should be used as the start address.
+     */
+    info->entry = 0;
+
     /* domain is NULL only for the hardware domain */
     if ( domain == NULL )
     {
-- 
2.17.1

On Fri, 13 Jan 2023, Ayan Kumar Halder wrote:
> Currently, kernel_uimage_probe() does not read the load/entry point address
> set in the uImge header. Thus, info->zimage.start is 0 (default value). 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.load" and set
> info->zimage.start. This will ensure that the binary is loaded at the
> correct address. Also, read "uimage.ep" and set info->entry (ie kernel entry
> address).
> 
> If user provides load address (ie "uimage.load") as 0x0, then the image is
> treated as position independent executable. Xen can load such an image at
> any address it considers appropriate. A position independent executable
> cannot have a fixed entry point address.
> 
> This behavior is applicable for both arm32 and arm64 platforms.
> 
> Earlier for arm32 and arm64 platforms, Xen was ignoring the load and entry
> point address set in the uImage header. With this commit, Xen will use them.
> This makes the behavior of Xen consistent with uboot for uimage headers.
> 
> Users who want to use Xen with statically partitioned domains, can provide
> non zero load address and entry address for the dom0/domU kernel. It is
> required that the load and entry address provided must be within the memory
> region allocated by Xen.
> 
> 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@amd.com>
> ---
> 
> 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".
> 
> v3 :-
> 1. Removed the constraint that the entry point should be same as the load
> address. Thus, Xen uses both the load address and entry point to determine
> where the image is to be copied and the start address.
> 2. Updated documentation to denote that load address and start address
> should be within the memory region allocated by Xen.
> 3. Added constraint that user cannot provide entry point for a position
> independent executable (PIE) image.
> 
> v4 :-
> 1. Explicitly mentioned the version in booting.txt from when the uImage
> probing behavior has changed.
> 2. Logged the requested load address and entry point parsed from the uImage
> header.
> 3. Some style issues.
> 
>  docs/misc/arm/booting.txt         | 26 ++++++++++++++++
>  xen/arch/arm/include/asm/kernel.h |  2 +-
>  xen/arch/arm/kernel.c             | 49 +++++++++++++++++++++++++++++--
>  3 files changed, 73 insertions(+), 4 deletions(-)
> 
> diff --git a/docs/misc/arm/booting.txt b/docs/misc/arm/booting.txt
> index 3e0c03e065..aeb0123e8d 100644
> --- a/docs/misc/arm/booting.txt
> +++ b/docs/misc/arm/booting.txt
> @@ -23,6 +23,28 @@ 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 header[3], zImage protocol for 32-bit
> +ARM Linux[1] and Image protocol defined for ARM64[2].
> +
> +Until Xen 4.17, in case of 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.
> +And the entry point is used as the kernel start address.
> +
> +A deviation from uboot is that, Xen treats "load address == 0x0" as
> +position independent execution (PIE). Thus, Xen will load such an image
> +at an address it considers appropriate. Also, user cannot specify the
> +entry point of a PIE image since the start address cennot be
> +predetermined.
> +
> +Users who want to use Xen with statically partitioned domains, can provide
> +the fixed non zero load address and start address for the dom0/domU kernel.
> +The load address and start address specified by the user in the header must
> +be within the memory region allocated by Xen.
>  
>  Firmware/bootloader requirements
>  --------------------------------
> @@ -39,3 +61,7 @@ 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
> +https://linux.die.net/man/1/mkimage
> 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..0b7f591857 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;

This is an issue because if we have a zImage64 kernel binary with a
uimage header, we are not setting zimage.text_offset appropriately, if I
am not mistaken.

The way booting.txt is written in this patch, I think the matching
behavior would be that if there is a uimage header, then the zImage64
header is ignored. If the uimage header has uimage.load == zero, then
we should allocate the load_addr for the kernel (PIE case).

I think it would also be OK if we choose the different behavior that if
there is a uimage header but uimage.load == zero, then we look at
zImage64.text_offset next.

Either way is OK for me as long as it is clearly specified in
booting.txt.




>  #endif
>  
> @@ -162,7 +162,12 @@ static void __init kernel_zimage_load(struct kernel_info *info)
>      void *kernel;
>      int rc;
>  
> -    info->entry = load_addr;
> +    /*
> +     * If the image does not have a fixed entry point, then use the load
> +     * address as the entry point.
> +     */
> +    if ( info->entry == 0 )
> +        info->entry = load_addr;
>  
>      place_modules(info, load_addr, load_addr + len);
>  
> @@ -223,10 +228,38 @@ static int __init kernel_uimage_probe(struct kernel_info *info,
>      if ( len > size - sizeof(uimage) )
>          return -EINVAL;
>  
> +    info->zimage.start = be32_to_cpu(uimage.load);
> +    info->entry = be32_to_cpu(uimage.ep);
> +
> +    /*
> +     * While uboot considers 0x0 to be a valid load/start address, for Xen
> +     * to maintain 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.
> +     */
> +    if ( info->zimage.start == 0 )
> +        printk(XENLOG_INFO
> +               "No load address provided. Xen will decide where to load it.\n");
> +    else
> +        printk(XENLOG_INFO
> +               "Provided load address: %"PRIpaddr" and entry address: %"PRIpaddr"\n",
> +               info->zimage.start, info->entry);
> +
> +    /*
> +     * If the image supports position independent execution, then user cannot
> +     * provide an entry point as Xen will load such an image at any appropriate
> +     * memory address. Thus, we need to return error.
> +     */
> +    if ( (info->zimage.start == 0) && (info->entry != 0) )
> +    {
> +        printk(XENLOG_ERR
> +               "Entry point cannot be non zero for PIE image.\n");
> +        return -EINVAL;
> +    }
> +
>      info->zimage.kernel_addr = addr + sizeof(uimage);
>      info->zimage.len = len;
>  
> -    info->entry = info->zimage.start;
>      info->load = kernel_zimage_load;
>  
>  #ifdef CONFIG_ARM_64
> @@ -366,6 +399,7 @@ static int __init kernel_zimage64_probe(struct kernel_info *info,
>      info->zimage.kernel_addr = addr;
>      info->zimage.len = end - start;
>      info->zimage.text_offset = zimage.text_offset;
> +    info->zimage.start = 0;
>  
>      info->load = kernel_zimage_load;
>  
> @@ -436,6 +470,15 @@ int __init kernel_probe(struct kernel_info *info,
>      u64 kernel_addr, initrd_addr, dtb_addr, size;
>      int rc;
>  
> +    /*
> +     * We need to initialize start to 0. This field may be populated during
> +     * kernel_xxx_probe() if the image has a fixed entry point (for e.g.
> +     * uimage.ep).
> +     * We will use this to determine if the image has a fixed entry point or
> +     * the load address should be used as the start address.
> +     */
> +    info->entry = 0;
> +
>      /* domain is NULL only for the hardware domain */
>      if ( domain == NULL )
>      {
> -- 
> 2.17.1
> 
>

Hi Stefano,

On 20/01/2023 22:28, Stefano Stabellini wrote:
> On Fri, 13 Jan 2023, Ayan Kumar Halder wrote:
>> Currently, kernel_uimage_probe() does not read the load/entry point address
>> set in the uImge header. Thus, info->zimage.start is 0 (default value). 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.load" and set
>> info->zimage.start. This will ensure that the binary is loaded at the
>> correct address. Also, read "uimage.ep" and set info->entry (ie kernel entry
>> address).
>>
>> If user provides load address (ie "uimage.load") as 0x0, then the image is
>> treated as position independent executable. Xen can load such an image at
>> any address it considers appropriate. A position independent executable
>> cannot have a fixed entry point address.
>>
>> This behavior is applicable for both arm32 and arm64 platforms.
>>
>> Earlier for arm32 and arm64 platforms, Xen was ignoring the load and entry
>> point address set in the uImage header. With this commit, Xen will use them.
>> This makes the behavior of Xen consistent with uboot for uimage headers.
>>
>> Users who want to use Xen with statically partitioned domains, can provide
>> non zero load address and entry address for the dom0/domU kernel. It is
>> required that the load and entry address provided must be within the memory
>> region allocated by Xen.
>>
>> 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@amd.com>
>> ---
>>
>> 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".
>>
>> v3 :-
>> 1. Removed the constraint that the entry point should be same as the load
>> address. Thus, Xen uses both the load address and entry point to determine
>> where the image is to be copied and the start address.
>> 2. Updated documentation to denote that load address and start address
>> should be within the memory region allocated by Xen.
>> 3. Added constraint that user cannot provide entry point for a position
>> independent executable (PIE) image.
>>
>> v4 :-
>> 1. Explicitly mentioned the version in booting.txt from when the uImage
>> probing behavior has changed.
>> 2. Logged the requested load address and entry point parsed from the uImage
>> header.
>> 3. Some style issues.
>>
>>   docs/misc/arm/booting.txt         | 26 ++++++++++++++++
>>   xen/arch/arm/include/asm/kernel.h |  2 +-
>>   xen/arch/arm/kernel.c             | 49 +++++++++++++++++++++++++++++--
>>   3 files changed, 73 insertions(+), 4 deletions(-)
>>
>> diff --git a/docs/misc/arm/booting.txt b/docs/misc/arm/booting.txt
>> index 3e0c03e065..aeb0123e8d 100644
>> --- a/docs/misc/arm/booting.txt
>> +++ b/docs/misc/arm/booting.txt
>> @@ -23,6 +23,28 @@ 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 header[3], zImage protocol for 32-bit
>> +ARM Linux[1] and Image protocol defined for ARM64[2].
>> +
>> +Until Xen 4.17, in case of 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.
>> +And the entry point is used as the kernel start address.
>> +
>> +A deviation from uboot is that, Xen treats "load address == 0x0" as
>> +position independent execution (PIE). Thus, Xen will load such an image
>> +at an address it considers appropriate. Also, user cannot specify the
>> +entry point of a PIE image since the start address cennot be
>> +predetermined.
>> +
>> +Users who want to use Xen with statically partitioned domains, can provide
>> +the fixed non zero load address and start address for the dom0/domU kernel.
>> +The load address and start address specified by the user in the header must
>> +be within the memory region allocated by Xen.
>>   
>>   Firmware/bootloader requirements
>>   --------------------------------
>> @@ -39,3 +61,7 @@ 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
>> +https://linux.die.net/man/1/mkimage
>> 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..0b7f591857 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;
> This is an issue because if we have a zImage64 kernel binary with a
> uimage header, we are not setting zimage.text_offset appropriately, if I
> am not mistaken.
>
> The way booting.txt is written in this patch, I think the matching
> behavior would be that if there is a uimage header, then the zImage64
> header is ignored.

I have followed this approach in

"[XEN v6] xen/arm: Probe the load/entry point address of an uImage 
correctly"

- Ayan

> If the uimage header has uimage.load == zero, then
> we should allocate the load_addr for the kernel (PIE case).
>
> I think it would also be OK if we choose the different behavior that if
> there is a uimage header but uimage.load == zero, then we look at
> zImage64.text_offset next.
>
> Either way is OK for me as long as it is clearly specified in
> booting.txt.
>
>
>
>
>>   #endif
>>   
>> @@ -162,7 +162,12 @@ static void __init kernel_zimage_load(struct kernel_info *info)
>>       void *kernel;
>>       int rc;
>>   
>> -    info->entry = load_addr;
>> +    /*
>> +     * If the image does not have a fixed entry point, then use the load
>> +     * address as the entry point.
>> +     */
>> +    if ( info->entry == 0 )
>> +        info->entry = load_addr;
>>   
>>       place_modules(info, load_addr, load_addr + len);
>>   
>> @@ -223,10 +228,38 @@ static int __init kernel_uimage_probe(struct kernel_info *info,
>>       if ( len > size - sizeof(uimage) )
>>           return -EINVAL;
>>   
>> +    info->zimage.start = be32_to_cpu(uimage.load);
>> +    info->entry = be32_to_cpu(uimage.ep);
>> +
>> +    /*
>> +     * While uboot considers 0x0 to be a valid load/start address, for Xen
>> +     * to maintain 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.
>> +     */
>> +    if ( info->zimage.start == 0 )
>> +        printk(XENLOG_INFO
>> +               "No load address provided. Xen will decide where to load it.\n");
>> +    else
>> +        printk(XENLOG_INFO
>> +               "Provided load address: %"PRIpaddr" and entry address: %"PRIpaddr"\n",
>> +               info->zimage.start, info->entry);
>> +
>> +    /*
>> +     * If the image supports position independent execution, then user cannot
>> +     * provide an entry point as Xen will load such an image at any appropriate
>> +     * memory address. Thus, we need to return error.
>> +     */
>> +    if ( (info->zimage.start == 0) && (info->entry != 0) )
>> +    {
>> +        printk(XENLOG_ERR
>> +               "Entry point cannot be non zero for PIE image.\n");
>> +        return -EINVAL;
>> +    }
>> +
>>       info->zimage.kernel_addr = addr + sizeof(uimage);
>>       info->zimage.len = len;
>>   
>> -    info->entry = info->zimage.start;
>>       info->load = kernel_zimage_load;
>>   
>>   #ifdef CONFIG_ARM_64
>> @@ -366,6 +399,7 @@ static int __init kernel_zimage64_probe(struct kernel_info *info,
>>       info->zimage.kernel_addr = addr;
>>       info->zimage.len = end - start;
>>       info->zimage.text_offset = zimage.text_offset;
>> +    info->zimage.start = 0;
>>   
>>       info->load = kernel_zimage_load;
>>   
>> @@ -436,6 +470,15 @@ int __init kernel_probe(struct kernel_info *info,
>>       u64 kernel_addr, initrd_addr, dtb_addr, size;
>>       int rc;
>>   
>> +    /*
>> +     * We need to initialize start to 0. This field may be populated during
>> +     * kernel_xxx_probe() if the image has a fixed entry point (for e.g.
>> +     * uimage.ep).
>> +     * We will use this to determine if the image has a fixed entry point or
>> +     * the load address should be used as the start address.
>> +     */
>> +    info->entry = 0;
>> +
>>       /* domain is NULL only for the hardware domain */
>>       if ( domain == NULL )
>>       {
>> -- 
>> 2.17.1
>>
>>

Hello all,

On 13/01/2023 13:24, Ayan Kumar Halder wrote:
> Currently, kernel_uimage_probe() does not read the load/entry point address
> set in the uImge header. Thus, info->zimage.start is 0 (default value). 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.load" and set
> info->zimage.start. This will ensure that the binary is loaded at the
> correct address. Also, read "uimage.ep" and set info->entry (ie kernel entry
> address).
> 
> If user provides load address (ie "uimage.load") as 0x0, then the image is
> treated as position independent executable. Xen can load such an image at
> any address it considers appropriate. A position independent executable
> cannot have a fixed entry point address.
> 
> This behavior is applicable for both arm32 and arm64 platforms.
> 
> Earlier for arm32 and arm64 platforms, Xen was ignoring the load and entry
> point address set in the uImage header. With this commit, Xen will use them.
> This makes the behavior of Xen consistent with uboot for uimage headers.
> 
> Users who want to use Xen with statically partitioned domains, can provide
> non zero load address and entry address for the dom0/domU kernel. It is
> required that the load and entry address provided must be within the memory
> region allocated by Xen.
> 
> 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@amd.com>

When looking at this patch, I spotted one incorrect Xen behavior related to supporting uImage kernels.
And if we want to document that we support such kernels, we should take a look at it.

Xen supports gzip compressed kernels and it tries to decompress them before kernel_XXX_probe.
For zImage and Image, their respective headers are built into the kernel itself and then such image is compressed.
This results in a gzip header appearing right at the top of the image and the workflow works smoothly.

However, uImage header is something that is always added as a last step in the image preparation using mkimage utility
and always appears right at the top of the image. That is why uImage header has a field "ih_comp" used to inform about the compression type.
So the uImage gzip compressed image will have the uImage header before gzip header. Because Xen tries to decompress the image
before probing its header, this will not work for uImage.

~Michal

On Thu, 19 Jan 2023, Michal Orzel wrote:
> Hello all,
> 
> On 13/01/2023 13:24, Ayan Kumar Halder wrote:
> > Currently, kernel_uimage_probe() does not read the load/entry point address
> > set in the uImge header. Thus, info->zimage.start is 0 (default value). 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.load" and set
> > info->zimage.start. This will ensure that the binary is loaded at the
> > correct address. Also, read "uimage.ep" and set info->entry (ie kernel entry
> > address).
> > 
> > If user provides load address (ie "uimage.load") as 0x0, then the image is
> > treated as position independent executable. Xen can load such an image at
> > any address it considers appropriate. A position independent executable
> > cannot have a fixed entry point address.
> > 
> > This behavior is applicable for both arm32 and arm64 platforms.
> > 
> > Earlier for arm32 and arm64 platforms, Xen was ignoring the load and entry
> > point address set in the uImage header. With this commit, Xen will use them.
> > This makes the behavior of Xen consistent with uboot for uimage headers.
> > 
> > Users who want to use Xen with statically partitioned domains, can provide
> > non zero load address and entry address for the dom0/domU kernel. It is
> > required that the load and entry address provided must be within the memory
> > region allocated by Xen.
> > 
> > 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@amd.com>
> 
> When looking at this patch, I spotted one incorrect Xen behavior related to supporting uImage kernels.
> And if we want to document that we support such kernels, we should take a look at it.
> 
> Xen supports gzip compressed kernels and it tries to decompress them before kernel_XXX_probe.
> For zImage and Image, their respective headers are built into the kernel itself and then such image is compressed.
> This results in a gzip header appearing right at the top of the image and the workflow works smoothly.
> 
> However, uImage header is something that is always added as a last step in the image preparation using mkimage utility
> and always appears right at the top of the image. That is why uImage header has a field "ih_comp" used to inform about the compression type.
> So the uImage gzip compressed image will have the uImage header before gzip header. Because Xen tries to decompress the image
> before probing its header, this will not work for uImage.

It looks like to solve both this problem and also the other one about
zimage64.text_offset we would need to move the kernel_uimage_probe()
call earlier, before kernel_decompress() and before
kernel_zimage64_probe().

However, I can see that this patch is already complex as is, so I would
also be OK if any additional changes (e.g. moving kernel_uimage_probe()
earlier) were done in a separate patch on top.