xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v4] xen: arm: Update arm64 image header
@ 2016-06-27  7:53 Dirk Behme
  2016-06-28 10:18 ` Julien Grall
  2016-06-29 10:32 ` Julien Grall
  0 siblings, 2 replies; 12+ messages in thread
From: Dirk Behme @ 2016-06-27  7:53 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 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

which introduced the second magic.

This is acceptable as the support of Xen for ARM64 in Linux has been added
in Linux 3.11 and the number of boards supported by Linux 3.11 on ARM64 is
very limited: ARM models and X-gene. And for the latter it was an early
support with only the serial and timer upstreamed.

Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
---

Changes in v4: Print header error only based on ZIMAGE64_MAGIC_V0. Drop comment
               and unnecessary if statement.

Changes in v3: Just update of the commit message regarding the support
               for kernels < 3.12. No change to the patch itself.

 xen/arch/arm/kernel.c | 43 +++++++++++++++++++++++++++----------------
 1 file changed, 27 insertions(+), 16 deletions(-)

diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
index 9871bd9..1797097 100644
--- a/xen/arch/arm/kernel.c
+++ b/xen/arch/arm/kernel.c
@@ -29,7 +29,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 +335,19 @@ 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;
+        union {
+                uint32_t code0;
+                uint32_t magic0; /* Old header magic */
+        };
+        uint32_t code1;
+        uint64_t text_offset;  /* Image load offset, little endian */
+        uint64_t image_size;   /* Effective Image size, little endian */
+        uint64_t flags;
         uint64_t res2;
