Re: [PATCH] Avoid additional relocations in trampoline code

[Frediano Ziglio <frediano.ziglio@xxxxxxxxx>]

On Tue, Aug 27, 2024 at 4:50 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 27.08.2024 15:56, Frediano Ziglio wrote:
> > On Mon, Aug 26, 2024 at 9:21 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
> >>
> >> On 22.08.2024 17:29, 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 all 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).
> >>>
> >>> Signed-off-by: Frediano Ziglio <frediano.ziglio@xxxxxxxxx>
> >>
> >> I think this wants splitting into one patch replacing sym_offs() and a
> >> 2nd one introducing the hand-crafted __XEN_VIRT_START additions (which
> >> may want macro-izing). Plus the justification for the change wants
> >> extending, to actually explain what the problem is - after all there's
> >> no issue anywhere right now.
> >
> > Should I explain the time dependency issue with source code?
>
> Time dependency? I guess I don't understand ...
>

You have a time dependency between 2 code path when one has to be
executed before/after another.
Some patterns are expected (you don't call a method on a destroyed
object) but nasty hidden ones makes code more complicated and less
robust, making code changes harder and program fragile.
In that specific case the copy of the trampoline, for EFI path, is
done after relocation rollback. In that specific code path, you are
executing code which is not meant to be executed at that location (now
should be executed at higher locations) hoping that compiler/binary
code can cope with it. The code should relocate back and switch to
higher locations as quick as possible.

> > I suppose I can describe where currently is and why it would be better
> > to have it removed (honestly I though I did but reading the commit
> > message I didn't).
> > Maybe for search reasons it would be better to define 2 macros like
> > the following?
> >
> > #define phys_addr(sym) sym ## _pa
> > #define virt_addr(sym) sym ## _va
> >
> > This way, for instance, searching for the "__high_start" word (like
> > "grep -rw __high_start") would produce a result.
>
> I assume it's best to keep everything there together, so see below.
>
> >> With the sym_offs() uses gone, I think it would be best if the macro was
> >> #undef-ed ahead of the inclusion of trampoline.S. Since x86_64.S uses the
> >> macro, that'll require careful re-arrangement of #include order.
> >>
> >
> > I think you mean including the trampoline after including x86_64.S in
> > head.S.
>
> Yes.
>

That worked like a charm!

> >>> --- a/xen/arch/x86/xen.lds.S
> >>> +++ b/xen/arch/x86/xen.lds.S
> >>> @@ -71,7 +71,12 @@ SECTIONS
> >>>    __2M_text_start = .;         /* Start of 2M superpages, mapped RX. */
> >>>  #endif
> >>>
> >>> -  start_pa = ABSOLUTE(start - __XEN_VIRT_START);
> >>> +#define DEFINE_PA_ADDRESS(sym) sym ## _pa = ABSOLUTE(sym - 
> >>> __XEN_VIRT_START)
> >>> +  DEFINE_PA_ADDRESS(start);
> >>> +  DEFINE_PA_ADDRESS(saved_magic);
> >>> +  DEFINE_PA_ADDRESS(idle_pg_table);
> >>> +  DEFINE_PA_ADDRESS(__high_start);
> >>> +  DEFINE_PA_ADDRESS(s3_resume);
> >>>
> >>>    . = __XEN_VIRT_START + XEN_IMG_OFFSET;
> >>>    _start = .;
> >>
> >> For the cases where in assembly code you add __XEN_VIRT_START this is 
> >> pretty
> >> odd: You subtract the value here just to add it back there. Did you 
> >> consider
> >> a more straightforward approach, like introducing absolute xxx_va symbols?
> >
> > I didn't consider. Would something like
> >
> > #define DEFINE_ABS_ADDRESSES(sym) \
> >    sym ## _pa = ABSOLUTE(sym - __XEN_VIRT_START); \
> >    sym ## _va = ABSOLUTE(sym)
> >
> > make sense? Maybe the _pa and _va suffixes are too similar? Maybe
> > _physaddr and _virtaddr? Or use capical letters and macros (as above)
> > to avoid possible clashes?
>
> I'd like to ask that we don't introduce symbols we don't actually use. Hence
> a single macro defining both is probably not going to be overly helpful. As
> to capital letters: I'm struggling with the "(as above)" - I don't see any
> use of capital letters in symbol names being generated. But yes, I was going
> to suggest to consider _VA and _PA tags, precisely to reduce the risk of
> clashes.
>

Unfortunately this worked but is producing these warnings which are
pretty annoying:

ld: ./.xen.efi.0xffff82d040000000.0: stripping non-representable
symbol 'saved_magic_va' (value 0xffff82d040816018)

I tried to remove them somehow, but I didn't find any way (no hidden
symbols, not been able to force the linker to strip it and not
complain).
I think I'll get back to the single "pa" symbol, this time with _PA
suffix, maybe hidden, and adding macros (phys_addr/virt_addr) to make
clear the purpose.

> Jan

I'll try to split the patch as suggested, although it will be hard to
come with a sensible message for the second commit.

Regards,
  Frediano