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

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

Nikunj A Dadhania <nikunj@amd.com> writes:

> "Huang, Kai" <kai.huang@intel.com> writes:
>
>> On Mon, 2025-09-15 at 08:59 +0000, Nikunj A Dadhania wrote:
>>> 
>>> 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.

As nested_ctl from L1 was getting copied into L2, PML flag was also set
for L2. I have disabled this now.

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

L2 GPA's were not logged because PML_ADDR and PML_INDEX of the VMCB02
was not populated.

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

Yes, this will be required as it is handling a case when there is a
dirty logging change for L1 while L2 is running. So this variable is
used to record such change and when L2 exits, the pending change needs
to be made in L1's VMCB.

Thanks for the detailed review.

Regards
Nikunj