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

[Tom Lendacky <thomas.lendacky@amd.com>]

On 4/22/26 08:20, 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.

I'm checking to see if I can get clarification on this.

Thanks,
Tom

> 
>>> 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.
> 
> As above, the PML index could be garbage.
> 
>> This is the case for VMX AFAICT, thus I wonder whether this also works for
>> SVM.
> 
> No, VMX is fine.
> 
>>> E.g. if a VM crashes while it's being
>>> migrated by the host, then it could end up with corrupted, incoherent data on
>>> the target due to leaving dirtied pages behind.
>>
>> If we leave PML index unchanged upon INIT as mentioned above, I don't think
>> we will lose any dirty GPA tracking.  But maybe I am missing something.
> 
> Correct.  The question is whether or not AMD's architecture guarantees PML Index
> will be valid/accurate after SHUTDOWN.
>