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: Thu, 14 Feb 2013 15:09:59 +0900 Message-ID: <511C7FB7.90907@jp.fujitsu.com> References: <5110A2DD.1080603@jp.fujitsu.com> <511A42E902000078000BDB5E@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="------------010909040807060703020204" Return-path: In-Reply-To: <511A42E902000078000BDB5E@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 --------------010909040807060703020204 Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit Hi Jan, Thanks for reviewing the code. On 02/12/2013 09:26 PM, Jan Beulich wrote: >>>> On 05.02.13 at 07:12, Kouya Shimura wrote: >> --- a/xen/arch/x86/hvm/pmtimer.c Tue Jan 22 09:33:10 2013 +0100 >> +++ b/xen/arch/x86/hvm/pmtimer.c Tue Feb 05 10:26:13 2013 +0900 >> @@ -244,21 +247,34 @@ 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; >> + uint64_t guest_time; >> 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 */ >> - 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 */ >> + guest_time = s->vcpu->arch.hvm_vcpu.guest_time; >> + /* Update the counter to the guest's current time only if the >> + * timer mode is delay_for_missed_ticks */ >> + if ( guest_time != 0 ) > > How is guest_time being (non-)zero an indication of mode being > delay_for_missed_ticks? I think you should check for the mode > explicitly, as the value here being zero can just be an effect of > a large enough negative v->arch.hvm_vcpu.stime_offset. > > And besides that you don't explain why the update being done > here is unnecessary in other modes - all you explain is that the > way it's being done in those modes is wrong. After due consideration, the update of PM-timer is necessary for other modes. I misunderstood it's just an adjustment for the delay_for_missed_ticks mode. The cause of this bug is to refer the vcpu->arch.hvm_vcpu.guest_time. Attached is the revised patch. Aside from this patch, I've found another problem about delay_for_missed_ticks. PM-timer might be broken after pmt_timer_callback is invoked. I'll think it over. Thanks, Kouya --------------010909040807060703020204 Content-Type: text/x-patch; name="fix_corrupt_saved_pmtimer_v2.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="fix_corrupt_saved_pmtimer_v2.patch" # HG changeset patch # User Kouya Shimura # Date 1360820290 -32400 # Node ID 9744ac3f23dd198e997bf032385a573408420762 # Parent 5af4f2ab06f33ce441fa550333a9049c09a9ef28 x86/hvm: fix corrupt ACPI PM-Timer during live migration ACPI PM-Timer value is broken on saving a VM. Since c/s 16274:44dde35cb2a6, vcpu->arch.hvm_vcpu.guest_time is not the correct guest's time any more. Instead, hvm_get_guest_time() should be used in pmtimer_save to calculate the timer. Signed-off-by: Kouya Shimura diff -r 5af4f2ab06f3 -r 9744ac3f23dd xen/arch/x86/hvm/pmtimer.c --- a/xen/arch/x86/hvm/pmtimer.c Tue Jan 22 09:33:10 2013 +0100 +++ b/xen/arch/x86/hvm/pmtimer.c Thu Feb 14 14:38:10 2013 +0900 @@ -85,7 +85,7 @@ void hvm_acpi_sleep_button(struct domain /* Set the correct value in the timer, accounting for time elapsed * since the last time we did that. */ -static void pmt_update_time(PMTState *s) +static void pmt_update_time(PMTState *s, bool_t update_sci) { uint64_t curr_gtime, tmp; uint32_t tmr_val = s->pm.tmr_val, msb = tmr_val & TMR_VAL_MSB; @@ -107,7 +107,8 @@ static void pmt_update_time(PMTState *s) if ( (tmr_val & TMR_VAL_MSB) != msb ) { s->pm.pm1a_sts |= TMR_STS; - pmt_update_sci(s); + if ( update_sci ) + pmt_update_sci(s); } } @@ -123,7 +124,7 @@ static void pmt_timer_callback(void *opa spin_lock(&s->lock); /* Recalculate the timer and make sure we get an SCI if we need one */ - pmt_update_time(s); + pmt_update_time(s, 1); /* How close are we to the next MSB flip? */ pmt_cycles_until_flip = TMR_VAL_MSB - (s->pm.tmr_val & (TMR_VAL_MSB - 1)); @@ -221,7 +222,7 @@ static int handle_pmt_io( if ( spin_trylock(&s->lock) ) { /* We hold the lock: update timer value and return it. */ - pmt_update_time(s); + pmt_update_time(s, 1); *val = s->pm.tmr_val; spin_unlock(&s->lock); } @@ -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 */ - 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); rc = hvm_save_entry(PMTIMER, 0, h, &s->pm); --------------010909040807060703020204 Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --------------010909040807060703020204--