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

[sashiko-bot@kernel.org]

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? 

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?

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