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

[Vitaly Kuznetsov <vkuznets@redhat.com>]

Vitaly Kuznetsov <vkuznets@redhat.com> writes:

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

...

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