[PATCH 2/3] KVM: fix apic timer save/migration when inactive

[PATCH 2/3] KVM: fix apic timer save/migration when inactive

[He, Qing]

[-- Attachment #1: Type: text/plain, Size: 4584 bytes --]

    
    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.

    Signed-off-by: Eddie (Yaozu) Dong <eddie.dong-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
    Signed-off-by: Qing He <qing.he-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>


---
 drivers/kvm/irq.h      |    2 ++
 drivers/kvm/kvm_main.c |    1 +
 drivers/kvm/lapic.c    |   41 +++++++++++++++++++++++++++++++++++------
 3 files changed, 38 insertions(+), 6 deletions(-)

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;
 		s64 period;	/* unit: ns */
 		u32 divide_count;
 		ktime_t last_update;
@@ -150,6 +151,7 @@ int kvm_apic_match_physical_addr(struct kvm_lapic
*apic, u16 dest);
 void kvm_ioapic_update_eoi(struct kvm *kvm, int vector);
 int kvm_apic_match_logical_addr(struct kvm_lapic *apic, u8 mda);
 int kvm_apic_set_irq(struct kvm_lapic *apic, u8 vec, u8 trig);
+void kvm_apic_pre_state_save(struct kvm_vcpu *vcpu);
 void kvm_apic_post_state_restore(struct kvm_vcpu *vcpu);
 int kvm_ioapic_init(struct kvm *kvm);
 void kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level);
diff --git a/drivers/kvm/kvm_main.c b/drivers/kvm/kvm_main.c
index f101430..694fe1e 100644
--- a/drivers/kvm/kvm_main.c
+++ b/drivers/kvm/kvm_main.c
@@ -2679,6 +2679,7 @@ static int kvm_vcpu_ioctl_get_lapic(struct
kvm_vcpu *vcpu,
 				    struct kvm_lapic_state *s)
 {
 	vcpu_load(vcpu);
+	kvm_apic_pre_state_save(vcpu);
 	memcpy(s->regs, vcpu->apic->regs, sizeof *s);
 	vcpu_put(vcpu);
 
diff --git a/drivers/kvm/lapic.c b/drivers/kvm/lapic.c
index 57810fa..ae85eb0 100644
--- a/drivers/kvm/lapic.c
+++ b/drivers/kvm/lapic.c
@@ -603,6 +603,7 @@ static void start_apic_timer(struct kvm_lapic *apic)
 	apic->timer.period = apic_get_reg(apic, APIC_TMICT) *
 		    APIC_BUS_CYCLE_NS * apic->timer.divide_count;
 	atomic_set(&apic->timer.pending, 0);
+	atomic_set(&apic->timer.active, 1);
 	hrtimer_start(&apic->timer.dev,
 		      ktime_add_ns(now, apic->timer.period),
 		      HRTIMER_MODE_ABS);
@@ -750,6 +751,7 @@ void kvm_free_apic(struct kvm_lapic *apic)
 	if (!apic)
 		return;
 
+	atomic_set(&apic->timer.active, 0);
 	hrtimer_cancel(&apic->timer.dev);
 
 	if (apic->regs_page) {
@@ -828,6 +830,7 @@ void kvm_lapic_reset(struct kvm_vcpu *vcpu)
 	ASSERT(apic != NULL);
 
 	/* Stop the timer in case it's a reset to an active apic */
+	atomic_set(&apic->timer.active, 0);
 	hrtimer_cancel(&apic->timer.dev);
 
 	apic_set_reg(apic, APIC_ID, vcpu->vcpu_id << 24);
@@ -900,6 +903,9 @@ static int __apic_timer_fn(struct kvm_lapic *apic)
 					apic->timer.dev.expires,
 					apic->timer.period);
 	}
+	else
+		atomic_set(&apic->timer.active, 0);
+		
 	return result;
 }
 
@@ -1018,9 +1024,23 @@ int kvm_get_apic_interrupt(struct kvm_vcpu *vcpu)
 	return vector;
 }
 
