Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:5 as permitted sender) client-ip=2620:137:e000::3:5;
Date:   Wed, 13 Sep 2023 15:15:51 +0000
In-Reply-To: <b83a52bf72b951e69d3df23fff144899b0d6c11d.camel@infradead.org>
Mime-Version: 1.0
References: <20230801034524.64007-1-likexu@tencent.com> <ZNa9QyRmuAjNAonC@google.com>
 <055482bec09cae1ea56f979893c6b67e9d6b26a2.camel@infradead.org>
 <ZQHMM8/7xXReZHdD@google.com> <b83a52bf72b951e69d3df23fff144899b0d6c11d.camel@infradead.org>
Message-ID: <ZQHSJ9Epx1oNTZGE@google.com>
Subject: Re: [PATCH v4] KVM: x86/tsc: Don't sync user changes to TSC with
 KVM-initiated change
From:   Sean Christopherson <seanjc@google.com>
To:     David Woodhouse <dwmw2@infradead.org>
Cc:     Like Xu <like.xu.linux@gmail.com>,
        Paolo Bonzini <pbonzini@redhat.com>,
        Oliver Upton <oliver.upton@linux.dev>, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Wed, Sep 13, 2023, David Woodhouse wrote:
> On Wed, 2023-09-13 at 07:50 -0700, Sean Christopherson wrote:
> > On Wed, Sep 13, 2023, David Woodhouse wrote:
> > > Userspace used to be able to force a sync by writing zero. You are
> > > removing that from the ABI without any explanation about why;
> >=20
> > No, my suggestion did not remove that from the ABI.=C2=A0 A @user_value=
 of '0' would
> > still force synchronization.
>=20
> Ah, OK. Yes, you're right. Thanks.
>=20
> > It's necessary for "user_set_tsc" to be an accurate name.=C2=A0 The cod=
e in v6 yields
> > "user_set_tsc_to_non_zero_value".=C2=A0 And I don't think it's just a n=
aming issue,
>=20
> In another thread, you said that the sync code doesn't differentiate
> between userspace initializing the TSC And userspace attempting to
> synchronize the TSC. I responded that *I* don't differentiate the two
> and couldn't see the difference.
>=20
> I think we were both wrong.=C2=A0Userspace does *explicitly* synchronize =
the
> TSC by writing zero, and the sync code *does* explicitly handle that,
> yes?

Doh, by "sync code" I meant the "conditionally sync" code, i.e. the data !=
=3D 0 path.

> And the reason I mention it here is that we could perhaps reasonable
> say that userspace *syncing* the TSC like that is not the same as
> userspace *setting* the TSC, and that it's OK for user_set_tsc to
> remain false? It saves adding another argument to kvm_synchronize_tsc()
> making it even more complex for a use case that just doesn't make sense
> anyway...
>=20
> > e.g. if userspace writes '0' immediately after creating, and then later=
 writes a
> > small delta, the v6 code wouldn't trigger synchronization because "user=
_set_tsc"
> > would be left unseft by the write of '0'.
>=20
> True, but that's the existing behaviour,

No?  The existing code will fall into the "conditionally sync" logic for an=
y
non-zero value.

		if (data =3D=3D 0) {
			/*
			 * detection of vcpu initialization -- need to sync
			 * with other vCPUs. This particularly helps to keep
			 * kvm_clock stable after CPU hotplug
			 */
			synchronizing =3D true;
		} else {
			u64 tsc_exp =3D kvm->arch.last_tsc_write +
						nsec_to_cycles(vcpu, elapsed);
			u64 tsc_hz =3D vcpu->arch.virtual_tsc_khz * 1000LL;
			/*
			 * Special case: TSC write with a small delta (1 second)
			 * of virtual cycle time against real time is
			 * interpreted as an attempt to synchronize the CPU.
			 */
			synchronizing =3D data < tsc_exp + tsc_hz &&
					data + tsc_hz > tsc_exp;
		}

> and it doesn't make much sense for the user to write 0 to trigger a sync
> immediately after creating, because the *kernel* does that anyway.

I don't care (in the Tommy Lee Jones[*] sense).  All I care about is minimi=
zing
the probability of breaking userspace, which means making the smallest poss=
ible
change to KVM's ABI.  For me, whether or not userspace is doing something s=
ensible
doesn't factor into that equation.

[*] https://www.youtube.com/watch?v=3DOoTbXu1qnbc