Re: [PATCH v2 4/4] KVM: SVM: Add Page modification logging support

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

"Huang, Kai" <kai.huang@intel.com> writes:

> On Mon, 2025-09-15 at 08:59 +0000, Nikunj A Dadhania wrote:
>> Currently, dirty logging relies on write protecting guest memory and
>> marking dirty GFNs during subsequent write faults. 
>> 
>
> Better to point out "On AMD platforms only".

Subject being "KVM: SVM: ", this seems redundant to me.

>
>> This method works but
>> incurs overhead due to additional write faults for each dirty GFN.
>> 
>> Implement support for the Page Modification Logging (PML) feature, a
>> hardware-assisted method for efficient dirty logging. PML automatically
>> logs dirty GPA[51:12] to a 4K buffer when the CPU sets NPT D-bits. Two new
>> VMCB fields are utilized: PML_ADDR and PML_INDEX. The PML_INDEX is
>> initialized to 511 (8 bytes per GPA entry), and the CPU decreases the
>> PML_INDEX after logging each GPA. When the PML buffer is full, a
>> VMEXIT(PML_FULL) with exit code 0x407 is generated.
>> 
>> PML is enabled by default when supported and can be disabled via the 'pml'
>> module parameter.
>
> This changelog mentions nothing about interaction between PML vs nested.
>
> On VMX, PML is emulated for L2 (for nested EPT) but is never enabled in
> hardware when CPU runs in L2, so:
>
> 1) PML is exposed to L1 (for nested EPT).
> 2) PML needs to be turned off when CPU runs in L2 otherwise L2's GPA 
>    could be logged, and turned on again after CPU leaves L2 (and restore
>    PML buffer/index of VMCS01).

I get your point and I see that when nested VM entry, PML is set in
the nested_ctl for L2.

I am trying to create this scenario, and couldnt get the L2 GPA's.

>
> It doesn't seem this series supports emulating PML for L2 (for nested
> NPT), because AMD's PML is also enumerated via a CPUID bit (while VMX
> doesn't) and it's not exposed to guest, so we don't need to handle nested
> PML_FULL VMEXIT etc.
>
> This is fine I think, and we can support this in the future if needed.
>
> But 2) is also needed anyway for AMD's PML AFAICT, regardless of whether
> 1) is supported or not ?

I see your point, we will need to disable PML for L2.

>
> If so, could we add some text to clarify all of these in the changelog?
>
>
> [...]
>
>>  
>> +void svm_update_cpu_dirty_logging(struct kvm_vcpu *vcpu)
>> +{
>> +	struct vcpu_svm *svm = to_svm(vcpu);
>> +
>> +	if (WARN_ON_ONCE(!pml))
>> +		return;
>> +
>> +	if (is_guest_mode(vcpu))
>> +		return;
>
> VMX has a vmx->nested.update_vmcs01_cpu_dirty_logging boolean.  It's set
> here to indicate PML enabling is not updated for L2 here, but later when
> switching to run in L1, the PML enabling needs to updated.
>
> Shouldn't SVM have similar handling?

Sure, will get back to you on this.

>
>> +
>> +	/*
>> +	 * Note, nr_memslots_dirty_logging can be changed concurrently 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))
>> +		svm->vmcb->control.nested_ctl |= SVM_NESTED_CTL_PML_ENABLE;
>> +	else
>> +		svm->vmcb->control.nested_ctl &= ~SVM_NESTED_CTL_PML_ENABLE;
>> +}
>> +
>> 
> [...]
>
>> --- a/arch/x86/kvm/svm/svm.h
>> +++ b/arch/x86/kvm/svm/svm.h
>> @@ -335,6 +335,8 @@ struct vcpu_svm {
>>  
>>  	/* Guest GIF value, used when vGIF is not enabled */
>>  	bool guest_gif;
>> +
>> +	struct page *pml_page;
>>  };
>
> This seems to be a leftover.

Sure.

Regards
Nikunj