Re: [PATCH v5 2/8] x86: annotate entry points with type and size

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.01.2024 18:45, Roger Pau Monné wrote:
> On Mon, Jan 15, 2024 at 03:34:56PM +0100, Jan Beulich wrote:
>> @@ -625,7 +627,7 @@ ENTRY(dom_crash_sync_extable)
>>  
>>  /* No special register assumptions. */
>>  #ifdef CONFIG_PV
>> -ENTRY(continue_pv_domain)
>> +FUNC(continue_pv_domain)
>>          ENDBR64
>>          call  check_wakeup_from_wait
>>  ret_from_intr:
>> @@ -640,26 +642,28 @@ ret_from_intr:
>>  #else
>>          jmp   test_all_events
>>  #endif
>> +END(continue_pv_domain)
>>  #else
>> -ret_from_intr:
>> +FUNC_LOCAL(ret_from_intr, 0)
> 
> Why does this need to have an alignment of 0? There's no fallthrough
> of previous code AFAICT.

It doesn't have to, but see the description for where I thought it would
make sense to newly introduce alignment; I simply didn't want to go too
far with such changes leading to generated code being altered. This (and
the other cases below) weren't in that group. Without ...

>>          ASSERT_CONTEXT_IS_XEN

... this I would be strongly inclined to switch ...

>>          jmp   restore_all_xen

... to

#define ret_from_intr restore_all_xen

anyway. And perhaps we ought to change the "#else" above to
"#elif !defined(NDEBUG)", at which point I'd say alignment isn't required
here at all.

>> @@ -713,12 +718,14 @@ ENTRY(common_interrupt)
>>          mov   %r15, STACK_CPUINFO_FIELD(xen_cr3)(%r14)
>>          mov   %bl, STACK_CPUINFO_FIELD(use_pv_cr3)(%r14)
>>          jmp ret_from_intr
>> +END(common_interrupt)
>>  
>> -ENTRY(entry_PF)
>> +FUNC(entry_PF)
>>          ENDBR64
>>          movl  $X86_EXC_PF, 4(%rsp)
>> +END(entry_PF)
>>  /* No special register assumptions. */
>> -GLOBAL(handle_exception)
>> +FUNC(handle_exception, 0)
> 
> Given patch 8/8 that enables support for placing FUNC() into separate
> sections, the fallthrough arrangement here with entry_PF is no longer
> guaranteed, as the linker could re-order the sections and thus
> entry_PF could fallthrough into another text section?
> 
> IOW: entry_PF needs a "jmp handle_exception", and then
> handle_exception itself can be padded as required by the default
> alignment?

Oh, yes, very much so. Thanks for noticing. I'll do that in the later
patch, though.

>> @@ -1149,7 +1176,7 @@ GLOBAL(autogen_entrypoints)
>>          .endm
>>  
>>          .popsection
>> -autogen_stubs: /* Automatically generated stubs. */
>> +FUNC_LOCAL(autogen_stubs, 0) /* Automatically generated stubs. */
> 
> Won't it be good to align the stubs?  As that's possible to make them
> faster?

Well. If I used default alignment here, it would be the 1st stub only
which gains alignment. I'd view that as simply inconsistent. You'll
find there already is an ALIGN inside the .rept below. That covers
only certain cases, but intentionally so, I believe: it's only entry
points which shouldn't be reached anyway which get no alignment. So
yes, in this case I clearly think there wants to explicitly be no
alignment here.

Jan