Re: [PATCH 19/29] x86/ibt,xen: Annotate away warnings

[Andrew Cooper <Andrew.Cooper3@citrix.com>]

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