+void kvm_apic_pre_state_save(struct kvm_vcpu *vcpu)
+{
+	struct kvm_lapic *apic = vcpu->apic;
+	u32 tmict;
+
+	tmict = apic_get_reg(apic, APIC_TMICT);
+
+	if (atomic_read(&apic->timer.active))
+		apic_set_reg(apic, APIC_TMCCT, tmict);
+	else
+		apic_set_reg(apic, APIC_TMCCT, 0);
+}
+
 void kvm_apic_post_state_restore(struct kvm_vcpu *vcpu)
 {
 	struct kvm_lapic *apic = vcpu->apic;
+	u32 tmcct;
 
 	apic->base_address = vcpu->apic_base &
 			     MSR_IA32_APICBASE_BASE;
@@ -1028,7 +1048,13 @@ void kvm_apic_post_state_restore(struct kvm_vcpu
*vcpu)
 	apic_update_ppr(apic);
 	hrtimer_cancel(&apic->timer.dev);
 	update_divide_count(apic);
-	start_apic_timer(apic);
+	tmcct = apic_get_reg(apic, APIC_TMCCT);
+	if (tmcct) {
+		atomic_set(&apic->timer.active, 1);
+		start_apic_timer(apic);
+	}
+	else
+		atomic_set(&apic->timer.active, 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))
+		hrtimer_start(timer, timer->expires,
+			      HRTIMER_MODE_ABS);
 }
 EXPORT_SYMBOL_GPL(kvm_migrate_apic_timer);
 


[-- Attachment #2: 02-kvm-inactive-apic-timer-sr.patch --]
[-- Type: application/octet-stream, Size: 4428 bytes --]

    KVM: fix apic timer save/migration when inactive
    
    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.

    Signed-off-by: Eddie (Yaozu) Dong <eddie.dong@intel.com>
    Signed-off-by: Qing He <qing.he@intel.com>


---
 drivers/kvm/irq.h      |    2 ++
 drivers/kvm/kvm_main.c |    1 +
 drivers/kvm/lapic.c    |   41 +++++++++++++++++++++++++++++++++++------
 3 files changed, 38 insertions(+), 6 deletions(-)

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;
 		s64 period;	/* unit: ns */
 		u32 divide_count;
 		ktime_t last_update;
@@ -150,6 +151,7 @@ int kvm_apic_match_physical_addr(struct kvm_lapic *apic, u16 dest);
 void kvm_ioapic_update_eoi(struct kvm *kvm, int vector);
 int kvm_apic_match_logical_addr(struct kvm_lapic *apic, u8 mda);
 int kvm_apic_set_irq(struct kvm_lapic *apic, u8 vec, u8 trig);
+void kvm_apic_pre_state_save(struct kvm_vcpu *vcpu);
 void kvm_apic_post_state_restore(struct kvm_vcpu *vcpu);
 int kvm_ioapic_init(struct kvm *kvm);
 void kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level);
diff --git a/drivers/kvm/kvm_main.c b/drivers/kvm/kvm_main.c
index f101430..694fe1e 100644
--- a/drivers/kvm/kvm_main.c
+++ b/drivers/kvm/kvm_main.c
@@ -2679,6 +2679,7 @@ static int kvm_vcpu_ioctl_get_lapic(struct kvm_vcpu *vcpu,
 				    struct kvm_lapic_state *s)
 {
 	vcpu_load(vcpu);
+	kvm_apic_pre_state_save(vcpu);
 	memcpy(s->regs, vcpu->apic->regs, sizeof *s);
 	vcpu_put(vcpu);
 
diff --git a/drivers/kvm/lapic.c b/drivers/kvm/lapic.c
index 57810fa..ae85eb0 100644
--- a/drivers/kvm/lapic.c
+++ b/drivers/kvm/lapic.c
@@ -603,6 +603,7 @@ static void start_apic_timer(struct kvm_lapic *apic)
 	apic->timer.period = apic_get_reg(apic, APIC_TMICT) *
 		    APIC_BUS_CYCLE_NS * apic->timer.divide_count;
 	atomic_set(&apic->timer.pending, 0);
+	atomic_set(&apic->timer.active, 1);
 	hrtimer_start(&apic->timer.dev,
 		      ktime_add_ns(now, apic->timer.period),
 		      HRTIMER_MODE_ABS);
@@ -750,6 +751,7 @@ void kvm_free_apic(struct kvm_lapic *apic)
 	if (!apic)
 		return;
 
+	atomic_set(&apic->timer.active, 0);
 	hrtimer_cancel(&apic->timer.dev);
 
 	if (apic->regs_page) {
@@ -828,6 +830,7 @@ void kvm_lapic_reset(struct kvm_vcpu *vcpu)
 	ASSERT(apic != NULL);
 
 	/* Stop the timer in case it's a reset to an active apic */
+	atomic_set(&apic->timer.active, 0);
 	hrtimer_cancel(&apic->timer.dev);
 
 	apic_set_reg(apic, APIC_ID, vcpu->vcpu_id << 24);
@@ -900,6 +903,9 @@ static int __apic_timer_fn(struct kvm_lapic *apic)
 					apic->timer.dev.expires,
 					apic->timer.period);
 	}
