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

[Dirk Behme <dirk.behme@xxxxxxxxxxxx>]

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@xxxxxxxxxxxx>
> > ---
> >   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@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel