| Message ID | 1466585959-12362-1-git-send-email-dirk.behme@de.bosch.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jun 22, 2016 at 10:59:19AM +0200, Dirk Behme wrote: > With the Linux kernel commits > > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/Documentation/arm64/booting.txt?id=4370eec05a887b0cd4392cd5dc5b2713174745c0 > > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/Documentation/arm64/booting.txt?id=a2c1d73b94ed49f5fac12e95052d7b140783f800 > > the arm64 image header changed. While the size of the header isn't changed, > some members have changed their usage. > > Update Xen to this updated image header. > > The main changes are that the first magic is gone and that there is an > image size, now. > > In case we read a size != 0, let's use this image size, now. This does > allow us to check if the kernel Image is larger than the size given in > the device tree, too. > > Additionally, add an error message if the magic is not found. This might > be the case with kernel's < 3.12 prior to Don't you want to still check for those kernels and use them? Thanks. > > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=4370eec05a887b0cd4392cd5dc5b2713174745c0
Hi Konrad, On 22/06/16 14:30, Konrad Rzeszutek Wilk wrote: > On Wed, Jun 22, 2016 at 10:59:19AM +0200, Dirk Behme wrote: >> With the Linux kernel commits >> >> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/Documentation/arm64/booting.txt?id=4370eec05a887b0cd4392cd5dc5b2713174745c0 >> >> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/Documentation/arm64/booting.txt?id=a2c1d73b94ed49f5fac12e95052d7b140783f800 >> >> the arm64 image header changed. While the size of the header isn't changed, >> some members have changed their usage. >> >> Update Xen to this updated image header. >> >> The main changes are that the first magic is gone and that there is an >> image size, now. >> >> In case we read a size != 0, let's use this image size, now. This does >> allow us to check if the kernel Image is larger than the size given in >> the device tree, too. >> >> Additionally, add an error message if the magic is not found. This might >> be the case with kernel's < 3.12 prior to > > Don't you want to still check for those kernels and use them? The support of Xen for ARM64 in Linux has been added in Linux 3.11, so only one release will not work with Xen 4.8 and onwards. Given that this is not a long-term support I think it is fine to drop it. Note that 3.12 is an LTS [1]. Stefano what do you think? Dirk, I think your commit message will need more background similar to what I wrote above and in an answer to the previous version. Cheers, [1] https://www.kernel.org/category/releases.html
On 22.06.2016 15:30, Konrad Rzeszutek Wilk wrote: > On Wed, Jun 22, 2016 at 10:59:19AM +0200, Dirk Behme wrote: >> With the Linux kernel commits >> >> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/Documentation/arm64/booting.txt?id=4370eec05a887b0cd4392cd5dc5b2713174745c0 >> >> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/Documentation/arm64/booting.txt?id=a2c1d73b94ed49f5fac12e95052d7b140783f800 >> >> the arm64 image header changed. While the size of the header isn't changed, >> some members have changed their usage. >> >> Update Xen to this updated image header. >> >> The main changes are that the first magic is gone and that there is an >> image size, now. >> >> In case we read a size != 0, let's use this image size, now. This does >> allow us to check if the kernel Image is larger than the size given in >> the device tree, too. >> >> Additionally, add an error message if the magic is not found. This might >> be the case with kernel's < 3.12 prior to > > Don't you want to still check for those kernels and use them? Please check the _existing_ code: It's if ( zimage.magic0 != ZIMAGE64_MAGIC_V0 && zimage.magic1 != ZIMAGE64_MAGIC_V1 ) return -EINVAL; My patch doesn't change anything regarding the fact that if the magics are not valid (due to quite old kernel version) the code does exit with an error. While the review of v1 of this patch, Julien asked for an error message to be added here. Fine. But if there is the request to change the behavior regarding which kernels are supported I'd think that this is independent on this patch and should be done in an additional patch. Best regards Dirk
On Wed, Jun 22, 2016 at 03:44:44PM +0200, Dirk Behme wrote: > On 22.06.2016 15:30, Konrad Rzeszutek Wilk wrote: > >On Wed, Jun 22, 2016 at 10:59:19AM +0200, Dirk Behme wrote: > >>With the Linux kernel commits > >> > >>https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/Documentation/arm64/booting.txt?id=4370eec05a887b0cd4392cd5dc5b2713174745c0 > >> > >>https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/Documentation/arm64/booting.txt?id=a2c1d73b94ed49f5fac12e95052d7b140783f800 > >> > >>the arm64 image header changed. While the size of the header isn't changed, > >>some members have changed their usage. > >> > >>Update Xen to this updated image header. > >> > >>The main changes are that the first magic is gone and that there is an > >>image size, now. > >> > >>In case we read a size != 0, let's use this image size, now. This does > >>allow us to check if the kernel Image is larger than the size given in > >>the device tree, too. > >> > >>Additionally, add an error message if the magic is not found. This might > >>be the case with kernel's < 3.12 prior to > > > >Don't you want to still check for those kernels and use them? > > > Please check the _existing_ code: It's > > > if ( zimage.magic0 != ZIMAGE64_MAGIC_V0 && > zimage.magic1 != ZIMAGE64_MAGIC_V1 ) > return -EINVAL; Oh, indeed! > > > My patch doesn't change anything regarding the fact that if the magics are > not valid (due to quite old kernel version) the code does exit with an > error. /me nods > > While the review of v1 of this patch, Julien asked for an error message to > be added here. Fine. > > But if there is the request to change the behavior regarding which kernels > are supported I'd think that this is independent on this patch and should be > done in an additional patch. Right, and I think it is fine to skip that - because as you say - it already ignores older kernels. Your comment in the description threw me off. Could you kindly update it to say "This does not change the behavior - we had been failing kernels older than 3.12 before - but without any error messages." or such? Thanks!
Hello, On 22/06/16 14:51, Konrad Rzeszutek Wilk wrote: > On Wed, Jun 22, 2016 at 03:44:44PM +0200, Dirk Behme wrote: >> On 22.06.2016 15:30, Konrad Rzeszutek Wilk wrote: >>> On Wed, Jun 22, 2016 at 10:59:19AM +0200, Dirk Behme wrote: >>>> With the Linux kernel commits >>>> >>>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/Documentation/arm64/booting.txt?id=4370eec05a887b0cd4392cd5dc5b2713174745c0 >>>> >>>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/Documentation/arm64/booting.txt?id=a2c1d73b94ed49f5fac12e95052d7b140783f800 >>>> >>>> the arm64 image header changed. While the size of the header isn't changed, >>>> some members have changed their usage. >>>> >>>> Update Xen to this updated image header. >>>> >>>> The main changes are that the first magic is gone and that there is an >>>> image size, now. >>>> >>>> In case we read a size != 0, let's use this image size, now. This does >>>> allow us to check if the kernel Image is larger than the size given in >>>> the device tree, too. >>>> >>>> Additionally, add an error message if the magic is not found. This might >>>> be the case with kernel's < 3.12 prior to >>> >>> Don't you want to still check for those kernels and use them? >> >> >> Please check the _existing_ code: It's >> >> >> if ( zimage.magic0 != ZIMAGE64_MAGIC_V0 && >> zimage.magic1 != ZIMAGE64_MAGIC_V1 ) >> return -EINVAL; It is an "&&" not "||". So it will fail only if none of the 2 magics are present. > Oh, indeed! >> >> >> My patch doesn't change anything regarding the fact that if the magics are >> not valid (due to quite old kernel version) the code does exit with an >> error. > > /me nods >> >> While the review of v1 of this patch, Julien asked for an error message to >> be added here. Fine. >> >> But if there is the request to change the behavior regarding which kernels >> are supported I'd think that this is independent on this patch and should be >> done in an additional patch. > > Right, and I think it is fine to skip that - because as you say - it > already ignores older kernels. This is not right. The current version of Xen is not ignoring older kernels. The current check allows kernels with only MAGIC_V0 (any Linux older than 3.12) to be detected by Xen. However, this is impacting only one version because support for Xen on ARM64 was added in Linux 3.11. Regards,
On 22.06.2016 15:44, Julien Grall wrote: > Hi Konrad, > > On 22/06/16 14:30, Konrad Rzeszutek Wilk wrote: >> On Wed, Jun 22, 2016 at 10:59:19AM +0200, Dirk Behme wrote: >>> With the Linux kernel commits >>> >>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/Documentation/arm64/booting.txt?id=4370eec05a887b0cd4392cd5dc5b2713174745c0 >>> >>> >>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/Documentation/arm64/booting.txt?id=a2c1d73b94ed49f5fac12e95052d7b140783f800 >>> >>> >>> the arm64 image header changed. While the size of the header isn't >>> changed, >>> some members have changed their usage. >>> >>> Update Xen to this updated image header. >>> >>> The main changes are that the first magic is gone and that there is an >>> image size, now. >>> >>> In case we read a size != 0, let's use this image size, now. This does >>> allow us to check if the kernel Image is larger than the size given in >>> the device tree, too. >>> >>> Additionally, add an error message if the magic is not found. This might >>> be the case with kernel's < 3.12 prior to >> >> Don't you want to still check for those kernels and use them? > > The support of Xen for ARM64 in Linux has been added in Linux 3.11, so > only one release will not work with Xen 4.8 and onwards. Given that this > is not a long-term support I think it is fine to drop it. Note that 3.12 > is an LTS [1]. Stefano what do you think? > > Dirk, I think your commit message will need more background similar to > what I wrote above and in an answer to the previous version. Yes, thanks, I'll update the commit message in a v3. Trying to not forget Konrad's proposal, too ;) Best regards Dirk
> >>Please check the _existing_ code: It's > >> > >> > >>if ( zimage.magic0 != ZIMAGE64_MAGIC_V0 && > >> zimage.magic1 != ZIMAGE64_MAGIC_V1 ) > >> return -EINVAL; > > It is an "&&" not "||". So it will fail only if none of the 2 magics are > present. > Argh. Brainfart! > >Oh, indeed! > >> > >> > >>My patch doesn't change anything regarding the fact that if the magics are > >>not valid (due to quite old kernel version) the code does exit with an > >>error. > > > >/me nods > >> > >>While the review of v1 of this patch, Julien asked for an error message to > >>be added here. Fine. > >> > >>But if there is the request to change the behavior regarding which kernels > >>are supported I'd think that this is independent on this patch and should be > >>done in an additional patch. > > > >Right, and I think it is fine to skip that - because as you say - it > >already ignores older kernels. > > This is not right. The current version of Xen is not ignoring older kernels. > > The current check allows kernels with only MAGIC_V0 (any Linux older than > 3.12) to be detected by Xen. > > However, this is impacting only one version because support for Xen on ARM64 > was added in Linux 3.11. And you ARM maintainers are OK ditching that support? It probably also means updating some Wiki page and docs.
Hi Konrad, On 22/06/16 15:29, Konrad Rzeszutek Wilk wrote: >>> Oh, indeed! >>>> >>>> >>>> My patch doesn't change anything regarding the fact that if the magics are >>>> not valid (due to quite old kernel version) the code does exit with an >>>> error. >>> >>> /me nods >>>> >>>> While the review of v1 of this patch, Julien asked for an error message to >>>> be added here. Fine. >>>> >>>> But if there is the request to change the behavior regarding which kernels >>>> are supported I'd think that this is independent on this patch and should be >>>> done in an additional patch. >>> >>> Right, and I think it is fine to skip that - because as you say - it >>> already ignores older kernels. >> >> This is not right. The current version of Xen is not ignoring older kernels. >> >> The current check allows kernels with only MAGIC_V0 (any Linux older than >> 3.12) to be detected by Xen. >> >> However, this is impacting only one version because support for Xen on ARM64 >> was added in Linux 3.11. > > And you ARM maintainers are OK ditching that support? The number of board supported by Linux 3.11 on ARM64 is very limited: ARM models and X-gene. For the latter it was an early support with only the serial and timer upstreamed (see [1]). So personally I am fine with that. Any opinions? > > It probably also means updating some Wiki page and docs. Good point. Although we don't seem to mention the first Linux version supported for ARM64 [2]. Cheers, [1] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/apm-storm.dtsi?h=v3.11 [2] http://wiki.xenproject.org/wiki/Xen_ARM_with_Virtualization_Extensions#Requirements
On Wed, 22 Jun 2016, Julien Grall wrote: > Hi Konrad, > > On 22/06/16 15:29, Konrad Rzeszutek Wilk wrote: > > > > Oh, indeed! > > > > > > > > > > > > > > > My patch doesn't change anything regarding the fact that if the magics > > > > > are > > > > > not valid (due to quite old kernel version) the code does exit with an > > > > > error. > > > > > > > > /me nods > > > > > > > > > > While the review of v1 of this patch, Julien asked for an error > > > > > message to > > > > > be added here. Fine. > > > > > > > > > > But if there is the request to change the behavior regarding which > > > > > kernels > > > > > are supported I'd think that this is independent on this patch and > > > > > should be > > > > > done in an additional patch. > > > > > > > > Right, and I think it is fine to skip that - because as you say - it > > > > already ignores older kernels. > > > > > > This is not right. The current version of Xen is not ignoring older > > > kernels. > > > > > > The current check allows kernels with only MAGIC_V0 (any Linux older than > > > 3.12) to be detected by Xen. > > > > > > However, this is impacting only one version because support for Xen on > > > ARM64 > > > was added in Linux 3.11. > > > > And you ARM maintainers are OK ditching that support? > > The number of board supported by Linux 3.11 on ARM64 is very limited: ARM > models and X-gene. > > For the latter it was an early support with only the serial and timer > upstreamed (see [1]). > > So personally I am fine with that. Any opinions? I agree with that
diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c index 9871bd9..9b9a793 100644 --- a/xen/arch/arm/kernel.c +++ b/xen/arch/arm/kernel.c @@ -28,8 +28,7 @@ #define ZIMAGE32_MAGIC 0x016f2818 -#define ZIMAGE64_MAGIC_V0 0x14000008 -#define ZIMAGE64_MAGIC_V1 0x644d5241 /* "ARM\x64" */ +#define ZIMAGE64_MAGIC 0x644d5241 /* "ARM\x64" */ struct minimal_dtb_header { uint32_t magic; @@ -335,17 +334,17 @@ static int kernel_zimage64_probe(struct kernel_info *info, { /* linux/Documentation/arm64/booting.txt */ struct { - uint32_t magic0; - uint32_t res0; - uint64_t text_offset; /* Image load offset */ - uint64_t res1; - uint64_t res2; + uint32_t code0; + uint32_t code1; + uint64_t text_offset; /* Image load offset, little endian */ + uint64_t image_size; /* Effective Image size, little endian */ + uint64_t flags; /* zImage V1 only from here */ + uint64_t res2; uint64_t res3; uint64_t res4; - uint64_t res5; - uint32_t magic1; - uint32_t res6; + uint32_t magic; /* Magic number, little endian, "ARM\x64" */ + uint32_t res5; } zimage; uint64_t start, end; @@ -354,20 +353,30 @@ static int kernel_zimage64_probe(struct kernel_info *info, copy_from_paddr(&zimage, addr, sizeof(zimage)); - if ( zimage.magic0 != ZIMAGE64_MAGIC_V0 && - zimage.magic1 != ZIMAGE64_MAGIC_V1 ) + if ( zimage.magic != ZIMAGE64_MAGIC ) { + printk(XENLOG_ERR "No valid magic found in header! Kernel too old?\n"); return -EINVAL; + } - /* Currently there is no length in the header, so just use the size */ start = 0; - end = size; /* - * Given the above this check is a bit pointless, but leave it - * here in case someone adds a length field in the future. + * Where image_size is non-zero image_size is little-endian + * and must be respected. */ - if ( (end - start) > size ) + if ( zimage.image_size ) + end = zimage.image_size; + else + end = size; + + if ( (end - start) > size ) { + if ( zimage.image_size ) { + printk(XENLOG_ERR "Error: Kernel Image size: %lu bytes > bootmodule size: %lu bytes\n", + zimage.image_size, (uint64_t)size); + printk(XENLOG_ERR "The field 'size' does not match the size of blob!\n"); + } return -EINVAL; + } info->zimage.kernel_addr = addr; info->zimage.len = end - start;
With the Linux kernel commits https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/Documentation/arm64/booting.txt?id=4370eec05a887b0cd4392cd5dc5b2713174745c0 https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/Documentation/arm64/booting.txt?id=a2c1d73b94ed49f5fac12e95052d7b140783f800 the arm64 image header changed. While the size of the header isn't changed, some members have changed their usage. Update Xen to this updated image header. The main changes are that the first magic is gone and that there is an image size, now. In case we read a size != 0, let's use this image size, now. This does allow us to check if the kernel Image is larger than the size given in the device tree, too. Additionally, add an error message if the magic is not found. This might be the case with kernel's < 3.12 prior to https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=4370eec05a887b0cd4392cd5dc5b2713174745c0 Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com> --- xen/arch/arm/kernel.c | 43 ++++++++++++++++++++++++++----------------- 1 file changed, 26 insertions(+), 17 deletions(-)