Re: [PATCH v7.1] KVM: SVM: Add Page modification logging support

["Nikunj A. Dadhania" <nikunj@amd.com>]

Hi Sean,

Thanks for the review!

On 9/2/2026 5:39 AM, Sean Christopherson wrote:
> On Fri, May 29, 2026, Nikunj A Dadhania wrote:
>> @@ -3331,6 +3364,53 @@ static int vmmcall_interception(struct kvm_vcpu *vcpu)
>>  	return kvm_emulate_hypercall(vcpu);
>>  }
>>  
>> +void svm_update_cpu_dirty_logging(struct kvm_vcpu *vcpu)
> 
> This can be static.
> 
>> +{
>> +	struct vcpu_svm *svm = to_svm(vcpu);
>> +	struct vmcb *vmcb01 = svm->vmcb01.ptr;
>> +
>> +	if (WARN_ON_ONCE(!vcpu->kvm->arch.cpu_dirty_log_size))
> 
> This can/should be moved to common code.
> 
>> +		return;
>> +
>> +	/*
>> +	 * Note, nr_memslots_dirty_logging can be changed concurrent with this
>> +	 * code, but in that case another update request will be made and so
>> +	 * the guest will never run with a stale PML value.
>> +	 */
> 
> Ditto with this copy+pasted comment.
> 
>> +	if (atomic_read(&vcpu->kvm->nr_memslots_dirty_logging))
> 
> And this check.  E.g. (incomplete, needs to be split up)

Sure, will refactor into a common kvm_update_cpu_dirty_logging()
wrapper in x86.c as suggested in a separate pre-patch.

> 
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 0a8ef0b4ddda..73564619a61e 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -3377,20 +3377,12 @@ static int vmmcall_interception(struct kvm_vcpu *vcpu)
>  	return kvm_emulate_hypercall(vcpu);
>  }
>  
> -void svm_update_cpu_dirty_logging(struct kvm_vcpu *vcpu)
> +static void svm_update_cpu_dirty_logging(struct kvm_vcpu *vcpu, bool enable)
>  {
>  	struct vcpu_svm *svm = to_svm(vcpu);
>  	struct vmcb *vmcb01 = svm->vmcb01.ptr;
>  
> -	if (WARN_ON_ONCE(!vcpu->kvm->arch.cpu_dirty_log_size))
> -		return;
> -
> -	/*
> -	 * Note, nr_memslots_dirty_logging can be changed concurrent with this
> -	 * code, but in that case another update request will be made and so
> -	 * the guest will never run with a stale PML value.
> -	 */
> -	if (atomic_read(&vcpu->kvm->nr_memslots_dirty_logging))
> +	if (enable)
>  		vmcb01->control.misc_ctl |= SVM_MISC_ENABLE_PML;
>  	else
>  		vmcb01->control.misc_ctl &= ~SVM_MISC_ENABLE_PML;
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 0052446e04d9..828a51418d83 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -8416,21 +8416,13 @@ static void vmx_update_hv_timer(struct kvm_vcpu *vcpu, bool force_immediate_exit
>  }
>  #endif
>  
> -void vmx_update_cpu_dirty_logging(struct kvm_vcpu *vcpu)
> +void vmx_update_cpu_dirty_logging(struct kvm_vcpu *vcpu, bool enable)
>  {
>  	struct vcpu_vmx *vmx = to_vmx(vcpu);
>  
> -	if (WARN_ON_ONCE(!vcpu->kvm->arch.cpu_dirty_log_size))
> -		return;
> -
>  	guard(vmx_vmcs01)(vcpu);
>  
> -	/*
> -	 * Note, nr_memslots_dirty_logging can be changed concurrent with this
> -	 * code, but in that case another update request will be made and so
> -	 * the guest will never run with a stale PML value.
> -	 */
> -	if (atomic_read(&vcpu->kvm->nr_memslots_dirty_logging))
> +	if (enable)
>  		secondary_exec_controls_setbit(vmx, SECONDARY_EXEC_ENABLE_PML);
>  	else
>  		secondary_exec_controls_clearbit(vmx, SECONDARY_EXEC_ENABLE_PML);
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 33715236afc9..4443796c4426 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -8073,6 +8073,21 @@ static void kvm_vcpu_reload_apic_access_page(struct kvm_vcpu *vcpu)
>  	kvm_x86_call(set_apic_access_page_addr)(vcpu);
>  }
>  
> +static void kvm_update_cpu_dirty_logging(struct kvm_vcpu *vcpu)
> +{
> +	/*
> +	 * Note, nr_memslots_dirty_logging can be changed concurrent with this
> +	 * code, but in that case another update request will be made and so
> +	 * the guest will never run with a stale PML value.
> +	 */
> +	bool enabled = atomic_read(&vcpu->kvm->nr_memslots_dirty_logging);
> +
> +	if (WARN_ON_ONCE(!vcpu->kvm->arch.cpu_dirty_log_size))
> +		return;
> +
> +	kvm_x86_call(update_cpu_dirty_logging)(vcpu, enable);
> +}
> +

One subtle ordering change for VMX: currently guard(vmx_vmcs01) is
taken before the atomic_read; with this refactoring the atomic_read
happens in common code before guard(vmx_vmcs01) is called.
The ordering flip looks harmless to me.

>  /*
>   * Called within kvm->srcu read side.
>   * Returns 1 to let vcpu_run() continue the guest execution loop without
> @@ -8238,8 +8253,14 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
>  		if (kvm_check_request(KVM_REQ_RECALC_INTERCEPTS, vcpu))
>  			kvm_x86_call(recalc_intercepts)(vcpu);
>  
> -		if (kvm_check_request(KVM_REQ_UPDATE_CPU_DIRTY_LOGGING, vcpu))
> -			kvm_x86_call(update_cpu_dirty_logging)(vcpu);
> +
> +		/*
> +		 * Note, nr_memslots_dirty_logging can be changed concurrent
> +		 * with this code, but in that case another update request will
> +		 * be made and so the guest will never run with stale PML state.
> +	 	 */

Looks like the comment got put in two places; will retain it only in
kvm_update_cpu_dirty_logging().

> +		if (kvm_check_request(KVM_REQ_UPDATE_CPU_DIRTY_LOGGING, vcpu)
> +			kvm_update_cpu_dirty_logging(vcpu);

...

>> @@ -5465,6 +5570,8 @@ struct kvm_x86_ops svm_x86_ops __initdata = {
>>  	.gmem_prepare = sev_gmem_prepare,
>>  	.gmem_invalidate = sev_gmem_invalidate,
>>  	.gmem_max_mapping_level = sev_gmem_max_mapping_level,
>> +
>> +	.update_cpu_dirty_logging = svm_update_cpu_dirty_logging,
> 
> Please keep the ordering somewhat similar to the other declarations, i.e. don't
> just put this at the end.  This seems like the least awful place:
> 
> 	.handle_exit_irqoff = svm_handle_exit_irqoff,
> 
> 	.update_cpu_dirty_logging = svm_update_cpu_dirty_logging,
> 
> 	.deliver_interrupt = svm_deliver_interrupt,

Ack, will fix.

>> @@ -832,6 +833,8 @@ static inline void svm_enable_intercept_for_msr(struct kvm_vcpu *vcpu,
>>  	svm_set_intercept_for_msr(vcpu, msr, type, true);
>>  }
>>  
>> +void svm_update_cpu_dirty_logging(struct kvm_vcpu *vcpu);
> 
> And this goes away.

Ack.

Regards,
Nikunj