Messages in this thread Patch in this message | | | From | Andrew Cooper <> | Subject | Re: [PATCH 19/29] x86/ibt,xen: Annotate away warnings | Date | Fri, 18 Feb 2022 23:07:15 +0000 |
| |
On 18/02/2022 21:05, Peter Zijlstra wrote: > On Fri, Feb 18, 2022 at 08:24:41PM +0000, Andrew Cooper wrote: >> at a minimum, and possibly also: >> >> diff --git a/arch/x86/xen/xen-asm.S b/arch/x86/xen/xen-asm.S >> index 444d824775f6..96db5c50a6e7 100644 >> --- a/arch/x86/xen/xen-asm.S >> +++ b/arch/x86/xen/xen-asm.S >> @@ -124,7 +124,7 @@ SYM_CODE_START(xen_\name) >> UNWIND_HINT_EMPTY >> pop %rcx >> pop %r11 >> - jmp \name >> + jmp \name + 4 * IS_ENABLED(CONFIG_X86_IBT) >> SYM_CODE_END(xen_\name) >> _ASM_NOKPROBE(xen_\name) >> .endm > objtool will do that for you, it will rewrite all direct jmp/call to > endbr.
Ah - great.
> Something like so then?
Looks plausible, although Juergen would be a better person to judge.
About paravirt_iret, this is all way more complicated than it needs to be.
Currently, there are two users of INTERRUPT_RETURN.
The first, in swapgs_restore_regs_and_return_to_usermode, is never going to execute until patching is complete, and is already behind an alternative causing XENPV to go a different way, which means that:
diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S index 97b1f84bb53f..f9a021e7688a 100644 --- a/arch/x86/entry/entry_64.S +++ b/arch/x86/entry/entry_64.S @@ -608,8 +608,8 @@ SYM_INNER_LABEL(swapgs_restore_regs_and_return_to_usermode, SYM_L_GLOBAL) /* Restore RDI. */ popq %rdi - SWAPGS - INTERRUPT_RETURN + swapgs + jmp native_iret SYM_INNER_LABEL(restore_regs_and_return_to_kernel, SYM_L_GLOBAL)
is correct AFAICT. (Tangent; then ESPFIX64 can be simplified because only the return-to-user path needs the LDT check, so the enter/exit user state can be dropped.)
That leaves the single INTERRUPT_RETURN in restore_regs_and_return_to_kernel. Xen PV is an easy environment to start up in, so:
diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S index 97b1f84bb53f..a9e7846cc176 100644 --- a/arch/x86/entry/entry_64.S +++ b/arch/x86/entry/entry_64.S @@ -626,7 +626,10 @@ SYM_INNER_LABEL(restore_regs_and_return_to_kernel, SYM_L_GLOBAL) * ARCH_HAS_MEMBARRIER_SYNC_CORE rely on IRET core serialization * when returning from IPI handler. */ - INTERRUPT_RETURN +#ifdef CONFIG_XEN_PV +early_iret_patch: +#endif + jmp native_iret SYM_INNER_LABEL_ALIGN(native_iret, SYM_L_GLOBAL) UNWIND_HINT_IRET_REGS diff --git a/arch/x86/xen/xen-head.S b/arch/x86/xen/xen-head.S index 6a64496edefb..31f136328c84 100644 --- a/arch/x86/xen/xen-head.S +++ b/arch/x86/xen/xen-head.S @@ -66,6 +66,10 @@ SYM_CODE_START(startup_xen) cdq wrmsr + mov $native_iret, %rax + sub $xen_iret, %rax + add %eax, 1 + early_iret_patch + call xen_start_kernel SYM_CODE_END(startup_xen) __FINIT
really should be good enough to drop INTERRUPT_RETURN and paravirt_iret entirely.
Obviously, that's very hacky, and might better be expressed like:
diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S index 97b1f84bb53f..af371e4f0dda 100644 --- a/arch/x86/entry/entry_64.S +++ b/arch/x86/entry/entry_64.S @@ -626,7 +626,7 @@ SYM_INNER_LABEL(restore_regs_and_return_to_kernel, SYM_L_GLOBAL) * ARCH_HAS_MEMBARRIER_SYNC_CORE rely on IRET core serialization * when returning from IPI handler. */ - INTERRUPT_RETURN + EARLY_ALTERNATIVE "jmp native_iret", "jmp xen_iret", X86_FEATURE_XENPV SYM_INNER_LABEL_ALIGN(native_iret, SYM_L_GLOBAL) UNWIND_HINT_IRET_REGS
or so, but my point is that the early Xen code, if it can identify this patch point separate to the list of everything, can easily arrange for it to be modified before HYPERCALL_set_trap_table (Xen PV's LIDT), and then return_to_kernel is in its fully configured state (paravirt or otherwise) before interrupts/exceptions can be taken.
~Andrew
| |