Re: [RFC PATCH 5/5] KVM: selftests: Test dirty logging of sparse nested mappings

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] KVM: selftests: test_dirty_log() uses default HugeTLB pool instead of 2M
- [Low] KVM: selftests: free_2m_hugepages() duplicates get_free_hugepages()
- [Low] KVM: selftests: test_dirty_log() pre-divides extra_mem_pages by 512
--

commit f746099353da91d06474f4e326f2f464849f22eb
Author: Fred Griffoul <fgriffo@amazon.co.uk>

KVM: selftests: Test dirty logging of sparse nested mappings

This commit adds an x86 KVM selftest that verifies dirty logging of sparsely
mapped L2 memory across shadow MMU rmap chunk boundaries, as well as eager
page splitting of 2 MiB nested TDP mappings when dirty logging is enabled.

> diff --git a/tools/testing/selftests/kvm/x86/nested_dirty_log_sparse_test.c b/tools/testing/selftests/kvm/x86/nested_dirty_log_sparse_test.c
> new file mode 100644
> index 0000000000000..bc1c6b8760c96
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/x86/nested_dirty_log_sparse_test.c
> @@ -0,0 +1,275 @@

[ ... ]

> +static void test_dirty_log(bool nested_tdp, bool huge)
> +{
> +	u64 npages = huge ? HUGE_MEM_PAGES : SPARSE_MEM_PAGES;
> +	enum vm_mem_backing_src_type src;
> +	u64 pages_2m = 0, pages_4k = 0;
> +	gva_t nested_gva = 0;
> +	struct kvm_vcpu *vcpu;
> +	unsigned long *bmap;
> +	struct kvm_vm *vm;
> +	struct ucall uc;
> +
> +	pr_info("Nested TDP: %s, L1 TDP page size: %s\n",
> +		nested_tdp ? "enabled" : "disabled", huge ? "2M" : "4K");
> +
> +	/* Room for L1's page tables and TDP page tables of the test memory. */
> +	vm = __vm_create_with_one_vcpu(&vcpu, 2 * (npages / 512 + 16),
> +				       l1_guest_code);

[Severity: Low]
Does passing 2 * (npages / 512 + 16) as extra_mem_pages divide npages by 512
twice?

__vm_create_with_one_vcpu() passes extra_mem_pages to vm_nr_pages_required(),
which already divides extra_mem_pages by PTES_PER_MIN_PAGE (512):

tools/testing/selftests/kvm/lib/kvm_util.c:vm_nr_pages_required() {
    ...
	nr_pages += (nr_pages + extra_mem_pages) / PTES_PER_MIN_PAGE * 2;
    ...
}

Because npages is pre-divided by 512 here, extra_mem_pages is 96 (or 40 when
huge is true), so (nr_pages + extra_mem_pages) / 512 evaluates to 1 and adds
no extra page-table pages beyond the default. Should this pass 2 * npages
instead?

> +	if (nested_tdp)
> +		vm_enable_tdp(vm);
> +
> +	if (kvm_cpu_has(X86_FEATURE_VMX))
> +		vcpu_alloc_vmx(vm, &nested_gva);
> +	else
> +		vcpu_alloc_svm(vm, &nested_gva);
> +	vcpu_args_set(vcpu, 1, nested_gva);
> +
> +	src = huge ? VM_MEM_SRC_ANONYMOUS_HUGETLB : VM_MEM_SRC_ANONYMOUS;
> +	vm_userspace_mem_region_add(vm, src, TEST_MEM_BASE, TEST_MEM_SLOT,
> +				    npages, 0);

[Severity: Medium]
Should src use VM_MEM_SRC_ANONYMOUS_HUGETLB_2MB instead of
VM_MEM_SRC_ANONYMOUS_HUGETLB when huge is true?

In main(), the test checks specifically for four free 2 MiB hugepages in
/sys/kernel/mm/hugepages/hugepages-2048kB/free_hugepages and sizes the region
to 8 MiB (HUGE_MEM_PAGES).

However, VM_MEM_SRC_ANONYMOUS_HUGETLB uses the system default HugeTLB page
size without MAP_HUGE_2MB:

tools/testing/selftests/kvm/lib/test_util.c:get_backing_src_pagesz() {
	switch (i) {
    ...
	case VM_MEM_SRC_ANONYMOUS_HUGETLB:
	case VM_MEM_SRC_SHARED_HUGETLB:
		return get_def_hugetlb_pagesz();
	default:
		return MAP_HUGE_PAGE_SIZE(flag);
	}
}

On a host booted with default_hugepagesz=1G that has 2 MiB hugepages allocated
in the 2048kB pool, get_def_hugetlb_pagesz() will check the 1 GiB pool in
/proc/meminfo and either skip the test if HugePages_Total is 0, or return 1
GiB and cause __kvm_mmap() in vm_mem_add() to fail with EINVAL because the 8
MiB region is not 1 GiB aligned.

[ ... ]

> +static u64 free_2m_hugepages(void)
> +{
> +	u64 nr = 0;
> +	FILE *f;
> +
> +	f = fopen("/sys/kernel/mm/hugepages/hugepages-2048kB/free_hugepages", "r");
> +	if (!f)
> +		return 0;
> +	if (fscanf(f, "%lu", &nr) != 1)
> +		nr = 0;
> +	fclose(f);
> +
> +	return nr;
> +}
> +
> +int main(int argc, char *argv[])
> +{
> +	TEST_REQUIRE(kvm_cpu_has(X86_FEATURE_VMX) || kvm_cpu_has(X86_FEATURE_SVM));
> +
> +	test_dirty_log(/*nested_tdp=*/false, /*huge=*/false);
> +
> +	if (!kvm_cpu_has_tdp())
> +		return 0;
> +
> +	test_dirty_log(/*nested_tdp=*/true, /*huge=*/false);
> +
> +	if (free_2m_hugepages() >= HUGE_MEM_PAGES / HUGE_PAGES_PER_2M &&
> +	    get_kvm_param_bool("tdp_mmu") && get_kvm_param_bool("eager_page_split"))
> +		test_dirty_log(/*nested_tdp=*/true, /*huge=*/true);

[Severity: Low]
This isn't a bug, but test_util.h already provides get_free_hugepages() in
tools/testing/selftests/kvm/lib/test_util.c, which reads the same sysfs file
and returns the free bytes:

tools/testing/selftests/kvm/lib/test_util.c:get_free_hugepages() {
	char path[128];
	size_t free;

	snprintf(path, sizeof(path),
		 "/sys/kernel/mm/hugepages/hugepages-%zukB/free_hugepages",
		 page_size >> 10);
	if (!test_sysfs_path(path))
		return 0;

	free = get_sysfs_val(path);
	return free * page_size;
}

Could main() use get_free_hugepages(SZ_2M) >= HUGE_MEM_PAGES * PAGE_SIZE
instead of defining free_2m_hugepages()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104005331.50600-1-griffoul@gmail.com?part=5