Re: [Xen-devel] [PATCH 5/7] x86/alt: Support for automatic padding calculations

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 12.02.18 at 12:23, <andrew.cooper3@xxxxxxxxxx> wrote:
> --- a/xen/arch/x86/alternative.c
> +++ b/xen/arch/x86/alternative.c
> @@ -180,13 +180,37 @@ void init_or_livepatch apply_alternatives(const struct 
> alt_instr *start,
>          uint8_t *orig = ALT_ORIG_PTR(a);
>          uint8_t *repl = ALT_REPL_PTR(a);
>          uint8_t buf[MAX_PATCH_LEN];
> +        unsigned int total_len = a->orig_len + a->pad_len;
>  
> -        BUG_ON(a->repl_len > a->orig_len);
> -        BUG_ON(a->orig_len > sizeof(buf));
> +        BUG_ON(a->repl_len > total_len);
> +        BUG_ON(total_len > sizeof(buf));
>          BUG_ON(a->cpuid >= NCAPINTS * 32);
>  
>          if ( !boot_cpu_has(a->cpuid) )
> +        {
> +            unsigned int i;
> +
> +            /* No replacement to make, but try to optimise any padding. */

Better move the comment ahead of the declaration?

> @@ -26,44 +27,64 @@ extern void apply_alternatives(const struct alt_instr 
> *start,
>                                 const struct alt_instr *end);
>  extern void alternative_instructions(void);
>  
> -#define OLDINSTR(oldinstr)      ".L%=_orig_s:\n\t" oldinstr 
> "\n.L%=_orig_e:\n"
> -
>  #define repl_s(num)             ".L%=_repl_s"#num
>  #define repl_e(num)             ".L%=_repl_e"#num
>  
>  #define alt_orig_len            "(.L%=_orig_e - .L%=_orig_s)"
> +#define alt_pad_len             "(.L%=_orig_p - .L%=_orig_e)"
> +#define alt_total_len           "(.L%=_orig_p - .L%=_orig_s)"
>  #define alt_repl_len(num)       "(" repl_e(num) " - " repl_s(num) ")"
> +#define gas_max(a, b)                                         \
> +    "((" a ") ^ (((" a ") ^ (" b ")) & -(-((" a ") < (" b ")))))"
> +
> +#define OLDINSTR_1(oldinstr, n1)                              \
> +    ".L%=_orig_s:\n\t" oldinstr "\n .L%=_orig_e:\n\t"         \
> +    ".skip (-(("alt_repl_len(n1)"-"alt_orig_len") > 0) * "    \
> +             "("alt_repl_len(n1)"-"alt_orig_len")), 0x90\n\t" \
> +    ".L%=_orig_p:\n\t"
> +
> +#define ALT_PADDING_LEN(n1, n2) \
> +    gas_max((alt_repl_len(n1), alt_repl_len(n2))"-"alt_orig_len
> +
> +#define OLDINSTR_2(oldinstr, n1, n2)                          \
> +    ".L%=_orig_s:\n\t" oldinstr "\n .L%=_orig_e:\n\t"         \
> +    ".skip (-(("ALT_PADDING_LEN(n1, n2)") > 0) * "            \
> +             "("ALT_PADDING_LEN(n1, n2)")), 0x90\n\t"         \
> +    ".L%=_orig_p:\n\t"
>  
>  #define ALTINSTR_ENTRY(feature, num)                                    \
>          " .long .L%=_orig_s - .\n"                /* label           */ \
>          " .long " repl_s(num)" - .\n"             /* new instruction */ \
>          " .word " __stringify(feature) "\n"       /* feature bit     */ \
>          " .byte " alt_orig_len "\n"               /* source len      */ \
> -        " .byte " alt_repl_len(num) "\n"          /* replacement len */
> +        " .byte " alt_repl_len(num) "\n"          /* replacement len */ \
> +        " .byte " alt_pad_len "\n"                /* padding len     */
>  
> -#define DISCARD_ENTRY(num)                        /* repl <= orig */    \
> -        " .byte 0xff + (" alt_repl_len(num) ") - (" alt_orig_len ")\n"
> +#define DISCARD_ENTRY(num)                        /* repl <= total */   \
> +        " .byte 0xff + (" alt_repl_len(num) ") - (" alt_total_len ")\n"

I don't think this is of much use anymore, now that you add the
padding automatically (same for the respective part of the
check in the assembler macro). Use

        ".byte " alt_total_len "\n" /* total_len <= 255 */

here instead (eliminating their explicit uses below)?

Jan


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