Re: [PATCH 4/7] x86/boot: apply trampoline relocations at destination position

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 18/03/2025 5:35 pm, Roger Pau Monne wrote:
> diff --git a/xen/arch/x86/boot/reloc-trampoline.c 
> b/xen/arch/x86/boot/reloc-trampoline.c
> index e35e7c78aa86..ac54aef14eaf 100644
> --- a/xen/arch/x86/boot/reloc-trampoline.c
> +++ b/xen/arch/x86/boot/reloc-trampoline.c
> @@ -20,19 +20,19 @@ void reloc_trampoline64(void)
>      uint32_t phys = trampoline_phys;
>      const int32_t *trampoline_ptr;
>  
> -    /*
> -     * Apply relocations to trampoline.
> -     *
> -     * This modifies the trampoline in place within Xen, so that it will
> -     * operate correctly when copied into place.
> -     */
> +    /* Apply relocations to trampoline after copy to destination. */

I think this needs expanding on a bit.

The relocations in __trampoline_*_{start,stop} relate to the trampoline
as it lives compiled into Xen, but we're applying them to the trampoline
already copied into low memory.

> +#define RELA_TARGET(ptr, bits) \
> +    *(uint ## bits ## _t *)(phys + *ptr + (long)ptr - (long)trampoline_start)
> +
>      for ( trampoline_ptr = __trampoline_rel_start;
>            trampoline_ptr < __trampoline_rel_stop;
>            ++trampoline_ptr )
> -        *(uint32_t *)(*trampoline_ptr + (long)trampoline_ptr) += phys;
> +        RELA_TARGET(trampoline_ptr, 32) += phys;
>  
>      for ( trampoline_ptr = __trampoline_seg_start;
>            trampoline_ptr < __trampoline_seg_stop;
>            ++trampoline_ptr )
> -        *(uint16_t *)(*trampoline_ptr + (long)trampoline_ptr) = phys >> 4;
> +        RELA_TARGET(trampoline_ptr, 16) = phys >> 4;
> +
> +#undef RELA_TARGET

I have a patch renaming trampoline_ptr to just ptr, on the grounds of
verbosity.  I'm not sure if it want's to go in ahead, merged with, or
after this patch.

Also, encoding bits in RELA_TARGET() isn't terribly nice.  What's wrong
with keeping the casts as-are, and having RELA_TARGET() only taking ptr?

~Andrew