| Message ID | 20221217193801.19485-1-ayan.kumar.halder@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [XEN,v3] xen/arm: Probe the entry point address of an uImage correctly | expand |
Hi Ayan, On 17/12/2022 20: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. > > Users who want to use Xen with statically partitioned domains, can > provide the fixed non zero load address for the dom0/domU kernel. > > A deviation from uboot behaviour is that we consider load address == 0x0, > to denote that the image supports position independent execution. This > is to make the behavior consistent across uImage and zImage. > > Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com> > --- > > Changes from v1 :- > 1. Added a check to ensure load address and entry address are the same. > 2. Considered load address == 0x0 as position independent execution. > 3. Ensured that the uImage header interpretation is consistent across > arm32 and arm64. > > v2 :- > 1. Mentioned the change in existing behavior in booting.txt. > 2. Updated booting.txt with a new section to document "Booting Guests". > > docs/misc/arm/booting.txt | 21 +++++++++++++++++++++ > xen/arch/arm/include/asm/kernel.h | 2 +- > xen/arch/arm/kernel.c | 27 ++++++++++++++++++++++++++- > 3 files changed, 48 insertions(+), 2 deletions(-) > > diff --git a/docs/misc/arm/booting.txt b/docs/misc/arm/booting.txt > index 3e0c03e065..01b12b49a5 100644 > --- a/docs/misc/arm/booting.txt > +++ b/docs/misc/arm/booting.txt > @@ -23,6 +23,24 @@ The exceptions to this on 32-bit ARM are as follows: > > There are no exception on 64-bit ARM. > > +Booting Guests > +-------------- > + > +Xen supports the legacy image protocol[3], zImage protocol for 32-bit ARM uImage is not a protocol. It is just a header with no other information \wrt boot requirements. > +Linux[1] and Image protocol defined for ARM64[2]. > + > +Earlier for legacy image protocol, Xen ignored the load address and entry > +point specified in the header. This has now changed. > + > +Now, it loads the image at the load address provided in the header. > +For now, it supports images where load address is same as entry point. Would be beneficial to add explanation why load address must be equal to start 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. > + > +Users who want to use Xen with statically partitioned domains, can provide > +the fixed non zero load address for the dom0/domU kernel. I think this section is missing a note that in case of not PIE, a load/start address specified by the user in a header must be within the memory region allocated by Xen. > > Firmware/bootloader requirements > -------------------------------- > @@ -39,3 +57,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..4617cdc83b 100644 > --- a/xen/arch/arm/include/asm/kernel.h > +++ b/xen/arch/arm/include/asm/kernel.h > @@ -72,7 +72,7 @@ struct kernel_info { > #ifdef CONFIG_ARM_64 > paddr_t text_offset; /* 64-bit Image only */ > #endif > - paddr_t start; /* 32-bit zImage only */ > + paddr_t start; /* Must be 0 for 64-bit Image */ > } zimage; > }; > }; > diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c > index 23b840ea9e..e9c18993ef 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,31 @@ 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(). Please make it clear that you are referring to uboot function. > + */ > + 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 message. > + */ > + if ( info->zimage.start == 0 ) > + printk(XENLOG_INFO > + "No load address provided. Xen will decide where to load it\n"); > + > info->zimage.kernel_addr = addr + sizeof(uimage); > info->zimage.len = len; > ~Michal
On 20/12/2022 09:44, Michal Orzel wrote: > Hi Ayan, > > On 17/12/2022 20: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. >> >> Users who want to use Xen with statically partitioned domains, can >> provide the fixed non zero load address for the dom0/domU kernel. >> >> A deviation from uboot behaviour is that we consider load address == 0x0, >> to denote that the image supports position independent execution. This >> is to make the behavior consistent across uImage and zImage. >> >> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com> >> --- >> >> Changes from v1 :- >> 1. Added a check to ensure load address and entry address are the same. >> 2. Considered load address == 0x0 as position independent execution. >> 3. Ensured that the uImage header interpretation is consistent across >> arm32 and arm64. >> >> v2 :- >> 1. Mentioned the change in existing behavior in booting.txt. >> 2. Updated booting.txt with a new section to document "Booting Guests". >> >> docs/misc/arm/booting.txt | 21 +++++++++++++++++++++ >> xen/arch/arm/include/asm/kernel.h | 2 +- >> xen/arch/arm/kernel.c | 27 ++++++++++++++++++++++++++- >> 3 files changed, 48 insertions(+), 2 deletions(-) >> >> diff --git a/docs/misc/arm/booting.txt b/docs/misc/arm/booting.txt >> index 3e0c03e065..01b12b49a5 100644 >> --- a/docs/misc/arm/booting.txt >> +++ b/docs/misc/arm/booting.txt >> @@ -23,6 +23,24 @@ The exceptions to this on 32-bit ARM are as follows: >> >> There are no exception on 64-bit ARM. >> >> +Booting Guests >> +-------------- >> + >> +Xen supports the legacy image protocol[3], zImage protocol for 32-bit ARM > uImage is not a protocol. It is just a header with no other information \wrt > boot requirements. > >> +Linux[1] and Image protocol defined for ARM64[2]. >> + >> +Earlier for legacy image protocol, Xen ignored the load address and entry >> +point specified in the header. This has now changed. >> + >> +Now, it loads the image at the load address provided in the header. >> +For now, it supports images where load address is same as entry point. > Would be beneficial to add explanation why load address must be equal to start 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. >> + >> +Users who want to use Xen with statically partitioned domains, can provide >> +the fixed non zero load address for the dom0/domU kernel. > > I think this section is missing a note that in case of not PIE, a load/start address > specified by the user in a header must be within the memory region allocated by Xen. > >> >> Firmware/bootloader requirements >> -------------------------------- >> @@ -39,3 +57,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..4617cdc83b 100644 >> --- a/xen/arch/arm/include/asm/kernel.h >> +++ b/xen/arch/arm/include/asm/kernel.h >> @@ -72,7 +72,7 @@ struct kernel_info { >> #ifdef CONFIG_ARM_64 >> paddr_t text_offset; /* 64-bit Image only */ >> #endif >> - paddr_t start; /* 32-bit zImage only */ >> + paddr_t start; /* Must be 0 for 64-bit Image */ >> } zimage; >> }; >> }; >> diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c >> index 23b840ea9e..e9c18993ef 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,31 @@ 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(). > Please make it clear that you are referring to uboot function. Could we avoid to mention the u-boot function? The reason I am asking is the code is in a different repo and the function name can easily change without us noticing (so the comment would rot). Is the behavior documented somewhere in U-boot (or online)? If not, what's the guarantee that it will not change? Cheers,
Hi Julien/Michal, On 20/12/2022 10:05, Julien Grall wrote: > On 20/12/2022 09:44, Michal Orzel wrote: >> Hi Ayan, >> >> On 17/12/2022 20: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. >>> >>> Users who want to use Xen with statically partitioned domains, can >>> provide the fixed non zero load address for the dom0/domU kernel. >>> >>> A deviation from uboot behaviour is that we consider load address == >>> 0x0, >>> to denote that the image supports position independent execution. This >>> is to make the behavior consistent across uImage and zImage. >>> >>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com> >>> --- >>> >>> Changes from v1 :- >>> 1. Added a check to ensure load address and entry address are the same. >>> 2. Considered load address == 0x0 as position independent execution. >>> 3. Ensured that the uImage header interpretation is consistent across >>> arm32 and arm64. >>> >>> v2 :- >>> 1. Mentioned the change in existing behavior in booting.txt. >>> 2. Updated booting.txt with a new section to document "Booting Guests". >>> >>> docs/misc/arm/booting.txt | 21 +++++++++++++++++++++ >>> xen/arch/arm/include/asm/kernel.h | 2 +- >>> xen/arch/arm/kernel.c | 27 ++++++++++++++++++++++++++- >>> 3 files changed, 48 insertions(+), 2 deletions(-) >>> >>> diff --git a/docs/misc/arm/booting.txt b/docs/misc/arm/booting.txt >>> index 3e0c03e065..01b12b49a5 100644 >>> --- a/docs/misc/arm/booting.txt >>> +++ b/docs/misc/arm/booting.txt >>> @@ -23,6 +23,24 @@ The exceptions to this on 32-bit ARM are as follows: >>> There are no exception on 64-bit ARM. >>> +Booting Guests >>> +-------------- >>> + >>> +Xen supports the legacy image protocol[3], zImage protocol for >>> 32-bit ARM >> uImage is not a protocol. It is just a header with no other >> information \wrt >> boot requirements. >> >>> +Linux[1] and Image protocol defined for ARM64[2]. >>> + >>> +Earlier for legacy image protocol, Xen ignored the load address and >>> entry >>> +point specified in the header. This has now changed. >>> + >>> +Now, it loads the image at the load address provided in the header. >>> +For now, it supports images where load address is same as entry point. >> Would be beneficial to add explanation why load address must be equal >> to start address. Answered below. >> >>> + >>> +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. >>> + >>> +Users who want to use Xen with statically partitioned domains, can >>> provide >>> +the fixed non zero load address for the dom0/domU kernel. >> >> I think this section is missing a note that in case of not PIE, a >> load/start address >> specified by the user in a header must be within the memory region >> allocated by Xen. >> >>> Firmware/bootloader requirements >>> -------------------------------- >>> @@ -39,3 +57,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..4617cdc83b 100644 >>> --- a/xen/arch/arm/include/asm/kernel.h >>> +++ b/xen/arch/arm/include/asm/kernel.h >>> @@ -72,7 +72,7 @@ struct kernel_info { >>> #ifdef CONFIG_ARM_64 >>> paddr_t text_offset; /* 64-bit Image only */ >>> #endif >>> - paddr_t start; /* 32-bit zImage only */ >>> + paddr_t start; /* Must be 0 for 64-bit Image */ >>> } zimage; >>> }; >>> }; >>> diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c >>> index 23b840ea9e..e9c18993ef 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,31 @@ 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(). >> Please make it clear that you are referring to uboot function. > > Could we avoid to mention the u-boot function? The reason I am asking > is the code is in a different repo and the function name can easily > change without us noticing (so the comment would rot). > > Is the behavior documented somewhere in U-boot (or online)? If not, > what's the guarantee that it will not change? I could not find any documentation which states this. I found this from the following in uboot source code. https://source.denx.de/u-boot/u-boot/-/blob/master/boot/image.c#L458 AFAIU when kernel_uimage_probe() get invoked, the image has already been decompressed. So, I added this as a limitation. I will remove the limitation if it does not look correct. - Ayan > > Cheers, >
Hi Ayan, On 20/12/2022 10:44, Ayan Kumar Halder wrote: >>>> + >>>> + /* >>>> + * 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(). >>> Please make it clear that you are referring to uboot function. >> >> Could we avoid to mention the u-boot function? The reason I am asking >> is the code is in a different repo and the function name can easily >> change without us noticing (so the comment would rot). >> >> Is the behavior documented somewhere in U-boot (or online)? If not, >> what's the guarantee that it will not change? > > I could not find any documentation which states this. I found this from > the following in uboot source code. > > https://source.denx.de/u-boot/u-boot/-/blob/master/boot/image.c#L458 > > AFAIU when kernel_uimage_probe() get invoked, the image has already been > decompressed. So, I added this as a limitation. I e looked at the U-boot code and, AFAIU, the check is merely to avoid the memcpy() if the image start matches the start of the decompression area. So I don't understand why we need the limitation in Xen. Now the lack of documentation (or at least I didn't find any) makes a bit more tricky to understand what the fields in the U-boot header are for. I have skimmed through the code and I disagree with the implementation you chose for Xen. The comment for 'ih_ep' suggests this is the entry point address. That's may be different from the beginning of your blob. I think this is what ih_load is for. So I think we want to load the blob at ih_load and then set pc to point to ih_ep. This will require a bit more work in Xen because the assumption so far is the entry point matches the start of the blob. Please let me known if I missed anything. Cheers,
On 20/12/2022 12:14, Julien Grall wrote: > Hi Ayan, Hi Julien, > > On 20/12/2022 10:44, Ayan Kumar Halder wrote: >>>>> + >>>>> + /* >>>>> + * 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(). >>>> Please make it clear that you are referring to uboot function. >>> >>> Could we avoid to mention the u-boot function? The reason I am >>> asking is the code is in a different repo and the function name can >>> easily change without us noticing (so the comment would rot). >>> >>> Is the behavior documented somewhere in U-boot (or online)? If not, >>> what's the guarantee that it will not change? >> >> I could not find any documentation which states this. I found this >> from the following in uboot source code. >> >> https://source.denx.de/u-boot/u-boot/-/blob/master/boot/image.c#L458 >> >> AFAIU when kernel_uimage_probe() get invoked, the image has already >> been decompressed. So, I added this as a limitation. > > I e looked at the U-boot code and, AFAIU, the check is merely to avoid > the memcpy() if the image start matches the start of the decompression > area. So I don't understand why we need the limitation in Xen. > > Now the lack of documentation (or at least I didn't find any) makes a > bit more tricky to understand what the fields in the U-boot header are > for. I have skimmed through the code and I disagree with the > implementation you chose for Xen. > > The comment for 'ih_ep' suggests this is the entry point address. > That's may be different from the beginning of your blob. I think this > is what ih_load is for. > > So I think we want to load the blob at ih_load and then set pc to > point to ih_ep. This will require a bit more work in Xen because the > assumption so far is the entry point matches the start of the blob. > > Please let me known if I missed anything. I think you are correct. I will rework this to correctly handle load address and entry point. - Ayan > > Cheers, >
Hi Julien, I need a clarification. On 20/12/2022 12:51, Ayan Kumar Halder wrote: > > On 20/12/2022 12:14, Julien Grall wrote: >> Hi Ayan, > Hi Julien, >> >> On 20/12/2022 10:44, Ayan Kumar Halder wrote: >>>>>> + >>>>>> + /* >>>>>> + * 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(). >>>>> Please make it clear that you are referring to uboot function. >>>> >>>> Could we avoid to mention the u-boot function? The reason I am >>>> asking is the code is in a different repo and the function name can >>>> easily change without us noticing (so the comment would rot). >>>> >>>> Is the behavior documented somewhere in U-boot (or online)? If not, >>>> what's the guarantee that it will not change? >>> >>> I could not find any documentation which states this. I found this >>> from the following in uboot source code. >>> >>> https://source.denx.de/u-boot/u-boot/-/blob/master/boot/image.c#L458 >>> >>> AFAIU when kernel_uimage_probe() get invoked, the image has already >>> been decompressed. So, I added this as a limitation. >> >> I e looked at the U-boot code and, AFAIU, the check is merely to >> avoid the memcpy() if the image start matches the start of the >> decompression area. So I don't understand why we need the limitation >> in Xen. >> >> Now the lack of documentation (or at least I didn't find any) makes a >> bit more tricky to understand what the fields in the U-boot header >> are for. I have skimmed through the code and I disagree with the >> implementation you chose for Xen. >> >> The comment for 'ih_ep' suggests this is the entry point address. >> That's may be different from the beginning of your blob. I think this >> is what ih_load is for. >> >> So I think we want to load the blob at ih_load and then set pc to >> point to ih_ep. This will require a bit more work in Xen because the >> assumption so far is the entry point matches the start of the blob. >> >> Please let me known if I missed anything. > > I think you are correct. I will rework this to correctly handle load > address and entry point. Is it fine to keep these two constraints ? /* * 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 message. */ if ( info->zimage.start == 0 ) printk(XENLOG_INFO "No load address provided. Xen will decide where to load it.\n"); /* * If the image supports position independent execution, then user cannot * provide an entry point as Xen will load such an image at any appropriate * memory address. Thus, we need to return error */ if ( (info->zimage.start == 0) && (info->entry != 0) ) { printk(XENLOG_ERROR "Entry point cannot be non zero for PIE image.\n"); return -EINVAL; } - Ayan > > - Ayan > >> >> Cheers, >>
diff --git a/docs/misc/arm/booting.txt b/docs/misc/arm/booting.txt index 3e0c03e065..01b12b49a5 100644 --- a/docs/misc/arm/booting.txt +++ b/docs/misc/arm/booting.txt @@ -23,6 +23,24 @@ The exceptions to this on 32-bit ARM are as follows: There are no exception on 64-bit ARM. +Booting Guests +-------------- + +Xen supports the legacy image protocol[3], zImage protocol for 32-bit ARM +Linux[1] and Image protocol defined for ARM64[2]. + +Earlier for legacy image protocol, Xen ignored the load address and entry +point specified in the header. This has now changed. + +Now, it loads the image at the load address provided in the header. +For now, it supports images where load address is same as entry point. + +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. + +Users who want to use Xen with statically partitioned domains, can provide +the fixed non zero load address for the dom0/domU kernel. Firmware/bootloader requirements -------------------------------- @@ -39,3 +57,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..4617cdc83b 100644 --- a/xen/arch/arm/include/asm/kernel.h +++ b/xen/arch/arm/include/asm/kernel.h @@ -72,7 +72,7 @@ struct kernel_info { #ifdef CONFIG_ARM_64 paddr_t text_offset; /* 64-bit Image only */ #endif - paddr_t start; /* 32-bit zImage only */ + paddr_t start; /* Must be 0 for 64-bit Image */ } zimage; }; }; diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c index 23b840ea9e..e9c18993ef 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,31 @@ 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 message. + */ + if ( info->zimage.start == 0 ) + printk(XENLOG_INFO + "No load address provided. Xen will decide where to load it\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. Users who want to use Xen with statically partitioned domains, can provide the fixed non zero load address for the dom0/domU kernel. A deviation from uboot behaviour is that we consider load address == 0x0, to denote that the image supports position independent execution. This is to make the behavior consistent across uImage and zImage. Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com> --- Changes from v1 :- 1. Added a check to ensure load address and entry address are the same. 2. Considered load address == 0x0 as position independent execution. 3. Ensured that the uImage header interpretation is consistent across arm32 and arm64. v2 :- 1. Mentioned the change in existing behavior in booting.txt. 2. Updated booting.txt with a new section to document "Booting Guests". docs/misc/arm/booting.txt | 21 +++++++++++++++++++++ xen/arch/arm/include/asm/kernel.h | 2 +- xen/arch/arm/kernel.c | 27 ++++++++++++++++++++++++++- 3 files changed, 48 insertions(+), 2 deletions(-)