| Message ID | 20221215123826.24145-1-ayan.kumar.halder@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [XEN,v2] xen/arm: Probe the entry point address of an uImage correctly | expand |
Hi, On 15/12/2022 12:38, Ayan Kumar Halder wrote: > Currently, kernel_uimage_probe() does not set info->zimage.start. As a > result, it contains the default value (ie 0). This causes, > kernel_zimage_place() to treat the binary (contained within uImage) as > position independent executable. Thus, it loads it at an incorrect address. > > The correct approach would be to read "uimage.ep" and set > info->zimage.start. This will ensure that the binary is loaded at the > correct address. Also, it checks that the load address and entry address > are the same. The reason being we currently support non compressed images > for uImage header. And as seen in uboot sources(image_decomp(), case > IH_COMP_NONE), load address and entry address can be the same. > > This behavior is applicable for both arm and arm64 platforms. > > Earlier for arm32 and arm64 platforms, Xen was ignoring the entry point > address set in the uImage header. With this commit, Xen will use the > kernel entry point address as specified in the header. This makes the > behavior of Xen consistent with uboot for uimage headers. At the cost of breaking existing users. I think this want to be spelt out clearly. > > A deviation from uboot behaviour is that we consider load address == 0x0, > to denote that the image supports position independent execution. This > is to make the behavior consistent across uImage and zImage. You want to explain why users want to use "0x0". Or maybe the other way around, that only a very limited set of user (e.g. in static environment) should provide a fixed address. > > Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com> > --- > > Changes from v1:- > 1. Added a check to ensure load address and entry address are the same. > 2. Considered load address == 0x0 as position independent execution. > 3. Ensured that the uImage header interpretation is consistent across > arm32 and arm64. > > Some unanswered queries from v1 :- > > Q1. "In non-statically allocated setup, a user doesn't know where the memory > for dom0/domU will be allocated.So I think this was correct to ignore the > address." > A. I am not sure the best way to handle this. Can we make the uImage probe > dependent on STATIC_MEMORY ? Currently, I have mentioned that user should > use load address = 0x0 if they want Xen to ignore it. See above. > > Q2. Zephyr or any other RTOS have different requirements, then we may need > to modify Xen and document it. > A. I will take it in a separate email/patch where we can list the > requirements for booting Zephyr. > > docs/misc/arm/booting.txt | 11 ++++++++++- > xen/arch/arm/include/asm/kernel.h | 2 +- > xen/arch/arm/kernel.c | 26 +++++++++++++++++++++++++- > 3 files changed, 36 insertions(+), 3 deletions(-) > > diff --git a/docs/misc/arm/booting.txt b/docs/misc/arm/booting.txt > index 3e0c03e065..872262686a 100644 > --- a/docs/misc/arm/booting.txt > +++ b/docs/misc/arm/booting.txt > @@ -4,7 +4,13 @@ Booting Xen > Xen follows the zImage protocol defined for 32-bit ARM Linux[1] and the > Image protocol defined for ARM64 Linux[2]. > > -In both cases the recommendation to boot in HYP/EL2 mode is a strict > +Xen also supports the legacy image protocol[3] for 32-bit ARM and ARM64. > +For now, it supports images where load address is same as entry address. > +A deviation from uboot is that, Xen treats "load address == 0x0" as > +position independent execution. Thus, Xen will load such an image at an > +address it considers appropriate. Hmmmm... The section above is related to Xen itself and not the guest Image. So I think this is misplaced. > + > +In all cases the recommendation to boot in HYP/EL2 mode is a strict > requirement for Xen. > > The exceptions to this on 32-bit ARM are as follows: > @@ -39,3 +45,6 @@ Latest version: http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/t > > [2] linux/Documentation/arm64/booting.rst > Latest version: http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/arm64/booting.rst > + > +[3] legacy format header > +Latest version: https://source.denx.de/u-boot/u-boot/-/blob/master/include/image.h#L315 > diff --git a/xen/arch/arm/include/asm/kernel.h b/xen/arch/arm/include/asm/kernel.h > index 5bb30c3f2f..ee69a47052 100644 > --- a/xen/arch/arm/include/asm/kernel.h > +++ b/xen/arch/arm/include/asm/kernel.h > @@ -72,7 +72,7 @@ struct kernel_info { > #ifdef CONFIG_ARM_64 > paddr_t text_offset; /* 64-bit Image only */ > #endif > - paddr_t start; /* 32-bit zImage only */ > + paddr_t start; I think the comment should be replaced by "Must be 0 for 64-bit Image". > } zimage; > }; > }; > diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c > index 23b840ea9e..81ac945f5b 100644 > --- a/xen/arch/arm/kernel.c > +++ b/xen/arch/arm/kernel.c > @@ -127,7 +127,7 @@ static paddr_t __init kernel_zimage_place(struct kernel_info *info) > paddr_t load_addr; > > #ifdef CONFIG_ARM_64 > - if ( info->type == DOMAIN_64BIT ) > + if ( (info->type == DOMAIN_64BIT) && (info->zimage.start == 0) ) > return info->mem.bank[0].start + info->zimage.text_offset; > #endif > > @@ -223,6 +223,30 @@ static int __init kernel_uimage_probe(struct kernel_info *info, > if ( len > size - sizeof(uimage) ) > return -EINVAL; > > + info->zimage.start = be32_to_cpu(uimage.ep); > + > + /* > + * Currently, we support uImage headers for uncompressed images only. > + * Thus, it is valid for the load address and start address to be the > + * same. This is consistent with the uboot behavior (Refer > + * "case IH_COMP_NONE" in image_decomp()). > + */ > + if ( info->zimage.start != be32_to_cpu(uimage.load) ) > + { > + panic("Unable to support mismatching load address and entry address\n"); > + return -EINVAL; > + } > + > + /* > + * While uboot considers 0x0 to be a valid load/start address, for Xen > + * to mantain parity with zimage, we consider 0x0 to denote position > + * independent image. That means Xen is free to load such an image at > + * any valid address. > + * Thus, we will print an appropriate warning. > + */ > + if ( info->zimage.start == 0 ) > + printk(XENLOG_WARNING "No valid load address provided\n"); This is likely the way user will want to use it in a non-static environment. So why is this a warning? Cheers,
On 16/12/2022 09:34, Julien Grall wrote: > Hi, Hi Julien, > > On 15/12/2022 12:38, Ayan Kumar Halder wrote: >> Currently, kernel_uimage_probe() does not set info->zimage.start. As a >> result, it contains the default value (ie 0). This causes, >> kernel_zimage_place() to treat the binary (contained within uImage) as >> position independent executable. Thus, it loads it at an incorrect >> address. >> >> The correct approach would be to read "uimage.ep" and set >> info->zimage.start. This will ensure that the binary is loaded at the >> correct address. Also, it checks that the load address and entry address >> are the same. The reason being we currently support non compressed >> images >> for uImage header. And as seen in uboot sources(image_decomp(), case >> IH_COMP_NONE), load address and entry address can be the same. >> >> This behavior is applicable for both arm and arm64 platforms. >> >> Earlier for arm32 and arm64 platforms, Xen was ignoring the entry point >> address set in the uImage header. With this commit, Xen will use the >> kernel entry point address as specified in the header. This makes the >> behavior of Xen consistent with uboot for uimage headers. > > At the cost of breaking existing users. I think this want to be spelt > out clearly. Do you mean I should document this in "docs/misc/arm/booting.txt" ? > >> >> A deviation from uboot behaviour is that we consider load address == >> 0x0, >> to denote that the image supports position independent execution. This >> is to make the behavior consistent across uImage and zImage. > > You want to explain why users want to use "0x0". Or maybe the other > way around, that only a very limited set of user (e.g. in static > environment) should provide a fixed address. Does the following explaination sound fine ? "Users who want to use Xen with statically partitioned domains, should provide the fixed non zero load address for the dom0/domU kernel." > >> >> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com> >> --- >> >> Changes from v1:- >> 1. Added a check to ensure load address and entry address are the same. >> 2. Considered load address == 0x0 as position independent execution. >> 3. Ensured that the uImage header interpretation is consistent across >> arm32 and arm64. >> >> Some unanswered queries from v1 :- >> >> Q1. "In non-statically allocated setup, a user doesn't know where the >> memory >> for dom0/domU will be allocated.So I think this was correct to ignore >> the >> address." >> A. I am not sure the best way to handle this. Can we make the uImage >> probe >> dependent on STATIC_MEMORY ? Currently, I have mentioned that user >> should >> use load address = 0x0 if they want Xen to ignore it. > See above. > >> >> Q2. Zephyr or any other RTOS have different requirements, then we may >> need >> to modify Xen and document it. >> A. I will take it in a separate email/patch where we can list the >> requirements for booting Zephyr. >> >> docs/misc/arm/booting.txt | 11 ++++++++++- >> xen/arch/arm/include/asm/kernel.h | 2 +- >> xen/arch/arm/kernel.c | 26 +++++++++++++++++++++++++- >> 3 files changed, 36 insertions(+), 3 deletions(-) >> >> diff --git a/docs/misc/arm/booting.txt b/docs/misc/arm/booting.txt >> index 3e0c03e065..872262686a 100644 >> --- a/docs/misc/arm/booting.txt >> +++ b/docs/misc/arm/booting.txt >> @@ -4,7 +4,13 @@ Booting Xen >> Xen follows the zImage protocol defined for 32-bit ARM Linux[1] and >> the >> Image protocol defined for ARM64 Linux[2]. >> -In both cases the recommendation to boot in HYP/EL2 mode is a strict >> +Xen also supports the legacy image protocol[3] for 32-bit ARM and >> ARM64. >> +For now, it supports images where load address is same as entry >> address. >> +A deviation from uboot is that, Xen treats "load address == 0x0" as >> +position independent execution. Thus, Xen will load such an image at an >> +address it considers appropriate. > > Hmmmm... The section above is related to Xen itself and not the guest > Image. So I think this is misplaced. I don't see a booting section for the guest. Should I create one in this file ? > >> + >> +In all cases the recommendation to boot in HYP/EL2 mode is a strict >> requirement for Xen. >> The exceptions to this on 32-bit ARM are as follows: >> @@ -39,3 +45,6 @@ Latest version: >> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/t >> [2] linux/Documentation/arm64/booting.rst >> Latest version: >> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/arm64/booting.rst >> + >> +[3] legacy format header >> +Latest version: >> https://source.denx.de/u-boot/u-boot/-/blob/master/include/image.h#L315 >> diff --git a/xen/arch/arm/include/asm/kernel.h >> b/xen/arch/arm/include/asm/kernel.h >> index 5bb30c3f2f..ee69a47052 100644 >> --- a/xen/arch/arm/include/asm/kernel.h >> +++ b/xen/arch/arm/include/asm/kernel.h >> @@ -72,7 +72,7 @@ struct kernel_info { >> #ifdef CONFIG_ARM_64 >> paddr_t text_offset; /* 64-bit Image only */ >> #endif >> - paddr_t start; /* 32-bit zImage only */ >> + paddr_t start; > > I think the comment should be replaced by "Must be 0 for 64-bit Image". But, 64 bit image can also have a fixed load address. For zImage(64), start is always 0. For uImage(64), start can be a fixed load address (if it is not 0). > >> } zimage; >> }; >> }; >> diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c >> index 23b840ea9e..81ac945f5b 100644 >> --- a/xen/arch/arm/kernel.c >> +++ b/xen/arch/arm/kernel.c >> @@ -127,7 +127,7 @@ static paddr_t __init kernel_zimage_place(struct >> kernel_info *info) >> paddr_t load_addr; >> #ifdef CONFIG_ARM_64 >> - if ( info->type == DOMAIN_64BIT ) >> + if ( (info->type == DOMAIN_64BIT) && (info->zimage.start == 0) ) >> return info->mem.bank[0].start + info->zimage.text_offset; >> #endif >> @@ -223,6 +223,30 @@ static int __init kernel_uimage_probe(struct >> kernel_info *info, >> if ( len > size - sizeof(uimage) ) >> return -EINVAL; >> + info->zimage.start = be32_to_cpu(uimage.ep); >> + >> + /* >> + * Currently, we support uImage headers for uncompressed images >> only. >> + * Thus, it is valid for the load address and start address to >> be the >> + * same. This is consistent with the uboot behavior (Refer >> + * "case IH_COMP_NONE" in image_decomp()). >> + */ >> + if ( info->zimage.start != be32_to_cpu(uimage.load) ) >> + { >> + panic("Unable to support mismatching load address and entry >> address\n"); >> + return -EINVAL; >> + } >> + >> + /* >> + * While uboot considers 0x0 to be a valid load/start address, >> for Xen >> + * to mantain parity with zimage, we consider 0x0 to denote >> position >> + * independent image. That means Xen is free to load such an >> image at >> + * any valid address. >> + * Thus, we will print an appropriate warning. >> + */ >> + if ( info->zimage.start == 0 ) >> + printk(XENLOG_WARNING "No valid load address provided\n"); > > This is likely the way user will want to use it in a non-static > environment. So why is this a warning? Sorry, it should be. printk(XENLOG_INFO, "No load address provided. Assuming domain is dynamically allocated\n"); I just wanted to give user a heads-up in case if they are using in a static environment. I can drop the message if it is not much useful. - Ayan > > Cheers, >
On 16/12/2022 17:50, Ayan Kumar Halder wrote: > > On 16/12/2022 09:34, Julien Grall wrote: >> Hi, > Hi Julien, >> >> On 15/12/2022 12:38, Ayan Kumar Halder wrote: >>> Currently, kernel_uimage_probe() does not set info->zimage.start. As a >>> result, it contains the default value (ie 0). This causes, >>> kernel_zimage_place() to treat the binary (contained within uImage) as >>> position independent executable. Thus, it loads it at an incorrect >>> address. >>> >>> The correct approach would be to read "uimage.ep" and set >>> info->zimage.start. This will ensure that the binary is loaded at the >>> correct address. Also, it checks that the load address and entry address >>> are the same. The reason being we currently support non compressed >>> images >>> for uImage header. And as seen in uboot sources(image_decomp(), case >>> IH_COMP_NONE), load address and entry address can be the same. >>> >>> This behavior is applicable for both arm and arm64 platforms. >>> >>> Earlier for arm32 and arm64 platforms, Xen was ignoring the entry point >>> address set in the uImage header. With this commit, Xen will use the >>> kernel entry point address as specified in the header. This makes the >>> behavior of Xen consistent with uboot for uimage headers. >> >> At the cost of breaking existing users. I think this want to be spelt >> out clearly. > Do you mean I should document this in "docs/misc/arm/booting.txt" ? Both the commit message and document. >> >>> >>> A deviation from uboot behaviour is that we consider load address == >>> 0x0, >>> to denote that the image supports position independent execution. This >>> is to make the behavior consistent across uImage and zImage. >> >> You want to explain why users want to use "0x0". Or maybe the other >> way around, that only a very limited set of user (e.g. in static >> environment) should provide a fixed address. > > Does the following explaination sound fine ? > > "Users who want to use Xen with statically partitioned domains, should > provide the fixed non zero load address for the dom0/domU kernel." I would replace "should" with "can" because there is no requirement to specify an address. > >> >>> >>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com> >>> --- >>> >>> Changes from v1:- >>> 1. Added a check to ensure load address and entry address are the same. >>> 2. Considered load address == 0x0 as position independent execution. >>> 3. Ensured that the uImage header interpretation is consistent across >>> arm32 and arm64. >>> >>> Some unanswered queries from v1 :- >>> >>> Q1. "In non-statically allocated setup, a user doesn't know where the >>> memory >>> for dom0/domU will be allocated.So I think this was correct to ignore >>> the >>> address." >>> A. I am not sure the best way to handle this. Can we make the uImage >>> probe >>> dependent on STATIC_MEMORY ? Currently, I have mentioned that user >>> should >>> use load address = 0x0 if they want Xen to ignore it. >> See above. >> >>> >>> Q2. Zephyr or any other RTOS have different requirements, then we may >>> need >>> to modify Xen and document it. >>> A. I will take it in a separate email/patch where we can list the >>> requirements for booting Zephyr. >>> >>> docs/misc/arm/booting.txt | 11 ++++++++++- >>> xen/arch/arm/include/asm/kernel.h | 2 +- >>> xen/arch/arm/kernel.c | 26 +++++++++++++++++++++++++- >>> 3 files changed, 36 insertions(+), 3 deletions(-) >>> >>> diff --git a/docs/misc/arm/booting.txt b/docs/misc/arm/booting.txt >>> index 3e0c03e065..872262686a 100644 >>> --- a/docs/misc/arm/booting.txt >>> +++ b/docs/misc/arm/booting.txt >>> @@ -4,7 +4,13 @@ Booting Xen >>> Xen follows the zImage protocol defined for 32-bit ARM Linux[1] and >>> the >>> Image protocol defined for ARM64 Linux[2]. >>> -In both cases the recommendation to boot in HYP/EL2 mode is a strict >>> +Xen also supports the legacy image protocol[3] for 32-bit ARM and >>> ARM64. >>> +For now, it supports images where load address is same as entry >>> address. >>> +A deviation from uboot is that, Xen treats "load address == 0x0" as >>> +position independent execution. Thus, Xen will load such an image at an >>> +address it considers appropriate. >> >> Hmmmm... The section above is related to Xen itself and not the guest >> Image. So I think this is misplaced. > I don't see a booting section for the guest. Should I create one in this > file ? I would create a new section. >> >>> + >>> +In all cases the recommendation to boot in HYP/EL2 mode is a strict >>> requirement for Xen. >>> The exceptions to this on 32-bit ARM are as follows: >>> @@ -39,3 +45,6 @@ Latest version: >>> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/t >>> [2] linux/Documentation/arm64/booting.rst >>> Latest version: >>> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/arm64/booting.rst >>> + >>> +[3] legacy format header >>> +Latest version: >>> https://source.denx.de/u-boot/u-boot/-/blob/master/include/image.h#L315 >>> diff --git a/xen/arch/arm/include/asm/kernel.h >>> b/xen/arch/arm/include/asm/kernel.h >>> index 5bb30c3f2f..ee69a47052 100644 >>> --- a/xen/arch/arm/include/asm/kernel.h >>> +++ b/xen/arch/arm/include/asm/kernel.h >>> @@ -72,7 +72,7 @@ struct kernel_info { >>> #ifdef CONFIG_ARM_64 >>> paddr_t text_offset; /* 64-bit Image only */ >>> #endif >>> - paddr_t start; /* 32-bit zImage only */ >>> + paddr_t start; >> >> I think the comment should be replaced by "Must be 0 for 64-bit Image". > > But, 64 bit image can also have a fixed load address. Note the uppercase 'I' which refers to the Linux kernel Image protocol. > > For zImage(64), start is always 0. The official name is Image not zImage. We could call it "linux Image" if you prefer. > > For uImage(64), start can be a fixed load address (if it is not 0). > >> >>> } zimage; >>> }; >>> }; >>> diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c >>> index 23b840ea9e..81ac945f5b 100644 >>> --- a/xen/arch/arm/kernel.c >>> +++ b/xen/arch/arm/kernel.c >>> @@ -127,7 +127,7 @@ static paddr_t __init kernel_zimage_place(struct >>> kernel_info *info) >>> paddr_t load_addr; >>> #ifdef CONFIG_ARM_64 >>> - if ( info->type == DOMAIN_64BIT ) >>> + if ( (info->type == DOMAIN_64BIT) && (info->zimage.start == 0) ) >>> return info->mem.bank[0].start + info->zimage.text_offset; >>> #endif >>> @@ -223,6 +223,30 @@ static int __init kernel_uimage_probe(struct >>> kernel_info *info, >>> if ( len > size - sizeof(uimage) ) >>> return -EINVAL; >>> + info->zimage.start = be32_to_cpu(uimage.ep); >>> + >>> + /* >>> + * Currently, we support uImage headers for uncompressed images >>> only. >>> + * Thus, it is valid for the load address and start address to >>> be the >>> + * same. This is consistent with the uboot behavior (Refer >>> + * "case IH_COMP_NONE" in image_decomp()). >>> + */ >>> + if ( info->zimage.start != be32_to_cpu(uimage.load) ) >>> + { >>> + panic("Unable to support mismatching load address and entry >>> address\n"); >>> + return -EINVAL; >>> + } >>> + >>> + /* >>> + * While uboot considers 0x0 to be a valid load/start address, >>> for Xen >>> + * to mantain parity with zimage, we consider 0x0 to denote >>> position >>> + * independent image. That means Xen is free to load such an >>> image at >>> + * any valid address. >>> + * Thus, we will print an appropriate warning. >>> + */ >>> + if ( info->zimage.start == 0 ) >>> + printk(XENLOG_WARNING "No valid load address provided\n"); >> >> This is likely the way user will want to use it in a non-static >> environment. So why is this a warning? > > Sorry, it should be. > > printk(XENLOG_INFO, "No load address provided. Assuming domain is > dynamically allocated\n"); > > I just wanted to give user a heads-up in case if they are using in a > static environment. Again there is no problem to let Xen choosing the address in the static environment... So if there is a message then it should simply be: "No load address provided. Xen will decide where to load it". Cheers,
diff --git a/docs/misc/arm/booting.txt b/docs/misc/arm/booting.txt index 3e0c03e065..872262686a 100644 --- a/docs/misc/arm/booting.txt +++ b/docs/misc/arm/booting.txt @@ -4,7 +4,13 @@ Booting Xen Xen follows the zImage protocol defined for 32-bit ARM Linux[1] and the Image protocol defined for ARM64 Linux[2]. -In both cases the recommendation to boot in HYP/EL2 mode is a strict +Xen also supports the legacy image protocol[3] for 32-bit ARM and ARM64. +For now, it supports images where load address is same as entry address. +A deviation from uboot is that, Xen treats "load address == 0x0" as +position independent execution. Thus, Xen will load such an image at an +address it considers appropriate. + +In all cases the recommendation to boot in HYP/EL2 mode is a strict requirement for Xen. The exceptions to this on 32-bit ARM are as follows: @@ -39,3 +45,6 @@ Latest version: http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/t [2] linux/Documentation/arm64/booting.rst Latest version: http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/arm64/booting.rst + +[3] legacy format header +Latest version: https://source.denx.de/u-boot/u-boot/-/blob/master/include/image.h#L315 diff --git a/xen/arch/arm/include/asm/kernel.h b/xen/arch/arm/include/asm/kernel.h index 5bb30c3f2f..ee69a47052 100644 --- a/xen/arch/arm/include/asm/kernel.h +++ b/xen/arch/arm/include/asm/kernel.h @@ -72,7 +72,7 @@ struct kernel_info { #ifdef CONFIG_ARM_64 paddr_t text_offset; /* 64-bit Image only */ #endif - paddr_t start; /* 32-bit zImage only */ + paddr_t start; } zimage; }; }; diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c index 23b840ea9e..81ac945f5b 100644 --- a/xen/arch/arm/kernel.c +++ b/xen/arch/arm/kernel.c @@ -127,7 +127,7 @@ static paddr_t __init kernel_zimage_place(struct kernel_info *info) paddr_t load_addr; #ifdef CONFIG_ARM_64 - if ( info->type == DOMAIN_64BIT ) + if ( (info->type == DOMAIN_64BIT) && (info->zimage.start == 0) ) return info->mem.bank[0].start + info->zimage.text_offset; #endif @@ -223,6 +223,30 @@ static int __init kernel_uimage_probe(struct kernel_info *info, if ( len > size - sizeof(uimage) ) return -EINVAL; + info->zimage.start = be32_to_cpu(uimage.ep); + + /* + * Currently, we support uImage headers for uncompressed images only. + * Thus, it is valid for the load address and start address to be the + * same. This is consistent with the uboot behavior (Refer + * "case IH_COMP_NONE" in image_decomp()). + */ + if ( info->zimage.start != be32_to_cpu(uimage.load) ) + { + panic("Unable to support mismatching load address and entry address\n"); + return -EINVAL; + } + + /* + * While uboot considers 0x0 to be a valid load/start address, for Xen + * to mantain parity with zimage, we consider 0x0 to denote position + * independent image. That means Xen is free to load such an image at + * any valid address. + * Thus, we will print an appropriate warning. + */ + if ( info->zimage.start == 0 ) + printk(XENLOG_WARNING "No valid load address provided\n"); + info->zimage.kernel_addr = addr + sizeof(uimage); info->zimage.len = len;
Currently, kernel_uimage_probe() does not set info->zimage.start. As a result, it contains the default value (ie 0). This causes, kernel_zimage_place() to treat the binary (contained within uImage) as position independent executable. Thus, it loads it at an incorrect address. The correct approach would be to read "uimage.ep" and set info->zimage.start. This will ensure that the binary is loaded at the correct address. Also, it checks that the load address and entry address are the same. The reason being we currently support non compressed images for uImage header. And as seen in uboot sources(image_decomp(), case IH_COMP_NONE), load address and entry address can be the same. This behavior is applicable for both arm and arm64 platforms. Earlier for arm32 and arm64 platforms, Xen was ignoring the entry point address set in the uImage header. With this commit, Xen will use the kernel entry point address as specified in the header. This makes the behavior of Xen consistent with uboot for uimage headers. A deviation from uboot behaviour is that we consider load address == 0x0, to denote that the image supports position independent execution. This is to make the behavior consistent across uImage and zImage. Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com> --- Changes from v1:- 1. Added a check to ensure load address and entry address are the same. 2. Considered load address == 0x0 as position independent execution. 3. Ensured that the uImage header interpretation is consistent across arm32 and arm64. Some unanswered queries from v1 :- Q1. "In non-statically allocated setup, a user doesn't know where the memory for dom0/domU will be allocated.So I think this was correct to ignore the address." A. I am not sure the best way to handle this. Can we make the uImage probe dependent on STATIC_MEMORY ? Currently, I have mentioned that user should use load address = 0x0 if they want Xen to ignore it. Q2. Zephyr or any other RTOS have different requirements, then we may need to modify Xen and document it. A. I will take it in a separate email/patch where we can list the requirements for booting Zephyr. docs/misc/arm/booting.txt | 11 ++++++++++- xen/arch/arm/include/asm/kernel.h | 2 +- xen/arch/arm/kernel.c | 26 +++++++++++++++++++++++++- 3 files changed, 36 insertions(+), 3 deletions(-)