Re: [Xen-devel] [PATCH v2 03/12] x86: infrastructure to allow converting certain indirect calls to direct ones

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 29/08/18 15:02, Jan Beulich wrote:
> @@ -235,13 +243,58 @@ void init_or_livepatch apply_alternative
>              continue;
>          }
>  
> -        base->priv = 1;
> -
>          memcpy(buf, repl, a->repl_len);
>  
>          /* 0xe8/0xe9 are relative branches; fix the offset. */
>          if ( a->repl_len >= 5 && (*buf & 0xfe) == 0xe8 )
> -            *(int32_t *)(buf + 1) += repl - orig;
> +        {
> +            /*
> +             * Detect the special case of indirect-to-direct branch patching:
> +             * - replacement is a direct CALL/JMP (opcodes 0xE8/0xE9; already
> +             *   checked above),
> +             * - replacement's displacement is -5 (pointing back at the very
> +             *   insn, which makes no sense in a real replacement insn),
> +             * - original is an indirect CALL/JMP (opcodes 0xFF/2 or 0xFF/4)
> +             *   using RIP-relative addressing.
> +             * Some function targets may not be available when we come here
> +             * the first time. Defer patching of those until the post-presmp-
> +             * initcalls re-invocation.

Which calls?  That smells like a bug which should be fixed rather than
worked around.  As for the other complexity here...

>  If at that point the target pointer is
> +             * still NULL, insert "UD2; UD0" (for ease of recognition) 
> instead
> +             * of CALL/JMP.
> +             */
> +            if ( a->cpuid == X86_FEATURE_ALWAYS &&
> +                 *(int32_t *)(buf + 1) == -5 &&
> +                 a->orig_len >= 6 &&
> +                 orig[0] == 0xff &&
> +                 orig[1] == (*buf & 1 ? 0x25 : 0x15) )
> +            {
> +                long disp = *(int32_t *)(orig + 2);
> +                const uint8_t *dest = *(void **)(orig + 6 + disp);
> +
> +                if ( dest )
> +                {
> +                    disp = dest - (orig + 5);
> +                    ASSERT(disp == (int32_t)disp);
> +                    *(int32_t *)(buf + 1) = disp;
> +                }
> +                else if ( force )
> +                {
> +                    buf[0] = 0x0f;
> +                    buf[1] = 0x0b;
> +                    buf[2] = 0x0f;
> +                    buf[3] = 0xff;
> +                    buf[4] = 0xff;
> +                }
> +                else
> +                    continue;
> +            }
> +            else if ( force && system_state < SYS_STATE_active )
> +                ASSERT_UNREACHABLE();
> +            else
> +                *(int32_t *)(buf + 1) += repl - orig;
> +        }
> +        else if ( force && system_state < SYS_STATE_active  )
> +            ASSERT_UNREACHABLE();
>          /* RIP-relative addressing is easy to check for in VEX-encoded 
> insns. */
>          else if ( a->repl_len >= 8 &&
>                    (*buf & ~1) == 0xc4 &&
> @@ -149,6 +150,203 @@ extern void alternative_instructions(voi
>  /* Use this macro(s) if you need more than one output parameter. */
>  #define ASM_OUTPUT2(a...) a
>  
> +/*
> + * Machinery to allow converting indirect to direct calls, when the called
> + * function is determined once at boot and later never changed.
> + */
> +
> +#define ALT_CALL_arg1 "rdi"
> +#define ALT_CALL_arg2 "rsi"
> +#define ALT_CALL_arg3 "rdx"
> +#define ALT_CALL_arg4 "rcx"
> +#define ALT_CALL_arg5 "r8"
> +#define ALT_CALL_arg6 "r9"
> +
> +#define ALT_CALL_ARG(arg, n) \
> +    register typeof((arg) ? (arg) : 0) a ## n ## _ \
> +    asm ( ALT_CALL_arg ## n ) = (arg)
> +#define ALT_CALL_NO_ARG(n) \
> +    register unsigned long a ## n ## _ asm ( ALT_CALL_arg ## n )
> +
> +#define ALT_CALL_NO_ARG6 ALT_CALL_NO_ARG(6)
> +#define ALT_CALL_NO_ARG5 ALT_CALL_NO_ARG(5); ALT_CALL_NO_ARG6
> +#define ALT_CALL_NO_ARG4 ALT_CALL_NO_ARG(4); ALT_CALL_NO_ARG5
> +#define ALT_CALL_NO_ARG3 ALT_CALL_NO_ARG(3); ALT_CALL_NO_ARG4
> +#define ALT_CALL_NO_ARG2 ALT_CALL_NO_ARG(2); ALT_CALL_NO_ARG3
> +#define ALT_CALL_NO_ARG1 ALT_CALL_NO_ARG(1); ALT_CALL_NO_ARG2
> +
> +/*
> + * Unfortunately ALT_CALL_NO_ARG() above can't use a fake initializer (to
> + * suppress "uninitialized variable" warnings), as various versions of gcc
> + * older than 8.1 fall on the nose in various ways with that (always because
> + * of some other construct elsewhere in the same function needing to use the
> + * same hard register). Otherwise the asm() below could uniformly use "+r"
> + * output constraints, making unnecessary all these ALT_CALL<n>_OUT macros.
> + */
> +#define ALT_CALL0_OUT "=r" (a1_), "=r" (a2_), "=r" (a3_), \
> +                      "=r" (a4_), "=r" (a5_), "=r" (a6_)
> +#define ALT_CALL1_OUT "+r" (a1_), "=r" (a2_), "=r" (a3_), \
> +                      "=r" (a4_), "=r" (a5_), "=r" (a6_)
> +#define ALT_CALL2_OUT "+r" (a1_), "+r" (a2_), "=r" (a3_), \
> +                      "=r" (a4_), "=r" (a5_), "=r" (a6_)
> +#define ALT_CALL3_OUT "+r" (a1_), "+r" (a2_), "+r" (a3_), \
> +                      "=r" (a4_), "=r" (a5_), "=r" (a6_)
> +#define ALT_CALL4_OUT "+r" (a1_), "+r" (a2_), "+r" (a3_), \
> +                      "+r" (a4_), "=r" (a5_), "=r" (a6_)
> +#define ALT_CALL5_OUT "+r" (a1_), "+r" (a2_), "+r" (a3_), \
> +                      "+r" (a4_), "+r" (a5_), "=r" (a6_)
> +#define ALT_CALL6_OUT "+r" (a1_), "+r" (a2_), "+r" (a3_), \
> +                      "+r" (a4_), "+r" (a5_), "+r" (a6_)
> +
> +#define alternative_callN(n, rettype, func) ({                     \
> +    rettype ret_;                                                  \
> +    register unsigned long r10_ asm("r10");                        \
> +    register unsigned long r11_ asm("r11");                        \
> +    asm volatile (__stringify(ALTERNATIVE "call *%c[addr](%%rip)", \
> +                                          "call .",                \
> +                                          X86_FEATURE_ALWAYS)      \
> +                  : ALT_CALL ## n ## _OUT, "=a" (ret_),            \
> +                    "=r" (r10_), "=r" (r11_)                       \
> +                  : [addr] "i" (&(func)), "g" (func)               \

There was a Linux thread (which I've lost track of) which went around
replacing "i" with "P" for labels like this, which behaves better for
relocatable targets.  Furthermore, I can't work out what the "g"
constraint is for, but if it is simply for making the reference visible,
I'd suggest "X" instead which avoids any interference with register
scheduling.

As for the complexity, how about implementing this as an unlikely block
with a single call in?  That way, patching turns the entry jmp imm32
into a call imm32 and we lose the complicated decoding and nop padding
from the result.

~Andrew

> +                  : "memory" );                                    \
> +    ret_;                                                          \
> +})
> +
>


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel