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

[Vitaly Kuznetsov <vkuznets@redhat.com>]

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