From mboxrd@z Thu Jan 1 00:00:00 1970 From: Radim =?utf-8?B?S3LEjW3DocWZ?= Subject: Re: x86: kvm: Revert "remove sched notifier for cross-cpu migrations" Date: Wed, 25 Mar 2015 12:08:14 +0100 Message-ID: <20150325110814.GE21522@potion.brq.redhat.com> References: <20150323232151.GA12772@amt.cnet> <20150324153412.GB21710@potion.brq.redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: Marcelo Tosatti , kvm-devel , stable , Paolo Bonzini To: Andy Lutomirski Return-path: Content-Disposition: inline In-Reply-To: Sender: stable-owner@vger.kernel.org List-Id: kvm.vger.kernel.org 2015-03-24 15:33-0700, Andy Lutomirski: > On Tue, Mar 24, 2015 at 8:34 AM, Radim Kr=C4=8Dm=C3=A1=C5=99 wrote: > > What is the problem? >=20 > The kvmclock spec says that the host will increment a version field t= o > an odd number, then update stuff, then increment it to an even number= =2E > The host is buggy and doesn't do this, and the result is observable > when one vcpu reads another vcpu's kvmclock data. >=20 > Since there's no good way for a guest kernel to keep its vdso from > reading a different vcpu's kvmclock data, this is a real corner-case > bug. This patch allows the vdso to retry when this happens. I don't > think it's a great solution, but it should mostly work. Great explanation, thank you. Reverting the patch protects us from any migration, but I don't think w= e need to care about changing VCPUs as long as we read a consistent data from kvmclock. (VCPU can change outside of this loop too, so it doesn'= t matter if we return a value not fit for this VCPU.) I think we could drop the second __getcpu if our kvmclock was being handled better; maybe with a patch like the one below: diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index cc2c759f69a3..8658599e0024 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -1658,12 +1658,24 @@ static int kvm_guest_time_update(struct kvm_vcp= u *v) &guest_hv_clock, sizeof(guest_hv_clock)))) return 0; =20 - /* - * The interface expects us to write an even number signaling that th= e - * update is finished. Since the guest won't see the intermediate - * state, we just increase by 2 at the end. + /* A guest can read other VCPU's kvmclock; specification says that + * version is odd if data is being modified and even after it is + * consistent. + * We write three times to be sure. + * 1) update version to odd number + * 2) write modified data (version is still odd) + * 3) update version to even number + * + * TODO: optimize + * - only two writes should be enough -- version is first + * - the second write could update just version */ - vcpu->hv_clock.version =3D guest_hv_clock.version + 2; + guest_hv_clock.version +=3D 1; + kvm_write_guest_cached(v->kvm, &vcpu->pv_time, + &guest_hv_clock, + sizeof(guest_hv_clock)); + + vcpu->hv_clock.version =3D guest_hv_clock.version; =20 /* retain PVCLOCK_GUEST_STOPPED if set in guest copy */ pvclock_flags =3D (guest_hv_clock.flags & PVCLOCK_GUEST_STOPPED); @@ -1684,6 +1696,11 @@ static int kvm_guest_time_update(struct kvm_vcpu= *v) kvm_write_guest_cached(v->kvm, &vcpu->pv_time, &vcpu->hv_clock, sizeof(vcpu->hv_clock)); + + vcpu->hv_clock.version +=3D 1; + kvm_write_guest_cached(v->kvm, &vcpu->pv_time, + &vcpu->hv_clock, + sizeof(vcpu->hv_clock)); return 0; } =20