From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [PATCH 2/3] KVM: fix apic timer save/migration when inactive Date: Thu, 06 Sep 2007 12:47:26 +0300 Message-ID: <46DFCCAE.3040507@qumranet.com> References: <37E52D09333DE2469A03574C88DBF40FA9C208@pdsmsx414.ccr.corp.intel.com> <46DFC523.7090806@qumranet.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Cc: kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org To: "He, Qing" Return-path: In-Reply-To: <46DFC523.7090806-atKUWr5tajBWk0Htik3J/w@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: kvm-devel-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org Errors-To: kvm-devel-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org List-Id: kvm.vger.kernel.org Avi Kivity wrote: > He, Qing wrote: >> When local apic timer is inactive or is expired in oneshot >> mode, it >> should not be restarted in save/restore or hrtimer migration. This >> patch fixes this. >> >> diff --git a/drivers/kvm/irq.h b/drivers/kvm/irq.h >> index 5f97e25..68d454c 100644 >> --- a/drivers/kvm/irq.h >> +++ b/drivers/kvm/irq.h >> @@ -110,6 +110,7 @@ struct kvm_lapic { >> struct kvm_io_device dev; >> struct { >> atomic_t pending; >> + atomic_t active; >> > > This is atomic, but you never use read-modify-write instructions (read > and write are atomic on a simple int). > >> + >> + if (atomic_read(&apic->timer.active)) >> > > What if the timer fires here? > >> + apic_set_reg(apic, APIC_TMCCT, tmict); >> + else >> + apic_set_reg(apic, APIC_TMCCT, 0); >> +} >> + >> void kvm_migrate_apic_timer(struct kvm_vcpu *vcpu) >> @@ -1036,11 +1062,14 @@ void kvm_migrate_apic_timer(struct kvm_vcpu >> *vcpu) >> struct kvm_lapic *apic = vcpu->apic; >> struct hrtimer *timer; >> >> - if (apic) { >> - timer = &apic->timer.dev; >> - hrtimer_cancel(timer); >> - hrtimer_start(timer, timer->expires, HRTIMER_MODE_ABS); >> - } >> + if (!apic) >> + return; >> + >> + timer = &apic->timer.dev; >> + hrtimer_cancel(timer); >> + if (atomic_read(&apic->timer.active)) >> > > Or here? > >> + hrtimer_start(timer, timer->expires, >> + HRTIMER_MODE_ABS); >> } >> EXPORT_SYMBOL_GPL(kvm_migrate_apic_timer); >> >> > I think you could use the return value of hrtimer_cancel() here: if (hrtimer_cancel(...)) hrtimer_start(...); -- Any sufficiently difficult bug is indistinguishable from a feature. ------------------------------------------------------------------------- This SF.net email is sponsored by: Splunk Inc. Still grepping through log files to find problems? Stop. Now Search log events and configuration files using AJAX and a browser. Download your FREE copy of Splunk now >> http://get.splunk.com/