Re: [PATCH v6 7/7] KVM: SVM: Add Page modification logging support

["Huang, Kai" <kai.huang@intel.com>]

On Wed, 2026-04-22 at 06:20 -0700, Sean Christopherson wrote:
> On Wed, Apr 22, 2026, Kai Huang wrote:
> > On Tue, 2026-04-21 at 17:30 -0700, Sean Christopherson wrote:
> > > On Tue, Apr 21, 2026, Kai Huang wrote:
> > > > On Tue, 2026-04-21 at 08:08 -0700, Sean Christopherson wrote:
> > > > > >    vCPU Reset:
> > > > > >      vcpu_enter_guest()
> > > > > >        ├─> kvm_check_request(KVM_REQ_EVENT)
> > > > > >        ├─> kvm_apic_accept_events()
> > > > > >        │     └─> kvm_vcpu_reset(..., true)
> > > > > >        │           └─> init_vmcb(..., true)
> > > > > >        │                 └─> control->pml_index = PML_HEAD_INDEX -- PML buffer was already flushed
> > > > > >        └─> kvm_x86_call(): Next VMRUN
> > > > > > 
> > > > > > > Could this result in the hypervisor losing track of dirty memory during live
> > > > > > > migration, leading to memory corruption on the destination host, since
> > > > > > > svm_flush_pml_buffer() isn't called before resetting the index?
> > > > > > 
> > > > > > AFAIU, no. The PML buffer is always flushed opportunistically at every VM exit.
> > > > > 
> > > > > Huh.  There's a pre-existing bug here.  Commit f7f39c50edb9 ("KVM: x86: Exit to
> > > > > userspace if fastpath triggers one on instruction skip") added a path that skips
> > > > > kvm_x86_ops.handle_exit(), and specifically can give userspace control without
> > > > > going through vmx_flush_pml_buffer():
> > > > > 
> > > > > 	if (unlikely(exit_fastpath == EXIT_FASTPATH_EXIT_USERSPACE))
> > > > > 		return 0;
> > > > > 
> > > > > 	r = kvm_x86_call(handle_exit)(vcpu, exit_fastpath);
> > > > > 
> > > > > Given that SVM support for PML is (obviously) on its way, it's mildly tempting
> > > > > to add a dedicated kvm_x86_ops hook to flush the buffer on a fastpath userspace
> > > > > exit.  But, I dislike one-off kvm_x86_ops hooks, and that only works if there's
> > > > > no other vendor action required.  E.g. very theoretically, a fastpath userspace
> > > > > exit could also be coincident with bus_lock_detected.
> > > > 
> > > > Seems vmx_vcpu_reset() doesn't reset PML index upon INIT event, which seems
> > > > to be fine since we are not losing any dirty GPA tracking AFAICT (otherwise
> > > > we already have a bug for VMX here)?
> > > > 
> > > > How about doing the same for SVM?
> > > 
> > > We don't really have that luxury.  On SHUTDOWN (even intercepted SHUTDOWN), the
> > > state of the VMCB is technically undefined.  I.e. KVM needs to write _something_.
> > 
> > You mean KVM needs to reset VMCB to reflect the architecturally defined INIT
> > state for a vCPU?  Or the hardware itself may reset VMCB thus may reset PML
> > index?
> 
> Neither.  The APM states that the VMCB is undefined after SHUTDOWN.   PML index
> could be anything:
> 
>   15.14.3 Shutdown Intercept
>   When this intercept occurs, any condition that normally causes a shutdown causes a #VMEXIT to the
>   VMM instead. After an intercepted shutdown, the state saved in the VMCB is undefined.
> 
> KVM synthesizes an INIT because it's the least awful option.
> 
>   static int shutdown_interception(struct kvm_vcpu *vcpu)
>   {
> 	struct kvm_run *kvm_run = vcpu->run;
> 	struct vcpu_svm *svm = to_svm(vcpu);
> 
> 
> 	/*
> 	 * VMCB is undefined after a SHUTDOWN intercept.  INIT the vCPU to put
> 	 * the VMCB in a known good state.  Unfortuately, KVM doesn't have
> 	 * KVM_MP_STATE_SHUTDOWN and can't add it without potentially breaking
> 	 * userspace.  At a platform view, INIT is acceptable behavior as
> 	 * there exist bare metal platforms that automatically INIT the CPU
> 	 * in response to shutdown.
> 	 *
> 	 * The VM save area for SEV-ES guests has already been encrypted so it
> 	 * cannot be reinitialized, i.e. synthesizing INIT is futile.
> 	 */
> 	if (!is_sev_es_guest(vcpu)) {
> 		clear_page(svm->vmcb);
> #ifdef CONFIG_KVM_SMM
> 		if (is_smm(vcpu))
> 			kvm_smm_changed(vcpu, false);
> #endif
> 		kvm_vcpu_reset(vcpu, true);
> 	}
> 
> 	kvm_run->exit_reason = KVM_EXIT_SHUTDOWN;
> 	return 0;
>   }
> 
> Now, maybe the APM is trying to say only the save area is undefined, in which
> case PML Index is fine and can and should be left alone.  But if that's the case,
> the APM needs to be updated to make explicitly clear what fields in the VMCS are
> and are not valid after SHUTDOWN.
> > > 

Thanks for all the info! :-)