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.

Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
---

I uncovered this issue while loading Zephyr as a dom0less domU with Xen on
R52 FVP. Zephyr builds with static device tree. Thus, the load address is
always fixed.

 xen/arch/arm/kernel.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
index 2556a45c38..e4e8c67669 100644
--- a/xen/arch/arm/kernel.c
+++ b/xen/arch/arm/kernel.c
@@ -222,6 +222,8 @@ 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);
+
     info->zimage.kernel_addr = addr + sizeof(uimage);
     info->zimage.len = len;
 
-- 
2.17.1

Hi,

Title extra NIT: I have seen it multiple time and so far refrain to say 
it. Please use 'arm' rather than 'Arm'. This is for consistency in the 
way we name the subsystem in the title.

On 08/12/2022 12:49, 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.

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. In fact, I am worry 
that...

> 
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> ---
> 
> I uncovered this issue while loading Zephyr as a dom0less domU with Xen on
> R52 FVP. Zephyr builds with static device tree. Thus, the load address is
> always fixed.
> 
>   xen/arch/arm/kernel.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
> index 2556a45c38..e4e8c67669 100644
> --- a/xen/arch/arm/kernel.c
> +++ b/xen/arch/arm/kernel.c
> @@ -222,6 +222,8 @@ 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);
... this will now ended up to break anyone that may have set an address 
but didn't care where it should be loaded.

I also understand your use case but now, we have contradictory 
approaches. I am not entirely sure how we can solve it. We may have to 
break those users (Cc some folks that may use it). But we should figure 
out what is the alternative for them.

If we decide to break those users, then this should be documented in the 
commit message and in docs/misc/arm/booting.txt (which interestingly 
didn't mention uImage).

Cheers,

-- 
Julien Grall

On 08.12.2022 14:51, Julien Grall wrote:
> On 08/12/2022 12:49, 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.
> 
> 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. In fact, I am worry 
> that...
> 
>> --- a/xen/arch/arm/kernel.c
>> +++ b/xen/arch/arm/kernel.c
>> @@ -222,6 +222,8 @@ 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);
> ... this will now ended up to break anyone that may have set an address 
> but didn't care where it should be loaded.
> 
> I also understand your use case but now, we have contradictory 
> approaches. I am not entirely sure how we can solve it. We may have to 
> break those users (Cc some folks that may use it). But we should figure 
> out what is the alternative for them.

I don't know anything about the uimage format, but is the ep field
required to be non-zero? If not, it being non-zero would retain
prior behavior.

Jan

Hi,

