Re: [PATCH v3 5/5] KVM: SVM: Add Page modification logging support

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

[-- Attachment #1: Type: text/plain, Size: 6687 bytes --]



On 9/29/2025 7:11 AM, Huang, Kai wrote:
> 
>> -	/* Copied from vmcb01.  msrpm_base can be overwritten later.  */
>> -	vmcb02->control.nested_ctl = vmcb01->control.nested_ctl;
>> +	/*
>> +	 * Copied from vmcb01.  msrpm_base can be overwritten later.
>> +	 * Disable PML for nested guest.
>> +	 */
>> +	vmcb02->control.nested_ctl = vmcb01->control.nested_ctl & ~SVM_NESTED_CTL_PML_ENABLE;
> 
> Nit: one side topic:
> 
> It's a bit surprising that currently vmcb01's nested_ctl is copied
> directly to vmcb02.  

At present, I see only SVM_NESTED_CTL_NP_ENABLE being set, other than PML.

> I thought the logic should be more like:
> 
> 	vmcb02->control.nested_ctl = VMCB02_NESTED_CTL_MINIMAL;
> 	if (nested_cpu_has(vmcb12, SOME_FEATURE))
> 		vmcb02->control.nested_ctl |=
> SVM_NESTED_CTL_SOME_FEATURE;
> 	...
> 
> But I guess we can enhance here later, when needed.
Agreed.

> 
>>  	vmcb02->control.iopm_base_pa = vmcb01->control.iopm_base_pa;
>>  	vmcb02->control.msrpm_base_pa = vmcb01->control.msrpm_base_pa;
>>  
>> @@ -1177,6 +1180,12 @@ int nested_svm_vmexit(struct vcpu_svm *svm)
>>  		svm_update_lbrv(vcpu);
>>  	}
>>  
>> +	/* Update dirty logging that might have changed while L2 ran */
>> +	if (svm->nested.update_vmcb01_cpu_dirty_logging) {
>> +		svm->nested.update_vmcb01_cpu_dirty_logging = false;
>> +		svm_update_cpu_dirty_logging(vcpu);
>> +	}
>> +
>>
> 
> [...]
> 
>>  
>> +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)) {
>> +		svm->nested.update_vmcb01_cpu_dirty_logging = true;
>> +		return;
>> +	}
>> +
>> +	/*
>> +	 * 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;
>> +}
>> +
>>
> 
> [...]
> 
>>  	if (lbrv) {
>>  		if (!boot_cpu_has(X86_FEATURE_LBRV))
>>  			lbrv = false;
>> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
>> index 70df7c6413cf..ce38f4a885d3 100644
>> --- a/arch/x86/kvm/svm/svm.h
>> +++ b/arch/x86/kvm/svm/svm.h
>> @@ -216,6 +216,9 @@ struct svm_nested_state {
>>  	 * on its side.
>>  	 */
>>  	bool force_msr_bitmap_recalc;
>> +
>> +	/* Indicates whether dirty logging changed while nested guest ran */
>> +	bool update_vmcb01_cpu_dirty_logging;
>>  };
>>  
>>  struct vcpu_sev_es_state {
>> @@ -717,6 +720,8 @@ static inline void svm_enable_intercept_for_msr(struct kvm_vcpu *vcpu,
>>  	svm_set_intercept_for_msr(vcpu, msr, type, true);
>>  }
>>  
>> +void svm_update_cpu_dirty_logging(struct kvm_vcpu *vcpu);
>> +
>>
> 
> There are duplicated code between SVM and VMX for the above chunks.  The
> logic of marking 'update_cpu_dirty_logging' as pending when vCPU is in L2
> mode and then actually updating the CPU dirty logging when existing from
> L2 to L1 can be made common, as both SVM and VMX share the same logic.

Yes, this can be consolidated as well.
 > How about below diff [*]? It could be split into multiple patches (e.g.,
> one to move the code around 'update_cpu_dirty_logging_pending' from VMX to
> x86 common, and the other one to apply SVM changes on top of that).
> 
> Build test only .. I plan to have a test as well (needing to setup testing
> environment) but it would be great to see whether it works at SVM side.
> > [*] The diff (also attached):

I tested the above patch and it needed few SVM and x86 changes, here is a
diff on top of your patch that works on SVM:

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 009cef2477f0..d3030c99dba3 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -3232,10 +3232,12 @@ static int bus_lock_exit(struct kvm_vcpu *vcpu)
 
 void svm_update_cpu_dirty_logging(struct kvm_vcpu *vcpu, bool enable)
 {
+	struct vcpu_svm *svm = to_svm(vcpu);
+
 	if (enable)
-		svm->vmcb->control.nested_ctl |= svm_nested_ctl_pml_enable;
+		svm->vmcb->control.nested_ctl |= SVM_NESTED_CTL_PML_ENABLE;
 	else
-		svm->vmcb->control.nested_ctl &= ~svm_nested_ctl_pml_enable;
+		svm->vmcb->control.nested_ctl &= ~SVM_NESTED_CTL_PML_ENABLE;
 }
 
 static void svm_flush_pml_buffer(struct kvm_vcpu *vcpu)
@@ -3628,7 +3630,7 @@ static int svm_handle_exit(struct kvm_vcpu *vcpu, fastpath_t exit_fastpath)
 	 * dirty bitmap current by processing logged GPAs rather than waiting for
 	 * PML_FULL exit.
 	 */
-	if (pml && !is_guest_mode(vcpu))
+	if (enable_pml && !is_guest_mode(vcpu))
 		svm_flush_pml_buffer(vcpu);
 
 	/* SEV-ES guests must use the CR write traps to track CR registers. */
@@ -5097,7 +5099,7 @@ static int svm_vm_init(struct kvm *kvm)
 			return ret;
 	}
 
