* 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: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
* 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