Re: [PATCH 3/3] x86/trampoline: Collect other scattered trampoline symbols

[Frediano Ziglio <frediano.ziglio@xxxxxxxxx>]

On Thu, Sep 5, 2024 at 5:10 PM Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:

On 05/09/2024 4:42 pm, Jan Beulich wrote:
> On 05.09.2024 15:06, Andrew Cooper wrote:
>> --- a/xen/arch/x86/efi/efi-boot.h
>> +++ b/xen/arch/x86/efi/efi-boot.h
>> @@ -102,9 +102,6 @@ static void __init efi_arch_relocate_image(unsigned long delta)
>>      }
>>  }
>> 
>> -extern const s32 __trampoline_rel_start[], __trampoline_rel_stop[];
>> -extern const s32 __trampoline_seg_start[], __trampoline_seg_stop[];
> I'd prefer if these stayed here, leaving the 4 symbols as minimally exposed as
> possible. Recall that efi-boot.h isn't really a header in that sense, but
> rather a .c file. Elsewhere we keep decls in .c files when they're used in just
> one CU.

See Frediano's RFC series, which needs to change this in order to move
the 32bit relocation logic from asm to C.

Not strictly necessary, I can declare in the C file as they were declared in efi-boot.h (which is more a .c file as an header as Jan said).

I think the idea of declaring into a source file is that if another file wants to use it has to declare it again, so a bit more friction.

But any access to trampoline variables should be considered as something to limit in any case, so having in a separate header helps (this does not mean that removing from the header is still increasing the friction).

Personally, I'm not strong about the 2 options here. Slightly prefer having all variable declared in a single header.

The only reason efi-boot.h can get away with this right now is because
the other logic is written entirely in asm.


Scope-limiting linker section boundaries more than regular variables is
weird to me.  It's not as if they magically take more care to use than
regular variables, and trampoline.h is not a wide scope by any means.

 

Frediano