From mboxrd@z Thu Jan 1 00:00:00 1970 From: Radim =?utf-8?B?S3LEjW3DocWZ?= Subject: Re: [RFC PATCH 0/2] kvmclock: fix ABI breakage from PVCLOCK_COUNTS_FROM_ZERO. Date: Mon, 21 Sep 2015 22:00:27 +0200 Message-ID: <20150921200026.GB2734@potion.brq.redhat.com> References: <1442591670-5216-1-git-send-email-rkrcmar@redhat.com> <20150920225742.GA27666@amt.cnet> <20150921151209.GA2734@potion.brq.redhat.com> <20150921155224.GA12938@amt.cnet> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: linux-kernel@vger.kernel.org, kvm@vger.kernel.org, Paolo Bonzini , Luiz Capitulino To: Marcelo Tosatti Return-path: Content-Disposition: inline In-Reply-To: <20150921155224.GA12938@amt.cnet> Sender: linux-kernel-owner@vger.kernel.org List-Id: kvm.vger.kernel.org 2015-09-21 12:52-0300, Marcelo Tosatti: > On Mon, Sep 21, 2015 at 05:12:10PM +0200, Radim Kr=C4=8Dm=C3=A1=C5=99= wrote: >> 2015-09-20 19:57-0300, Marcelo Tosatti: >>> Is it counting from zero that breaks SLES10? >>=20 >> Not by itself, treating MSR_KVM_SYSTEM_TIME as one-shot initializer = did. >> The guest wants to write to MSR_KVM_SYSTEM_TIME as much as it likes = to, >> while still keeping system time; we used to allow that, which means= an >> ABI breakage. (And we can't even say that guest's behaviour is agai= nst >> the spec ...) >=20 > Because this behaviour was not defined. It is defined by implementation. > Can't you just condition PVCLOCK_COUNTS_FROM_ZERO behaviour on > boot_vcpu_runs_old_kvmclock =3D=3D false?=20 > The patch would be much simpler. If you mean the hunk in cover letter, I don't like it because we presum= e that no other guests were broken. I really don't like it so I thought about other problems with PVCLOCK_COUNTS_FROM_ZERO ... have you tried to hot-replug VCPU 0? It doesn't work well ;) We don't want to guess what the guest wants so I'd go for the opt-in paravirt feature unless counting from zero can be done in guest alone. > The problem is, "selecting one read as the initial point" is inherent= ly > racy: that delta is relative to one moment (kvmclock read) at one vcp= u, > but must be applied to all vcpus. I don't think that is a problem. Kvmclock has a notion of a global system_time in nanoseconds (one value that defines the time, assigned with VM ioctl KVM_SET_CLOCK) and tries to propagate system_time into guest VCPUs as precisely as possible with the help of TSC. sched_clock uses kvmclock to get nanoseconds since the system was brought up and [1/2] only works with this abstracted ns count. A poorly synchronized initial read is irrelevant because all VCPUs will be using the same constant offset. (We can never know the precise time anyway.) > Besides: >=20 > 1) Stable sched clock in guest does not depend on > KVM_FEATURE_CLOCKSOURCE_STABLE_BIT. Yes, thanks, I will remove that requirement in v1. (We'd need to improve a loss of PVCLOCK_TSC_STABLE_BIT otherwise anyway.) Because the clutchy dependency on PVCLOCK_TSC_STABLE_BIT is going away, there is now one possible unsigned overflow: in case the clock was very imprecise and VCPU1 managed to get smaller system_time than VCPU0 at th= e time of initialization. It's very unlikely that we'll ever reach legal overflow so I can add a condition there. > 2) You rely on monotonicity across vcpus to perform=20 > the 'minus delta that was read on vcpu0' calculation, but=20 > monotonicity across vcpus can fail during runtime > (say host clocksource goes tsc->hpet due to tsc instabilit= y). That could be a problem, but I'm adding a VCPU independent constant to all reads -- does the new code rely on monoticity in places where the old one didn't?