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

[Vitaly Kuznetsov <vkuznets@redhat.com>]

Sean Christopherson <seanjc@google.com> writes:

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

Oh, sorry, missed that. Patches always look weird in the browser :-)

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

Sure, please go ahead. There are some basic selftests for TSC page
since:

commit 2c7f76b4c42bd5d953bc821e151644434865f999
Author: Vitaly Kuznetsov <vkuznets@redhat.com>
Date:   Thu Mar 18 15:09:49 2021 +0100

    selftests: kvm: Add basic Hyper-V clocksources tests

but I'll have to refresh my memory on the problematic migration scenario
when kvm_hv_invalidate_tsc_page() got introduced.

Thanks!

-- 
Vitaly