| Message ID | 20121031224823.865958085@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Oct 31, 2012 at 08:46:57PM -0200, Marcelo Tosatti wrote: > Otherwise its possible for an unrelated KVM_REQ_UPDATE_CLOCK (such as due to CPU > migration) to clear the bit. > > Noticed by Paolo Bonzini. > > Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com> > Reviewed-by: Gleb Natapov <gleb@redhat.com> Small nitpick bellow. Also do we really need to call kvm_guest_time_update() on a guest pause? Wouldn't separate request bit, which only sets the flag, suffice? > Index: vsyscall/arch/x86/kvm/x86.c > =================================================================== > --- vsyscall.orig/arch/x86/kvm/x86.c > +++ vsyscall/arch/x86/kvm/x86.c > @@ -1143,6 +1143,7 @@ static int kvm_guest_time_update(struct > unsigned long this_tsc_khz; > s64 kernel_ns, max_kernel_ns; > u64 tsc_timestamp; > + struct pvclock_vcpu_time_info *guest_hv_clock; > u8 pvclock_flags; > > /* Keep irq disabled to prevent changes to the clock */ > @@ -1226,13 +1227,6 @@ static int kvm_guest_time_update(struct > vcpu->last_kernel_ns = kernel_ns; > vcpu->last_guest_tsc = tsc_timestamp; > > - pvclock_flags = 0; > - if (vcpu->pvclock_set_guest_stopped_request) { > - pvclock_flags |= PVCLOCK_GUEST_STOPPED; > - vcpu->pvclock_set_guest_stopped_request = false; > - } > - > - vcpu->hv_clock.flags = pvclock_flags; > > /* > * The interface expects us to write an even number signaling that the > @@ -1243,6 +1237,18 @@ static int kvm_guest_time_update(struct > > shared_kaddr = kmap_atomic(vcpu->time_page); > > + guest_hv_clock = shared_kaddr + vcpu->time_offset; > + > + /* retain PVCLOCK_GUEST_STOPPED if set in guest copy */ > + pvclock_flags = (guest_hv_clock->flags & PVCLOCK_GUEST_STOPPED); > + > + if (vcpu->pvclock_set_guest_stopped_request) { > + pvclock_flags |= PVCLOCK_GUEST_STOPPED; > + vcpu->pvclock_set_guest_stopped_request = false; > + } > + > + vcpu->hv_clock.flags = pvclock_flags; > + > memcpy(shared_kaddr + vcpu->time_offset, &vcpu->hv_clock, We can use guest_hv_clock here now. > sizeof(vcpu->hv_clock)); > > -- Gleb. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/01/2012 02:46 AM, Marcelo Tosatti wrote: > Otherwise its possible for an unrelated KVM_REQ_UPDATE_CLOCK (such as due to CPU > migration) to clear the bit. > > Noticed by Paolo Bonzini. > > Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com> > > Index: vsyscall/arch/x86/kvm/x86.c > =================================================================== Reviewed-by: Glauber Costa <glommer@parallels.com> -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Nov 01, 2012 at 12:39:30PM +0200, Gleb Natapov wrote: > On Wed, Oct 31, 2012 at 08:46:57PM -0200, Marcelo Tosatti wrote: > > Otherwise its possible for an unrelated KVM_REQ_UPDATE_CLOCK (such as due to CPU > > migration) to clear the bit. > > > > Noticed by Paolo Bonzini. > > > > Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com> > > > Reviewed-by: Gleb Natapov <gleb@redhat.com> > > Small nitpick bellow. > > Also do we really need to call kvm_guest_time_update() on a guest pause? > Wouldn't separate request bit, which only sets the flag, suffice? Management of vcpu clock area better be isolated in a single place. > > Index: vsyscall/arch/x86/kvm/x86.c > > =================================================================== > > --- vsyscall.orig/arch/x86/kvm/x86.c > > +++ vsyscall/arch/x86/kvm/x86.c > > @@ -1143,6 +1143,7 @@ static int kvm_guest_time_update(struct > > unsigned long this_tsc_khz; > > s64 kernel_ns, max_kernel_ns; > > u64 tsc_timestamp; > > + struct pvclock_vcpu_time_info *guest_hv_clock; > > u8 pvclock_flags; > > > > /* Keep irq disabled to prevent changes to the clock */ > > @@ -1226,13 +1227,6 @@ static int kvm_guest_time_update(struct > > vcpu->last_kernel_ns = kernel_ns; > > vcpu->last_guest_tsc = tsc_timestamp; > > > > - pvclock_flags = 0; > > - if (vcpu->pvclock_set_guest_stopped_request) { > > - pvclock_flags |= PVCLOCK_GUEST_STOPPED; > > - vcpu->pvclock_set_guest_stopped_request = false; > > - } > > - > > - vcpu->hv_clock.flags = pvclock_flags; > > > > /* > > * The interface expects us to write an even number signaling that the > > @@ -1243,6 +1237,18 @@ static int kvm_guest_time_update(struct > > > > shared_kaddr = kmap_atomic(vcpu->time_page); > > > > + guest_hv_clock = shared_kaddr + vcpu->time_offset; > > + > > + /* retain PVCLOCK_GUEST_STOPPED if set in guest copy */ > > + pvclock_flags = (guest_hv_clock->flags & PVCLOCK_GUEST_STOPPED); > > + > > + if (vcpu->pvclock_set_guest_stopped_request) { > > + pvclock_flags |= PVCLOCK_GUEST_STOPPED; > > + vcpu->pvclock_set_guest_stopped_request = false; > > + } > > + > > + vcpu->hv_clock.flags = pvclock_flags; > > + > > memcpy(shared_kaddr + vcpu->time_offset, &vcpu->hv_clock, > We can use guest_hv_clock here now. > > > sizeof(vcpu->hv_clock)); > > > > > > -- > Gleb. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Index: vsyscall/arch/x86/kvm/x86.c =================================================================== --- vsyscall.orig/arch/x86/kvm/x86.c +++ vsyscall/arch/x86/kvm/x86.c @@ -1143,6 +1143,7 @@ static int kvm_guest_time_update(struct unsigned long this_tsc_khz; s64 kernel_ns, max_kernel_ns; u64 tsc_timestamp; + struct pvclock_vcpu_time_info *guest_hv_clock; u8 pvclock_flags; /* Keep irq disabled to prevent changes to the clock */ @@ -1226,13 +1227,6 @@ static int kvm_guest_time_update(struct vcpu->last_kernel_ns = kernel_ns; vcpu->last_guest_tsc = tsc_timestamp; - pvclock_flags = 0; - if (vcpu->pvclock_set_guest_stopped_request) { - pvclock_flags |= PVCLOCK_GUEST_STOPPED; - vcpu->pvclock_set_guest_stopped_request = false; - } - - vcpu->hv_clock.flags = pvclock_flags; /* * The interface expects us to write an even number signaling that the @@ -1243,6 +1237,18 @@ static int kvm_guest_time_update(struct shared_kaddr = kmap_atomic(vcpu->time_page); + guest_hv_clock = shared_kaddr + vcpu->time_offset; + + /* retain PVCLOCK_GUEST_STOPPED if set in guest copy */ + pvclock_flags = (guest_hv_clock->flags & PVCLOCK_GUEST_STOPPED); + + if (vcpu->pvclock_set_guest_stopped_request) { + pvclock_flags |= PVCLOCK_GUEST_STOPPED; + vcpu->pvclock_set_guest_stopped_request = false; + } + + vcpu->hv_clock.flags = pvclock_flags; + memcpy(shared_kaddr + vcpu->time_offset, &vcpu->hv_clock, sizeof(vcpu->hv_clock));
Otherwise its possible for an unrelated KVM_REQ_UPDATE_CLOCK (such as due to CPU migration) to clear the bit. Noticed by Paolo Bonzini. Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com> -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html