Re: [PATCH v5 30/34] KVM: selftests: Add Xen runstate migration test

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Memory leak of the VM metadata structures and userspace memory regions.
- [Low] The new test file is missing from the KVM selftests Makefile, preventing it from being compiled.
- [Low] The test uses undefined KVM UAPI ioctls, causing compilation failures.
--

commit 0780d159e1213b2e5be1d76bb2647a1007af3fdf
Author: David Woodhouse <dwmw@amazon.co.uk>

Test that Xen runstate (steal time) is correctly accounted across a
simulated live migration using KVM_XEN_VCPU_ATTR and KVM_[GS]ET_CLOCK_GUEST.

This commit adds a new KVM selftest to verify Xen runstate tracking across
a simulated VM migration. It creates a VM with Xen HVM configuration,
accumulates runstate time, saves and restores the VM state, and verifies
that the elapsed time during migration is properly accounted as steal time.

>  .../selftests/kvm/x86/xen_migration_test.c    | 194 ++++++++++++++++++
>  1 file changed, 194 insertions(+)
>  create mode 100644 tools/testing/selftests/kvm/x86/xen_migration_test.c

[Severity: Low]
Is the new xen_migration_test.c file missing from the KVM selftests Makefile?
Without adding it to TEST_GEN_PROGS_x86 (or an equivalent target) in
tools/testing/selftests/kvm/Makefile, the test is ignored by the build system
and will never be compiled or executed.

> diff --git a/tools/testing/selftests/kvm/x86/xen_migration_test.c b/tools/testing/selftests/kvm/x86/xen_migration_test.c
> new file mode 100644
> index 000000000000..37e8ace00611
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/x86/xen_migration_test.c
[ ... ]
> +int main(void)
> +{
> +	struct pvclock_vcpu_time_info pvti;
> +	struct kvm_xen_vcpu_attr runstate_save;
[ ... ]
> +	pr_info("  Guest sees: runnable=%" PRIu64 " running=%" PRIu64 "\n",
> +		uc.args[2], uc.args[3]);
> +
> +	/* Save clock state */
> +	ret = __vcpu_ioctl(vcpu, KVM_GET_CLOCK_GUEST, &pvti);

[Severity: Low]
Does this code fail to compile because KVM_GET_CLOCK_GUEST is an undeclared
identifier? It does not appear to exist in the standard KVM UAPI headers.

> +	TEST_ASSERT(!ret, "KVM_GET_CLOCK_GUEST failed");
> +
> +	/* Save TSC offset */
[ ... ]
> +	uint64_t saved_runnable = runstate_save.u.runstate.time_runnable;
> +
> +	kvm_vm_release(vm);

[Severity: Medium]
Does calling kvm_vm_release() here instead of kvm_vm_free() leak the 
struct kvm_vm object and its mapped userspace memory regions?

kvm_vm_release() is an internal helper that releases vCPUs and file
descriptors, but it leaves the VM structure and memory maps allocated
right before the vm pointer is overwritten with a new VM.

> +
> +	/* === MIGRATION GAP === */
> +	pr_info("=== Simulating migration (sleeping 10ms) ===\n");
> +	usleep(10000);
> +
> +	/* === DESTINATION SIDE === */
> +	pr_info("=== Destination: create new VM and restore ===\n");
> +	vm = create_xen_vm(&vcpu);
> +
> +	/* Restore TSC offset */
> +	vcpu_set_msr(vcpu, MSR_IA32_TSC_ADJUST, tsc_offset);
> +
> +	/* Restore clock — kvmclock will now be ~10ms ahead of the snapshot */
> +	vcpu_ioctl(vcpu, KVM_SET_CLOCK_GUEST, &pvti);

[Severity: Low]
Similarly, does this fail to compile because KVM_SET_CLOCK_GUEST does not
exist in the standard KVM UAPI headers?

> +
> +	/* Restore runstate exactly as saved (state=runnable) */
[ ... ]
> +	TEST_ASSERT(steal_increase >= 10000000ULL &&
> +		    steal_increase < 5000000000ULL,
> +		    "Steal time increase %" PRIu64 " ns not in expected range "
> +		    "[10ms, 5s]", steal_increase);
> +
> +	kvm_vm_release(vm);

[Severity: Medium]
Does this also permanently leak the second VM's metadata structures and
memory maps before the test exits?

> +	pr_info("PASS: Migration gap correctly accounted as steal time\n");
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260608145455.89187-1-dwmw2@infradead.org?part=30