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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 07.03.18 at 16:51, <andrew.cooper3@xxxxxxxxxx> wrote:
> @@ -175,18 +175,47 @@ void init_or_livepatch apply_alternatives(const struct 
> alt_instr *start,
>       * So be careful if you want to change the scan order to any other
>       * order.
>       */
> -    for ( a = start; a < end; a++ )
> +    for ( a = base = start; a < end; a++ )
>      {
>          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);
>  
> +        /*
> +         * Detect sequences of alt_instr's patching the same origin site, and
> +         * keep base pointing at the first alt_instr entry.  This is so we 
> can
> +         * refer to a single ->priv field for patching decisions.
> +         *
> +         * ->priv being nonzero means that the origin site has already been
> +         * modified, and we shouldn't try to optimise the nops again.
> +         */
> +        if ( ALT_ORIG_PTR(base) != orig )
> +            base = a;

I don't understand why you need the new "priv" field - have a
boolean local variable which you reset instead of base here, and
which you check/set instead of base->priv below.

Everything else looks fine to me.

Jan


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