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

[Julien Grall <julien.grall@xxxxxxx>]

On 31/08/2018 19:04, André Przywara wrote:
> On 08/31/2018 06:14 PM, Julien Grall wrote:
>
> Hi,
Hi,

> > On 08/31/2018 06:01 PM, Amit Singh Tomar wrote:
> > > While porting XEN on Amlogic SoC we found that in absence of
> > > image_size field of XEN image header, bootloader(U-BOOT)
> > > relocates[1] the XEN image to an address(not appropriate
> > > for Amlogic) derived from text_offset.
> > IHMO, this is a bug in U-Boot rather than Xen. The current format, while
> > old, is still valid.
> As tempting at is it to blame the "other guy", I think it's really Xen
> not up to par here. The kernel doc [2] says: ...
> "Where image_size is zero, text_offset can be assumed to be 0x80000."
>
> This is what U-Boot implements.
And Xen comply to that... Xen is actually able to boot at any address 
that is 4KB aligned.
> So you load Xen, say, to 16MB into DRAM, and U-Boot moves it to 512 KB,
> again complying with the kernel doc:
> "NOTE: versions prior to v4.6 cannot make use of memory below the
> physical offset of the Image so it is recommended that the Image be
> placed as close as possible to the start of system RAM."
> U-Boot derives this property from bit 3 of the flags being clear.
What's wrong with clearing bit 3? This is a valid things to do per the 
new format.
So that does not explain why the bug lie in Xen...

> > In the case of U-Boot, you usually need to be really careful on the
> > position of all the blobs (e.g Xen, DTB, Kernel, Initramfs) in the memory.
> Yes, and in fact it seems one can work around this by cleverly
> constructing the load addresses, but it's really time to bring Xen into
> the 21st century (or so) when it comes to the kernel image header ;-)
Even with the new format, you will have to cleverly construct the load 
addresses because nothing prevents U-boot to move the binaries around...
> > > This unwanted situation can be fixed by updating image_size field
> > > along side kernel flags so that image wouldn't relocate from initial
> > > load address.
> > I think the first step is to fix your U-boot and rethink where you load
> > your binaries.
> I think U-Boot perfectly complies with the kernel document. Xen not so
> much. The kernel image format was deliberately updated to become more
> flexible with certain memory layout situations as we have here.
> There is for instance a problem if there is something precious at 512KB
> into DRAM (secure memory owned by firmware), as regardless of the load
> addresses the user chooses U-Boot will (rightfully!) revert to the
> original "512KB into DRAM" address to keep compatibility with older
> kernels - and it believes Xen is such a one because of the ancient
> header format.
You could construct the same behavior with the new format (text-offset = 
0x80000 and flag 3 cleared). So how come this does not work?
This is why the spec says: "As close as possible to start of System RAM" 
not "At the start of System RAM". U-boot is probably too pedantic here.
> But ...
>
> > Regarding the patch in itself, I think this is a good addition as it
> > allow Xen to be loaded in more places. But please rewrite the commit
> > message accordingly, this is an update to a new version.
> I totally agree with that, the commit message should be reworded to
> stress that we want to comply with a newer version of the kernel image
> header (which is around for four years by now!), and just mention that
> it fixes problems with non-ancient U-Boots on certain platforms as an
> additional reason.
Either you or Amit needs to explain what is the real problem because at 
the moment, it still feels this is just a workaround for a bug in the 
firmware.
Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel