Re: [XEN v3] xen/arm: Probe the entry point address of an uImage correctly

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

Hi Julien,

I need a clarification.

On 20/12/2022 12:51, Ayan Kumar Halder wrote:
> On 20/12/2022 12:14, Julien Grall wrote:
> > Hi Ayan,
> Hi Julien,
> > On 20/12/2022 10:44, Ayan Kumar Halder wrote:
> > > > > > +
> > > > > > +    /*
> > > > > > +     * Currently, we support uImage headers for uncompressed 
> > > > > > images only.
> > > > > > +     * Thus, it is valid for the load address and start address 
> > > > > > to be the
> > > > > > +     * same. This is consistent with the uboot behavior (Refer
> > > > > > +     * "case IH_COMP_NONE" in image_decomp().
> > > > > Please make it clear that you are referring to uboot function.
> > > > Could we avoid to mention the u-boot function? The reason I am 
> > > > asking is the code is in a different repo and the function name can 
> > > > easily change without us noticing (so the comment would rot).
> > > > Is the behavior documented somewhere in U-boot (or online)? If not, 
> > > > what's the guarantee that it will not change?
> > > I could not find any documentation which states this. I found this 
> > > from the following in uboot source code.
> > > https://source.denx.de/u-boot/u-boot/-/blob/master/boot/image.c#L458
> > >
> > > AFAIU when kernel_uimage_probe() get invoked, the image has already 
> > > been decompressed. So, I added this as a limitation.
> > I e looked at the U-boot code and, AFAIU, the check is merely to 
> > avoid the memcpy() if the image start matches the start of the 
> > decompression area. So I don't understand why we need the limitation 
> > in Xen.
> > Now the lack of documentation (or at least I didn't find any) makes a 
> > bit more tricky to understand what the fields in the U-boot header 
> > are for. I have skimmed through the code and I disagree with the 
> > implementation you chose for Xen.
> > The comment for 'ih_ep' suggests this is the entry point address. 
> > That's may be different from the beginning of your blob. I think this 
> > is what ih_load is for.
> > So I think we want to load the blob at ih_load and then set pc to 
> > point to ih_ep. This will require a bit more work in Xen because the 
> > assumption so far is the entry point matches the start of the blob.
> > Please let me known if I missed anything.
> I think you are correct. I will rework this to correctly handle load 
> address and entry point.
Is it fine to keep these two constraints ?

    /*
     * While uboot considers 0x0 to be a valid load/start address, for Xen
     * to mantain parity with zimage, we consider 0x0 to denote position
     * independent image. That means Xen is free to load such an image at
     * any valid address.
     * Thus, we will print an appropriate message.
     */
    if ( info->zimage.start == 0 )
        printk(XENLOG_INFO
               "No load address provided. Xen will decide where to load 
it.\n");
    /*
     * If the image supports position independent execution, then user 
cannot
     * provide an entry point as Xen will load such an image at any 
appropriate
     * memory address. Thus, we need to return error
     */
    if ( (info->zimage.start == 0) && (info->entry != 0) )
    {
        printk(XENLOG_ERROR
               "Entry point cannot be non zero for PIE image.\n");
        return -EINVAL;
    }

- Ayan

> - Ayan
>
> > Cheers,