-	if (pml)
+	if (enable_pml)
 		kvm->arch.cpu_dirty_log_size = PML_LOG_NR_ENTRIES;
 
 	svm_srso_vm_init();
@@ -5457,7 +5459,7 @@ static __init int svm_hardware_setup(void)
 	nrips = nrips && boot_cpu_has(X86_FEATURE_NRIPS);
 
 	enable_pml = enable_pml && npt_enabled && cpu_feature_enabled(X86_FEATURE_PML);
-	if (pml)
+	if (enable_pml)
 		pr_info("Page modification logging supported\n");
 
 	if (lbrv) {
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index ce38f4a885d3..a73306592f18 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -51,6 +51,7 @@ extern bool intercept_smi;
 extern bool x2avic_enabled;
 extern bool vnmi;
 extern int lbrv;
+extern bool __read_mostly enable_pml;
 
 /*
  * Clean bits in VMCB.
@@ -216,9 +217,6 @@ struct svm_nested_state {
 	 * on its side.
 	 */
 	bool force_msr_bitmap_recalc;
-
-	/* Indicates whether dirty logging changed while nested guest ran */
-	bool update_vmcb01_cpu_dirty_logging;
 };
 
 struct vcpu_sev_es_state {
@@ -720,7 +718,7 @@ static inline void svm_enable_intercept_for_msr(struct kvm_vcpu *vcpu,
 	svm_set_intercept_for_msr(vcpu, msr, type, true);
 }
 
-void svm_update_cpu_dirty_logging(struct kvm_vcpu *vcpu);
+void svm_update_cpu_dirty_logging(struct kvm_vcpu *vcpu, bool enable);
 
 /* nested.c */
 
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 95843c854b11..35a748b0d4af 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -146,6 +146,7 @@ struct kvm_x86_ops kvm_x86_ops __read_mostly;
 #include <asm/kvm-x86-ops.h>
 EXPORT_STATIC_CALL_GPL(kvm_x86_get_cs_db_l_bits);
 EXPORT_STATIC_CALL_GPL(kvm_x86_cache_reg);
+EXPORT_STATIC_CALL_GPL(kvm_x86_update_cpu_dirty_logging);
 
 static bool __read_mostly ignore_msrs = 0;
 module_param(ignore_msrs, bool, 0644);




[-- Attachment #2: pml_x86_svm_fixes.diff --]
[-- Type: text/plain, Size: 2924 bytes --]

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 009cef2477f0..d3030c99dba3 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -3232,10 +3232,12 @@ static int bus_lock_exit(struct kvm_vcpu *vcpu)
 
 void svm_update_cpu_dirty_logging(struct kvm_vcpu *vcpu, bool enable)
 {
+	struct vcpu_svm *svm = to_svm(vcpu);
+
 	if (enable)
-		svm->vmcb->control.nested_ctl |= svm_nested_ctl_pml_enable;
+		svm->vmcb->control.nested_ctl |= SVM_NESTED_CTL_PML_ENABLE;
 	else
-		svm->vmcb->control.nested_ctl &= ~svm_nested_ctl_pml_enable;
+		svm->vmcb->control.nested_ctl &= ~SVM_NESTED_CTL_PML_ENABLE;
 }
 
 static void svm_flush_pml_buffer(struct kvm_vcpu *vcpu)
@@ -3628,7 +3630,7 @@ static int svm_handle_exit(struct kvm_vcpu *vcpu, fastpath_t exit_fastpath)
 	 * dirty bitmap current by processing logged GPAs rather than waiting for
 	 * PML_FULL exit.
 	 */
-	if (pml && !is_guest_mode(vcpu))
+	if (enable_pml && !is_guest_mode(vcpu))
 		svm_flush_pml_buffer(vcpu);
 
 	/* SEV-ES guests must use the CR write traps to track CR registers. */
@@ -5097,7 +5099,7 @@ static int svm_vm_init(struct kvm *kvm)
 			return ret;
 	}
 
-	if (pml)
+	if (enable_pml)
 		kvm->arch.cpu_dirty_log_size = PML_LOG_NR_ENTRIES;
 
 	svm_srso_vm_init();
@@ -5457,7 +5459,7 @@ static __init int svm_hardware_setup(void)
 	nrips = nrips && boot_cpu_has(X86_FEATURE_NRIPS);
 
 	enable_pml = enable_pml && npt_enabled && cpu_feature_enabled(X86_FEATURE_PML);
-	if (pml)
+	if (enable_pml)
 		pr_info("Page modification logging supported\n");
 
 	if (lbrv) {
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index ce38f4a885d3..a73306592f18 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -51,6 +51,7 @@ extern bool intercept_smi;
 extern bool x2avic_enabled;
 extern bool vnmi;
 extern int lbrv;
+extern bool __read_mostly enable_pml;
 
 /*
  * Clean bits in VMCB.
@@ -216,9 +217,6 @@ struct svm_nested_state {
 	 * on its side.
 	 */
 	bool force_msr_bitmap_recalc;
-
-	/* Indicates whether dirty logging changed while nested guest ran */
-	bool update_vmcb01_cpu_dirty_logging;
 };
 
 struct vcpu_sev_es_state {
@@ -720,7 +718,7 @@ static inline void svm_enable_intercept_for_msr(struct kvm_vcpu *vcpu,
 	svm_set_intercept_for_msr(vcpu, msr, type, true);
 }
 
-void svm_update_cpu_dirty_logging(struct kvm_vcpu *vcpu);
+void svm_update_cpu_dirty_logging(struct kvm_vcpu *vcpu, bool enable);
 
 /* nested.c */
 
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 95843c854b11..35a748b0d4af 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -146,6 +146,7 @@ struct kvm_x86_ops kvm_x86_ops __read_mostly;
 #include <asm/kvm-x86-ops.h>
 EXPORT_STATIC_CALL_GPL(kvm_x86_get_cs_db_l_bits);
 EXPORT_STATIC_CALL_GPL(kvm_x86_cache_reg);
+EXPORT_STATIC_CALL_GPL(kvm_x86_update_cpu_dirty_logging);
 
 static bool __read_mostly ignore_msrs = 0;
 module_param(ignore_msrs, bool, 0644);