From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kouya Shimura Subject: Re: [PATCH 2/2] x86/hvm: fix corrupt ACPI PM-Timer during live migration Date: Fri, 22 Mar 2013 10:12:35 +0900 Message-ID: <514BB003.1070903@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> <514A9BC4.40508@jp.fujitsu.com> <514AB78A.3010109@jp.fujitsu.com> <514AE87902000078000C76E3@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: <514AE87902000078000C76E3@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: Tim Deegan , Keir Fraser , xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On 03/21/2013 07:01 PM, Jan Beulich wrote: >>>> On 21.03.13 at 08:32, Kouya Shimura wrote: >> @@ -244,19 +245,11 @@ 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 on the previous patch version (which you said this one is > identical to) I stated that you lose this property of guaranteeing > no backward move. Am I right in assuming that patch 1 is now > supposed to take care of this? Yes. Patch 1 guarantees that the timer counter only goes forwards. > In any case I'll have to defer to Keir or Tim for that first one. > >> - 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; >> + pmt_update_time(s, 0); > > Here I can only quote part of my previous reply, which I don't > think you responded to: > > "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())." > > You said you'd think about it, but I don't recall seeing any other > outcome from that than the two patches, and I can't relate the > first patch to this aspect. In patch 1, pmt_update_time() calls the new function hvm_get_base_time() which returns the frozen time when the vcpu is de-scheduled. And I confirmed pmtimer_save() is surely called after pt_freeze_time() from my test and review. So, if both patches are applied, there is no difference in delay_for_missed_ticks mode. Thanks, Kouya