Re: [PATCH] Avoid additional relocations in trampoline code

[Frediano Ziglio <frediano.ziglio@xxxxxxxxx>]

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

> > --- a/xen/arch/x86/boot/trampoline.S
> > +++ b/xen/arch/x86/boot/trampoline.S
> > @@ -73,7 +73,7 @@ trampoline_protmode_entry:
> >          mov     %ecx,%cr4
> >
> >          /* Load pagetable base register. */
> > -        mov     $sym_offs(idle_pg_table),%eax
> > +        mov     $idle_pg_table_pa, %eax
> >          add     bootsym_rel(trampoline_xen_phys_start,4,%eax)
> >          mov     %eax,%cr3
> >
> > @@ -113,7 +113,7 @@ trampoline_protmode_entry:
> >          .code64
> >  start64:
> >          /* Jump to high mappings. */
> > -        movabs  $__high_start, %rdi
> > +        movabs  $__high_start_pa + __XEN_VIRT_START, %rdi
> >          jmpq    *%rdi
> >
> >  #include "video.h"
> > --- a/xen/arch/x86/boot/wakeup.S
> > +++ b/xen/arch/x86/boot/wakeup.S
> > @@ -99,7 +99,7 @@ wakeup_32:
> >          mov     $bootsym_rel(wakeup_stack, 4, %esp)
> >
> >          # check saved magic again
> > -        mov     $sym_offs(saved_magic),%eax
> > +        mov     $saved_magic_pa, %eax
> >          add     bootsym_rel(trampoline_xen_phys_start, 4, %eax)
> >          mov     (%eax), %eax
> >          cmp     $0x9abcdef0, %eax
> > @@ -112,7 +112,7 @@ wakeup_32:
> >          mov     %ecx, %cr4
> >
> >          /* Load pagetable base register */
> > -        mov     $sym_offs(idle_pg_table),%eax
> > +        mov     $idle_pg_table_pa ,%eax
> >          add     bootsym_rel(trampoline_xen_phys_start,4,%eax)
> >          mov     %eax,%cr3
> >
> > @@ -156,7 +156,7 @@ wakeup_32:
> >          .code64
> >  wakeup_64:
> >          /* Jump to high mappings and the higher-level wakeup code. */
> > -        movabs  $s3_resume, %rbx
> > +        movabs  $s3_resume_pa + __XEN_VIRT_START, %rbx
> >          jmp     *%rbx
> >
> >  bogus_saved_magic:
>
> 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. I can do it.

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

> Also, as a general request: Can you please become used to adding meaningful
> subject prefixes to your patches?
>

Sure

> Jan

Frediano