Re: [Xen-devel] [PATCH v3 1/9] x86: infrastructure to allow converting certain indirect calls to direct ones

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Tue, Sep 11, 2018 at 07:32:04AM -0600, Jan Beulich wrote:
> In a number of cases the targets of indirect calls get determined once
> at boot time. In such cases we can replace those calls with direct ones
> via our alternative instruction patching mechanism.
> 
> Some of the targets (in particular the hvm_funcs ones) get established
> only in pre-SMP initcalls, making necessary a second passs through the
> alternative patching code. Therefore some adjustments beyond the
> recognition of the new special pattern are necessary there.
> 
> Note that patching such sites more than once is not supported (and the
> supplied macros also don't provide any means to do so).
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> v3: Use "X" constraint instead of "g" in alternative_callN(). Pre-
>     calculate values to be put into local register variables.
> v2: Introduce and use count_va_arg(). Don't omit middle operand from
>     ?: in ALT_CALL_ARG(). Re-base.
> 
> --- a/xen/arch/x86/alternative.c
> +++ b/xen/arch/x86/alternative.c
> @@ -178,8 +178,9 @@ text_poke(void *addr, const void *opcode
>   * APs have less capabilities than the boot processor are not handled.
>   * Tough. Make sure you disable such features by hand.
>   */
> -void init_or_livepatch apply_alternatives(struct alt_instr *start,
> -                                          struct alt_instr *end)
> +static void init_or_livepatch _apply_alternatives(struct alt_instr *start,
> +                                                  struct alt_instr *end,
> +                                                  bool force)
>  {
>      struct alt_instr *a, *base;
>  
> @@ -218,6 +219,13 @@ void init_or_livepatch apply_alternative

I think you need to fix the comment before this if statement. At the
very least you're now using two ->priv to make decision on patching.

Also I wonder why you keep base, since ...

>          if ( ALT_ORIG_PTR(base) != orig )
>              base = a;
>  
> +        /* Skip patch sites already handled during the first pass. */
> +        if ( a->priv )
> +        {
> +            ASSERT(force);
> +            continue;
> +        }
> +
>          /* If there is no replacement to make, see about optimising the 
> nops. */
>          if ( !boot_cpu_has(a->cpuid) )
>          {
> @@ -225,7 +233,7 @@ void init_or_livepatch apply_alternative
>              if ( base->priv )
>                  continue;

... base is guaranteed to be a at this point, furthermore there is
already a check to skip patching added in this patch.

>  
> -            base->priv = 1;
> +            a->priv = 1;
>  
>              /* Nothing useful to do? */
>              if ( toolchain_nops_are_ideal || a->pad_len <= 1 )
> @@ -236,20 +244,74 @@ 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. 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;

I think these are opcodes for "UD2; UD0". Please add a comment for them.
Having to go through SDM to figure out what they are isn't nice.

At this point I also think the name "force" is not very good. What/who
is forced here? Why not use a more descriptive name like "post_init" or
"system_active"?

The rest of the patch looks fine to me.

Wei.

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