On 08/12/2022 14:51, Julien Grall wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> 
> 
> Hi,
> 
> Title extra NIT: I have seen it multiple time and so far refrain to say
> it. Please use 'arm' rather than 'Arm'. This is for consistency in the
> way we name the subsystem in the title.
> 
> On 08/12/2022 12:49, 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.
> 
> 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. In fact, I am worry
> that...
> 
>>
>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
>> ---
>>
>> I uncovered this issue while loading Zephyr as a dom0less domU with Xen on
>> R52 FVP. Zephyr builds with static device tree. Thus, the load address is
>> always fixed.
>>
>>   xen/arch/arm/kernel.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
>> index 2556a45c38..e4e8c67669 100644
>> --- a/xen/arch/arm/kernel.c
>> +++ b/xen/arch/arm/kernel.c
>> @@ -222,6 +222,8 @@ 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);
> ... this will now ended up to break anyone that may have set an address
> but didn't care where it should be loaded.
> 
> I also understand your use case but now, we have contradictory
> approaches. I am not entirely sure how we can solve it. We may have to
> break those users (Cc some folks that may use it). But we should figure
> out what is the alternative for them.
> 
> If we decide to break those users, then this should be documented in the
> commit message and in docs/misc/arm/booting.txt (which interestingly
> didn't mention uImage).
> 
So the first issue with Zephyr is that it does not support zImage protocol for arm32.
Volodymyr added support only for Image header for arm64 Zephyr.
I guess this is why Ayan, willing to boot it on Xen (arm32), decided to add u-boot header.

Now, there is also a question about supporting arm64 uImage kernels. In Xen kernel_zimage_place,
we do:
#ifdef CONFIG_ARM_64
    if ( info->type == DOMAIN_64BIT )
        return info->mem.bank[0].start + info->zimage.text_offset;
#endif

So if we modify the uImage behavior for arm32, we will break consistency with arm64
(we would take uImage entry point address into account for arm32 but not for arm64).
At the moment at least they are in sync.


> Cheers,
> 
> --
> Julien Grall

~Michal

Hi,

On 08/12/2022 15:24, Michal Orzel wrote:
> On 08/12/2022 14:51, Julien Grall wrote:
>> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>>
>>
>> Hi,
>>
>> Title extra NIT: I have seen it multiple time and so far refrain to say
>> it. Please use 'arm' rather than 'Arm'. This is for consistency in the
>> way we name the subsystem in the title.
>>
>> On 08/12/2022 12:49, 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.
>>
>> 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. In fact, I am worry
>> that...
>>
>>>
>>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
>>> ---
>>>
>>> I uncovered this issue while loading Zephyr as a dom0less domU with Xen on
>>> R52 FVP. Zephyr builds with static device tree. Thus, the load address is
>>> always fixed.
>>>
>>>    xen/arch/arm/kernel.c | 2 ++
>>>    1 file changed, 2 insertions(+)
>>>
>>> diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
>>> index 2556a45c38..e4e8c67669 100644
>>> --- a/xen/arch/arm/kernel.c
>>> +++ b/xen/arch/arm/kernel.c
>>> @@ -222,6 +222,8 @@ 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);
>> ... this will now ended up to break anyone that may have set an address
>> but didn't care where it should be loaded.
>>
>> I also understand your use case but now, we have contradictory
>> approaches. I am not entirely sure how we can solve it. We may have to
>> break those users (Cc some folks that may use it). But we should figure
>> out what is the alternative for them.
>>
>> If we decide to break those users, then this should be documented in the
>> commit message and in docs/misc/arm/booting.txt (which interestingly
>> didn't mention uImage).
>>
> So the first issue with Zephyr is that it does not support zImage protocol for arm32.
> Volodymyr added support only for Image header for arm64 Zephyr.
> I guess this is why Ayan, willing to boot it on Xen (arm32), decided to add u-boot header.

If that's the only reason, then I would rather prefer if we go with 
zImage for a few reasons:
  - The zImage protocol has at least some documentation (not perfect) of 
the expected state of the memory/registers when jumping to the image.
  - AFAICT libxl is not (yet) supporting uImage. So this means zephyr 
cannot be loaded on older Xen releases (not great).

Note this doesn't mean we should not fix Xen for uImage.

> Now, there is also a question about supporting arm64 uImage kernels. In Xen kernel_zimage_place,
> we do:
> #ifdef CONFIG_ARM_64
>      if ( info->type == DOMAIN_64BIT )
>          return info->mem.bank[0].start + info->zimage.text_offset;
> #endif
> 
> So if we modify the uImage behavior for arm32, we will break consistency with arm64
> (we would take uImage entry point address into account for arm32 but not for arm64).
> At the moment at least they are in sync.

That's a good point. It would be best if the behavior is consistent.

Cheers,

-- 
Julien Grall

On 08/12/2022 16:53, Julien Grall wrote:
> Hi,
Hi,
>
> On 08/12/2022 15:24, Michal Orzel wrote:
>> On 08/12/2022 14:51, Julien Grall wrote:
>>> Caution: This message originated from an External Source. Use proper 
>>> caution when opening attachments, clicking links, or responding.
>>>
>>>
>>> Hi,
>>>
>>> Title extra NIT: I have seen it multiple time and so far refrain to say
>>> it. Please use 'arm' rather than 'Arm'. This is for consistency in the
>>> way we name the subsystem in the title.
>>>
>>> On 08/12/2022 12:49, 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.
>>>
>>> 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. In fact, I am worry
>>> that...
>>>
>>>>
>>>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
>>>> ---
>>>>
>>>> I uncovered this issue while loading Zephyr as a dom0less domU with 
>>>> Xen on
>>>> R52 FVP. Zephyr builds with static device tree. Thus, the load 
>>>> address is
>>>> always fixed.
>>>>
>>>>    xen/arch/arm/kernel.c | 2 ++
>>>>    1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
>>>> index 2556a45c38..e4e8c67669 100644
>>>> --- a/xen/arch/arm/kernel.c
>>>> +++ b/xen/arch/arm/kernel.c
>>>> @@ -222,6 +222,8 @@ 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);
>>> ... this will now ended up to break anyone that may have set an address
>>> but didn't care where it should be loaded.
>>>
>>> I also understand your use case but now, we have contradictory
>>> approaches. I am not entirely sure how we can solve it. We may have to
>>> break those users (Cc some folks that may use it). But we should figure
>>> out what is the alternative for them.
>>>
>>> If we decide to break those users, then this should be documented in 
>>> the
>>> commit message and in docs/misc/arm/booting.txt (which interestingly
>>> didn't mention uImage).
>>>
>> So the first issue with Zephyr is that it does not support zImage 
>> protocol for arm32.
>> Volodymyr added support only for Image header for arm64 Zephyr.
>> I guess this is why Ayan, willing to boot it on Xen (arm32), decided 
>> to add u-boot header.
>
> If that's the only reason, then I would rather prefer if we go with 
> zImage for a few reasons:
>  - The zImage protocol has at least some documentation (not perfect) 
> of the expected state of the memory/registers when jumping to the image.
>  - AFAICT libxl is not (yet) supporting uImage. So this means zephyr 
> cannot be loaded on older Xen releases (not great).

I am exploring for a similar option as Volodymyr ie support zimage 
protocol for arm32.

But for that I need some public documentation that explains the zimage 
header format for arm32.

Refer xen/arch/arm/kernel.c

#define ZIMAGE32_MAGIC_OFFSET 0x24
#define ZIMAGE32_START_OFFSET 0x28
#define ZIMAGE32_END_OFFSET   0x2c
#define ZIMAGE32_HEADER_LEN   0x30

#define ZIMAGE32_MAGIC 0x016f2818

Is this documented anywhere ?

I had a look at 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/arm/booting.rst 
, but there is nothing that explains the header format.

>
> Note this doesn't mean we should not fix Xen for uImage.
>
>> Now, there is also a question about supporting arm64 uImage kernels. 
>> In Xen kernel_zimage_place,
>> we do:
>> #ifdef CONFIG_ARM_64
>>      if ( info->type == DOMAIN_64BIT )
>>          return info->mem.bank[0].start + info->zimage.text_offset;
>> #endif
>>
>> So if we modify the uImage behavior for arm32, we will break 
>> consistency with arm64
>> (we would take uImage entry point address into account for arm32 but 
>> not for arm64).
>> At the moment at least they are in sync.
>
> That's a good point. It would be best if the behavior is consistent.

Currently, kernel_zimage_place() is called for both uImage and zImage.

Will it be sane if we write a different function for uImage ?

Something like this ...

static paddr_t __init kernel_uimage_place(struct kernel_info *info)

{

     /* Read and return uImage header's load address */

     return be32_to_cpu(uimage.load);

}

This will be consistent across arm32 and arm64

- Ayan

>
> Cheers,
>

On 08/12/2022 19:42, Ayan Kumar Halder wrote:
> 
> On 08/12/2022 16:53, Julien Grall wrote:
>> Hi,
> Hi,
>>
>> On 08/12/2022 15:24, Michal Orzel wrote:
>>> On 08/12/2022 14:51, Julien Grall wrote:
>>>> Caution: This message originated from an External Source. Use proper 
>>>> caution when opening attachments, clicking links, or responding.
>>>>
>>>>
>>>> Hi,
>>>>
>>>> Title extra NIT: I have seen it multiple time and so far refrain to say
>>>> it. Please use 'arm' rather than 'Arm'. This is for consistency in the
>>>> way we name the subsystem in the title.
>>>>
>>>> On 08/12/2022 12:49, 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.
>>>>
>>>> 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. In fact, I am worry
>>>> that...
>>>>
>>>>>
>>>>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
>>>>> ---
>>>>>
>>>>> I uncovered this issue while loading Zephyr as a dom0less domU with 
>>>>> Xen on
>>>>> R52 FVP. Zephyr builds with static device tree. Thus, the load 
>>>>> address is
>>>>> always fixed.
>>>>>
>>>>>    xen/arch/arm/kernel.c | 2 ++
>>>>>    1 file changed, 2 insertions(+)
>>>>>
>>>>> diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
>>>>> index 2556a45c38..e4e8c67669 100644
>>>>> --- a/xen/arch/arm/kernel.c
>>>>> +++ b/xen/arch/arm/kernel.c
>>>>> @@ -222,6 +222,8 @@ 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);
>>>> ... this will now ended up to break anyone that may have set an address
>>>> but didn't care where it should be loaded.
>>>>
>>>> I also understand your use case but now, we have contradictory
>>>> approaches. I am not entirely sure how we can solve it. We may have to
>>>> break those users (Cc some folks that may use it). But we should figure
>>>> out what is the alternative for them.
>>>>
>>>> If we decide to break those users, then this should be documented in 
>>>> the
>>>> commit message and in docs/misc/arm/booting.txt (which interestingly
>>>> didn't mention uImage).
>>>>
>>> So the first issue with Zephyr is that it does not support zImage 
>>> protocol for arm32.
>>> Volodymyr added support only for Image header for arm64 Zephyr.
>>> I guess this is why Ayan, willing to boot it on Xen (arm32), decided 
>>> to add u-boot header.
>>
>> If that's the only reason, then I would rather prefer if we go with 
>> zImage for a few reasons:
>>  - The zImage protocol has at least some documentation (not perfect) 
>> of the expected state of the memory/registers when jumping to the image.
>>  - AFAICT libxl is not (yet) supporting uImage. So this means zephyr 
>> cannot be loaded on older Xen releases (not great).
> 
> I am exploring for a similar option as Volodymyr ie support zimage 
> protocol for arm32.
> 
> But for that I need some public documentation that explains the zimage 
> header format for arm32.
> 
> Refer xen/arch/arm/kernel.c
> 
> #define ZIMAGE32_MAGIC_OFFSET 0x24
> #define ZIMAGE32_START_OFFSET 0x28
> #define ZIMAGE32_END_OFFSET   0x2c
> #define ZIMAGE32_HEADER_LEN   0x30
> 
> #define ZIMAGE32_MAGIC 0x016f2818
> 
> Is this documented anywhere ?
> 
> I had a look at 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/arm/booting.rst 
> , but there is nothing that explains the header format.
> 
>>
>> Note this doesn't mean we should not fix Xen for uImage.
>>
>>> Now, there is also a question about supporting arm64 uImage kernels. 
>>> In Xen kernel_zimage_place,
>>> we do:
>>> #ifdef CONFIG_ARM_64
>>>      if ( info->type == DOMAIN_64BIT )
>>>          return info->mem.bank[0].start + info->zimage.text_offset;
>>> #endif
>>>
>>> So if we modify the uImage behavior for arm32, we will break 
>>> consistency with arm64
>>> (we would take uImage entry point address into account for arm32 but 
>>> not for arm64).
>>> At the moment at least they are in sync.
>>
>> That's a good point. It would be best if the behavior is consistent.
> 
> Currently, kernel_zimage_place() is called for both uImage and zImage.
> 
> Will it be sane if we write a different function for uImage ?
> 
> Something like this ...
> 
> static paddr_t __init kernel_uimage_place(struct kernel_info *info)
> 
> {
> 
>      /* Read and return uImage header's load address */
> 
>      return be32_to_cpu(uimage.load);
> 
> }
> 
> This will be consistent across arm32 and arm64
> 
All of these does not make a lot of sense because we are allocating memory for a domain
before probing the kernel image. This means that the load/entry address for a kernel
must be within the bank allocated for a domain. So the kernel already needs to know
that it is running e.g. as a Xen domU, and add corresponding u-boot header to load
us at e.g. GUEST_RAM0_BASE. Otherwise Xen will fail trying to copy the kernel into domain's
memory. Whereas for domU it is easy to guess the memory bank, for dom0 it is not.

zImage and Image are image protocols, uImage is not. It is just a legacy u-boot header (no requirements
\wrt booting,memory,registers, etc.). It can be added on top of anything (even vmlinux or a text file).
So I guess this is why Xen states that it supports zImage/Image and does not mention uImage.
A header is one thing, the boot requirements are another. Supporting uImage is ok but if we specify
that it must be a u-boot header added on top of zImage/Image.

> - Ayan
> 
>>
>> Cheers,
>>
~Michal

Hi Michal,

On 09/12/2022 08:53, Michal Orzel wrote:
> On 08/12/2022 19:42, Ayan Kumar Halder wrote:
>> On 08/12/2022 16:53, Julien Grall wrote:
>>> Hi,
>> Hi,
>>> On 08/12/2022 15:24, Michal Orzel wrote:
>>>> On 08/12/2022 14:51, Julien Grall wrote:
>>>>> Caution: This message originated from an External Source. Use proper
>>>>> caution when opening attachments, clicking links, or responding.
>>>>>
>>>>>
>>>>> Hi,
>>>>>
>>>>> Title extra NIT: I have seen it multiple time and so far refrain to say
>>>>> it. Please use 'arm' rather than 'Arm'. This is for consistency in the
>>>>> way we name the subsystem in the title.
>>>>>
>>>>> On 08/12/2022 12:49, 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.
>>>>> 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. In fact, I am worry
>>>>> that...
>>>>>
>>>>>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
>>>>>> ---
>>>>>>
>>>>>> I uncovered this issue while loading Zephyr as a dom0less domU with
>>>>>> Xen on
>>>>>> R52 FVP. Zephyr builds with static device tree. Thus, the load
>>>>>> address is
>>>>>> always fixed.
>>>>>>
>>>>>>     xen/arch/arm/kernel.c | 2 ++
>>>>>>     1 file changed, 2 insertions(+)
>>>>>>
>>>>>> diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
>>>>>> index 2556a45c38..e4e8c67669 100644
>>>>>> --- a/xen/arch/arm/kernel.c
>>>>>> +++ b/xen/arch/arm/kernel.c
>>>>>> @@ -222,6 +222,8 @@ 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);
>>>>> ... this will now ended up to break anyone that may have set an address
>>>>> but didn't care where it should be loaded.
>>>>>
>>>>> I also understand your use case but now, we have contradictory
>>>>> approaches. I am not entirely sure how we can solve it. We may have to
>>>>> break those users (Cc some folks that may use it). But we should figure
>>>>> out what is the alternative for them.
>>>>>
>>>>> If we decide to break those users, then this should be documented in
>>>>> the
>>>>> commit message and in docs/misc/arm/booting.txt (which interestingly
>>>>> didn't mention uImage).
>>>>>
>>>> So the first issue with Zephyr is that it does not support zImage
>>>> protocol for arm32.
>>>> Volodymyr added support only for Image header for arm64 Zephyr.
>>>> I guess this is why Ayan, willing to boot it on Xen (arm32), decided
>>>> to add u-boot header.
>>> If that's the only reason, then I would rather prefer if we go with
>>> zImage for a few reasons:
>>>   - The zImage protocol has at least some documentation (not perfect)
>>> of the expected state of the memory/registers when jumping to the image.
>>>   - AFAICT libxl is not (yet) supporting uImage. So this means zephyr
>>> cannot be loaded on older Xen releases (not great).
>> I am exploring for a similar option as Volodymyr ie support zimage
>> protocol for arm32.
>>
>> But for that I need some public documentation that explains the zimage
>> header format for arm32.
>>
>> Refer xen/arch/arm/kernel.c
>>
>> #define ZIMAGE32_MAGIC_OFFSET 0x24
>> #define ZIMAGE32_START_OFFSET 0x28
>> #define ZIMAGE32_END_OFFSET   0x2c
>> #define ZIMAGE32_HEADER_LEN   0x30
>>
>> #define ZIMAGE32_MAGIC 0x016f2818
>>
>> Is this documented anywhere ?
>>
>> I had a look at
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/arm/booting.rst
>> , but there is nothing that explains the header format.
>>
>>> Note this doesn't mean we should not fix Xen for uImage.
>>>
>>>> Now, there is also a question about supporting arm64 uImage kernels.
>>>> In Xen kernel_zimage_place,
>>>> we do:
>>>> #ifdef CONFIG_ARM_64
>>>>       if ( info->type == DOMAIN_64BIT )
>>>>           return info->mem.bank[0].start + info->zimage.text_offset;
>>>> #endif
>>>>
>>>> So if we modify the uImage behavior for arm32, we will break
>>>> consistency with arm64
>>>> (we would take uImage entry point address into account for arm32 but
>>>> not for arm64).
>>>> At the moment at least they are in sync.
>>> That's a good point. It would be best if the behavior is consistent.
>> Currently, kernel_zimage_place() is called for both uImage and zImage.
>>
>> Will it be sane if we write a different function for uImage ?
>>
>> Something like this ...
>>
>> static paddr_t __init kernel_uimage_place(struct kernel_info *info)
>>
>> {
>>
>>       /* Read and return uImage header's load address */
>>
>>       return be32_to_cpu(uimage.load);
>>
>> }
>>
>> This will be consistent across arm32 and arm64
>>
> All of these does not make a lot of sense because we are allocating memory for a domain
> before probing the kernel image. This means that the load/entry address for a kernel
> must be within the bank allocated for a domain. So the kernel already needs to know
> that it is running e.g. as a Xen domU, and add corresponding u-boot header to load
> us at e.g. GUEST_RAM0_BASE. Otherwise Xen will fail trying to copy the kernel into domain's
> memory. Whereas for domU it is easy to guess the memory bank, for dom0 it is not.

I am trying to see if I understand you correctly.

Currently, the sequence is like this. Refer construct_domU()

1. kernel_probe()

2. allocate_memory()

So, we allocate memory after we probe the kernel image.

In kernel_probe(), we do read the headers in kernel_zimage32_probe() and 
set "info->zimage.kernel_addr" accordingly.

Later in allocate_memory(), we determine the memory for the domain.

IIUC, it is assumed that "info->zimage.kernel_addr" is within the memory 
range for the domain. Else, Xen may crash.

If this is correct understanding, then we should also be able to probe 
uImage header and set the kernel_addr (similar to kernel_zimage32_probe()).

>
> zImage and Image are image protocols, uImage is not. It is just a legacy u-boot header (no requirements
> \wrt booting,memory,registers, etc.). It can be added on top of anything (even vmlinux or a text file).
> So I guess this is why Xen states that it supports zImage/Image and does not mention uImage.
> A header is one thing, the boot requirements are another. Supporting uImage is ok but if we specify
> that it must be a u-boot header added on top of zImage/Image.

Let me first confine to Arm32 only.

zephyr.bin has to be loaded at a fixed address with which it has been built.

So, we could either use zImage header (where 'start_address' can be used 
to specify the load address).

Or uImage (where -a  is used to specify the load address).

Adding uImage header on top of zImage does not make sense.

Now for Arm64,  we do need to parse the zImage header

#ifdef CONFIG_ARM_64
     if ( info->type == DOMAIN_64BIT )
     {
         return info->mem.bank[0].start + info->zimage.text_offset;
     }
#endif

Again, adding uImage header on top of zImage header does not make sense 
as well.

Also, I believe zImage boot requirements are specific for linux kernel.

zephyr or any other RTOS may not follow the same boot requirements.

- Ayan


>
>> - Ayan
>>
>>> Cheers,
>>>
> ~Michal
>

Hi Ayan,

On 09/12/2022 19:10, Ayan Kumar Halder wrote:
>> zImage and Image are image protocols, uImage is not. It is just a 
>> legacy u-boot header (no requirements
>> \wrt booting,memory,registers, etc.). It can be added on top of 
>> anything (even vmlinux or a text file).
>> So I guess this is why Xen states that it supports zImage/Image and 
>> does not mention uImage.
>> A header is one thing, the boot requirements are another. Supporting 
>> uImage is ok but if we specify
>> that it must be a u-boot header added on top of zImage/Image.
> 
> Let me first confine to Arm32 only.
> 
> zephyr.bin has to be loaded at a fixed address with which it has been 
> built.
> 
> So, we could either use zImage header (where 'start_address' can be used 
> to specify the load address).
> 
> Or uImage (where -a  is used to specify the load address).
> 
> Adding uImage header on top of zImage does not make sense.
> 
> Now for Arm64,  we do need to parse the zImage header
> 
> #ifdef CONFIG_ARM_64
>      if ( info->type == DOMAIN_64BIT )
>      {
>          return info->mem.bank[0].start + info->zimage.text_offset;
>      }
> #endif
> 
> Again, adding uImage header on top of zImage header does not make sense 
> as well.
> 
> Also, I believe zImage boot requirements are specific for linux kernel.

Correct. But then this is what Xen tries to adhere to when preparing the 
guest. So...

> zephyr or any other RTOS may not follow the same boot requirements.

... if Zephyr or any other RTOS have different requirements, then we may 
need to modify Xen.

Can you describe the expectation of Zephyr for the following components:
   - State of the memory/cache:
	* Should the image be cleaned to PoC or more?
         * What about the area not part of the binary (e.g. BSS)?
         * What about the rest of the memory
   - State of the co-processor registers:
         * Can we call the kernel with I-cache enabled?
         * ...
   - State of the general purpose registers:
         * For instance, Linux expects a pointer to the device-tree in r0

Cheers,

-- 
Julien Grall

On 09/12/2022 19:19, Julien Grall wrote:
> Hi Ayan,

Hi Julien,

I checked with the Zephyr mantainers. Their response is provided [1].

>
> On 09/12/2022 19:10, Ayan Kumar Halder wrote:
>>> zImage and Image are image protocols, uImage is not. It is just a 
>>> legacy u-boot header (no requirements
>>> \wrt booting,memory,registers, etc.). It can be added on top of 
>>> anything (even vmlinux or a text file).
>>> So I guess this is why Xen states that it supports zImage/Image and 
>>> does not mention uImage.
>>> A header is one thing, the boot requirements are another. Supporting 
>>> uImage is ok but if we specify
>>> that it must be a u-boot header added on top of zImage/Image.
>>
>> Let me first confine to Arm32 only.
>>
>> zephyr.bin has to be loaded at a fixed address with which it has been 
>> built.
>>
>> So, we could either use zImage header (where 'start_address' can be 
>> used to specify the load address).
>>
>> Or uImage (where -a  is used to specify the load address).
>>
>> Adding uImage header on top of zImage does not make sense.
>>
>> Now for Arm64,  we do need to parse the zImage header
>>
>> #ifdef CONFIG_ARM_64
>>      if ( info->type == DOMAIN_64BIT )
>>      {
>>          return info->mem.bank[0].start + info->zimage.text_offset;
>>      }
>> #endif
>>
>> Again, adding uImage header on top of zImage header does not make 
>> sense as well.
>>
>> Also, I believe zImage boot requirements are specific for linux kernel.
>
> Correct. But then this is what Xen tries to adhere to when preparing 
> the guest. So...
>
>> zephyr or any other RTOS may not follow the same boot requirements.
>
> ... if Zephyr or any other RTOS have different requirements, then we 
> may need to modify Xen.
>
> Can you describe the expectation of Zephyr for the following components:
>   - State of the memory/cache:
>     * Should the image be cleaned to PoC or more?
>         * What about the area not part of the binary (e.g. BSS)?
>         * What about the rest of the memory
Zephyr is expected to run as a baremetal binary. When loading from Xen 
or uboot, the interrupts should be disabled before jumping to Zephyr.

I/D caches need to be disabled as well.

The image should be cleaned to PoC. The BSS is cleared as part of the 
Zephyr boot process with z_bss_zero() and data is copied with 
z_data_copy(), see [2] for more details.

>   - State of the co-processor registers:
>         * Can we call the kernel with I-cache enabled?
I cache needs to be disabled before calling kernel.
>         * ...
>   - State of the general purpose registers:
>         * For instance, Linux expects a pointer to the device-tree in r0

Zephyr does not make any assumption about the state of the GPR at boot. 
Also, Zephyr is built with a device tree.

- Ayan

>
> Cheers,
>
[1] https://lists.zephyrproject.org/g/devel/message/8805

[2] 
https://github.com/zephyrproject-rtos/zephyr/blob/main/arch/arm64/core/prep_c.c#L54
On 12/12/2022 13:37, Ayan Kumar Halder wrote:
> Hi Zephyr Arm Team,

Hello,

> A) Zephyr is expected to run as a baremetal binary. When loading
> from Xen or uboot, the interrupts should be disabled.

