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

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

On Wed, 2026-04-22 at 11:29 +0530, Nikunj A. Dadhania wrote:
> 
> On 4/22/2026 7:12 AM, Huang, Kai 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);
> 
> Ah right.
> 
> > > > > 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?
> > 
> > > 
> > > Hmm, actually, how is that going to work?  Dropping PML entries just because a
> > > vCPU hit SHUTDOWN isn't going to fly.  
> > > 
> > 
> > Not dropping, but just leave PML index unchanged.  The PML buffer itself is
> > still there unchanged, and PML is still working in hardware thus the buffer
> > will eventually get flushed.
> > 
> > This is the case for VMX AFAICT, thus I wonder whether this also works for
> > SVM.
> 
> Theoretically, while the migration is active and we did a fast path user space
> exit and never returned, the entries remain in PML buffer. If we flush every time
> on VM exit, as per Sean's suggestion, PML buffer is always flushed before returning
> to user space.

Oh indeed I missed something here.  I assumed the last GET_DIRTY_LOG ioctl 
can always flush the PML buffer.  But I forgot KVM flushes the buffer by
kicking all vcpus.

So yeah we have a loophole on the EXIT_FASTPATH_EXIT_USERSPACE, which seems
can be controlled by userspace (e.g., single step debugging?).

So what Sean posted makes sense.  Sorry for the noise.

Or maybe we could simply flush PML buffer in {vmx|svm}_vcpu_enter_exit()?