Re: [PATCH v8 8/8] KVM: SVM: Add Page modification logging support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Hardware PML is disabled for nested L2 guests, but the KVM MMU is not informed, causing all memory dirtied by L2 to be silently lost.
- [High] The PML index is reset in memory but the VMCB is not marked dirty, causing the processor to use a stale cached index and livelock.
- [Medium] Missing .flush_pml_buffer implementation in svm_x86_ops causes KVM_GET_DIRTY_LOG to return incomplete data.
--

commit d1428c9c095cb0cc4ca5ce0ed8ebb044a3eb17a3
Author: Nikunj A Dadhania <nikunj@amd.com>
Subject: KVM: SVM: Add Page modification logging support

This commit adds support for the Page Modification Logging (PML) feature
on AMD processors. PML automatically logs dirty guest physical addresses to
a buffer when the CPU sets NPT D-bits, offering a hardware-assisted method
for efficient dirty logging.

> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index 73f37b050d0a0..da417c4c59154 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c
> @@ -901,6 +901,13 @@ static void nested_vmcb02_prepare_control(struct vcpu_svm *svm)
>  	vmcb02->control.msrpm_base_pa = vmcb01->control.msrpm_base_pa;
>  	vmcb_mark_dirty(vmcb02, VMCB_PERM_MAP);
>  
> +	/*
> +	 * PML is never enabled in hardware for L2.  Make sure that an
> +	 * unexpected PML write would trigger a PML_FULL VM-Exit.
> +	 */
> +	if (pml)
> +		vmcb02->control.pml_index = -1;

[Severity: Critical]
By intentionally leaving hardware PML disabled for nested L2 guests in 
nested_vmcb02_prepare_control without informing the KVM MMU, doesn't this 
cause all memory dirtied by L2 to be silently lost?

When an L2 guest writes to memory and the host KVM has PML enabled, the KVM 
MMU creates L2 shadow NPT SPTEs as writable but clears the D-bit. If hardware 
PML is disabled for L2, the hardware will set the D-bit but will not log the 
GPA, potentially leading to silent memory corruption upon live migration.

[ ... ]

> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 7d59d301e1e54..950c250a77b51 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
[ ... ]
> @@ -3344,6 +3376,45 @@ static int vmmcall_interception(struct kvm_vcpu *vcpu)
>  	return kvm_emulate_hypercall(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 (enable)
> +		vmcb01->control.misc_ctl |= SVM_MISC_ENABLE_PML;
> +	else
> +		vmcb01->control.misc_ctl &= ~SVM_MISC_ENABLE_PML;
> +
> +	vmcb_mark_dirty(vmcb01, VMCB_NPT);
> +}
> +
> +static void svm_flush_pml_buffer(struct kvm_vcpu *vcpu)
> +{
> +	struct vcpu_svm *svm = to_svm(vcpu);
> +	struct vmcb_control_area *control = &svm->vmcb->control;
> +
> +	/* Do nothing if PML buffer is empty */
> +	if (control->pml_index == PML_HEAD_INDEX)
> +		return;
> +
> +	kvm_flush_pml_buffer(vcpu, control->pml_index);
> +
> +	/* Reset the PML index */
> +	control->pml_index = PML_HEAD_INDEX;
> +}

[Severity: High]
Since the PML index is reset here without marking the VMCB_NPT field dirty via
vmcb_mark_dirty(svm->vmcb, VMCB_NPT), will the CPU hardware ignore the reset
and use a stale cached value?

This could lead to a situation where the processor continuously generates a
PML_FULL VMEXIT on every subsequent write because the cached index remains at
zero, causing guest livelock and a performance collapse.

[ ... ]
> @@ -5429,6 +5525,8 @@ struct kvm_x86_ops svm_x86_ops __initdata = {
>  	.check_intercept = svm_check_intercept,
>  	.handle_exit_irqoff = svm_handle_exit_irqoff,
>  
> +	.update_cpu_dirty_logging = svm_update_cpu_dirty_logging,
> +
>  	.deliver_interrupt = svm_deliver_interrupt,
>  	.pi_update_irte = avic_pi_update_irte,
>  	.setup_mce = svm_setup_mce,

[Severity: Medium]
Is there a missing implementation of .flush_pml_buffer in svm_x86_ops?

Without .flush_pml_buffer exported to the generic KVM x86 ops, 
kvm_vcpu_flush_pml() might silently do nothing. This would result in
KVM_GET_DIRTY_LOG returning incomplete data, potentially delaying live
migration convergence or confusing userspace tools expecting an accurate
snapshot.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260907063906.1964557-1-nikunj@amd.com?part=8