Re: [PATCH v4 1/3] x86: Put trampoline in separate .init.trampoline section

[Frediano Ziglio <frediano.ziglio@xxxxxxxxx>]

On Mon, Sep 23, 2024 at 4:17 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 19.09.2024 10:00, Frediano Ziglio wrote:
> > This change put the trampoline in a separate, not executable section.
> > The trampoline contains a mix of code and data (data which
> > is modified from C code during early start so must be writable).
> > This is in preparation for W^X patch in order to satisfy UEFI CA
> > memory mitigation requirements.
> > At the moment .init.text and .init.data in EFI mode are put together
> > so they will be in the same final section as before this patch.
> > Putting in a separate section (even in final executables) allows
> > to easily disassembly that section. As we need to have a writable
> > section and as we can't have code and data together to satisfy W^X
> > requirement we need to have a data section. However tools like objdump
> > by default do not disassemble data sections. Forcing disassembly of
> > data sections would result in a very large output and possibly crash
> > of tools. Putting in a separate section allows to selectively
> > disassemble that part of code using a command like
> >
> >     objdump -m i386 -j .init.trampoline -d xen-syms
>
> For xen.efi it won't be quite as neat. One of the reason all .init.*
> are folded into a single section there is that the longer section names
> aren't properly represented, because of the linker apparently preferring
> to truncate them instead of using the "long section names" extension. To
> disassemble there one will need to remember to use "-j .init.tr". I'll
> have to check if there's a linker option we fail to enable, but in the
> absence of that we may want to consider to name the output section just
> ".trampoline" there, abbreviating to ".trampol" (i.e. at least a little
> more descriptive).
>

Long names are working for me, probably some issues with older binutils tools.
".trampol" looks fine for me.

> > --- a/xen/arch/x86/boot/head.S
> > +++ b/xen/arch/x86/boot/head.S
> > @@ -882,8 +882,9 @@ cmdline_parse_early:
> >  reloc:
> >          .incbin "reloc.bin"
> >
> > +#include "x86_64.S"
> > +
> > +        .section .init.trampoline, "aw", @progbits
>
> I think the lack of x here requires a comment.
>

Sure.

> Also did I miss any reply by you to Andrew's suggestion to move the
> trampoline to its own translation unit?
>

Yes, I stated the reason code was included in head.S (for some
assembly symbols computation) and spotted the instances of such
computations.
I was expecting some yes/no before changing.

> Jan

Frediano