Correct. We disable the interrupts at boot for good measure before but
in theory you want the interrupts to be disabled before jumping to Zephyr.

> There is nothing mentioned about the state of caches, but I assume 
> I/D caches will have to be disabled. Otherwise, there is a chance of 
> reading stale instructions from I cache.

Correct. When zephyr is chain-loaded by u-boot we usually disable I/D
caches from u-boot itself before jumping.

> The state of the system should be similar to how it is out of reset 
> state (SVC or EL1).

Yes, the only caveat is that in Zephyr you have to set CONFIG_ARMV8_A_NS
whether you are booting from EL3 or EL1.

> When Zephyr is loaded in memory, I am expecting the image to be 
> cleaned to PoC. However, I am not very sure on this.

The BSS is cleared as part of the boot process with z_bss_zero() and
data is copied with z_data_copy(), see [1] for more details.

> A) I assume I cache needs to be disabled.

Yup.

> A) The general purpose registers can be in an in-determinate state.

We do not make any assumption about the state of the GPR at boot. If
there is an hidden dependency, that is a bug to fix.

> A) Zephyr is built with a device tree. Thus, it does not expect 
> pre-determined values in r0, r1, etc.

Correct.

Ciao!

[1]
https://github.com/zephyrproject-rtos/zephyr/blob/main/arch/arm64/core/prep_c.c#L54

