Re: [PATCH v2] kvm: x86: Disable KVM_HC_CLOCK_PAIRING if tsc is in always catchup mode

[Sean Christopherson <seanjc@google.com>]

On Wed, Feb 16, 2022, Vitaly Kuznetsov wrote:
> Anton Romanov <romanton@google.com> writes:
> 
> > If vcpu has tsc_always_catchup set each request updates pvclock data.
> > KVM_HC_CLOCK_PAIRING consumers such as ptp_kvm_x86 rely on tsc read on
> > host's side and do hypercall inside pvclock_read_retry loop leading to
> > infinite loop in such situation.
> >
> > v2:
> >     Added warn

Versioning info goes in the "ignored" section, not the changelog.

> > Signed-off-by: Anton Romanov <romanton@google.com>
> > ---

This part is ignored by git.  Versioning info, and/or any commentary that doesn't
belong in the changelog, for a patch goes here.

> >  arch/x86/kvm/x86.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> >
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index 7131d735b1ef..aaafb46a6048 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -8945,6 +8945,15 @@ static int kvm_pv_clock_pairing(struct kvm_vcpu *vcpu, gpa_t paddr,
> >  	if (!kvm_get_walltime_and_clockread(&ts, &cycle))
> >  		return -KVM_EOPNOTSUPP;
> >  
> > +	/*
> > +	 * When tsc is in permanent catchup mode guests won't be able to use
> > +	 * pvclock_read_retry loop to get consistent view of pvclock
> > +	 */
> > +	if (vcpu->arch.tsc_always_catchup) {
> > +		pr_warn_ratelimited("KVM_HC_CLOCK_PAIRING not supported if vcpu is in tsc catchup mode\n");
> > +		return -KVM_EOPNOTSUPP;
> 
> I'm not sure this warn is a good idea. It is guest triggerable and
> 'tsc_always_catchup' is not a bug, it is a perfectly valid situation in
> the configuration when TSC scaling is unavailable. Even ratelimited,
> it's not nice when guests can pollute host's logs.

Agreed.  And if we want to alert the user/admin, it'd probably be better do so on
tsc_always_catchup first being set.  Doubt it's worth it though, assuming my other
patch to prevent KVM from setting tsc_always_catchup=true on non-ancient hardware
without userspace interaction gets merged.

> Also, EOPNOTSUPP makes it sound like the hypercall is unsupported, I'd
> suggest changing this to KVM_EFAULT.

Eh, it's consistent with the above check though, where KVM returns KVM_EOPNOTSUPP
due to the vclock mode being incompatible.  This is more or less the same, it's
just a different "mode".  KVM_EFAULT suggests that the guest did something wrong
and/or that the guest can remedy the problem in someway, e.g. by providing a
different address.  This issue is purely in the host's domain.