[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [XEN v3] xen/arm: Probe the entry point address of an uImage correctly
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@xxxxxxx> > --- > > 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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |