Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) client-ip=23.128.96.36;
Date:   Tue, 31 Oct 2023 22:58:48 +0000
In-Reply-To: <028f629d16377f9a7e9fd87ef9564846b0ab4ed9.camel@infradead.org>
Mime-Version: 1.0
References: <20231031115748.622578-1-paul@xen.org> <ZUGCPQegUeTutsrb@google.com>
 <028f629d16377f9a7e9fd87ef9564846b0ab4ed9.camel@infradead.org>
Message-ID: <ZUGGqOCU7TAU6c6p@google.com>
Subject: Re: [PATCH v2] KVM x86/xen: add an override for PVCLOCK_TSC_STABLE_BIT
From:   Sean Christopherson <seanjc@google.com>
To:     David Woodhouse <dwmw2@infradead.org>
Cc:     Paul Durrant <paul@xen.org>, Paolo Bonzini <pbonzini@redhat.com>,
        Jonathan Corbet <corbet@lwn.net>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        Dave Hansen <dave.hansen@linux.intel.com>, x86@kernel.org,
        "H. Peter Anvin" <hpa@zytor.com>, kvm@vger.kernel.org,
        linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, Oct 31, 2023, David Woodhouse wrote:
> On Tue, 2023-10-31 at 15:39 -0700, Sean Christopherson wrote:
> > On Tue, Oct 31, 2023, Paul Durrant wrote:
> > Any reason not to make this a generic "capability" instead of a Xen spe=
cific flag?
> > E.g. I assume these problematic guests would mishandle PVCLOCK_TSC_STAB=
LE_BIT if
> > it showed up in kvmclock, but they don't use kvmclock so it's not a pro=
blem in
> > practice.
>=20
> No, those guests are just fine with kvmclock. It's the *Xen* page they
> forgot to map to userspace for the vDSO to use. And it's Xen (true Xen)
> which made you jump through hoops to use the TSC that way, such that it
> would actually expose the PVCLOCK_TSC_STABLE_BIT. We don't expect, and
> have never seen, such issues with native KVM guests.

Hmm, and I suppose theoretically the guest kernel could choose to ignore th=
e Xen
interface for whatever reason.  Mostly out of curiosity, is this flag somet=
hing
that'd be set anytime Xen is advertised to the guest?

> > I doubt there's a real need or use case, but it'd require less churn an=
d IMO is
> > simpler overall, e.g.
> >=20
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index e3eb608b6692..731b201bfd5a 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -3225,7 +3225,7 @@ static int kvm_guest_time_update(struct kvm_vcpu =
*v)
> > =C2=A0
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 /* If the host uses TSC cloc=
ksource, then it is stable */
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 pvclock_flags =3D 0;
> > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (use_master_clock)
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (use_master_clock && !vcpu->kv=
m.force_tsc_unstable)
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0 pvclock_flags |=3D PVCLOCK_TSC_STABLE_BIT;
> > =C2=A0
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 vcpu->hv_clock.flags =3D pvc=
lock_flags;
> >=20
> > I also assume this is a "set and forget" thing?=C2=A0 I.e. KVM can requ=
ire the flag
> > to be set before any vCPUs are created.
>=20
> Hrm, not sure we have previously required that the KVM_XEN_HVM_CONFIG
> setup be done before any vCPUs were created.

Oh, I was asking in the context of adding a generic capability.

> I tend to prefer *not* to push ordering requirements onto userspace.

For per-VM flags that are consumed by vCPUs, it makes reasoning about corre=
ctness
and what is/isn't allowed much, much easier.

> Does it need to be a per-vcpu thing?=20

Huh?  No, I was only asking (again, for a generic capability) if we could d=
o

		mutex_lock(&kvm->lock);
		if (!kvm->created_vcpus) {
			kvm->arch.force_tsc_unstable =3D true;
			r =3D 0;
		}
		mutex_unlock(&kvm->lock);

So that it would be blatantly obvious that there's no race with checking a =
per-VM
flag without any lock/RCU protections.