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

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

[Anton Romanov]

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

Signed-off-by: Anton Romanov <romanton@google.com>
---
 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;
+	}
+
 	clock_pairing.sec = ts.tv_sec;
 	clock_pairing.nsec = ts.tv_nsec;
 	clock_pairing.tsc = kvm_read_l1_tsc(vcpu, cycle);
-- 
2.35.1.265.g69c8d7142f-goog

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

[Vitaly Kuznetsov]

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
>
> Signed-off-by: Anton Romanov <romanton@google.com>
> ---
>  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.

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

> +	}
> +
>  	clock_pairing.sec = ts.tv_sec;
>  	clock_pairing.nsec = ts.tv_nsec;
>  	clock_pairing.tsc = kvm_read_l1_tsc(vcpu, cycle);

-- 
Vitaly

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

[Sean Christopherson]

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.

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

[Vitaly Kuznetsov]

Sean Christopherson <seanjc@google.com> writes:

> On Wed, Feb 16, 2022, Vitaly Kuznetsov wrote:

...
>
>> 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.

Ack, Paolo's already made the change back to KVM_EOPNOTSUPP upon commit
(but I still mildly dislike using 'EOPNOTSUPP' for a temporary condition
on the host).

-- 
Vitaly

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

[Paolo Bonzini]

On 2/17/22 17:09, Vitaly Kuznetsov wrote:
> Sean Christopherson <seanjc@google.com> writes:
> 
>> On Wed, Feb 16, 2022, Vitaly Kuznetsov wrote:
> 
> ...
>>
>>> 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.
> 
> Ack, Paolo's already made the change back to KVM_EOPNOTSUPP upon commit
> (but I still mildly dislike using 'EOPNOTSUPP' for a temporary condition
> on the host).

It's not temporary, always-catchup is set on KVM_SET_TSC_KHZ and never 
goes away.

Paolo

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

[Vitaly Kuznetsov]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 2/17/22 17:09, Vitaly Kuznetsov wrote:
...
>> (but I still mildly dislike using 'EOPNOTSUPP' for a temporary condition
>> on the host).
>
> It's not temporary, always-catchup is set on KVM_SET_TSC_KHZ and never 
> goes away.

Even if the guest is migrated (back) to the host which supports TSC
scaling?

-- 
Vitaly

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

[Paolo Bonzini]

On 2/17/22 18:16, Vitaly Kuznetsov wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
>> On 2/17/22 17:09, Vitaly Kuznetsov wrote:
> ...
>>> (but I still mildly dislike using 'EOPNOTSUPP' for a temporary condition
>>> on the host).
>>
>> It's not temporary, always-catchup is set on KVM_SET_TSC_KHZ and never
>> goes away.
> 
> Even if the guest is migrated (back) to the host which supports TSC
> scaling?

Ah right, in that case no.  But this hypercall does not have a CPUID 
bit, so the supported status can change on migration anyway.

Paolo