+	else
+		atomic_set(&apic->timer.active, 0);
+		
 	return result;
 }
 
@@ -1018,9 +1024,23 @@ int kvm_get_apic_interrupt(struct kvm_vcpu *vcpu)
 	return vector;
 }
 
+void kvm_apic_pre_state_save(struct kvm_vcpu *vcpu)
+{
+	struct kvm_lapic *apic = vcpu->apic;
+	u32 tmict;
+
+	tmict = apic_get_reg(apic, APIC_TMICT);
+
+	if (atomic_read(&apic->timer.active))
+		apic_set_reg(apic, APIC_TMCCT, tmict);
+	else
+		apic_set_reg(apic, APIC_TMCCT, 0);
+}
+
 void kvm_apic_post_state_restore(struct kvm_vcpu *vcpu)
 {
 	struct kvm_lapic *apic = vcpu->apic;
+	u32 tmcct;
 
 	apic->base_address = vcpu->apic_base &
 			     MSR_IA32_APICBASE_BASE;
@@ -1028,7 +1048,13 @@ void kvm_apic_post_state_restore(struct kvm_vcpu *vcpu)
 	apic_update_ppr(apic);
 	hrtimer_cancel(&apic->timer.dev);
 	update_divide_count(apic);
-	start_apic_timer(apic);
+	tmcct = apic_get_reg(apic, APIC_TMCCT);
+	if (tmcct) {
+		atomic_set(&apic->timer.active, 1);
+		start_apic_timer(apic);
+	}
+	else
+		atomic_set(&apic->timer.active, 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))
+		hrtimer_start(timer, timer->expires,
+			      HRTIMER_MODE_ABS);
 }
 EXPORT_SYMBOL_GPL(kvm_migrate_apic_timer);
 


