Re: warning in kvm_hv_invalidate_tsc_page due to writes to guest memory from VM ioctl context

[Sean Christopherson <seanjc@google.com>]

On Wed, Feb 09, 2022, Vitaly Kuznetsov wrote:
> Sean Christopherson <seanjc@google.com> writes:
> 
> > On Tue, Feb 08, 2022, Vitaly Kuznetsov wrote:
> >> Maxim Levitsky <mlevitsk@redhat.com> writes:
> >> > and hv-avic only mentions AutoEOI feature.
> >> 
> >> True, this is hidden in "The enlightenment allows to use Hyper-V SynIC
> >> with hardware APICv/AVIC enabled". Any suggestions on how to improve
> >> this are more than welcome!.
> >
> > Specifically for the WARN, does this approach makes sense?
> >
> > https://lore.kernel.org/all/YcTpJ369cRBN4W93@google.com
> 
> (Sorry for missing this dicsussion back in December)
> 
> It probably does but the patch just introduces
> HV_TSC_PAGE_UPDATE_REQUIRED flag and drops kvm_write_guest() completely,
> the flag is never reset and nothing ever gets written to guest's
> memory. I suppose you've forgotten to commit a hunk :-)

I don't think so, the idea is that kvm_hv_setup_tsc_page() handles the write.

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index c194a8cbd25f..c1adc9efea28 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -2848,7 +2848,7 @@ static void kvm_end_pvclock_update(struct kvm *kvm)
>
>  static void kvm_update_masterclock(struct kvm *kvm)
>  {
> -	kvm_hv_invalidate_tsc_page(kvm);
> +	kvm_hv_request_tsc_page_update(kvm);
>  	kvm_start_pvclock_update(kvm);
>  	pvclock_update_vm_gtod_copy(kvm);
>  	kvm_end_pvclock_update(kvm);
> @@ -3060,8 +3060,7 @@ static int kvm_guest_time_update(struct kvm_vcpu *v)
>  				       offsetof(struct compat_vcpu_info, time));
>  	if (vcpu->xen.vcpu_time_info_set)
>  		kvm_setup_pvclock_page(v, &vcpu->xen.vcpu_time_info_cache, 0);
> -	if (!v->vcpu_idx)
> -		kvm_hv_setup_tsc_page(v->kvm, &vcpu->hv_clock);
> +	kvm_hv_setup_tsc_page(v->kvm, &vcpu->hv_clock);

This change sends all vCPUs through the helper, not just vCPU 0.  Then the common
helper checks HV_TSC_PAGE_UPDATE_REQUIRED under lock.

	if (!(hv->hv_tsc_page_status & HV_TSC_PAGE_UPDATE_REQUIRED))
		goto out_unlock;


	--- error checking ---

	/* Write the struct entirely before the non-zero sequence.  */
	smp_wmb();

	hv->tsc_ref.tsc_sequence = tsc_seq;
	if (kvm_write_guest(kvm, gfn_to_gpa(gfn),
			    &hv->tsc_ref, sizeof(hv->tsc_ref.tsc_sequence)))
		goto out_err;

	hv->hv_tsc_page_status = HV_TSC_PAGE_SET;
	goto out_unlock;

out_err:
	hv->hv_tsc_page_status = HV_TSC_PAGE_BROKEN;
out_unlock:
	mutex_unlock(&hv->hv_lock);


If there are no errors, the kvm_write_guest() goes through and the status is
"reset".  If there are errors, the status is set to BROKEN.

Should I send an RFC to facilitate discussion?