-- 
Carlo Caione

On 12/12/2022 19:04, Ayan Kumar Halder wrote:
> 
> On 09/12/2022 19:19, Julien Grall wrote:
>> Hi Ayan,
> 
> Hi Julien,

Hi,

> I checked with the Zephyr mantainers. Their response is provided [1].

Thanks for checking.

> 
>>
>> On 09/12/2022 19:10, Ayan Kumar Halder wrote:
>>>> zImage and Image are image protocols, uImage is not. It is just a 
>>>> legacy u-boot header (no requirements
>>>> \wrt booting,memory,registers, etc.). It can be added on top of 
>>>> anything (even vmlinux or a text file).
>>>> So I guess this is why Xen states that it supports zImage/Image and 
>>>> does not mention uImage.
>>>> A header is one thing, the boot requirements are another. Supporting 
>>>> uImage is ok but if we specify
>>>> that it must be a u-boot header added on top of zImage/Image.
>>>
>>> Let me first confine to Arm32 only.
>>>
>>> zephyr.bin has to be loaded at a fixed address with which it has been 
>>> built.
>>>
>>> So, we could either use zImage header (where 'start_address' can be 
>>> used to specify the load address).
>>>
>>> Or uImage (where -a  is used to specify the load address).
>>>
>>> Adding uImage header on top of zImage does not make sense.
>>>
>>> Now for Arm64,  we do need to parse the zImage header
>>>
>>> #ifdef CONFIG_ARM_64
>>>      if ( info->type == DOMAIN_64BIT )
>>>      {
>>>          return info->mem.bank[0].start + info->zimage.text_offset;
>>>      }
>>> #endif
>>>
>>> Again, adding uImage header on top of zImage header does not make 
>>> sense as well.
>>>
>>> Also, I believe zImage boot requirements are specific for linux kernel.
>>
>> Correct. But then this is what Xen tries to adhere to when preparing 
>> the guest. So...
>>
>>> zephyr or any other RTOS may not follow the same boot requirements.
>>
>> ... if Zephyr or any other RTOS have different requirements, then we 
>> may need to modify Xen.
>>
>> Can you describe the expectation of Zephyr for the following components:
>>   - State of the memory/cache:
>>     * Should the image be cleaned to PoC or more?
>>         * What about the area not part of the binary (e.g. BSS)?
>>         * What about the rest of the memory
> Zephyr is expected to run as a baremetal binary. When loading from Xen 
> or uboot, the interrupts should be disabled before jumping to Zephyr.
> 
> I/D caches need to be disabled as well.

