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

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

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.
- Ayan

> Cheers,