Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) client-ip=2620:137:e000::3:2;
Date:   Fri, 13 Oct 2023 12:02:47 -0700
In-Reply-To: <5ea168df6dfbe910524a381b88347636e1a6a3bc.camel@infradead.org>
Mime-Version: 1.0
References: <ZRtl94_rIif3GRpu@google.com> <9975969725a64c2ba2b398244dba3437bff5154e.camel@infradead.org>
 <ZRysGAgk6W1bpXdl@google.com> <d6dc1242ff731cf0f2826760816081674ade9ff9.camel@infradead.org>
 <ZR2pwdZtO3WLCwjj@google.com> <34057852-f6c0-d6d5-261f-bbb5fa056425@oracle.com>
 <ZSXqZOgLYkwLRWLO@google.com> <8f3493ca4c0e726d5c3876bb7dd2cfc432d9deaa.camel@infradead.org>
 <ZSmHcECyt5PdZyIZ@google.com> <5ea168df6dfbe910524a381b88347636e1a6a3bc.camel@infradead.org>
Message-ID: <ZSmUV3AoFWBTMx-o@google.com>
Subject: Re: [PATCH RFC 1/1] KVM: x86: add param to update master clock periodically
From:   Sean Christopherson <seanjc@google.com>
To:     David Woodhouse <dwmw2@infradead.org>
Cc:     Dongli Zhang <dongli.zhang@oracle.com>,
        Joe Jin <joe.jin@oracle.com>, x86@kernel.org,
        kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
        pbonzini@redhat.com, tglx@linutronix.de, mingo@redhat.com,
        bp@alien8.de, dave.hansen@linux.intel.com
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Fri, Oct 13, 2023, David Woodhouse wrote:
> On Fri, 2023-10-13 at 11:07 -0700, Sean Christopherson wrote:
> > I generally support the idea, but I think it needs to an opt-in from us=
erspace.
> > Essentially a "I pinky swear to give all vCPUs the same TSC frequency, =
to not
> > suspend the host, and to not run software/firmware that writes IA32_TSC=
_ADJUST".
> > AFAICT, there are too many edge cases and assumptions about userspace f=
or KVM to
> > safely couple kvmclock to guest TSC by default.
>=20
> I think IA32_TSC_ADJUST is OK, isn't it? There is a "real" TSC value
> and if vCPUs adjust themselves forward and backwards from that, it's
> just handled as a delta.

I meant the host writing IA32_TSC_ADJUST.  E.g. if a host SMM handler mucks=
 with
TSC offsets to try and hide the time spent in the SMM handler, then the pla=
tform
owner gets to keep the pieces.

> And we solved 'give all vCPUS the same TSC frequency' by making that
> KVM-wide.
>=20
> Maybe suspending and resuming the host can be treated like live
> migration, where you know the host TSC is different so you have to make
> do with a delta based on CLOCK_TAI.
>=20
> But while I'm picking on the edge cases and suggesting that we *can*
> cope with some of them, I do agree with your suggestion that "let
> kvmclock run by itself without being clamped back to
> CLOCK_MONOTONIC_RAW" should be an opt *in* feature.

Yeah, I'm of the mind that just because we can cope with some edge cases, d=
oesn't
mean we should.  At this point, kvmclock really should be considered deprec=
ated
on modern hardware.  I.e. needs to be supported for older VMs, but shouldn'=
t be
advertised/used when creating entirely new VMs.

Hence my desire to go with a low effort solution for getting kvmclock to pl=
ay nice
with modern hardware.

> > > [1] Yes, I believe "back" does happen. I have test failures in my que=
ue
> > > to look at, where guests see the "Xen" clock going backwards.
> >=20
> > Yeah, I assume "back" can happen based purely on the wierdness of the p=
vclock math.o
> >=20
> > What if we add a module param to disable KVM's TSC synchronization craz=
iness
> > entirely?=C2=A0 If we first clean up the peroidic sync mess, then it se=
ems like it'd
> > be relatively straightforward to let kill off all of the synchronizatio=
n, including
> > the synchronization of kvmclock to the host's TSC-based CLOCK_MONOTONIC=
_RAW.
> >=20
> > Not intended to be a functional patch...
>=20
> Will stare harder at the actual patch when it isn't Friday night.
>=20
> In the meantime, I do think a KVM cap that the VMM opts into is better
> than a module param?

Hmm, yeah, I think a capability would be cleaner overall.  Then KVM could r=
eturn
-EINVAL instead of silently forcing synchronization if the platform conditi=
ons
aren't meant, e.g. if the TSC isn't constant or if the host timekeeping isn=
't
using TSC.

The interaction with kvmclock_periodic_sync might be a bit awkward, but tha=
t's
easy enough to solve with a wrapper.