Re: [PATCH v2 1/2] x86/boot: Avoid relocations in trampoline code to physical addresses

[Jan Beulich <jbeulich@xxxxxxxx>]

On 28.08.2024 11:19, Frediano Ziglio wrote:
> The trampoline could have "manual" relocation entries (created
> by assembly macros and some code to use them) and (in case of PE)
> normal executable relocations.
> Remove some normal executable ones. In this way we don't have to
> worry about applying both correctly (they need proper order
> which is hard to spot looking at the code).

I don't theink the order of applying relocations matters - the overall
outcome will be the same for any order. What does matter is ...

> Specifically in efi_arch_post_exit_boot trampoline is copied after
> fixing relocations with efi_arch_relocate_image.

... whether they're applied by the time certain operations take place.

> These time dependencies
> between different part of code are hard to spot making hard to change
> code.

Relocation and copying sitting literally next to each other makes it
not really hard to spot, imo.

> In this case the copy is done in a state where code should be run
> at higher locations so it would be better to reduce the code between
> calling efi_arch_relocate_image and jumping to higher location.
> Absolute symbols are defined by linker in order to avoid relocations.
> These symbols use a "_PA" suffix to avoid possible clashes.
> phys_addr macro is used to make more clear the address we want and making
> symbol search easier.

At the price of introducing more absolute symbols, which are often
frowned upon. For example I fear this may (and the 2nd patch will)
get in the way of us (finally) randomizing Xen's virtual position
at load/boot time. Especially with xen.efi (where we already have
the base relocs) this shouldn't be overly difficult to arrange - as
long as there are no absolute symbols to take care of (ones used
only very early are okay of course).

> --- a/xen/arch/x86/boot/head.S
> +++ b/xen/arch/x86/boot/head.S
> @@ -870,8 +870,10 @@ cmdline_parse_early:
>  reloc:
>          .incbin "reloc.bin"
>  
> +#include "x86_64.S"
> +
> +        .section .init.text, "ax", @progbits
> +
>  ENTRY(trampoline_start)
>  #include "trampoline.S"
>  ENTRY(trampoline_end)
> -
> -#include "x86_64.S"

I take it that this is superseded by the patch introducing
.init.trampoline?

Jan