For both 32-bit and 64-bit Linux, the instruction can may be on or off.

At the moment, Xen is choosing to disable it. But if that's a 
requirement for Zephyr, then I think we should document it.

That said, from the answer on the other thread, it was not clear whether 
this is a strong requirement or just because U-boot is doing it.

> 
> The image should be cleaned to PoC. The BSS is cleared as part of the 
> Zephyr boot process with z_bss_zero() and data is copied with 
> z_data_copy(), see [2] for more details.
> 
>>   - State of the co-processor registers:
>>         * Can we call the kernel with I-cache enabled?
> I cache needs to be disabled before calling kernel.

See above for the I-cache. However, this question was not only about the 
I-cache. It was just an example to what I am looking for in this category.

It might be easier if you read linux/Documentation/arm/booting.rst and 
let me know whether there is any expectation that don't match.

>>         * ...
>>   - State of the general purpose registers:
>>         * For instance, Linux expects a pointer to the device-tree in r0
> 
> Zephyr does not make any assumption about the state of the GPR at boot. 
> Also, Zephyr is built with a device tree.

Cheers,

-- 
Julien Grall

On 08/12/2022 18:42, Ayan Kumar Halder wrote:
> On 08/12/2022 16:53, Julien Grall wrote:
>> On 08/12/2022 15:24, Michal Orzel wrote:
>>> So the first issue with Zephyr is that it does not support zImage
>>> protocol for arm32.
>>> Volodymyr added support only for Image header for arm64 Zephyr.
>>> I guess this is why Ayan, willing to boot it on Xen (arm32), decided
>>> to add u-boot header.
>>
>> If that's the only reason, then I would rather prefer if we go with
>> zImage for a few reasons:
>>  - The zImage protocol has at least some documentation (not perfect)
>> of the expected state of the memory/registers when jumping to the image.
>>  - AFAICT libxl is not (yet) supporting uImage. So this means zephyr
>> cannot be loaded on older Xen releases (not great).
>
> I am exploring for a similar option as Volodymyr ie support zimage
> protocol for arm32.
>
> But for that I need some public documentation that explains the zimage
> header format for arm32.
>
> Refer xen/arch/arm/kernel.c
>
> #define ZIMAGE32_MAGIC_OFFSET 0x24
> #define ZIMAGE32_START_OFFSET 0x28
> #define ZIMAGE32_END_OFFSET   0x2c
> #define ZIMAGE32_HEADER_LEN   0x30
>
> #define ZIMAGE32_MAGIC 0x016f2818
>
> Is this documented anywhere ?