-        /* zImage V1 only from here */
         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 +356,29 @@ 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 ) {
+        if ( zimage.magic0 == ZIMAGE64_MAGIC_V0 )
+             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 ) {
+        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;
-- 
2.8.0


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

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

* Re: [PATCH v4] xen: arm: Update arm64 image header
  2016-06-27  7:53 [PATCH v4] xen: arm: Update arm64 image header Dirk Behme
@ 2016-06-28 10:18 ` Julien Grall
  2016-06-28 18:33   ` Andrew Cooper
  2016-06-29 10:32 ` Julien Grall
  1 sibling, 1 reply; 12+ messages in thread
From: Julien Grall @ 2016-06-28 10:18 UTC (permalink / raw)
  To: Dirk Behme, xen-devel, Stefano Stabellini

Hi Dirk,

On 27/06/16 08:53, 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
>
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=4370eec05a887b0cd4392cd5dc5b2713174745c0
>
> which introduced the second magic.
>
> This is acceptable as the support of Xen for ARM64 in Linux has been added
> in Linux 3.11 and the number of boards supported by Linux 3.11 on ARM64 is
> very limited: ARM models and X-gene. And for the latter it was an early
> support with only the serial and timer upstreamed.
>
> Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>

Reviewed-by: Julien Grall <julien.grall@arm.com>

Regards,

-- 
Julien Grall

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

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

* Re: [PATCH v4] xen: arm: Update arm64 image header
  2016-06-28 10:18 ` Julien Grall
@ 2016-06-28 18:33   ` Andrew Cooper
  0 siblings, 0 replies; 12+ messages in thread
From: Andrew Cooper @ 2016-06-28 18:33 UTC (permalink / raw)
  To: Julien Grall, Dirk Behme, xen-devel, Stefano Stabellini

On 28/06/16 11:18, Julien Grall wrote:
>
>> This is acceptable as the support of Xen for ARM64 in Linux has been
>> added
>> in Linux 3.11 and the number of boards supported by Linux 3.11 on
>> ARM64 is
>> very limited: ARM models and X-gene. And for the latter it was an early
>> support with only the serial and timer upstreamed.
>>
>> Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
>
> Reviewed-by: Julien Grall <julien.grall@arm.com>
>
> Regards,
>

Committed, thanks.

~Andrew

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

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

* Re: [PATCH v4] xen: arm: Update arm64 image header
  2016-06-27  7:53 [PATCH v4] xen: arm: Update arm64 image header Dirk Behme
  2016-06-28 10:18 ` Julien Grall
@ 2016-06-29 10:32 ` Julien Grall
  2016-06-29 11:08   ` Dirk Behme
  1 sibling, 1 reply; 12+ messages in thread
From: Julien Grall @ 2016-06-29 10:32 UTC (permalink / raw)
  To: Dirk Behme, xen-devel, Stefano Stabellini, Andrew Cooper, Andre Przywara

Hi Dirk,

On 27/06/2016 08:53, Dirk Behme wrote:
> +    if ( (end - start) > 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;
> +    }

This patch is breaking boot on any ARM64 platform (UEFI and 
bootwrapper). The 'image_size' is the size of the kernel with BSS 
included. So the bootmodule will always be smaller than 'image_size'.

(XEN) Loading kernel from boot module @ 0000000080080000
(XEN) Error: Kernel Image size: 16482304 bytes > bootmodule size: 
15925760bytes
(XEN) The field 'size' does not match the size of blob!

So I think this field is intended to be used to tell the bootloader that 
kernel footprint will "image_size". So the bootloader will not attempt 
to place any modules (DT, ramdisk...) within this region.

We don't want to use this field to copy the kernel because we may end up 
to copy extra memory which could contain sensitive data.

Given that the main purpose of this patch is to use the field 
'image_size', I would just revert the patch until we figured out what to do.

Dirk, as this patch is meant to be ARM64 only, I am wondering how you 
were able to boot DOM0 with the patch applied.

Currently the pushgate for ARM64 is only build testing, so I should have 
tested this patch before giving my ack. I am sorry for that.

Regards,

-- 
Julien Grall

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

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

* Re: [PATCH v4] xen: arm: Update arm64 image header
  2016-06-29 10:32 ` Julien Grall
@ 2016-06-29 11:08   ` Dirk Behme
  2016-06-29 11:22     ` Dirk Behme
                       ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Dirk Behme @ 2016-06-29 11:08 UTC (permalink / raw)
  To: Julien Grall, xen-devel, Stefano Stabellini, Andrew Cooper,
	Andre Przywara

Hi Julien,

On 29.06.2016 12:32, Julien Grall wrote:
> Hi Dirk,
>
> On 27/06/2016 08:53, Dirk Behme wrote:
>> +    if ( (end - start) > 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;
>> +    }
>
> This patch is breaking boot on any ARM64 platform (UEFI and
> bootwrapper).


Well, I wonder if it breaks because the kernel is too large? As intended?

You are the expert on this, so I just can give my (limited?) understanding:

In my use case, starting with the Xen development and not really knowing 
the details, taking a random example from the net I had configured 10MB 
in the device tree:

module@0x48200000 {
        compatible = "xen,linux-zimage", "xen,multiboot-module";
        reg = <0x48200000 0x00A00000>; /* Max Image size 10MB */
};

This failed silently. No error message.

Without knowing any details, my first workaround was to make the kernel 
smaller. Having a kernel Image smaller than 10MB worked, then.

While debugging it, I found that these 0x00A00000 are used for 'size'. 
And increasing it to 0x00F00000 (15MB) does work for me, now.

I don't know anything aobut UEFI and bootwrapper, but could you check 
the size given for 'size' and the real size of the kernel Image? What's 
about if you increase 'size'?

Yes, this check is there just to avoid the silent failing I observed. If 
we have the error message, as I have implemented it, it would have saved 
some debugging time for me ;) So it's not about using the size for real 
loading, its just used for checking.

A short term workaround would be to convert the ERR into WARN and remove 
the return.

It's somehow my feeling that there might be an issue regarding the sizes 
if the warning is there.

But maybe I'm totally wrong ;)

Best regards

Dirk

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

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

* Re: [PATCH v4] xen: arm: Update arm64 image header
  2016-06-29 11:08   ` Dirk Behme
@ 2016-06-29 11:22     ` Dirk Behme
  2016-06-29 11:31     ` Dirk Behme
  2016-06-29 11:33     ` Julien Grall
  2 siblings, 0 replies; 12+ messages in thread
From: Dirk Behme @ 2016-06-29 11:22 UTC (permalink / raw)
  To: Julien Grall, xen-devel, Stefano Stabellini, Andrew Cooper,
	Andre Przywara

On 29.06.2016 13:08, Dirk Behme wrote:
> Hi Julien,
>
> On 29.06.2016 12:32, Julien Grall wrote:
>> Hi Dirk,
>>
>> On 27/06/2016 08:53, Dirk Behme wrote:
>>> +    if ( (end - start) > 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;
>>> +    }
>>
>> This patch is breaking boot on any ARM64 platform (UEFI and
>> bootwrapper).
>
>
> Well, I wonder if it breaks because the kernel is too large? As intended?
>
> You are the expert on this, so I just can give my (limited?) understanding:
>
> In my use case, starting with the Xen development and not really knowing
> the details, taking a random example from the net I had configured 10MB
> in the device tree:
>
> module@0x48200000 {
>        compatible = "xen,linux-zimage", "xen,multiboot-module";
>        reg = <0x48200000 0x00A00000>; /* Max Image size 10MB */
> };
>
> This failed silently. No error message.
>
> Without knowing any details, my first workaround was to make the kernel
> smaller. Having a kernel Image smaller than 10MB worked, then.
>
> While debugging it, I found that these 0x00A00000 are used for 'size'.
> And increasing it to 0x00F00000 (15MB) does work for me, now.
>
> I don't know anything aobut UEFI and bootwrapper, but could you check
> the size given for 'size' and the real size of the kernel Image? What's
> about if you increase 'size'?
>
> Yes, this check is there just to avoid the silent failing I observed. If
> we have the error message, as I have implemented it, it would have saved
> some debugging time for me ;) So it's not about using the size for real
> loading, its just used for checking.
>
> A short term workaround would be to convert the ERR into WARN and remove
> the return.
>
> It's somehow my feeling that there might be an issue regarding the sizes
> if the warning is there.


Just fyi, with

13517824 arch/arm64/boot/Image

and a quick printk I get

(XEN) -> image_size: 13807616 bytes

So, yes, image_size is ~300k larger than the file size. Yes, that might 
be due to the BSS.

Best regards

Dirk


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

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

* Re: [PATCH v4] xen: arm: Update arm64 image header
  2016-06-29 11:08   ` Dirk Behme
  2016-06-29 11:22     ` Dirk Behme
@ 2016-06-29 11:31     ` Dirk Behme
  2016-06-29 11:47       ` Julien Grall
  2016-06-29 11:33     ` Julien Grall
  2 siblings, 1 reply; 12+ messages in thread
From: Dirk Behme @ 2016-06-29 11:31 UTC (permalink / raw)
  To: Julien Grall, xen-devel, Stefano Stabellini, Andrew Cooper,
	Andre Przywara

On 29.06.2016 13:08, Dirk Behme wrote:
> Hi Julien,
>
> On 29.06.2016 12:32, Julien Grall wrote:
>> Hi Dirk,
>>
>> On 27/06/2016 08:53, Dirk Behme wrote:
>>> +    if ( (end - start) > 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;
>>> +    }
>>
>> This patch is breaking boot on any ARM64 platform (UEFI and
>> bootwrapper).
>
>
> Well, I wonder if it breaks because the kernel is too large? As intended?
>
> You are the expert on this, so I just can give my (limited?) understanding:
>
> In my use case, starting with the Xen development and not really knowing
> the details, taking a random example from the net I had configured 10MB
> in the device tree:
>
> module@0x48200000 {
>        compatible = "xen,linux-zimage", "xen,multiboot-module";
>        reg = <0x48200000 0x00A00000>; /* Max Image size 10MB */
> };
>
> This failed silently. No error message.
>
> Without knowing any details, my first workaround was to make the kernel
> smaller. Having a kernel Image smaller than 10MB worked, then.
>
> While debugging it, I found that these 0x00A00000 are used for 'size'.
> And increasing it to 0x00F00000 (15MB) does work for me, now.
>
> I don't know anything aobut UEFI and bootwrapper,


Just a question: In case of UEFI and bootwrapper, does Xen know the 
exact real file size of the Image file?

Then that's the difference to the device tree case, where we don't know 
the Image file size and use a max size estimation from the device tree.

Then we should limit the image_size error message to the device tree 
case only. Ignoring the BSS overhead issue, as the size given by the 
device tree is an estimation, anyhow.

Best regards

Dirk

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

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

* Re: [PATCH v4] xen: arm: Update arm64 image header
  2016-06-29 11:08   ` Dirk Behme
  2016-06-29 11:22     ` Dirk Behme
  2016-06-29 11:31     ` Dirk Behme
@ 2016-06-29 11:33     ` Julien Grall
  2016-06-29 11:38       ` Dirk Behme
  2 siblings, 1 reply; 12+ messages in thread
From: Julien Grall @ 2016-06-29 11:33 UTC (permalink / raw)
  To: Dirk Behme, xen-devel, Stefano Stabellini, Andrew Cooper, Andre Przywara



On 29/06/2016 12:08, Dirk Behme wrote:
> On 29.06.2016 12:32, Julien Grall wrote:
>> Hi Dirk,
>>
>> On 27/06/2016 08:53, Dirk Behme wrote:
>>> +    if ( (end - start) > 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;
>>> +    }
>>
>> This patch is breaking boot on any ARM64 platform (UEFI and
>> bootwrapper).
>
>
> Well, I wonder if it breaks because the kernel is too large? As intended?

Please read my previous e-mail, I gave an explanation why it breaks on 
UEFI/bootwrapper.

To summarize, the field 'image_size' is the total size of the kernel in 
memory BSS included. The section BSS is not included in the blob because 
it is all zeros and it is a waste of disk space.

>
> You are the expert on this, so I just can give my (limited?) understanding:
>
> In my use case, starting with the Xen development and not really knowing
> the details, taking a random example from the net I had configured 10MB
> in the device tree:
>
> module@0x48200000 {
>        compatible = "xen,linux-zimage", "xen,multiboot-module";
>        reg = <0x48200000 0x00A00000>; /* Max Image size 10MB */
> };
>
> This failed silently. No error message.
>
> Without knowing any details, my first workaround was to make the kernel
> smaller. Having a kernel Image smaller than 10MB worked, then.
>
> While debugging it, I found that these 0x00A00000 are used for 'size'.
> And increasing it to 0x00F00000 (15MB) does work for me, now.
>
> I don't know anything aobut UEFI and bootwrapper, but could you check
> the size given for 'size' and the real size of the kernel Image? What's
> about if you increase 'size'?

(XEN) Loading kernel from boot module @ 0000000080080000
(XEN) Error: Kernel Image size: 16482304 bytes > bootmodule size: 
15925760bytes
(XEN) The field 'size' does not match the size of blob!

For both UEFI and Bootwrapper, the bootmodule is created automatically 
and the size of the Image on the disk (e.g BSS not included) is used.

>
> Yes, this check is there just to avoid the silent failing I observed. If
> we have the error message, as I have implemented it, it would have saved
> some debugging time for me ;) So it's not about using the size for real
> loading, its just used for checking.
>
> A short term workaround would be to convert the ERR into WARN and remove
> the return.

This warning will always be printed for all the platform where the size 
is retrieved from the firmware (e.g UEFI, GRUB).

As mentioned in my previous mail, we should not copy more than the size 
of the bootmodule. Otherwise we may copy sensitive data in DOM0.

> It's somehow my feeling that there might be an issue regarding the sizes
> if the warning is there.

No, there is no issue. We misinterpreted the meaning of the field 
'image_size'. In the case of Xen, the size should only be used for 
placing the module.

Regards,

-- 
Julien Grall

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

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

* Re: [PATCH v4] xen: arm: Update arm64 image header
  2016-06-29 11:33     ` Julien Grall
@ 2016-06-29 11:38       ` Dirk Behme
  0 siblings, 0 replies; 12+ messages in thread
From: Dirk Behme @ 2016-06-29 11:38 UTC (permalink / raw)
  To: Julien Grall, xen-devel, Stefano Stabellini, Andrew Cooper,
	Andre Przywara

Hi Julien,

On 29.06.2016 13:33, Julien Grall wrote:
>
>
> On 29/06/2016 12:08, Dirk Behme wrote:
>> On 29.06.2016 12:32, Julien Grall wrote:
>>> Hi Dirk,
>>>
>>> On 27/06/2016 08:53, Dirk Behme wrote:
>>>> +    if ( (end - start) > 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;
>>>> +    }
>>>
>>> This patch is breaking boot on any ARM64 platform (UEFI and
>>> bootwrapper).
>>
>>
>> Well, I wonder if it breaks because the kernel is too large? As intended?
>
> Please read my previous e-mail, I gave an explanation why it breaks on
> UEFI/bootwrapper.
>
> To summarize, the field 'image_size' is the total size of the kernel in
> memory BSS included. The section BSS is not included in the blob because
> it is all zeros and it is a waste of disk space.
>
>>
>> You are the expert on this, so I just can give my (limited?)
>> understanding:
>>
>> In my use case, starting with the Xen development and not really knowing
>> the details, taking a random example from the net I had configured 10MB
>> in the device tree:
>>
>> module@0x48200000 {
>>        compatible = "xen,linux-zimage", "xen,multiboot-module";
>>        reg = <0x48200000 0x00A00000>; /* Max Image size 10MB */
>> };
>>
>> This failed silently. No error message.
>>
>> Without knowing any details, my first workaround was to make the kernel
>> smaller. Having a kernel Image smaller than 10MB worked, then.
>>
>> While debugging it, I found that these 0x00A00000 are used for 'size'.
>> And increasing it to 0x00F00000 (15MB) does work for me, now.
>>
>> I don't know anything aobut UEFI and bootwrapper, but could you check
>> the size given for 'size' and the real size of the kernel Image? What's
>> about if you increase 'size'?
>
> (XEN) Loading kernel from boot module @ 0000000080080000
> (XEN) Error: Kernel Image size: 16482304 bytes > bootmodule size:
> 15925760bytes
> (XEN) The field 'size' does not match the size of blob!
>
> For both UEFI and Bootwrapper, the bootmodule is created automatically
> and the size of the Image on the disk (e.g BSS not included) is used.



Ok, yes, that answers my question from the previous mail. Thanks!


>> Yes, this check is there just to avoid the silent failing I observed. If
>> we have the error message, as I have implemented it, it would have saved
>> some debugging time for me ;) So it's not about using the size for real
>> loading, its just used for checking.
>>
>> A short term workaround would be to convert the ERR into WARN and remove
>> the return.
>
> This warning will always be printed for all the platform where the size
> is retrieved from the firmware (e.g UEFI, GRUB).
>
> As mentioned in my previous mail, we should not copy more than the size
> of the bootmodule. Otherwise we may copy sensitive data in DOM0.
>
>> It's somehow my feeling that there might be an issue regarding the sizes
>> if the warning is there.
>
> No, there is no issue. We misinterpreted the meaning of the field
> 'image_size'. In the case of Xen, the size should only be used for
> placing the module.


Yes.

Then the device tree case I like to cover is special.

Would it be the solution to add an additional

if ( device_tree )

and maybe convert it to a warning?

Best regards

Dirk

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

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

* Re: [PATCH v4] xen: arm: Update arm64 image header
  2016-06-29 11:31     ` Dirk Behme
@ 2016-06-29 11:47       ` Julien Grall
  2016-06-29 11:55         ` Dirk Behme
  0 siblings, 1 reply; 12+ messages in thread
From: Julien Grall @ 2016-06-29 11:47 UTC (permalink / raw)
  To: Dirk Behme, xen-devel, Stefano Stabellini, Andrew Cooper, Andre Przywara



On 29/06/2016 12:31, Dirk Behme wrote:
> On 29.06.2016 13:08, Dirk Behme wrote:
>> Hi Julien,
>>
>> On 29.06.2016 12:32, Julien Grall wrote:
>>> Hi Dirk,
>>>
>>> On 27/06/2016 08:53, Dirk Behme wrote:
>>>> +    if ( (end - start) > 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;
>>>> +    }
>>>
>>> This patch is breaking boot on any ARM64 platform (UEFI and
>>> bootwrapper).
>>
>>
>> Well, I wonder if it breaks because the kernel is too large? As intended?
>>
>> You are the expert on this, so I just can give my (limited?)
>> understanding:
>>
>> In my use case, starting with the Xen development and not really knowing
>> the details, taking a random example from the net I had configured 10MB
>> in the device tree:
>>
>> module@0x48200000 {
>>        compatible = "xen,linux-zimage", "xen,multiboot-module";
>>        reg = <0x48200000 0x00A00000>; /* Max Image size 10MB */
>> };
>>
>> This failed silently. No error message.
>>
>> Without knowing any details, my first workaround was to make the kernel
>> smaller. Having a kernel Image smaller than 10MB worked, then.
>>
>> While debugging it, I found that these 0x00A00000 are used for 'size'.
>> And increasing it to 0x00F00000 (15MB) does work for me, now.
>>
>> I don't know anything aobut UEFI and bootwrapper,
>
>
> Just a question: In case of UEFI and bootwrapper, does Xen know the
> exact real file size of the Image file?

Yes.

>
> Then that's the difference to the device tree case, where we don't know
> the Image file size and use a max size estimation from the device tree.

What do you mean by "device tree case"? UEFI and bootwrapper are device 
tree based. The latter will create the node in the device tree with the 
correct size, whilst the former will directly fill Xen internal 
structure (see arch/arm/efi).

>
> Then we should limit the image_size error message to the device tree
> case only. Ignoring the BSS overhead issue, as the size given by the
> device tree is an estimation, anyhow.

Which firmware are you using? If you use u-boot, there are runes to 
create the proper device-tree node on the wiki [1].

To be honest, the device-tree bindings does not mention any estimation 
(see [2]). I have noticed that some pages on the wiki use hardcoded DT 
(see [3]). So the documentation needs to be fixed.

Regards,

[1] 
http://wiki.xenproject.org/wiki/Xen_ARM_with_Virtualization_Extensions#Boot_Modules
[2] 
http://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=docs/misc/arm/device-tree/booting.txt;h=8da1e0b8fcf9c98888ed63cd45bd11f1a880288b;hb=HEAD
[3] 
http://wiki.xenproject.org/wiki/Xen_ARM_with_Virtualization_Extensions/Lager

-- 
Julien Grall

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

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

* Re: [PATCH v4] xen: arm: Update arm64 image header
  2016-06-29 11:47       ` Julien Grall
@ 2016-06-29 11:55         ` Dirk Behme
  2016-06-29 12:08           ` Julien Grall
  0 siblings, 1 reply; 12+ messages in thread
From: Dirk Behme @ 2016-06-29 11:55 UTC (permalink / raw)
  To: Julien Grall, xen-devel, Stefano Stabellini, Andrew Cooper,
	Andre Przywara

On 29.06.2016 13:47, Julien Grall wrote:
>
>
> On 29/06/2016 12:31, Dirk Behme wrote:
>> On 29.06.2016 13:08, Dirk Behme wrote:
>>> Hi Julien,
>>>
>>> On 29.06.2016 12:32, Julien Grall wrote:
>>>> Hi Dirk,
>>>>
>>>> On 27/06/2016 08:53, Dirk Behme wrote:
>>>>> +    if ( (end - start) > 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;
>>>>> +    }
>>>>
>>>> This patch is breaking boot on any ARM64 platform (UEFI and
>>>> bootwrapper).
>>>
>>>
>>> Well, I wonder if it breaks because the kernel is too large? As
>>> intended?
>>>
>>> You are the expert on this, so I just can give my (limited?)
>>> understanding:
>>>
>>> In my use case, starting with the Xen development and not really knowing
>>> the details, taking a random example from the net I had configured 10MB
>>> in the device tree:
>>>
>>> module@0x48200000 {
>>>        compatible = "xen,linux-zimage", "xen,multiboot-module";
>>>        reg = <0x48200000 0x00A00000>; /* Max Image size 10MB */
>>> };
>>>
>>> This failed silently. No error message.
>>>
>>> Without knowing any details, my first workaround was to make the kernel
>>> smaller. Having a kernel Image smaller than 10MB worked, then.
>>>
>>> While debugging it, I found that these 0x00A00000 are used for 'size'.
>>> And increasing it to 0x00F00000 (15MB) does work for me, now.
>>>
>>> I don't know anything aobut UEFI and bootwrapper,
>>
>>
>> Just a question: In case of UEFI and bootwrapper, does Xen know the
>> exact real file size of the Image file?
>
> Yes.
>
>>
>> Then that's the difference to the device tree case, where we don't know
>> the Image file size and use a max size estimation from the device tree.
>
> What do you mean by "device tree case"? UEFI and bootwrapper are device
> tree based. The latter will create the node in the device tree with the
> correct size, whilst the former will directly fill Xen internal
> structure (see arch/arm/efi).
>
>>
>> Then we should limit the image_size error message to the device tree
>> case only. Ignoring the BSS overhead issue, as the size given by the
>> device tree is an estimation, anyhow.
>
> Which firmware are you using? If you use u-boot, there are runes to
> create the proper device-tree node on the wiki [1].


I'm trying to get it working with ATF. Therefore I think I used the hard 
coded example from [3].


> To be honest, the device-tree bindings does not mention any estimation
> (see [2]). I have noticed that some pages on the wiki use hardcoded DT
> (see [3]). So the documentation needs to be fixed.


Which ignores the fact that it silently fails if the sizes don't match ;)

The only thing I'm looking for is a helpful warning message :)


> Regards,
>
> [1]
> http://wiki.xenproject.org/wiki/Xen_ARM_with_Virtualization_Extensions#Boot_Modules
>
> [2]
> http://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=docs/misc/arm/device-tree/booting.txt;h=8da1e0b8fcf9c98888ed63cd45bd11f1a880288b;hb=HEAD
>
> [3]
> http://wiki.xenproject.org/wiki/Xen_ARM_with_Virtualization_Extensions/Lager


Best regards

Dirk



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

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

* Re: [PATCH v4] xen: arm: Update arm64 image header
  2016-06-29 11:55         ` Dirk Behme
@ 2016-06-29 12:08           ` Julien Grall
  0 siblings, 0 replies; 12+ messages in thread
From: Julien Grall @ 2016-06-29 12:08 UTC (permalink / raw)
  To: Dirk Behme, xen-devel, Stefano Stabellini, Andrew Cooper, Andre Przywara



On 29/06/2016 12:55, Dirk Behme wrote:
> On 29.06.2016 13:47, Julien Grall wrote:
>> To be honest, the device-tree bindings does not mention any estimation
>> (see [2]). I have noticed that some pages on the wiki use hardcoded DT
>> (see [3]). So the documentation needs to be fixed.
>
>
> Which ignores the fact that it silently fails if the sizes don't match ;)
> The only thing I'm looking for is a helpful warning message :)

Whilst I am usually happy to see helpful warning message. I don't want 
to see a warning message when the behavior is actually valid.

There is no way to know whether the device tree has been crafted by the 
developer or generated by the firmware.

So the only solution is to update the documentation about the 
device-tree bindings and hope that people will read the doc starting to 
boot Xen.

Regards,

-- 
Julien Grall

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

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

end of thread, other threads:[~2016-06-29 12:08 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-27  7:53 [PATCH v4] xen: arm: Update arm64 image header Dirk Behme
2016-06-28 10:18 ` Julien Grall
2016-06-28 18:33   ` Andrew Cooper
2016-06-29 10:32 ` Julien Grall
2016-06-29 11:08   ` Dirk Behme
2016-06-29 11:22     ` Dirk Behme
2016-06-29 11:31     ` Dirk Behme
2016-06-29 11:47       ` Julien Grall
2016-06-29 11:55         ` Dirk Behme
2016-06-29 12:08           ` Julien Grall
2016-06-29 11:33     ` Julien Grall
2016-06-29 11:38       ` Dirk Behme

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).