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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 08/03/18 15:42, Jan Beulich wrote:
>>>> 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.

That can break in a "corrupted instruction stream" kind of way if we
perform two passes over the same set of alt_instr's, e.g. after loading
microcode, finding some new features, and rerunning alternatives.

~Andrew

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