xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] xen: arm: Update arm64 image header
@ 2016-06-21  9:08 Dirk Behme
  2016-06-21 11:14 ` Julien Grall
  0 siblings, 1 reply; 4+ messages in thread
From: Dirk Behme @ 2016-06-21  9:08 UTC (permalink / raw)
  To: xen-devel, Julien Grall, Stefano Stabellini; +Cc: Dirk Behme

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 warn if the kernel Image is larger than the size given in
the device tree, too.

Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
---
 xen/arch/arm/kernel.c | 41 ++++++++++++++++++++++++-----------------
 1 file changed, 24 insertions(+), 17 deletions(-)

diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
index 3f6cce3..1cfaf02 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,28 @@ 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 )
         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 "Check the device tree configuration!\n");
+        }
         return -EINVAL;
+    }
 
     info->zimage.kernel_addr = addr;
     info->zimage.len = end - start;
-- 
2.8.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH] xen: arm: Update arm64 image header
  2016-06-21  9:08 [PATCH] xen: arm: Update arm64 image header Dirk Behme
@ 2016-06-21 11:14 ` Julien Grall
  2016-06-21 11:40   ` Dirk Behme
  0 siblings, 1 reply; 4+ messages in thread
From: Julien Grall @ 2016-06-21 11:14 UTC (permalink / raw)
  To: Dirk Behme, xen-devel, Stefano Stabellini

Hello Dirk,

On 21/06/16 10:08, 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.

Whilst the first magic is gone in the new version of the header, older 
kernel will still use it. So we have to support them.

>
> In case we read a size != 0, let's use this image size, now. This does
> allow us to warn if the kernel Image is larger than the size given in
> the device tree, too.

Based on the code below, you don't warn but return an error.

>
> Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
> ---
>   xen/arch/arm/kernel.c | 41 ++++++++++++++++++++++++-----------------
>   1 file changed, 24 insertions(+), 17 deletions(-)
>
> diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
> index 3f6cce3..1cfaf02 100644
> --- a/xen/arch/arm/kernel.c
> +++ b/xen/arch/arm/kernel.c

[...]

> @@ -354,20 +353,28 @@ 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 )
>           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.

Can you explain what "must be respected" stands for?

>        */
> -    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 "Check the device tree configuration!\n");

This message is not really helpful when using UEFI. In this case, 
multiboot is not used and the kernel image will be loaded by UEFI.
However, the size of the kernel may still mismatch the value in the 
field image_size.

> +        }
>           return -EINVAL;
> +    }
>
>       info->zimage.kernel_addr = addr;
>       info->zimage.len = end - start;
>

Regards,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] xen: arm: Update arm64 image header
  2016-06-21 11:14 ` Julien Grall
@ 2016-06-21 11:40   ` Dirk Behme
  2016-06-21 11:59     ` Julien Grall
  0 siblings, 1 reply; 4+ messages in thread
From: Dirk Behme @ 2016-06-21 11:40 UTC (permalink / raw)
  To: Julien Grall; +Cc: xen-devel, Stefano Stabellini

Hi Julien,

On 21.06.2016 13:14, Julien Grall wrote:
> Hello Dirk,
>
> On 21/06/16 10:08, 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.
>
> Whilst the first magic is gone in the new version of the header, older
> kernel will still use it. So we have to support them.


Hmm, the check for the first magic is dropped. What do you mean with 
"support them"? I.e. if we don't check for it, we support kernel images 
with and without this magic.


>> In case we read a size != 0, let's use this image size, now. This does
>> allow us to warn if the kernel Image is larger than the size given in
>> the device tree, too.
>
> Based on the code below, you don't warn but return an error.


Yes, I should rephrase the commit message here.

In the device tree case, it's correct to return an error, as the system 
doesn't boot if the kernel is larger than the memory reserved for it via 
the device tree. Btw, up to now it failed silently, then.


>> Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
>> ---
>>   xen/arch/arm/kernel.c | 41 ++++++++++++++++++++++++-----------------
>>   1 file changed, 24 insertions(+), 17 deletions(-)
>>
>> diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
>> index 3f6cce3..1cfaf02 100644
>> --- a/xen/arch/arm/kernel.c
>> +++ b/xen/arch/arm/kernel.c
>
> [...]
>
>> @@ -354,20 +353,28 @@ 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 )
>>           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.
>
> Can you explain what "must be respected" stands for?


I copied this comment from

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/Documentation/arm64/booting.txt?id=a2c1d73b94ed49f5fac12e95052d7b140783f800

;)

