Re: [RFC PATCH 2/5] KVM: nSVM: Add nested PML emulation

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

On 10/15/2026 4:33 PM, sashiko-bot@kernel.org wrote:
> [You don't often get email from sashiko-bot@kernel.org. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] kvm: nSVM: spurious PML_FULL VM-Exit injection on discarded faults
> --
> 
> commit ac3a6ec9423b67a67c9dfb9dd5ebd698f451f130
> Author: Nikunj A Dadhania <nikunj@amd.com>
> 
> KVM: nSVM: Add nested PML emulation
> 
> This commit emulates Page Modification Logging (PML) for nested SVM guests,
> allowing L1 hypervisors to track dirty pages in their L2 guests. KVM emulates
> PML in software by write-protecting L2 SPTEs and logging dirty GPAs to L1's
> PML buffer.
> 
>> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
>> index af0ef678433d7..8251b38d14790 100644
>> --- a/arch/x86/kvm/svm/nested.c
>> +++ b/arch/x86/kvm/svm/nested.c
> 
> [ ... ]
> 
>> @@ -63,7 +64,14 @@ static void nested_svm_inject_npf_exit(struct kvm_vcpu *vcpu,
>>       if (WARN_ON_ONCE(hweight64(fault_stage) != 1))
>>               fault_stage = PFERR_GUEST_FINAL_MASK;
>>
>> -     vmcb->control.exit_code = SVM_EXIT_NPF;
>> +     /*
>> +      * Only the exit code differs for a PML Full; hardware leaves the
>> +      * EXITINFO fields as the nested page fault that filled the buffer
>> +      * would set them.
>> +      */
>> +     vmcb->control.exit_code = svm->nested.pml_full ? SVM_EXIT_PML_FULL : SVM_EXIT_NPF;
>> +     svm->nested.pml_full = false;
>> +
>>       vmcb->control.exit_info_1 = fault_stage |
>>                                   (fault->error_code & ~PFERR_GUEST_FAULT_STAGE_MASK);
>>       vmcb->control.exit_info_2 = fault->address;
> 
> [Severity: High]
> If L2 triggers a legitimate but unrelated nested page fault (such as a read
> fault) after a previous discarded fault left pml_full set, does this blindly
> consume the lingering pml_full flag?

Yes.  Both findings are about the same flag - one at the point it is
consumed, one at the point it is left set - so this answers both.

nVMX does the same: vmx->nested.pml_full is set in
nested_vmx_write_pml_buffer() and consumed unconditionally in
nested_ept_inject_page_fault(), with the same discard paths in shared MMU
code.  I kept the flag here to match that rather than introduce a
different mechanism in a first posting.

The effect is bounded: L1 takes one spurious PML Full, and as
pml_full_interception() ignores the EXITINFO fields, L1 drains,
re-enters L2 and gets the real fault on the retry.

The clean fix could be to carry the indication on the fault rather than
on the vCPU - a synthetic PFERR bit, set by the walk that raised it and
tested in nested_svm_inject_npf_exit(). Something like the below, roughly:

      #define PFERR_PML_FULL          BIT_ULL(50)
      #define PFERR_SYNTHETIC_MASK    (PFERR_IMPLICIT_ACCESS | \
                                       PFERR_PRIVATE_ACCESS | PFERR_PML_FULL)

      /* update_accessed_dirty_bits() */
      if (w == &vcpu->arch.ngpa_walk &&
          kvm_nested_call(write_log_dirty)(vcpu, addr))
              return -EBUSY;

      /* walk_addr_generic(), tagging the fault it is about to raise */
      if (unlikely(ret == -EBUSY))
              errcode |= PFERR_PML_FULL;

      /* nested_svm_inject_npf_exit() */
      pml_full = fault->error_code & PFERR_PML_FULL;
      vmcb->control.exit_code = pml_full ? SVM_EXIT_PML_FULL : SVM_EXIT_NPF;

It would also let nVMX drop its flag and the WARN_ON_ONCE in
nested_ept_inject_page_fault().

Separately, Yosry asked on this thread about factoring
nested_svm_write_pml_buffer() and its nVMX counterpart into common code.
I will wait for Sean's input on both before reworking either.

> 
> It looks like this would inject an SVM_EXIT_PML_FULL exit into L1 carrying
> the error code of a read fault. Since hardware PML only logs writes, could
> this architecturally impossible state crash or confuse the L1 hypervisor?
> 
> [ ... ]
> 
>> @@ -2169,6 +2200,47 @@ static gpa_t svm_translate_nested_gpa(struct kvm_vcpu *vcpu, gpa_t gpa,
>>       return w->gva_to_gpa(vcpu, w, gpa, access, exception);
>>  }
>>
>> +static int nested_svm_write_pml_buffer(struct kvm_vcpu *vcpu, gpa_t gpa)
>> +{
>> +     struct vcpu_svm *svm = to_svm(vcpu);
>> +     u16 index;
>> +     gpa_t dst;
>> +
>> +     if (WARN_ON_ONCE(!is_guest_mode(vcpu)) || !nested_pml_enabled(svm))
>> +             return 0;
>> +
>> +     /* The flag can be set here if the fault that raised it was discarded. */
>> +     if (svm->nested.pml_full)
>> +             return 1;
>> +
>> +     index = svm->nested.ctl.pml_index;
>> +     if (index >= PML_LOG_NR_ENTRIES) {
>> +             svm->nested.pml_full = true;
>> +             return 1;
>> +     }
> 
> [Severity: High]
> If KVM resolves or discards the page fault internally without injecting it
> into L1, this leaves the pml_full flag set while L2 execution resumes.
> 
> The comment acknowledges that the flag can be left set by discarded faults.
> However, is it safe to leave it set if the next fault isn't guaranteed to be
> a write?

Regards
Nikunj