[-- Attachment #3: Type: text/plain, Size: 315 bytes --]

-------------------------------------------------------------------------
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/

[-- Attachment #4: Type: text/plain, Size: 186 bytes --]

_______________________________________________
kvm-devel mailing list
kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Re: [PATCH 2/3] KVM: fix apic timer save/migration when inactive

[Avi Kivity]

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);
>  
>   


-- 
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/

Re: [PATCH 2/3] KVM: fix apic timer save/migration when inactive

[Avi Kivity]

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/

Re: [PATCH 2/3] KVM: fix apic timer save/migration when inactive

[He, Qing]

[-- Attachment #1: Type: text/plain, Size: 2588 bytes --]



>-----Original Message-----
>From: Avi Kivity [mailto:avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org]
>Sent: 2007年9月6日 17:47
>To: He, Qing
>Cc: kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
>Subject: Re: [kvm-devel] [PATCH 2/3] KVM: fix apic timer save/migration when inactive
>
>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(...);

Thanks for commenting, previously I have thought of something like:
	if (hrtimer_active(...)) {
		hrtimer_cancel(...);
		hrtimer_start(...);
	}

But it is not able to handle pending state this way. Seems the return value of hrtimer_cancel is fine. Another question is, when the hrtimer function returns HRTIMER_NORESTART, does the state finally turn to HRTIMER_STATE_INACTIVE?

>
>
>
>--
>Any sufficiently difficult bug is indistinguishable from a feature.


[-- Attachment #2: Type: text/plain, Size: 315 bytes --]

-------------------------------------------------------------------------
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/

[-- Attachment #3: Type: text/plain, Size: 186 bytes --]

_______________________________________________
kvm-devel mailing list
kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Re: [PATCH 2/3] KVM: fix apic timer save/migration when inactive

[Avi Kivity]

[-- Attachment #1: Type: text/plain, Size: 1006 bytes --]

He, Qing wrote:
>
>>>> +    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(...);
>>     
>
> Thanks for commenting, previously I have thought of something like:
> 	if (hrtimer_active(...)) {
> 		hrtimer_cancel(...);
> 		hrtimer_start(...);
> 	}
>
> But it is not able to handle pending state this way. Seems the return value of hrtimer_cancel is fine. Another question is, when the hrtimer function returns HRTIMER_NORESTART, does the state finally turn to HRTIMER_STATE_INACTIVE?
>   

>From my reading of the hrtimer code, yes. But I would view any code
which depends on the state with suspicion.


-- 
error compiling committee.c: too many arguments to function



[-- Attachment #2: Type: text/plain, Size: 228 bytes --]

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

[-- Attachment #3: Type: text/plain, Size: 186 bytes --]

_______________________________________________
kvm-devel mailing list
kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Re: [PATCH 2/3] KVM: fix apic timer save/migration when inactive

[He, Qing]

[-- Attachment #1: Type: text/plain, Size: 1688 bytes --]

Avi,
	This is the updated patch, which avoids inactive hrtimers from unintended migrations.

save/restore timer logic is not included because it does not call hrtimer_cancel (hrtimer_active is different). It has relatively small impact, for it only affects save/restore at maybe guest BIOS time, maybe we can defer it to some later time

>-----Original Message-----
>From: Avi Kivity [mailto:avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org]
>Sent: 2007年9月9日 15:39
>To: He, Qing
>Cc: kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
>Subject: Re: [kvm-devel] [PATCH 2/3] KVM: fix apic timer save/migration when inactive
>
>He, Qing wrote:
>>
>>>>> +    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(...);
>>>
>>
>> Thanks for commenting, previously I have thought of something like:
>> 	if (hrtimer_active(...)) {
>> 		hrtimer_cancel(...);
>> 		hrtimer_start(...);
>> 	}
>>
>> But it is not able to handle pending state this way. Seems the return value of
>hrtimer_cancel is fine. Another question is, when the hrtimer function returns
>HRTIMER_NORESTART, does the state finally turn to HRTIMER_STATE_INACTIVE?
>>
>
>From my reading of the hrtimer code, yes. But I would view any code
>which depends on the state with suspicion.
>
>
>--
>error compiling committee.c: too many arguments to function

[-- Attachment #2: hrtimer-compat-wa.patch --]
[-- Type: application/octet-stream, Size: 449 bytes --]

diff --git a/kernel/external-module-compat.h b/kernel/external-module-compat.h
index 00b43f1..74bc072 100644
--- a/kernel/external-module-compat.h
+++ b/kernel/external-module-compat.h
@@ -365,6 +365,6 @@ static inline void preempt_notifier_sys_exit(void) {}
 #endif
 
 /* HRTIMER_MODE_ABS started life with a different name */
-#ifndef HRTIMER_MODE_ABS
+#if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,21)
 #define HRTIMER_MODE_ABS HRTIMER_ABS
 #endif

[-- Attachment #3: Type: text/plain, Size: 228 bytes --]

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

[-- Attachment #4: Type: text/plain, Size: 186 bytes --]

_______________________________________________
kvm-devel mailing list
kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Re: [PATCH 2/3] KVM: fix apic timer save/migration wheninactive

[He, Qing]

[-- Attachment #1: Type: text/plain, Size: 2188 bytes --]

resended, due to wrong attachment

>-----Original Message-----
>From: kvm-devel-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
>[mailto:kvm-devel-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org] On Behalf Of He, Qing
>Sent: 2007年9月10日 13:59
>To: Avi Kivity
>Cc: kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
>Subject: Re: [kvm-devel] [PATCH 2/3] KVM: fix apic timer save/migration wheninactive
>
>Avi,
>	This is the updated patch, which avoids inactive hrtimers from unintended
>migrations.
>
>save/restore timer logic is not included because it does not call hrtimer_cancel
>(hrtimer_active is different). It has relatively small impact, for it only affects save/restore
>at maybe guest BIOS time, maybe we can defer it to some later time
>
>>-----Original Message-----
>>From: Avi Kivity [mailto:avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org]
>>Sent: 2007年9月9日 15:39
>>To: He, Qing
>>Cc: kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
>>Subject: Re: [kvm-devel] [PATCH 2/3] KVM: fix apic timer save/migration when inactive
>>
>>He, Qing wrote:
>>>
>>>>>> +    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(...);
>>>>
>>>
>>> Thanks for commenting, previously I have thought of something like:
>>> 	if (hrtimer_active(...)) {
>>> 		hrtimer_cancel(...);
>>> 		hrtimer_start(...);
>>> 	}
>>>
>>> But it is not able to handle pending state this way. Seems the return value of
>>hrtimer_cancel is fine. Another question is, when the hrtimer function returns
>>HRTIMER_NORESTART, does the state finally turn to HRTIMER_STATE_INACTIVE?
>>>
>>
>>From my reading of the hrtimer code, yes. But I would view any code
>>which depends on the state with suspicion.
>>
>>
>>--
>>error compiling committee.c: too many arguments to function

[-- Attachment #2: kvm-hrtimer-migration-fix.patch --]
[-- Type: application/octet-stream, Size: 631 bytes --]

 drivers/kvm/lapic.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/kvm/lapic.c b/drivers/kvm/lapic.c
index 4374855..a894dee 100644
--- a/drivers/kvm/lapic.c
+++ b/drivers/kvm/lapic.c
@@ -1037,11 +1037,12 @@ 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);
+	if (!apic)
+		return;
+
+	timer = &apic->timer.dev;
+	if (hrtimer_cancel(timer))
 		hrtimer_start(timer, timer->expires, HRTIMER_MODE_ABS);
-	}
 }
 EXPORT_SYMBOL_GPL(kvm_migrate_apic_timer);
 

[-- Attachment #3: Type: text/plain, Size: 228 bytes --]

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

[-- Attachment #4: Type: text/plain, Size: 186 bytes --]

_______________________________________________
kvm-devel mailing list
kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Re: [PATCH 2/3] KVM: fix apic timer save/migration wheninactive

[Avi Kivity]

[-- Attachment #1: Type: text/plain, Size: 3207 bytes --]

He, Qing wrote:
> resended, due to wrong attachment
>
>   

Patch looks good, but needs a changelog comment and a signoff.
>> -----Original Message-----
>> From: kvm-devel-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
>> [mailto:kvm-devel-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org] On Behalf Of He, Qing
>> Sent: 2007年9月10日 13:59
>> To: Avi Kivity
>> Cc: kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
>> Subject: Re: [kvm-devel] [PATCH 2/3] KVM: fix apic timer save/migration wheninactive
>>
>> Avi,
>> 	This is the updated patch, which avoids inactive hrtimers from unintended
>> migrations.
>>
>> save/restore timer logic is not included because it does not call hrtimer_cancel
>> (hrtimer_active is different). It has relatively small impact, for it only affects save/restore
>> at maybe guest BIOS time, maybe we can defer it to some later time
>>
>>     
>>> -----Original Message-----
>>> From: Avi Kivity [mailto:avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org]
>>> Sent: 2007年9月9日 15:39
>>> To: He, Qing
>>> Cc: kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
>>> Subject: Re: [kvm-devel] [PATCH 2/3] KVM: fix apic timer save/migration when inactive
>>>
>>> He, Qing wrote:
>>>       
>>>>>>> +    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(...);
>>>>>
>>>>>           
>>>> Thanks for commenting, previously I have thought of something like:
>>>> 	if (hrtimer_active(...)) {
>>>> 		hrtimer_cancel(...);
>>>> 		hrtimer_start(...);
>>>> 	}
>>>>
>>>> But it is not able to handle pending state this way. Seems the return value of
>>>>         
>>> hrtimer_cancel is fine. Another question is, when the hrtimer function returns
>>> HRTIMER_NORESTART, does the state finally turn to HRTIMER_STATE_INACTIVE?
>>>       
>> >From my reading of the hrtimer code, yes. But I would view any code
>>     
>>> which depends on the state with suspicion.
>>>
>>>
>>> --
>>> error compiling committee.c: too many arguments to function
>>>       
>>> ------------------------------------------------------------------------
>>>
>>> -------------------------------------------------------------------------
>>> This SF.net email is sponsored by: Microsoft
>>> Defy all challenges. Microsoft(R) Visual Studio 2005.
>>> http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
>>> ------------------------------------------------------------------------
>>>
>>> _______________________________________________
>>> kvm-devel mailing list
>>> kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
>>> https://lists.sourceforge.net/lists/listinfo/kvm-devel
>>>       


-- 
error compiling committee.c: too many arguments to function



[-- Attachment #2: Type: text/plain, Size: 228 bytes --]

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

[-- Attachment #3: Type: text/plain, Size: 186 bytes --]

_______________________________________________
kvm-devel mailing list
kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Re: [PATCH 2/3] KVM: fix apic timer save/migration wheninactive

[He, Qing]

[-- Attachment #1: Type: text/plain, Size: 3846 bytes --]

KVM: fix apic timer migration when inactive

When local apic timer is inactive or is expired in one shot mode, it should not be restarted on vcpu and hrtimer migration. This patch fixes this.

Signed-off-by: Qing He <qing.he-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Signed-off-by: Avi Kivity <avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org>



>-----Original Message-----
>From: Avi Kivity [mailto:avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org]
>Sent: 2007年9月10日 15:27
>To: He, Qing
>Cc: kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
>Subject: Re: [kvm-devel] [PATCH 2/3] KVM: fix apic timer save/migration wheninactive
>
>He, Qing wrote:
>> resended, due to wrong attachment
>>
>>
>
>Patch looks good, but needs a changelog comment and a signoff.
>>> -----Original Message-----
>>> From: kvm-devel-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
>>> [mailto:kvm-devel-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org] On Behalf Of He, Qing
>>> Sent: 2007年9月10日 13:59
>>> To: Avi Kivity
>>> Cc: kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
>>> Subject: Re: [kvm-devel] [PATCH 2/3] KVM: fix apic timer save/migration
>wheninactive
>>>
>>> Avi,
>>> 	This is the updated patch, which avoids inactive hrtimers from unintended
>>> migrations.
>>>
>>> save/restore timer logic is not included because it does not call hrtimer_cancel
>>> (hrtimer_active is different). It has relatively small impact, for it only affects
>save/restore
>>> at maybe guest BIOS time, maybe we can defer it to some later time
>>>
>>>
>>>> -----Original Message-----
>>>> From: Avi Kivity [mailto:avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org]
>>>> Sent: 2007年9月9日 15:39
>>>> To: He, Qing
>>>> Cc: kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
>>>> Subject: Re: [kvm-devel] [PATCH 2/3] KVM: fix apic timer save/migration when
>inactive
>>>>
>>>> He, Qing wrote:
>>>>
>>>>>>>> +    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(...);
>>>>>>
>>>>>>
>>>>> Thanks for commenting, previously I have thought of something like:
>>>>> 	if (hrtimer_active(...)) {
>>>>> 		hrtimer_cancel(...);
>>>>> 		hrtimer_start(...);
>>>>> 	}
>>>>>
>>>>> But it is not able to handle pending state this way. Seems the return value of
>>>>>
>>>> hrtimer_cancel is fine. Another question is, when the hrtimer function returns
>>>> HRTIMER_NORESTART, does the state finally turn to HRTIMER_STATE_INACTIVE?
>>>>
>>> >From my reading of the hrtimer code, yes. But I would view any code
>>>
>>>> which depends on the state with suspicion.
>>>>
>>>>
>>>> --
>>>> error compiling committee.c: too many arguments to function
>>>>
>>>> ------------------------------------------------------------------------
>>>>
>>>> -------------------------------------------------------------------------
>>>> This SF.net email is sponsored by: Microsoft
>>>> Defy all challenges. Microsoft(R) Visual Studio 2005.
>>>> http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
>>>> ------------------------------------------------------------------------
>>>>
>>>> _______________________________________________
>>>> kvm-devel mailing list
>>>> kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
>>>> https://lists.sourceforge.net/lists/listinfo/kvm-devel
>>>>
>
>
>--
>error compiling committee.c: too many arguments to function


[-- Attachment #2: Type: text/plain, Size: 228 bytes --]

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

[-- Attachment #3: Type: text/plain, Size: 186 bytes --]

_______________________________________________
kvm-devel mailing list
kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/kvm-devel