zImage (32) is entirely undocumented.  What exists is from reverse
engineering.  I found this while doing some XTF work, and went right
back to the source - the Linux maintainers.

There are things which Xen does wrong with zImage handling.  But I
haven't had time to transcribe my notes into something more coherent and
submit fixes.

~Andrew

On Thu, 8 Dec 2022, Ayan Kumar Halder wrote:
> On 08/12/2022 16:53, Julien Grall wrote:
> > Hi,
> Hi,
> > 
> > On 08/12/2022 15:24, Michal Orzel wrote:
> > > On 08/12/2022 14:51, Julien Grall wrote:
> > > > Caution: This message originated from an External Source. Use proper
> > > > caution when opening attachments, clicking links, or responding.
> > > > 
> > > > 
> > > > Hi,
> > > > 
> > > > Title extra NIT: I have seen it multiple time and so far refrain to say
> > > > it. Please use 'arm' rather than 'Arm'. This is for consistency in the
> > > > way we name the subsystem in the title.
> > > > 
> > > > On 08/12/2022 12:49, 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.
> > > > 
> > > > 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. In fact, I am worry
> > > > that...
> > > > 
> > > > > 
> > > > > Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> > > > > ---
> > > > > 
> > > > > I uncovered this issue while loading Zephyr as a dom0less domU with
> > > > > Xen on
> > > > > R52 FVP. Zephyr builds with static device tree. Thus, the load address
> > > > > is
> > > > > always fixed.
> > > > > 
> > > > >    xen/arch/arm/kernel.c | 2 ++
> > > > >    1 file changed, 2 insertions(+)
> > > > > 
> > > > > diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
> > > > > index 2556a45c38..e4e8c67669 100644
> > > > > --- a/xen/arch/arm/kernel.c
> > > > > +++ b/xen/arch/arm/kernel.c
> > > > > @@ -222,6 +222,8 @@ 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);
> > > > ... this will now ended up to break anyone that may have set an address
> > > > but didn't care where it should be loaded.
> > > > 
> > > > I also understand your use case but now, we have contradictory
> > > > approaches. I am not entirely sure how we can solve it. We may have to
> > > > break those users (Cc some folks that may use it). But we should figure
> > > > out what is the alternative for them.
> > > > 
> > > > If we decide to break those users, then this should be documented in the
> > > > commit message and in docs/misc/arm/booting.txt (which interestingly
> > > > didn't mention uImage).
> > > > 
> > > So the first issue with Zephyr is that it does not support zImage protocol
> > > for arm32.
> > > Volodymyr added support only for Image header for arm64 Zephyr.
> > > I guess this is why Ayan, willing to boot it on Xen (arm32), decided to
> > > add u-boot header.
> > 
> > If that's the only reason, then I would rather prefer if we go with zImage
> > for a few reasons:
> >  - The zImage protocol has at least some documentation (not perfect) of the
> > expected state of the memory/registers when jumping to the image.
> >  - AFAICT libxl is not (yet) supporting uImage. So this means zephyr cannot
> > be loaded on older Xen releases (not great).
> 
> I am exploring for a similar option as Volodymyr ie support zimage protocol
> for arm32.
> 
> But for that I need some public documentation that explains the zimage header
> format for arm32.
> 
> Refer xen/arch/arm/kernel.c
> 
> #define ZIMAGE32_MAGIC_OFFSET 0x24
> #define ZIMAGE32_START_OFFSET 0x28
> #define ZIMAGE32_END_OFFSET   0x2c
> #define ZIMAGE32_HEADER_LEN   0x30
> 
> #define ZIMAGE32_MAGIC 0x016f2818
> 
> Is this documented anywhere ?
> 
> I had a look at
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/arm/booting.rst
> , but there is nothing that explains the header format.

I think this is the closest to documentation of it:

http://www.simtec.co.uk/products/SWLINUX/files/booting_article.html

On 08/12/2022 15:24, Michal Orzel wrote:
> Hi,
Hi,
>
> On 08/12/2022 14:51, Julien Grall wrote:
>> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>>
>>
>> Hi,
>>
>> Title extra NIT: I have seen it multiple time and so far refrain to say
>> it. Please use 'arm' rather than 'Arm'. This is for consistency in the
>> way we name the subsystem in the title.
>>
>> On 08/12/2022 12:49, 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.
>> 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. In fact, I am worry
>> that...
>>
>>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
>>> ---
>>>
>>> I uncovered this issue while loading Zephyr as a dom0less domU with Xen on
>>> R52 FVP. Zephyr builds with static device tree. Thus, the load address is
>>> always fixed.
>>>
>>>    xen/arch/arm/kernel.c | 2 ++
>>>    1 file changed, 2 insertions(+)
>>>
>>> diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
>>> index 2556a45c38..e4e8c67669 100644
>>> --- a/xen/arch/arm/kernel.c
>>> +++ b/xen/arch/arm/kernel.c
>>> @@ -222,6 +222,8 @@ 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);
>> ... this will now ended up to break anyone that may have set an address
>> but didn't care where it should be loaded.
>>
>> I also understand your use case but now, we have contradictory
>> approaches. I am not entirely sure how we can solve it. We may have to
>> break those users (Cc some folks that may use it). But we should figure
>> out what is the alternative for them.
>>
>> If we decide to break those users, then this should be documented in the
>> commit message and in docs/misc/arm/booting.txt (which interestingly
>> didn't mention uImage).
>>
> So the first issue with Zephyr is that it does not support zImage protocol for arm32.
> Volodymyr added support only for Image header for arm64 Zephyr.
> I guess this is why Ayan, willing to boot it on Xen (arm32), decided to add u-boot header.
>
> Now, there is also a question about supporting arm64 uImage kernels. In Xen kernel_zimage_place,
> we do:
> #ifdef CONFIG_ARM_64
>      if ( info->type == DOMAIN_64BIT )
>          return info->mem.bank[0].start + info->zimage.text_offset;
> #endif
>
> So if we modify the uImage behavior for arm32, we will break consistency with arm64
> (we would take uImage entry point address into account for arm32 but not for arm64).
> At the moment at least they are in sync.

IIUC, at present arm64 and arm32 are not in sync.

For Arm32, we do not consider "info->zimage.text_offset".

- Ayan

>
>
>> Cheers,
>>
>> --
>> Julien Grall
> ~Michal

On 08/12/2022 13:51, Julien Grall wrote:
> Hi,
Hi Julien,
>
> Title extra NIT: I have seen it multiple time and so far refrain to 
> say it. Please use 'arm' rather than 'Arm'. This is for consistency in 
> the way we name the subsystem in the title.
I will take care of it henceforth.
>
> On 08/12/2022 12:49, 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.
>
> 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. In fact, I am worry 
> that...
>
>>
>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
>> ---
>>
>> I uncovered this issue while loading Zephyr as a dom0less domU with 
>> Xen on
>> R52 FVP. Zephyr builds with static device tree. Thus, the load 
>> address is
>> always fixed.
>>
>>   xen/arch/arm/kernel.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
>> index 2556a45c38..e4e8c67669 100644
>> --- a/xen/arch/arm/kernel.c
>> +++ b/xen/arch/arm/kernel.c
>> @@ -222,6 +222,8 @@ 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);
> ... this will now ended up to break anyone that may have set an 
> address but didn't care where it should be loaded.

Can we use a config option (STATIC_MEMORY or any option you may suggest) 
to distinguish between the two ?

Or using some magic number (eg 0xdeadbeef) for uimage.ep which will 
denote position independent ? This needs to be documented in 
docs/misc/arm/booting.txt.

Or ...

>
> I also understand your use case but now, we have contradictory 
> approaches. I am not entirely sure how we can solve it. We may have to 
> break those users (Cc some folks that may use it). But we should 
> figure out what is the alternative for them.
I am open to any alternative suggestion.
>
> If we decide to break those users, then this should be documented in 
> the commit message and in docs/misc/arm/booting.txt (which 
> interestingly didn't mention uImage).

I agree.

- Ayan

>
> Cheers,
>