Vitaly Kuznetsov <vkuznets@xxxxxxxxxx> writes: > 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. >> ... > > but I'll have to refresh my memory on the problematic migration scenario > when kvm_hv_invalidate_tsc_page() got introduced. I've smoke-tested your patch with both selftests and Win10+WSL2 migration and nothing blew up. I, however, don't quite like the idea to make HV_TSC_PAGE_UPDATE_REQUIRED a bit flag which is orthogonal to all other HV_TSC_PAGE_ state machine states. E.g. we have the following in get_time_ref_counter(): if (hv->hv_tsc_page_status != HV_TSC_PAGE_SET) return div_u64(get_kvmclock_ns(kvm), 100); the following in tsc_page_update_unsafe(): return (hv->hv_tsc_page_status != HV_TSC_PAGE_GUEST_CHANGED) && hv->hv_tsc_emulation_control; and the followin in what's now kvm_hv_request_tsc_page_update(): if (hv->hv_tsc_page_status == HV_TSC_PAGE_BROKEN || hv->hv_tsc_page_status == HV_TSC_PAGE_UNSET || tsc_page_update_unsafe(hv)) goto out_unlock; and while I don't see how HV_TSC_PAGE_UPDATE_REQUIRED breaks any of these, it cetainly takes an extra effort to understand these checks as we're now comparing something more than just a state machine's state. Same goes to all assignments to hv->hv_tsc_page_status: HV_TSC_PAGE_UPDATE_REQUIRED gets implicitly overwritten (i.e. we're not just switching from state A to state B, we're also clearing the flag). Again, I don't see how is this incorrect, just unnecessary complicated (and that's what get me confused when I said you're missing something in your patch!). In case making HV_TSC_PAGE_UPDATE_REQUIRED a real state (or, actually, several new states) is too cumbersome I'd suggest to explore two options: - adding helpers to set/check hv->hv_tsc_page_status and making HV_TSC_PAGE_UPDATE_REQUIRED clearing/masking explicit. - adding a boolean (e.g. "tsc_page_update_required") to 'struct kvm_hv'. -- Vitaly