To my understanding, it wants to say that non-zero image sizes have to 
be used instead of "using as much memory as possible" (?)


>>        */
>> -    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 "Check the device tree configuration!\n");
>
> This message is not really helpful when using UEFI. In this case,
> multiboot is not used and the kernel image will be loaded by UEFI.
> However, the size of the kernel may still mismatch the value in the
> field image_size.


I don't know much about the UEFI boot case. Could you help a little? 
What's needed for that case? Just dropping the "Check the device tree" 
message? Where does size come from in the UEFI case? Is it set similar 
to the device tree case?


>> +        }
>>           return -EINVAL;
>> +    }
>>
>>       info->zimage.kernel_addr = addr;
>>       info->zimage.len = end - start;
>>


Thanks,

Dirk



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] xen: arm: Update arm64 image header
  2016-06-21 11:40   ` Dirk Behme
@ 2016-06-21 11:59     ` Julien Grall
  0 siblings, 0 replies; 4+ messages in thread
From: Julien Grall @ 2016-06-21 11:59 UTC (permalink / raw)
  To: Dirk Behme; +Cc: xen-devel, Stefano Stabellini



On 21/06/16 12:40, Dirk Behme wrote:
> On 21.06.2016 13:14, Julien Grall wrote:
>> On 21/06/16 10:08, 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.
>>
>> Whilst the first magic is gone in the new version of the header, older
>> kernel will still use it. So we have to support them.
>
>
> Hmm, the check for the first magic is dropped. What do you mean with
> "support them"? I.e. if we don't check for it, we support kernel images
> with and without this magic.

Any kernel older than commit 4370eec05a887b0cd4392cd5dc5b2713174745c0 
"arm64: Expand arm64 image header" will not have the second magic (i.e 
ARM\x64). This commit was introduced in Linux 3.12 whilst Xen support 
for ARM64 was added in Linux 3.11.

You can argue that 3.11 will unlikely be used on Xen. However this would 
need to be mentioned in the commit message with maybe an error message 
in Xen.

>>> In case we read a size != 0, let's use this image size, now. This does
>>> allow us to warn if the kernel Image is larger than the size given in
>>> the device tree, too.
>>
>> Based on the code below, you don't warn but return an error.
>
>
> Yes, I should rephrase the commit message here.
>
> In the device tree case, it's correct to return an error, as the system
> doesn't boot if the kernel is larger than the memory reserved for it via
> the device tree. Btw, up to now it failed silently, then.

Currently it will never failed because 'end' is always set to 'size'. 
The check is here in case someone adds a length field in the future (see 
comment above the check).

>
>
>>> Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
>>> ---
>>>   xen/arch/arm/kernel.c | 41 ++++++++++++++++++++++++-----------------
>>>   1 file changed, 24 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
>>> index 3f6cce3..1cfaf02 100644
>>> --- a/xen/arch/arm/kernel.c
>>> +++ b/xen/arch/arm/kernel.c
>>
>> [...]
>>
>>> @@ -354,20 +353,28 @@ 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 )
>>>           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.
>>
>> Can you explain what "must be respected" stands for?
>
>
> I copied this comment from
>
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/Documentation/arm64/booting.txt?id=a2c1d73b94ed49f5fac12e95052d7b140783f800
>
>
> ;)
>
> To my understanding, it wants to say that non-zero image sizes have to
> be used instead of "using as much memory as possible" (?)

Sorry I misread the comment. I thought it was said "zero image_size". So 
I am fine with it.

>
>
>>>        */
>>> -    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 "Check the device tree
>>> configuration!\n");
>>
>> This message is not really helpful when using UEFI. In this case,
>> multiboot is not used and the kernel image will be loaded by UEFI.
>> However, the size of the kernel may still mismatch the value in the
>> field image_size.
>
>
> I don't know much about the UEFI boot case. Could you help a little?
> What's needed for that case? Just dropping the "Check the device tree"
> message? Where does size come from in the UEFI case? Is it set similar
> to the device tree case?

Sorry my comment was not clear. I meant that the error message should be 
more generic. Such as "The field 'size' does not match the size of blob" 
or something similar.

>
>
>>> +        }
>>>           return -EINVAL;
>>> +    }
>>>
>>>       info->zimage.kernel_addr = addr;
>>>       info->zimage.len = end - start;
>>>

Regards,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2016-06-21 11:59 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-21  9:08 [PATCH] xen: arm: Update arm64 image header Dirk Behme
2016-06-21 11:14 ` Julien Grall
2016-06-21 11:40   ` Dirk Behme
2016-06-21 11:59     ` Julien Grall

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).