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

[Maxim Levitsky <mlevitsk@redhat.com>]

On Tue, 2022-02-08 at 16:15 +0100, Vitaly Kuznetsov wrote:
> Maxim Levitsky <mlevitsk@redhat.com> writes:
> 
> > [102140.117649] WARNING: CPU: 10 PID: 579353 at arch/x86/kvm/../../../virt/kvm/kvm_main.c:3161 mark_page_dirty_in_slot+0x6c/0x80 [kvm]
> ...
> > [102140.123834] Call Trace:
> > [102140.123910]  <TASK>
> > [102140.123976]  ? kvm_write_guest+0x114/0x120 [kvm]
> > [102140.124183]  kvm_hv_invalidate_tsc_page+0x9e/0xf0 [kvm]
> > [102140.124396]  kvm_arch_vm_ioctl+0xa26/0xc50 [kvm]
> 
> ...
> 
> > This happens because kvm_hv_invalidate_tsc_page is called from kvm_vm_ioctl_set_clock
> > which is a VM wide ioctl and thus doesn't have to be called with an active vCPU.
> >  
> > But as I see the warring states that guest memory writes must alway be done
> > while a vCPU is active to allow the write to be recorded in its dirty track ring.
> >  
> > I _think_ it might be OK to drop this invalidation,
> > and rely on the fact that kvm_hv_setup_tsc_page will update it,
> > and it is called when vCPU0 processes KVM_REQ_CLOCK_UPDATE which is raised in the
> > kvm_vm_ioctl_set_clock eventually.
> >  
> > Vitaly, any thoughts on this?
> >  
> 
> TSC page (as well as SynIC pages) are supposed to be "overlay" pages
> which are mapped over guest's memory but KVM doesn't do that and just
> writes to guest's memory. This kind of works as Windows/Hyper-V guests
> never disable these features and expecting the memory behind them to
> stay intact.
> 
> Dirty tracking for active TSC page can be omited, I belive. Let me take
> a look at this.
> 
> > For reference those are my HV flags:
> >  
> >     $cpu_flags: |
> >         $cpu_flags,
> >         hv_relaxed,hv_spinlocks=0x1fff,hv_vpindex,     # General HV features
> >         hv_runtime,hv_time,hv-frequencies,             # Clock stuff        
> >         hv_synic,hv_stimer,hv-stimer-direct,#hv-vapic, # APIC extensions
> >         #hv-tlbflush,hv-ipi,                           # IPI extensions
> >         hv-reenlightenment,                            # nested stuff
> >  
> >  
> >  
> > PS: unrelated question:
> >  
> > Vitaly, do you know why hv-evmcs needs hv-vapic?
> >  
> >  
> > I know that they stuffed the eVMCS pointer to HV_X64_MSR_VP_ASSIST_PAGE,
> >  
> > But can't we set HV_APIC_ACCESS_AVAILABLE but not HV_APIC_ACCESS_RECOMMENDED
> > so that guest would hopefully still know that HV assist page is available,
> > but should not use it for APIC acceleration?
> 
> Yes,
> 
> "hv-vapic" enables so-called "VP Assist" page and Enlightened VMCS GPA
> sits there, it is used instead of VMPTRLD (which becomes unsupported)
> 
> Take a look at the newly introduced "hv-apicv"/"hv-avic" (the same
> thing) in QEMU: 
> 
> commit e1f9a8e8c90ae54387922e33e5ac4fd759747d01
> Author: Vitaly Kuznetsov <vkuznets@redhat.com>
> Date:   Thu Sep 2 11:35:28 2021 +0200
> 
>     i386: Implement pseudo 'hv-avic' ('hv-apicv') enlightenment
> 
> when enabled, HV_APIC_ACCESS_RECOMMENDED is not set even with "hv-vapic"
> (but HV_APIC_ACCESS_AVAILABLE remains). 
> 

Cool, I didn't expect this. I thought that hv-vapic only enables the AutoEOI
deprecation bit.

This needs to be updated in hyperv.txt in qemu - it currently states that
hv-evmcs disables posted interrupts (that is APICv) and hv-avic
only mentions AutoEOI feature.

Thanks!
Best regards,
	Maxim Levitsky