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

Huang, Kai
2026-04-22T22:14:48Z

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! :-)