Re: [XEN v1] xen/Arm: Probe the entry point address of an uImage correctly
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
Re: [XEN v1] xen/Arm: Probe the entry point address of an uImage correctly
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
--- Begin Message ---
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
--- End Message ---
Re: [XEN v1] xen/Arm: Probe the entry point address of an uImage correctly
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
Re: [XEN v1] xen/Arm: Probe the entry point address of an uImage correctly
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
---
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
Re: [XEN v1] xen/Arm: Probe the entry point address of an uImage correctly
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
> ---
>
> 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
Re: [XEN v1] xen/Arm: Probe the entry point address of an uImage correctly
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
Re: [XEN v1] xen/Arm: Probe the entry point address of an uImage correctly
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 > > > > > --- > > > > > > > > > > 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
Re: [XEN v1] xen/Arm: Probe the entry point address of an uImage correctly
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
---
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,
Re: [XEN v1] xen/Arm: Probe the entry point address of an uImage correctly
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 --- 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
Re: [XEN v1] xen/Arm: Probe the entry point address of an uImage correctly
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 --- 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
Re: [XEN v1] xen/Arm: Probe the entry point address of an uImage correctly
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
Re: [XEN v1] xen/Arm: Probe the entry point address of an uImage correctly
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 >> --- >> >> 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
Re: [XEN v1] xen/Arm: Probe the entry point address of an uImage correctly
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 --- 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,
Re: [XEN v1] xen/Arm: Probe the entry point address of an uImage correctly
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 --- 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
[XEN v1] xen/Arm: Probe the entry point address of an uImage correctly
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 --- 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
