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

[Julien Grall <julien.grall@xxxxxxx>]

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@xxxxxxxxxxxx>
> ---
>
> 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.

> +        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.
>           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.
> +            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;
Regards,

--
Julien Grall

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