From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kouya Shimura Subject: Re: [PATCH] x86/hvm: fix corrupt ACPI PM-Timer during live migration Date: Fri, 8 Mar 2013 16:59:59 +0900 Message-ID: <51399A7F.500@jp.fujitsu.com> References: <5110A2DD.1080603@jp.fujitsu.com> <511A42E902000078000BDB5E@nat28.tlf.novell.com> <511C7FB7.90907@jp.fujitsu.com> <511E742A02000078000BEDAE@nat28.tlf.novell.com> <51247E56.4060904@jp.fujitsu.com> <5138C72802000078000C3FB0@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <5138C72802000078000C3FB0@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org > Ping? Sorry for late. Almost done, but the test is not enough yet. Maybe I can post it next week. Thanks, Kouya On 03/08/2013 12:58 AM, Jan Beulich wrote: >>>> On 20.02.13 at 08:42, Kouya Shimura wrote: >> On 02/16/2013 01:45 AM, Jan Beulich wrote: >>>>>> On 14.02.13 at 07:09, Kouya Shimura 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 >