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

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

On 9/7/2026 12:30 PM, sashiko-bot@kernel.org wrote:

>> 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?

No, kvm_mmu_page_ad_need_write_protect() returns true when
cpu_dirty_log_size is set and the shadow page is in guest_mode,
which forces write-protection on L2 SPTEs instead of relying on
hardware PML. L2 dirty pages are tracked via write faults, not PML.
This is the same approach VMX uses for nested guests.

> 
> 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?

No clean bits are defined for the PML_ADDR and PML_INDEX fields, so on a
VMRUN, hardware always loads them from the VMCB if the feature is enabled.

> 
> 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?

No. flush_pml_buffer is not a kvm_x86_ops operation. SVM calls
svm_flush_pml_buffer() directly from svm_handle_exit() on every
VM exit, including SVM_EXIT_PML_FULL.
 
> 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.

Regards,
Nikunj