Sean Christopherson <seanjc@xxxxxxxxxx> writes: > On Wed, Feb 09, 2022, Vitaly Kuznetsov wrote: >> Sean Christopherson <seanjc@xxxxxxxxxx> writes: >> >> > On Tue, Feb 08, 2022, Vitaly Kuznetsov wrote: >> >> Maxim Levitsky <mlevitsk@xxxxxxxxxx> 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@xxxxxxxxxx >> >> (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@xxxxxxxxxx> 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
