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

Nikunj A. Dadhania
2025-09-29T08:52:28Z

[-- 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);