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
