Re: [RFC PATCH v2 1/7] KVM: x86: Avoid silent kvm-clock activation failures

[Sean Christopherson <seanjc@google.com>]

On Thu, Feb 26, 2026, Takahiro Itazuri wrote:
> kvm_write_system_time() previously ignored the return value of
> kvm_gpc_activate().  As a result, kvm-clock activation could fail
> silently, making debugging harder.
> 
> Propagate the return value so that the MSR write fail properly instead
> of continuing silently.

Hrm.  For better or worse, KVM's ABI when it comes to PV stuff is to silently
ignore failures.  I 100% agree it makes debugging painful, but it's unfortunately
also "safer" in many cases, e.g. often results in degraded behavior versus flat
out crashing the guest.

The other wrinkle is that success isn't actually guaranteed, because the actual
writes don't happen until KVM_RUN via kvm_guest_time_update(), i.e. only failing
in _some_ cases creates a weird ABI.

And most importantly, this would be a breaking change in guest- and user-visible
behavior.  So while I agree silently failing is ugly, all things considered I
think it's the least awful choice here :-/

> Signed-off-by: Takahiro Itazuri <itazur@amazon.com>
> ---
>  arch/x86/kvm/x86.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index a447663d5eff..a729b8419b61 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -2438,7 +2438,7 @@ static void kvm_write_wall_clock(struct kvm *kvm, gpa_t wall_clock, int sec_hi_o
>  	kvm_write_guest(kvm, wall_clock, &version, sizeof(version));
>  }
>  
> -static void kvm_write_system_time(struct kvm_vcpu *vcpu, gpa_t system_time,
> +static int kvm_write_system_time(struct kvm_vcpu *vcpu, gpa_t system_time,
>  				  bool old_msr, bool host_initiated)
>  {
>  	struct kvm_arch *ka = &vcpu->kvm->arch;
> @@ -2455,12 +2455,12 @@ static void kvm_write_system_time(struct kvm_vcpu *vcpu, gpa_t system_time,
>  
>  	/* we verify if the enable bit is set... */
>  	if (system_time & 1)
> -		kvm_gpc_activate(&vcpu->arch.pv_time, system_time & ~1ULL,
> -				 sizeof(struct pvclock_vcpu_time_info));
> -	else
> -		kvm_gpc_deactivate(&vcpu->arch.pv_time);
> +		return kvm_gpc_activate(&vcpu->arch.pv_time,
> +					system_time & ~1ULL,
> +					sizeof(struct pvclock_vcpu_time_info));
>  
> -	return;
> +	kvm_gpc_deactivate(&vcpu->arch.pv_time);
> +	return 0;
>  }
>  
>  static uint32_t div_frac(uint32_t dividend, uint32_t divisor)
> @@ -4156,13 +4156,15 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  		if (!guest_pv_has(vcpu, KVM_FEATURE_CLOCKSOURCE2))
>  			return 1;
>  
> -		kvm_write_system_time(vcpu, data, false, msr_info->host_initiated);
> +		if (kvm_write_system_time(vcpu, data, false, msr_info->host_initiated))
> +			return 1;
>  		break;
>  	case MSR_KVM_SYSTEM_TIME:
>  		if (!guest_pv_has(vcpu, KVM_FEATURE_CLOCKSOURCE))
>  			return 1;
>  
> -		kvm_write_system_time(vcpu, data, true,  msr_info->host_initiated);
> +		if (kvm_write_system_time(vcpu, data, true,  msr_info->host_initiated))
> +			return 1;
>  		break;
>  	case MSR_KVM_ASYNC_PF_EN:
>  		if (!guest_pv_has(vcpu, KVM_FEATURE_ASYNC_PF))
> -- 
> 2.50.1
>