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

[André Przywara <andre.przywara@xxxxxxx>]

On 08/31/2018 09:39 PM, Julien Grall wrote:

Hi,

tl;dr: I think we don't disagree on the usefulness of this change in
general, so let's just change the commit towards something like:

Update the Xen image header to match the newest Linux kernel definition.
This allows to specify some relaxed properties of Xen and allows a
bootloader to place the Xen kernel more flexibly.
This fixes the boot on some systems which have other data or code close
to the beginning of DRAM.

More explanation why I don't think this is a U-Boot bug:

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

Yes, Xen is much more relaxed then even the newest kernel (ignoring
TEXT_OFFSET), but it doesn't tell the boot loader. So that assumes the
worst. Fair enough, works mostly, but not in every case, like this one
here. The kernel image header got improved to cope with those cases, and
Xen should just be updated to do as well. There is no regression
expected, it's just improving the situation in some cases.
Actually regardless of that we could just update the image header.

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

There is no bug in Xen, it's just not very precise in advertising its
needs and properties and this bites us in corner cases like this one.

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

U-Boot will *try* to not move the image around, to avoid overwriting
other images (Dom0, DT, ramdisk, firmware...). But if the information it
gets from the kernel image give it no choice, it goes the extra mile to
make the kernel happy (as early as possible, 512KB offset). Xen doesn't
need all that, so we should tell that to the bootloader.

>>>> 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?

The old kernel had some requirements, which got relaxed over time. The
image format now reflects this. You might run into similar issues with
an old kernel, but people don't care so much.
So at the moment it works somehow and mostly, but is unnecessarily
complicated. You load Xen to address X, but then have to know that
U-Boot relocates Xen (and just Xen!) to some other address and adjust
your load addresses accordingly. Not very user friendly.

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

As a human I tend to agree, but what should U-Boot do? "As close as
possible" means 0, unless you have other information (which we don't).

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

Let me try it another way: Xen decided to piggy back on a "foreign"
image format, which is good (and I like it!). But Xen doesn't control
it's definition, so it should keep up with any changes done there. So
far this wasn't a problem (although I remember some awkward workarounds
in the past for other boards due to TEXT_OFFSET 0), but now we have a
good reason to update it.

Actually: even if I would agree with you that the problem is in U-Boot,
I wouldn't know how to fix it without making the situation worse for
Linux kernels. The U-Boot implementation seems perfectly fine for both
older and newer Linux kernels according to the spec. So shall we add a
hack to U-Boot just because of Xen? Especially if the Xen fix proposed
is easy, backwards compatible and would have been done anyway?

People would probably run into similar problems with pre v3.17 kernels,
but then we could just ask to update the kernel. I would like to give
the same advice to Xen users.

Frankly, let's stop bikeshedding about this simple thing. Since I saw a
similar patch in one of your branches, I think we don't disagree about
the usefulness ;-)

Cheers,
Andre.

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