Re: [Xen-devel] [PATCH] x86/hvm: fix corrupt ACPI PM-Timer during live migration

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 20.02.13 at 08:42, Kouya Shimura <kouya@xxxxxxxxxxxxxx> wrote:
> On 02/16/2013 01:45 AM, Jan Beulich wrote:
>>>>> On 14.02.13 at 07:09, Kouya Shimura <kouya@xxxxxxxxxxxxxx> wrote:
>>> @@ -244,21 +245,13 @@ static int handle_pmt_io(
>>> static int pmtimer_save(struct domain *d, hvm_domain_context_t *h)
>>> {
>>>      PMTState *s = &d->arch.hvm_domain.pl_time.vpmt;
>>> -    uint32_t x, msb = s->pm.tmr_val & TMR_VAL_MSB;
>>>      int rc;
>>>
>>>      spin_lock(&s->lock);
>>>
>>> -    /* Update the counter to the guest's current time.  We always save
>>> -     * with the domain paused, so the saved time should be after the
>>> -     * last_gtime, but just in case, make sure we only go forwards */
>>
>> So you lose this property of guaranteeing no backward move.
> 
> Exactly.
> But the backward move is impossible except delay_for_missed_ticks mode.
> Since the monotonicity of hvm_get_guest_time() is guaranteed for other 
> modes.
> About the problem of delay_for_missed_ticks mode, I slightly mentioned
> in the previous mail.
> 
> The backward move can be happend as the following:
> 1) pt_freeze_time     ... real-time:100 guest-time:100   last_gtime:90
> 2) pmt_timer_callback ... real-time:110 guest-time:110   last_gtime:110
> 3) pt_thaw_time       ... real-time:120 guest-time:100   last_gtime:110
> 4) pmt_update_time    ... real-time:125 guest-time:105 < last_gtime:110
> 
> So, it can be happend not only in pmtimer_save() but also
> in handle_pmt_io().
> 
> Maybe pmt_udpate_time() should check the backward move.
> 
> 
>>
>>> -    x = ((s->vcpu->arch.hvm_vcpu.guest_time - s->last_gtime) * s->scale) 
>>> >> 32;
>>> -    if ( x < 1UL<<31 )
>>> -        s->pm.tmr_val += x;
>>> -    if ( (s->pm.tmr_val & TMR_VAL_MSB) != msb )
>>> -        s->pm.pm1a_sts |= TMR_STS;
>>>      /* No point in setting the SCI here because we'll already have saved 
> the
>>>       * IRQ and *PIC state; we'll fix it up when we restore the domain */
>>> +    pmt_update_time(s, 0);
>>
>> And using this function you also have the new side effect of
>> s->last_gtime being updated.
>>
>> Perhaps the new parameter should be renamed (to, say,
>> "saving"), and then allow suppressing all these behavioral
>> changes.
> 
> 
> The new side effect is also a bug fix, I think.
> Since updating s->pm.tmr_val and s->last_gtime should be
> done at the same time. Updating only s->pm.tmr_val is wrong.
> Or else, when the vm is resumed on the same machine (migration failure,
> check-pointing, etc), the PM-timer value is double counted.
> 
> 
>>
>> Also, in delay_for_missed_ticks mode you now use a slightly
>> different time for updating s->pm - did you double check that
>> this is not going to be a problem? Or else, the flag above could
>> similarly be used to circumvent this, or hvm_get_guest_time()
>> could be made return the frozen time (I suppose, but didn't
>> verify - as it appears to be an assumption already before your
>> patch -, that pt_freeze_time() runs before pmtimer_save()).
> 
> Modifying hvm_get_guest_time() is my thought, too.
> But I don't like the idea because the delay_for_missed_ticks mode
> is unusual. I wonder who uses it.
> 
> Let me think it over.

Ping?

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel