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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.09.2024 21:46, Andrew Cooper wrote:
> On 06/09/2024 6:58 am, Jan Beulich wrote:
>> On 05.09.2024 18:10, Andrew Cooper 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.
>> And it moves the declarations to the new .c file. Visibility still limited
>> to that one file. And (afaics) no Misra violation, contrary to what your
>> later reply to Frediano suggests.
> 
> If only there were an easy way to answer the question.
> 
> https://gitlab.com/xen-project/people/andyhhp/xen/-/jobs/7766305370
> 
> Failure: 4 regressions found for clean guidelines
>   service MC3R1.R8.5: (required) An external object or function shall be
> declared once in one and only one file:
>    violation: 4

I'm afraid I'm having trouble locating, in that .log file, where the actual
regressions are pointed out. I guess I'm simply not used to reading such
logs yet, and hence I just don't know what to search for. In any event, I
think there's a set of issues here:
- Eclair apparently considered efi-boot.h a header file, which (as said
  earlier) isn't quite right.
- Declarations there are thus deemed okay (when they shouldn't, unless
  deviated).
- Movement to a proper .c file points out that those decls may have been
  missing "asmlinkage" already before.

>>> 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.
>> It's not "more than", it's "like" (i.e. no matter whether a linker script
>> or assembly is the origin of the symbol).
> 
> I'm asking why linker-section-boundary symbols should be treated
> specially, and not seeing an answer.

IOW you're not taking "they're no different from symbols defined in .S
files, and hence shouldn't be treated differently" as a possible position
to take? See __page_tables_{start,end}, cpu0_stack[], or multiboot_ptr as
examples.

> I assert they do not warrant special treatment, and should live in
> header files like every other extern symbol we use.

Then the same would also apply to symbols coming from .S files. Which in
turn were deliberately deviated (rather than moved) in the course of
dealing with the Misra rule relevant here.

Jan