Re: [PATCH 18/18] x86: add multiboot2 protocol support for EFI platforms

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 10/02/2015 21:27, Daniel Kiper wrote:
> On Fri, Jan 30, 2015 at 06:54:22PM +0100, Daniel Kiper wrote:
>> Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
>> ---
>>  xen/arch/x86/boot/head.S          |  174 +++++++++++++++++++++++++++++++++++--
>>  xen/arch/x86/efi/efi-boot.h       |   29 +++++++
>>  xen/arch/x86/setup.c              |   23 ++---
>>  xen/arch/x86/x86_64/asm-offsets.c |    2 +
>>  xen/common/efi/boot.c             |   11 +++
>>  5 files changed, 222 insertions(+), 17 deletions(-)
> After some testing we have found at least one machine on which this thing
> does not work. It is Dell PowerEdge R820 with latest firmware. Machine
> crashes/stops because early 32-bit code is not relocatable and must live
> under 0x100000 address. (side note: I am surprised how it worked without
> any issue until now; Multiboot protocol, any version, does not guarantee
> that OS image will be loaded at specified/requested address; So, it looks
> that there are not any legacy BIOS machines with e.g. 1 MiB - 2 MiB region
> reserved in the wild or they are not very common; Am I missing something?).
> Sadly, this region is used by BS, so, GRUB2 loads Xen higher (at least
> above 64 MiB). Please look at memory map on this machine:
>
> Type       Start            End              # Pages          Attributes
> BS_Data    0000000000010000-000000000009FFFF 0000000000000090 000000000000000F
> BS_Data    0000000000100000-0000000003FFFFFF 0000000000003F00 000000000000000F
> Available  0000000004000000-000000000FFFEFFF 000000000000BFFF 000000000000000F
> BS_Code    000000000FFFF000-000000001006CFFF 000000000000006E 000000000000000F
> Available  000000001006D000-00000000B3E73FFF 00000000000A3E07 000000000000000F
>
> [...]
>
> Additionally, early Xen boot code maps only first 16 MiB of memory. Hence,
> it means that jump into __high_start fails immediately.
>
> Now I see two solutions for these issues:
>
> 1) We can make early 32-bit code relocatable. We may use something similar
>    to xen/arch/x86/boot/trampoline.S:bootsym_rel(). Additionally, I think
>    that early code should not blindly map first 16 MiB of memory. It should
>    map first 1 MiB of memory and then 16 MiB of memory starting from
>    xen_phys_start. This way we also fix long standing bug in early code
>    which I described earlier.
>
> 2) We can jump from EFI x86-64 mode directly into "Xen x86-64 mode" like
>    it is done in case of EFI loader. However, then we must duplicate multiboot2
>    protocol implementation in x86-64 mode (if we wish that multiboot2 protocol
>    can be used on legacy BIOS and EFI platforms; I think that we should support
>    this protocol on both for users convenience). Additionally, we must use
>    a workaround to relocate trampoline if boot services uses memory below 1 MiB
>    (please check commit c1f2dfe8f6a559bc28935f24e31bb33d17d9713d, x86/EFI: make
>    trampoline allocation more flexible, for more details).
>
> I prefer #1 because this way we do not duplicate multiboot2 protocol implementation
> (one for legacy BIOS and EFI) and we avoid issues with trampoline relocation when
> low memory is occupied by boot services and/or 1:1 EFI page tables.
>
> Daniel
>
> PS I have just realized that commit c1f2dfe8f6a559bc28935f24e31bb33d17d9713d
>    will not work if trampoline code will overwrite some of EFI 1:1 page tables.
>    Dell PowerEdge R820 store part of 1:1 page tables below 1 MiB. Xen loaded
>    by native EFI loader boots but it is only lucky coincidence that it does
>    not overwrite used entries. So, I tend to go and choose #1 even more.

I have to admit to also being surprised at the fragility of the Xen, and
how it cannot function if _start isn't loaded on the 1MB boundary.

Given that there is provably one system in the wild which causes us to
fall over this limitation, it clearly needs fixing.

I would also agree that making the entry point relocatable is the
correct way forwards.  However, you cannot use something like
bootsym_rel() as that requires infrastructure to patch the references
(observe the loop over __trampoline_rel_{start,end} just before the call
to cmdline_parse_early())

This can probably be achieved by having a small bit of hand-written PIC
which puts an appropriate offset into %ds.

~Andrew