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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 08.03.18 at 17:23, <andrew.cooper3@xxxxxxxxxx> wrote:
> 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.

Well, we don't do anything like that today (which first of all would
require all that stuff to come out of .init.*). But yes, if you want
the code be prepared for that, fine with me:

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

But it would be nice if you called out that extra aspect in the
description.

Jan


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