Re: [PATCH] x86/hvm: fix corrupt ACPI PM-Timer during live migration

[Kouya Shimura <kouya@jp.fujitsu.com>]

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

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 <kouya@jp.fujitsu.com> 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


[-- Attachment #2: fix_corrupt_saved_pmtimer_v2.patch --]
[-- Type: text/x-patch, Size: 2951 bytes --]

# HG changeset patch
# User Kouya Shimura <kouya@jp.fujitsu.com>
# 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 <kouya@jp.fujitsu.com>

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

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel