Re: [PATCH v3] xen: arm: Update arm64 image header

[Dirk Behme <dirk.behme@gmail.com>]

Hi Julien,

On 23.06.2016 17:18, Julien Grall wrote:
> Hi Dirk,
>
> On 23/06/16 07:38, 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>
>> ---
>>
>> 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, 26 insertions(+), 17 deletions(-)
>>
>> 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 */
>
> I think this comment is irrelevant now.


Yes :) I'll remove that.


>> +        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");
>
> I have found why there were no error messages here before. The
> function kernel_probe will try the different formats supported one by
> one.
>
> So this message will be printed if the kernel is an ARM32 image, which
> will confuse the user. So I would print this message only when
> zimage.magic0 is equal to ZIMAGE64_MAGIC_V0.


Which we don't have with the recent header format any more. This does 
mean I drop this message again, as it doesn't make sense if the magic 
is used for the format detection.


>>           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 ) {
>
> This check is not necessary. "(end - start) > size" will only succeed
> when zimage.image_size is different than 0.


Yes, indeed :) I'll remove that.


>> +            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;


Best regards

Dirk

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