| Message ID | 1369922720-10015-3-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Thu, 2013-05-30 at 15:05 +0100, Julien Grall wrote: > If a DTB is appended to the DOM0 zImage, it's probably means that the linux > decompression stage will replace the DBT register value (r2 on arm32) > by the address of the appended DTB. I don't think we should do this, it certainly violates the principal of least surprise, since no other "bootloader" does anything like this AFAIK. Yes that means you occasionally trip over your DTB changes appearing not to take affect, but that's true on native and is one of the known and understood bad things about appended DTB. So I think this is a case of "don't do that then". At most we could issue a warning, I suppose. But really we shouldn't be making any assumptions (good or bad) about what happens to live in the memory just past the end of the kernel, it may or may not be an appended DTB. > In this case, to avoid issue with Linux, Xen needs to load the new device tree > just after the kernel. > > Signed-off-by: Julien Grall <julien.grall@linaro.org> > --- > xen/arch/arm/kernel.c | 26 ++++++++++++-------------- > 1 file changed, 12 insertions(+), 14 deletions(-) > > diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c > index f8c8850..1d6c927 100644 > --- a/xen/arch/arm/kernel.c > +++ b/xen/arch/arm/kernel.c > @@ -123,20 +123,6 @@ static int kernel_try_zimage_prepare(struct kernel_info *info, > if ( (end - start) > size ) > return -EINVAL; > > - /* > - * Check for an appended DTB. > - */ > - if ( addr + end - start + sizeof(dtb_hdr) <= size ) > - { > - copy_from_paddr(&dtb_hdr, addr + end - start, > - sizeof(dtb_hdr), DEV_SHARED); > - if (be32_to_cpu(dtb_hdr.magic) == DTB_MAGIC) { > - end += be32_to_cpu(dtb_hdr.total_size); > - > - if ( end > addr + size ) > - return -EINVAL; > - } > - } > > info->zimage.kernel_addr = addr; > > @@ -154,6 +140,18 @@ static int kernel_try_zimage_prepare(struct kernel_info *info, > info->load = kernel_zimage_load; > info->type = KERNEL_ZIMAGE; > > + /* > + * If there is an appended DTB, ask XEN to replace the DTB > + * by the generate one. > + */ > + if ( info->zimage.len + sizeof(dtb_hdr) <= size ) > + { > + copy_from_paddr(&dtb_hdr, addr + end - start, > + sizeof(dtb_hdr), DEV_SHARED); > + if (be32_to_cpu(dtb_hdr.magic) == DTB_MAGIC) > + info->dtb_paddr = info->zimage.load_addr + info->zimage.len; > + } > + > return 0; > } >
On 05/30/2013 03:24 PM, Ian Campbell wrote: > On Thu, 2013-05-30 at 15:05 +0100, Julien Grall wrote: >> If a DTB is appended to the DOM0 zImage, it's probably means that the linux >> decompression stage will replace the DBT register value (r2 on arm32) >> by the address of the appended DTB. > > I don't think we should do this, it certainly violates the principal of > least surprise, since no other "bootloader" does anything like this > AFAIK. Yes that means you occasionally trip over your DTB changes > appearing not to take affect, but that's true on native and is one of > the known and understood bad things about appended DTB. Shall we load the DTB if there is one appended? If yes, I think the user will be confused because Linux won't use the right device tree. It's hard for the user to find the issue because there is no log. > So I think this is a case of "don't do that then". At most we could > issue a warning, I suppose. But really we shouldn't be making any > assumptions (good or bad) about what happens to live in the memory just > past the end of the kernel, it may or may not be an appended DTB. I can add a warning and also fix the check the line: if ( addr + end - start + sizeof(dtb_hdr) <= size ) must be replace by: if ( end - start + sizeof(dtb_hdr) <= size ) >> In this case, to avoid issue with Linux, Xen needs to load the new device tree >> just after the kernel. >> >> Signed-off-by: Julien Grall <julien.grall@linaro.org> >> --- >> xen/arch/arm/kernel.c | 26 ++++++++++++-------------- >> 1 file changed, 12 insertions(+), 14 deletions(-) >> >> diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c >> index f8c8850..1d6c927 100644 >> --- a/xen/arch/arm/kernel.c >> +++ b/xen/arch/arm/kernel.c >> @@ -123,20 +123,6 @@ static int kernel_try_zimage_prepare(struct kernel_info *info, >> if ( (end - start) > size ) >> return -EINVAL; >> >> - /* >> - * Check for an appended DTB. >> - */ >> - if ( addr + end - start + sizeof(dtb_hdr) <= size ) >> - { >> - copy_from_paddr(&dtb_hdr, addr + end - start, >> - sizeof(dtb_hdr), DEV_SHARED); >> - if (be32_to_cpu(dtb_hdr.magic) == DTB_MAGIC) { >> - end += be32_to_cpu(dtb_hdr.total_size); >> - >> - if ( end > addr + size ) >> - return -EINVAL; >> - } >> - } >> >> info->zimage.kernel_addr = addr; >> >> @@ -154,6 +140,18 @@ static int kernel_try_zimage_prepare(struct kernel_info *info, >> info->load = kernel_zimage_load; >> info->type = KERNEL_ZIMAGE; >> >> + /* >> + * If there is an appended DTB, ask XEN to replace the DTB >> + * by the generate one. >> + */ >> + if ( info->zimage.len + sizeof(dtb_hdr) <= size ) >> + { >> + copy_from_paddr(&dtb_hdr, addr + end - start, >> + sizeof(dtb_hdr), DEV_SHARED); >> + if (be32_to_cpu(dtb_hdr.magic) == DTB_MAGIC) >> + info->dtb_paddr = info->zimage.load_addr + info->zimage.len; >> + } >> + >> return 0; >> } >> > >
On Thu, 2013-05-30 at 15:47 +0100, Julien Grall wrote: > On 05/30/2013 03:24 PM, Ian Campbell wrote: > > > On Thu, 2013-05-30 at 15:05 +0100, Julien Grall wrote: > >> If a DTB is appended to the DOM0 zImage, it's probably means that the linux > >> decompression stage will replace the DBT register value (r2 on arm32) > >> by the address of the appended DTB. > > > > I don't think we should do this, it certainly violates the principal of > > least surprise, since no other "bootloader" does anything like this > > AFAIK. Yes that means you occasionally trip over your DTB changes > > appearing not to take affect, but that's true on native and is one of > > the known and understood bad things about appended DTB. > > > Shall we load the DTB if there is one appended? We shouldn't make any assumptions based on whether we suspect there might be an appended DTB. We should just carry on. The presence or absence of an appended DTB is purely the guests internal concern, it is none of our business, even if it breaks things > If yes, I think the user > will be confused because Linux won't use the right device tree. Yes. Take a look at the Kconfig help text for the kernel option. This is a known shortcoming of APPEND_DTB, it's a hack and shouldn't be used. > It's hard for the user to find the issue because there is no log. > > > So I think this is a case of "don't do that then". At most we could > > issue a warning, I suppose. But really we shouldn't be making any > > assumptions (good or bad) about what happens to live in the memory just > > past the end of the kernel, it may or may not be an appended DTB. > > > I can add a warning and also fix the check the line: > if ( addr + end - start + sizeof(dtb_hdr) <= size ) > must be replace by: > if ( end - start + sizeof(dtb_hdr) <= size ) > > >> In this case, to avoid issue with Linux, Xen needs to load the new device tree > >> just after the kernel. > >> > >> Signed-off-by: Julien Grall <julien.grall@linaro.org> > >> --- > >> xen/arch/arm/kernel.c | 26 ++++++++++++-------------- > >> 1 file changed, 12 insertions(+), 14 deletions(-) > >> > >> diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c > >> index f8c8850..1d6c927 100644 > >> --- a/xen/arch/arm/kernel.c > >> +++ b/xen/arch/arm/kernel.c > >> @@ -123,20 +123,6 @@ static int kernel_try_zimage_prepare(struct kernel_info *info, > >> if ( (end - start) > size ) > >> return -EINVAL; > >> > >> - /* > >> - * Check for an appended DTB. > >> - */ > >> - if ( addr + end - start + sizeof(dtb_hdr) <= size ) > >> - { > >> - copy_from_paddr(&dtb_hdr, addr + end - start, > >> - sizeof(dtb_hdr), DEV_SHARED); > >> - if (be32_to_cpu(dtb_hdr.magic) == DTB_MAGIC) { > >> - end += be32_to_cpu(dtb_hdr.total_size); > >> - > >> - if ( end > addr + size ) > >> - return -EINVAL; > >> - } > >> - } > >> > >> info->zimage.kernel_addr = addr; > >> > >> @@ -154,6 +140,18 @@ static int kernel_try_zimage_prepare(struct kernel_info *info, > >> info->load = kernel_zimage_load; > >> info->type = KERNEL_ZIMAGE; > >> > >> + /* > >> + * If there is an appended DTB, ask XEN to replace the DTB > >> + * by the generate one. > >> + */ > >> + if ( info->zimage.len + sizeof(dtb_hdr) <= size ) > >> + { > >> + copy_from_paddr(&dtb_hdr, addr + end - start, > >> + sizeof(dtb_hdr), DEV_SHARED); > >> + if (be32_to_cpu(dtb_hdr.magic) == DTB_MAGIC) > >> + info->dtb_paddr = info->zimage.load_addr + info->zimage.len; > >> + } > >> + > >> return 0; > >> } > >> > > > > > >
On 05/30/2013 03:52 PM, Ian Campbell wrote: > On Thu, 2013-05-30 at 15:47 +0100, Julien Grall wrote: >> On 05/30/2013 03:24 PM, Ian Campbell wrote: >> >>> On Thu, 2013-05-30 at 15:05 +0100, Julien Grall wrote: >>>> If a DTB is appended to the DOM0 zImage, it's probably means that the linux >>>> decompression stage will replace the DBT register value (r2 on arm32) >>>> by the address of the appended DTB. >>> >>> I don't think we should do this, it certainly violates the principal of >>> least surprise, since no other "bootloader" does anything like this >>> AFAIK. Yes that means you occasionally trip over your DTB changes >>> appearing not to take affect, but that's true on native and is one of >>> the known and understood bad things about appended DTB. >> >> >> Shall we load the DTB if there is one appended? > > We shouldn't make any assumptions based on whether we suspect there > might be an appended DTB. We should just carry on. > > The presence or absence of an appended DTB is purely the guests internal > concern, it is none of our business, even if it breaks things Ok. I will rework the patch and send it back alone. >> If yes, I think the user >> will be confused because Linux won't use the right device tree. > > Yes. Take a look at the Kconfig help text for the kernel option. This is > a known shortcoming of APPEND_DTB, it's a hack and shouldn't be used. > >> It's hard for the user to find the issue because there is no log. >> >>> So I think this is a case of "don't do that then". At most we could >>> issue a warning, I suppose. But really we shouldn't be making any >>> assumptions (good or bad) about what happens to live in the memory just >>> past the end of the kernel, it may or may not be an appended DTB. >> >> >> I can add a warning and also fix the check the line: >> if ( addr + end - start + sizeof(dtb_hdr) <= size ) >> must be replace by: >> if ( end - start + sizeof(dtb_hdr) <= size ) >> >>>> In this case, to avoid issue with Linux, Xen needs to load the new device tree >>>> just after the kernel. >>>> >>>> Signed-off-by: Julien Grall <julien.grall@linaro.org> >>>> --- >>>> xen/arch/arm/kernel.c | 26 ++++++++++++-------------- >>>> 1 file changed, 12 insertions(+), 14 deletions(-) >>>> >>>> diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c >>>> index f8c8850..1d6c927 100644 >>>> --- a/xen/arch/arm/kernel.c >>>> +++ b/xen/arch/arm/kernel.c >>>> @@ -123,20 +123,6 @@ static int kernel_try_zimage_prepare(struct kernel_info *info, >>>> if ( (end - start) > size ) >>>> return -EINVAL; >>>> >>>> - /* >>>> - * Check for an appended DTB. >>>> - */ >>>> - if ( addr + end - start + sizeof(dtb_hdr) <= size ) >>>> - { >>>> - copy_from_paddr(&dtb_hdr, addr + end - start, >>>> - sizeof(dtb_hdr), DEV_SHARED); >>>> - if (be32_to_cpu(dtb_hdr.magic) == DTB_MAGIC) { >>>> - end += be32_to_cpu(dtb_hdr.total_size); >>>> - >>>> - if ( end > addr + size ) >>>> - return -EINVAL; >>>> - } >>>> - } >>>> >>>> info->zimage.kernel_addr = addr; >>>> >>>> @@ -154,6 +140,18 @@ static int kernel_try_zimage_prepare(struct kernel_info *info, >>>> info->load = kernel_zimage_load; >>>> info->type = KERNEL_ZIMAGE; >>>> >>>> + /* >>>> + * If there is an appended DTB, ask XEN to replace the DTB >>>> + * by the generate one. >>>> + */ >>>> + if ( info->zimage.len + sizeof(dtb_hdr) <= size ) >>>> + { >>>> + copy_from_paddr(&dtb_hdr, addr + end - start, >>>> + sizeof(dtb_hdr), DEV_SHARED); >>>> + if (be32_to_cpu(dtb_hdr.magic) == DTB_MAGIC) >>>> + info->dtb_paddr = info->zimage.load_addr + info->zimage.len; >>>> + } >>>> + >>>> return 0; >>>> } >>>> >>> >>> >> >> > >
diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c index f8c8850..1d6c927 100644 --- a/xen/arch/arm/kernel.c +++ b/xen/arch/arm/kernel.c @@ -123,20 +123,6 @@ static int kernel_try_zimage_prepare(struct kernel_info *info, if ( (end - start) > size ) return -EINVAL; - /* - * Check for an appended DTB. - */ - if ( addr + end - start + sizeof(dtb_hdr) <= size ) - { - copy_from_paddr(&dtb_hdr, addr + end - start, - sizeof(dtb_hdr), DEV_SHARED); - if (be32_to_cpu(dtb_hdr.magic) == DTB_MAGIC) { - end += be32_to_cpu(dtb_hdr.total_size); - - if ( end > addr + size ) - return -EINVAL; - } - } info->zimage.kernel_addr = addr; @@ -154,6 +140,18 @@ static int kernel_try_zimage_prepare(struct kernel_info *info, info->load = kernel_zimage_load; info->type = KERNEL_ZIMAGE; + /* + * If there is an appended DTB, ask XEN to replace the DTB + * by the generate one. + */ + if ( info->zimage.len + sizeof(dtb_hdr) <= size ) + { + copy_from_paddr(&dtb_hdr, addr + end - start, + sizeof(dtb_hdr), DEV_SHARED); + if (be32_to_cpu(dtb_hdr.magic) == DTB_MAGIC) + info->dtb_paddr = info->zimage.load_addr + info->zimage.len; + } + return 0; }
If a DTB is appended to the DOM0 zImage, it's probably means that the linux decompression stage will replace the DBT register value (r2 on arm32) by the address of the appended DTB. In this case, to avoid issue with Linux, Xen needs to load the new device tree just after the kernel. Signed-off-by: Julien Grall <julien.grall@linaro.org> --- xen/arch/arm/kernel.c | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-)