From 89ea56a4043ab6ec598e7969a9b88883815f53a0 Mon Sep 17 00:00:00 2001 From: Sean Christopherson Date: Thu, 30 Jan 2025 08:31:35 -0800 Subject: [PATCH 01/31] KVM: selftests: Actually emit forced emulation prefix for kvm_asm_safe_fep() Use KVM_ASM_SAFE_FEP, not simply KVM_ASM_SAFE, for kvm_asm_safe_fep(), as the non-FEP version doesn't force emulation (stating the obvious). Note, there are currently no users of kvm_asm_safe_fep(). Fixes: ab3b6a7de8df ("KVM: selftests: Add a forced emulation variation of KVM_ASM_SAFE()") Link: https://lore.kernel.org/r/20250130163135.270770-1-seanjc@google.com Signed-off-by: Sean Christopherson --- tools/testing/selftests/kvm/include/x86/processor.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/testing/selftests/kvm/include/x86/processor.h b/tools/testing/selftests/kvm/include/x86/processor.h index d60da8966772..9d365144ac3e 100644 --- a/tools/testing/selftests/kvm/include/x86/processor.h +++ b/tools/testing/selftests/kvm/include/x86/processor.h @@ -1244,7 +1244,7 @@ void vm_install_exception_handler(struct kvm_vm *vm, int vector, uint64_t ign_error_code; \ uint8_t vector; \ \ - asm volatile(KVM_ASM_SAFE(insn) \ + asm volatile(KVM_ASM_SAFE_FEP(insn) \ : KVM_ASM_SAFE_OUTPUTS(vector, ign_error_code) \ : inputs \ : KVM_ASM_SAFE_CLOBBERS); \ From fe49f80052578cfe79411b6b47bb9ea278acbea8 Mon Sep 17 00:00:00 2001 From: Maxim Levitsky Date: Fri, 10 Jan 2025 16:29:45 -0800 Subject: [PATCH 02/31] KVM: selftests: Support multiple write retires in dirty_log_test If dirty_log_test is run nested, it is possible for entries in the emulated PML log to appear before the actual memory write is committed to the RAM, due to the way KVM retries memory writes as a response to a MMU fault. In addition to that in some very rare cases retry can happen more than once, which will lead to the test failure because once the write is finally committed it may have a very outdated iteration value. Detect and avoid this case. Cc: Peter Xu Signed-off-by: Maxim Levitsky Link: https://lore.kernel.org/r/20250111003004.1235645-2-seanjc@google.com Signed-off-by: Sean Christopherson --- tools/testing/selftests/kvm/dirty_log_test.c | 52 +++++++++++++++++++- 1 file changed, 50 insertions(+), 2 deletions(-) diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c index aacf80f57439..cdae103314fc 100644 --- a/tools/testing/selftests/kvm/dirty_log_test.c +++ b/tools/testing/selftests/kvm/dirty_log_test.c @@ -152,6 +152,7 @@ static atomic_t vcpu_sync_stop_requested; * sem_vcpu_stop and before vcpu continues to run. */ static bool dirty_ring_vcpu_ring_full; + /* * This is only used for verifying the dirty pages. Dirty ring has a very * tricky case when the ring just got full, kvm will do userspace exit due to @@ -166,7 +167,51 @@ static bool dirty_ring_vcpu_ring_full; * dirty gfn we've collected, so that if a mismatch of data found later in the * verifying process, we let it pass. */ -static uint64_t dirty_ring_last_page; +static uint64_t dirty_ring_last_page = -1ULL; + +/* + * In addition to the above, it is possible (especially if this + * test is run nested) for the above scenario to repeat multiple times: + * + * The following can happen: + * + * - L1 vCPU: Memory write is logged to PML but not committed. + * + * - L1 test thread: Ignores the write because its last dirty ring entry + * Resets the dirty ring which: + * - Resets the A/D bits in EPT + * - Issues tlb flush (invept), which is intercepted by L0 + * + * - L0: frees the whole nested ept mmu root as the response to invept, + * and thus ensures that when memory write is retried, it will fault again + * + * - L1 vCPU: Same memory write is logged to the PML but not committed again. + * + * - L1 test thread: Ignores the write because its last dirty ring entry (again) + * Resets the dirty ring which: + * - Resets the A/D bits in EPT (again) + * - Issues tlb flush (again) which is intercepted by L0 + * + * ... + * + * N times + * + * - L1 vCPU: Memory write is logged in the PML and then committed. + * Lots of other memory writes are logged and committed. + * ... + * + * - L1 test thread: Sees the memory write along with other memory writes + * in the dirty ring, and since the write is usually not + * the last entry in the dirty-ring and has a very outdated + * iteration, the test fails. + * + * + * Note that this is only possible when the write was the last log entry + * write during iteration N-1, thus remember last iteration last log entry + * and also don't fail when it is reported in the next iteration, together with + * an outdated iteration count. + */ +static uint64_t dirty_ring_prev_iteration_last_page; enum log_mode_t { /* Only use KVM_GET_DIRTY_LOG for logging */ @@ -316,6 +361,8 @@ static uint32_t dirty_ring_collect_one(struct kvm_dirty_gfn *dirty_gfns, struct kvm_dirty_gfn *cur; uint32_t count = 0; + dirty_ring_prev_iteration_last_page = dirty_ring_last_page; + while (true) { cur = &dirty_gfns[*fetch_index % test_dirty_ring_count]; if (!dirty_gfn_is_dirtied(cur)) @@ -613,7 +660,8 @@ static void vm_dirty_log_verify(enum vm_guest_mode mode, unsigned long *bmap) */ min_iter = iteration - 1; continue; - } else if (page == dirty_ring_last_page) { + } else if (page == dirty_ring_last_page || + page == dirty_ring_prev_iteration_last_page) { /* * Please refer to comments in * dirty_ring_last_page. From 67428ee7b7460146db98365c4e87664f91e122ad Mon Sep 17 00:00:00 2001 From: Sean Christopherson Date: Fri, 10 Jan 2025 16:29:46 -0800 Subject: [PATCH 03/31] KVM: selftests: Sync dirty_log_test iteration to guest *before* resuming Sync the new iteration to the guest prior to restarting the vCPU, otherwise it's possible for the vCPU to dirty memory for the next iteration using the current iteration's value. Note, because the guest can be interrupted between the vCPU's load of the iteration and its write to memory, it's still possible for the guest to store the previous iteration to memory as the previous iteration may be cached in a CPU register (which the test accounts for). Note #2, the test's current approach of collecting dirty entries *before* stopping the vCPU also results dirty memory having the previous iteration. E.g. if page is dirtied in the previous iteration, but not the current iteration, the verification phase will observe the previous iteration's value in memory. That wart will be remedied in the near future, at which point synchronizing the iteration before restarting the vCPU will guarantee the only way for verification to observe stale iterations is due to the CPU register caching case, or due to a dirty entry being collected before the store retires. Link: https://lore.kernel.org/r/20250111003004.1235645-3-seanjc@google.com Signed-off-by: Sean Christopherson --- tools/testing/selftests/kvm/dirty_log_test.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c index cdae103314fc..41c158cf5444 100644 --- a/tools/testing/selftests/kvm/dirty_log_test.c +++ b/tools/testing/selftests/kvm/dirty_log_test.c @@ -859,9 +859,9 @@ static void run_test(enum vm_guest_mode mode, void *arg) */ if (++iteration == p->iterations) WRITE_ONCE(host_quit, true); + sync_global_to_guest(vm, iteration); sem_post(&sem_vcpu_cont); - sync_global_to_guest(vm, iteration); } pthread_join(vcpu_thread, NULL); From ff0efc77bc964d8a33ab3505fb17b3c73b40c040 Mon Sep 17 00:00:00 2001 From: Sean Christopherson Date: Fri, 10 Jan 2025 16:29:47 -0800 Subject: [PATCH 04/31] KVM: selftests: Drop signal/kick from dirty ring testcase Drop the signal/kick from dirty_log_test's dirty ring handling, as kicking the vCPU adds marginal value, at the cost of adding significant complexity to the test. Asynchronously interrupting the vCPU isn't novel; unless the kernel is fully tickless, the vCPU will be interrupted by IRQs for any decently large interval. And exiting to userspace mode in the middle of a sequence isn't novel either, as the vCPU will do so every time the ring becomes full. Link: https://lore.kernel.org/r/20250111003004.1235645-4-seanjc@google.com Signed-off-by: Sean Christopherson --- tools/testing/selftests/kvm/dirty_log_test.c | 106 +++---------------- 1 file changed, 15 insertions(+), 91 deletions(-) diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c index 41c158cf5444..d9911e20337f 100644 --- a/tools/testing/selftests/kvm/dirty_log_test.c +++ b/tools/testing/selftests/kvm/dirty_log_test.c @@ -236,24 +236,6 @@ static enum log_mode_t host_log_mode; static pthread_t vcpu_thread; static uint32_t test_dirty_ring_count = TEST_DIRTY_RING_COUNT; -static void vcpu_kick(void) -{ - pthread_kill(vcpu_thread, SIG_IPI); -} - -/* - * In our test we do signal tricks, let's use a better version of - * sem_wait to avoid signal interrupts - */ -static void sem_wait_until(sem_t *sem) -{ - int ret; - - do - ret = sem_wait(sem); - while (ret == -1 && errno == EINTR); -} - static bool clear_log_supported(void) { return kvm_has_cap(KVM_CAP_MANUAL_DIRTY_LOG_PROTECT2); @@ -292,17 +274,14 @@ static void vcpu_handle_sync_stop(void) /* It means main thread is sleeping waiting */ atomic_set(&vcpu_sync_stop_requested, false); sem_post(&sem_vcpu_stop); - sem_wait_until(&sem_vcpu_cont); + sem_wait(&sem_vcpu_cont); } } -static void default_after_vcpu_run(struct kvm_vcpu *vcpu, int ret, int err) +static void default_after_vcpu_run(struct kvm_vcpu *vcpu) { struct kvm_run *run = vcpu->run; - TEST_ASSERT(ret == 0 || (ret == -1 && err == EINTR), - "vcpu run failed: errno=%d", err); - TEST_ASSERT(get_ucall(vcpu, NULL) == UCALL_SYNC, "Invalid guest sync status: exit_reason=%s", exit_reason_str(run->exit_reason)); @@ -371,7 +350,6 @@ static uint32_t dirty_ring_collect_one(struct kvm_dirty_gfn *dirty_gfns, "%u != %u", cur->slot, slot); TEST_ASSERT(cur->offset < num_pages, "Offset overflow: " "0x%llx >= 0x%x", cur->offset, num_pages); - //pr_info("fetch 0x%x page %llu\n", *fetch_index, cur->offset); __set_bit_le(cur->offset, bitmap); dirty_ring_last_page = cur->offset; dirty_gfn_set_collected(cur); @@ -382,13 +360,6 @@ static uint32_t dirty_ring_collect_one(struct kvm_dirty_gfn *dirty_gfns, return count; } -static void dirty_ring_wait_vcpu(void) -{ - /* This makes sure that hardware PML cache flushed */ - vcpu_kick(); - sem_wait_until(&sem_vcpu_stop); -} - static void dirty_ring_continue_vcpu(void) { pr_info("Notifying vcpu to continue\n"); @@ -400,18 +371,6 @@ static void dirty_ring_collect_dirty_pages(struct kvm_vcpu *vcpu, int slot, uint32_t *ring_buf_idx) { uint32_t count = 0, cleared; - bool continued_vcpu = false; - - dirty_ring_wait_vcpu(); - - if (!dirty_ring_vcpu_ring_full) { - /* - * This is not a ring-full event, it's safe to allow - * vcpu to continue - */ - dirty_ring_continue_vcpu(); - continued_vcpu = true; - } /* Only have one vcpu */ count = dirty_ring_collect_one(vcpu_map_dirty_ring(vcpu), @@ -427,16 +386,13 @@ static void dirty_ring_collect_dirty_pages(struct kvm_vcpu *vcpu, int slot, TEST_ASSERT(cleared == count, "Reset dirty pages (%u) mismatch " "with collected (%u)", cleared, count); - if (!continued_vcpu) { - TEST_ASSERT(dirty_ring_vcpu_ring_full, - "Didn't continue vcpu even without ring full"); + if (READ_ONCE(dirty_ring_vcpu_ring_full)) dirty_ring_continue_vcpu(); - } pr_info("Iteration %ld collected %u pages\n", iteration, count); } -static void dirty_ring_after_vcpu_run(struct kvm_vcpu *vcpu, int ret, int err) +static void dirty_ring_after_vcpu_run(struct kvm_vcpu *vcpu) { struct kvm_run *run = vcpu->run; @@ -444,17 +400,14 @@ static void dirty_ring_after_vcpu_run(struct kvm_vcpu *vcpu, int ret, int err) if (get_ucall(vcpu, NULL) == UCALL_SYNC) { /* We should allow this to continue */ ; - } else if (run->exit_reason == KVM_EXIT_DIRTY_RING_FULL || - (ret == -1 && err == EINTR)) { + } else if (run->exit_reason == KVM_EXIT_DIRTY_RING_FULL) { /* Update the flag first before pause */ - WRITE_ONCE(dirty_ring_vcpu_ring_full, - run->exit_reason == KVM_EXIT_DIRTY_RING_FULL); + WRITE_ONCE(dirty_ring_vcpu_ring_full, true); sem_post(&sem_vcpu_stop); - pr_info("vcpu stops because %s...\n", - dirty_ring_vcpu_ring_full ? - "dirty ring is full" : "vcpu is kicked out"); - sem_wait_until(&sem_vcpu_cont); + pr_info("Dirty ring full, waiting for it to be collected\n"); + sem_wait(&sem_vcpu_cont); pr_info("vcpu continues now.\n"); + WRITE_ONCE(dirty_ring_vcpu_ring_full, false); } else { TEST_ASSERT(false, "Invalid guest sync status: " "exit_reason=%s", @@ -473,7 +426,7 @@ struct log_mode { void *bitmap, uint32_t num_pages, uint32_t *ring_buf_idx); /* Hook to call when after each vcpu run */ - void (*after_vcpu_run)(struct kvm_vcpu *vcpu, int ret, int err); + void (*after_vcpu_run)(struct kvm_vcpu *vcpu); } log_modes[LOG_MODE_NUM] = { { .name = "dirty-log", @@ -544,47 +497,24 @@ static void log_mode_collect_dirty_pages(struct kvm_vcpu *vcpu, int slot, mode->collect_dirty_pages(vcpu, slot, bitmap, num_pages, ring_buf_idx); } -static void log_mode_after_vcpu_run(struct kvm_vcpu *vcpu, int ret, int err) +static void log_mode_after_vcpu_run(struct kvm_vcpu *vcpu) { struct log_mode *mode = &log_modes[host_log_mode]; if (mode->after_vcpu_run) - mode->after_vcpu_run(vcpu, ret, err); + mode->after_vcpu_run(vcpu); } static void *vcpu_worker(void *data) { - int ret; struct kvm_vcpu *vcpu = data; uint64_t pages_count = 0; - struct kvm_signal_mask *sigmask = alloca(offsetof(struct kvm_signal_mask, sigset) - + sizeof(sigset_t)); - sigset_t *sigset = (sigset_t *) &sigmask->sigset; - - /* - * SIG_IPI is unblocked atomically while in KVM_RUN. It causes the - * ioctl to return with -EINTR, but it is still pending and we need - * to accept it with the sigwait. - */ - sigmask->len = 8; - pthread_sigmask(0, NULL, sigset); - sigdelset(sigset, SIG_IPI); - vcpu_ioctl(vcpu, KVM_SET_SIGNAL_MASK, sigmask); - - sigemptyset(sigset); - sigaddset(sigset, SIG_IPI); while (!READ_ONCE(host_quit)) { - /* Clear any existing kick signals */ pages_count += TEST_PAGES_PER_LOOP; /* Let the guest dirty the random pages */ - ret = __vcpu_run(vcpu); - if (ret == -1 && errno == EINTR) { - int sig = -1; - sigwait(sigset, &sig); - assert(sig == SIG_IPI); - } - log_mode_after_vcpu_run(vcpu, ret, errno); + vcpu_run(vcpu); + log_mode_after_vcpu_run(vcpu); } pr_info("Dirtied %"PRIu64" pages\n", pages_count); @@ -838,7 +768,7 @@ static void run_test(enum vm_guest_mode mode, void *arg) * we need to stop vcpu when verify data. */ atomic_set(&vcpu_sync_stop_requested, true); - sem_wait_until(&sem_vcpu_stop); + sem_wait(&sem_vcpu_stop); /* * NOTE: for dirty ring, it's possible that we didn't stop at * GUEST_SYNC but instead we stopped because ring is full; @@ -905,7 +835,6 @@ int main(int argc, char *argv[]) .interval = TEST_HOST_LOOP_INTERVAL, }; int opt, i; - sigset_t sigset; sem_init(&sem_vcpu_stop, 0, 0); sem_init(&sem_vcpu_cont, 0, 0); @@ -964,11 +893,6 @@ int main(int argc, char *argv[]) srandom(time(0)); - /* Ensure that vCPU threads start with SIG_IPI blocked. */ - sigemptyset(&sigset); - sigaddset(&sigset, SIG_IPI); - pthread_sigmask(SIG_BLOCK, &sigset, NULL); - if (host_log_mode_option == LOG_MODE_ALL) { /* Run each log mode */ for (i = 0; i < LOG_MODE_NUM; i++) { From 1230907864d7cf6c280e3b6c00883e0ef7ed3319 Mon Sep 17 00:00:00 2001 From: Sean Christopherson Date: Fri, 10 Jan 2025 16:29:48 -0800 Subject: [PATCH 05/31] KVM: selftests: Drop stale srandom() initialization from dirty_log_test Drop an srandom() initialization that was leftover from the conversion to use selftests' guest_random_xxx() APIs. Reviewed-by: Maxim Levitsky Link: https://lore.kernel.org/r/20250111003004.1235645-5-seanjc@google.com Signed-off-by: Sean Christopherson --- tools/testing/selftests/kvm/dirty_log_test.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c index d9911e20337f..55a744373c80 100644 --- a/tools/testing/selftests/kvm/dirty_log_test.c +++ b/tools/testing/selftests/kvm/dirty_log_test.c @@ -891,8 +891,6 @@ int main(int argc, char *argv[]) pr_info("Test iterations: %"PRIu64", interval: %"PRIu64" (ms)\n", p.iterations, p.interval); - srandom(time(0)); - if (host_log_mode_option == LOG_MODE_ALL) { /* Run each log mode */ for (i = 0; i < LOG_MODE_NUM; i++) { From af2d85d34d158c4ab49e1d41c19a100ee83c148a Mon Sep 17 00:00:00 2001 From: Sean Christopherson Date: Fri, 10 Jan 2025 16:29:49 -0800 Subject: [PATCH 06/31] KVM: selftests: Precisely track number of dirty/clear pages for each iteration Track and print the number of dirty and clear pages for each iteration. This provides parity between all log modes, and will allow collecting the dirty ring multiple times per iteration without spamming the console. Opportunistically drop the "Dirtied N pages" print, which is redundant and wrong. For the dirty ring testcase, the vCPU isn't guaranteed to complete a loop. And when the vCPU does complete a loot, there are no guarantees that it has *dirtied* that many pages; because the writes are to random address, the vCPU may have written the same page over and over, i.e. only dirtied one page. While the number of writes performed by the vCPU is also interesting, e.g. the pr_info() could be tweaked to use different verbiage, pages_count doesn't correctly track the number of writes either (because loops aren't guaranteed to a complete). Delete the print for now, as a future patch will precisely track the number of writes, at which point the verification phase can report the number of writes performed by each iteration. Link: https://lore.kernel.org/r/20250111003004.1235645-6-seanjc@google.com Signed-off-by: Sean Christopherson --- tools/testing/selftests/kvm/dirty_log_test.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c index 55a744373c80..08cbecd1a135 100644 --- a/tools/testing/selftests/kvm/dirty_log_test.c +++ b/tools/testing/selftests/kvm/dirty_log_test.c @@ -388,8 +388,6 @@ static void dirty_ring_collect_dirty_pages(struct kvm_vcpu *vcpu, int slot, if (READ_ONCE(dirty_ring_vcpu_ring_full)) dirty_ring_continue_vcpu(); - - pr_info("Iteration %ld collected %u pages\n", iteration, count); } static void dirty_ring_after_vcpu_run(struct kvm_vcpu *vcpu) @@ -508,24 +506,20 @@ static void log_mode_after_vcpu_run(struct kvm_vcpu *vcpu) static void *vcpu_worker(void *data) { struct kvm_vcpu *vcpu = data; - uint64_t pages_count = 0; while (!READ_ONCE(host_quit)) { - pages_count += TEST_PAGES_PER_LOOP; /* Let the guest dirty the random pages */ vcpu_run(vcpu); log_mode_after_vcpu_run(vcpu); } - pr_info("Dirtied %"PRIu64" pages\n", pages_count); - return NULL; } static void vm_dirty_log_verify(enum vm_guest_mode mode, unsigned long *bmap) { + uint64_t page, nr_dirty_pages = 0, nr_clean_pages = 0; uint64_t step = vm_num_host_pages(mode, 1); - uint64_t page; uint64_t *value_ptr; uint64_t min_iter = 0; @@ -544,7 +538,7 @@ static void vm_dirty_log_verify(enum vm_guest_mode mode, unsigned long *bmap) if (__test_and_clear_bit_le(page, bmap)) { bool matched; - host_dirty_count++; + nr_dirty_pages++; /* * If the bit is set, the value written onto @@ -605,7 +599,7 @@ static void vm_dirty_log_verify(enum vm_guest_mode mode, unsigned long *bmap) " incorrect (iteration=%"PRIu64")", page, *value_ptr, iteration); } else { - host_clear_count++; + nr_clean_pages++; /* * If cleared, the value written can be any * value smaller or equals to the iteration @@ -639,6 +633,12 @@ static void vm_dirty_log_verify(enum vm_guest_mode mode, unsigned long *bmap) } } } + + pr_info("Iteration %2ld: dirty: %-6lu clean: %-6lu\n", + iteration, nr_dirty_pages, nr_clean_pages); + + host_dirty_count += nr_dirty_pages; + host_clear_count += nr_clean_pages; } static struct kvm_vm *create_vm(enum vm_guest_mode mode, struct kvm_vcpu **vcpu, From f2228aa083240f44794c3617d4a22134ab26217c Mon Sep 17 00:00:00 2001 From: Sean Christopherson Date: Fri, 10 Jan 2025 16:29:50 -0800 Subject: [PATCH 07/31] KVM: selftests: Read per-page value into local var when verifying dirty_log_test Cache the page's value during verification in a local variable, re-reading from the pointer is ugly and error prone, e.g. allows for bugs like checking the pointer itself instead of the value. No functional change intended. Reviewed-by: Maxim Levitsky Link: https://lore.kernel.org/r/20250111003004.1235645-7-seanjc@google.com Signed-off-by: Sean Christopherson --- tools/testing/selftests/kvm/dirty_log_test.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c index 08cbecd1a135..5a04a7bd73e0 100644 --- a/tools/testing/selftests/kvm/dirty_log_test.c +++ b/tools/testing/selftests/kvm/dirty_log_test.c @@ -520,11 +520,10 @@ static void vm_dirty_log_verify(enum vm_guest_mode mode, unsigned long *bmap) { uint64_t page, nr_dirty_pages = 0, nr_clean_pages = 0; uint64_t step = vm_num_host_pages(mode, 1); - uint64_t *value_ptr; uint64_t min_iter = 0; for (page = 0; page < host_num_pages; page += step) { - value_ptr = host_test_mem + page * host_page_size; + uint64_t val = *(uint64_t *)(host_test_mem + page * host_page_size); /* If this is a special page that we were tracking... */ if (__test_and_clear_bit_le(page, host_bmap_track)) { @@ -545,11 +544,10 @@ static void vm_dirty_log_verify(enum vm_guest_mode mode, unsigned long *bmap) * the corresponding page should be either the * previous iteration number or the current one. */ - matched = (*value_ptr == iteration || - *value_ptr == iteration - 1); + matched = (val == iteration || val == iteration - 1); if (host_log_mode == LOG_MODE_DIRTY_RING && !matched) { - if (*value_ptr == iteration - 2 && min_iter <= iteration - 2) { + if (val == iteration - 2 && min_iter <= iteration - 2) { /* * Short answer: this case is special * only for dirty ring test where the @@ -597,7 +595,7 @@ static void vm_dirty_log_verify(enum vm_guest_mode mode, unsigned long *bmap) TEST_ASSERT(matched, "Set page %"PRIu64" value %"PRIu64 " incorrect (iteration=%"PRIu64")", - page, *value_ptr, iteration); + page, val, iteration); } else { nr_clean_pages++; /* @@ -619,11 +617,11 @@ static void vm_dirty_log_verify(enum vm_guest_mode mode, unsigned long *bmap) * we'll see that page P is cleared, with * value "iteration-1". */ - TEST_ASSERT(*value_ptr <= iteration, + TEST_ASSERT(val <= iteration, "Clear page %"PRIu64" value %"PRIu64 " incorrect (iteration=%"PRIu64")", - page, *value_ptr, iteration); - if (*value_ptr == iteration) { + page, val, iteration); + if (val == iteration) { /* * This page is _just_ modified; it * should report its dirtyness in the From 9b1feec83e1ab1cbab37232b8aab16301066336d Mon Sep 17 00:00:00 2001 From: Sean Christopherson Date: Fri, 10 Jan 2025 16:29:51 -0800 Subject: [PATCH 08/31] KVM: selftests: Continuously reap dirty ring while vCPU is running Continue collecting entries from the dirty ring for the entire time the vCPU is running. Collecting exactly once all but guarantees the vCPU will encounter a "ring full" event and stop. While testing ring full is interesting, stopping and doing nothing is not, especially for larger intervals as the test effectively does nothing for a much longer time. To balance continuous collection with letting the guest make forward progress, chunk the interval waiting into 1ms loops (which also makes the math dead simple). To maintain coverage for "ring full", collect entries on subsequent iterations if and only if the ring has been filled at least once. I.e. let the ring fill up (if the interval allows), but after that contiuously empty it so that the vCPU can keep running. Opportunistically drop unnecessary zero-initialization of "count". Reviewed-by: Maxim Levitsky Link: https://lore.kernel.org/r/20250111003004.1235645-8-seanjc@google.com Signed-off-by: Sean Christopherson --- tools/testing/selftests/kvm/dirty_log_test.c | 63 ++++++++++++++------ 1 file changed, 46 insertions(+), 17 deletions(-) diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c index 5a04a7bd73e0..2aee047b8b1c 100644 --- a/tools/testing/selftests/kvm/dirty_log_test.c +++ b/tools/testing/selftests/kvm/dirty_log_test.c @@ -340,8 +340,6 @@ static uint32_t dirty_ring_collect_one(struct kvm_dirty_gfn *dirty_gfns, struct kvm_dirty_gfn *cur; uint32_t count = 0; - dirty_ring_prev_iteration_last_page = dirty_ring_last_page; - while (true) { cur = &dirty_gfns[*fetch_index % test_dirty_ring_count]; if (!dirty_gfn_is_dirtied(cur)) @@ -360,17 +358,11 @@ static uint32_t dirty_ring_collect_one(struct kvm_dirty_gfn *dirty_gfns, return count; } -static void dirty_ring_continue_vcpu(void) -{ - pr_info("Notifying vcpu to continue\n"); - sem_post(&sem_vcpu_cont); -} - static void dirty_ring_collect_dirty_pages(struct kvm_vcpu *vcpu, int slot, void *bitmap, uint32_t num_pages, uint32_t *ring_buf_idx) { - uint32_t count = 0, cleared; + uint32_t count, cleared; /* Only have one vcpu */ count = dirty_ring_collect_one(vcpu_map_dirty_ring(vcpu), @@ -385,9 +377,6 @@ static void dirty_ring_collect_dirty_pages(struct kvm_vcpu *vcpu, int slot, */ TEST_ASSERT(cleared == count, "Reset dirty pages (%u) mismatch " "with collected (%u)", cleared, count); - - if (READ_ONCE(dirty_ring_vcpu_ring_full)) - dirty_ring_continue_vcpu(); } static void dirty_ring_after_vcpu_run(struct kvm_vcpu *vcpu) @@ -404,7 +393,6 @@ static void dirty_ring_after_vcpu_run(struct kvm_vcpu *vcpu) sem_post(&sem_vcpu_stop); pr_info("Dirty ring full, waiting for it to be collected\n"); sem_wait(&sem_vcpu_cont); - pr_info("vcpu continues now.\n"); WRITE_ONCE(dirty_ring_vcpu_ring_full, false); } else { TEST_ASSERT(false, "Invalid guest sync status: " @@ -755,11 +743,52 @@ static void run_test(enum vm_guest_mode mode, void *arg) pthread_create(&vcpu_thread, NULL, vcpu_worker, vcpu); while (iteration < p->iterations) { + bool saw_dirty_ring_full = false; + unsigned long i; + + dirty_ring_prev_iteration_last_page = dirty_ring_last_page; + /* Give the vcpu thread some time to dirty some pages */ - usleep(p->interval * 1000); - log_mode_collect_dirty_pages(vcpu, TEST_MEM_SLOT_INDEX, - bmap, host_num_pages, - &ring_buf_idx); + for (i = 0; i < p->interval; i++) { + usleep(1000); + + /* + * Reap dirty pages while the guest is running so that + * dirty ring full events are resolved, i.e. so that a + * larger interval doesn't always end up with a vCPU + * that's effectively blocked. Collecting while the + * guest is running also verifies KVM doesn't lose any + * state. + * + * For bitmap modes, KVM overwrites the entire bitmap, + * i.e. collecting the bitmaps is destructive. Collect + * the bitmap only on the first pass, otherwise this + * test would lose track of dirty pages. + */ + if (i && host_log_mode != LOG_MODE_DIRTY_RING) + continue; + + /* + * For the dirty ring, empty the ring on subsequent + * passes only if the ring was filled at least once, + * to verify KVM's handling of a full ring (emptying + * the ring on every pass would make it unlikely the + * vCPU would ever fill the fing). + */ + if (READ_ONCE(dirty_ring_vcpu_ring_full)) + saw_dirty_ring_full = true; + if (i && !saw_dirty_ring_full) + continue; + + log_mode_collect_dirty_pages(vcpu, TEST_MEM_SLOT_INDEX, + bmap, host_num_pages, + &ring_buf_idx); + + if (READ_ONCE(dirty_ring_vcpu_ring_full)) { + pr_info("Dirty ring emptied, restarting vCPU\n"); + sem_post(&sem_vcpu_cont); + } + } /* * See vcpu_sync_stop_requested definition for details on why From deb8b8400e31505e0c151dcc4cf436863df4f8d9 Mon Sep 17 00:00:00 2001 From: Maxim Levitsky Date: Fri, 10 Jan 2025 16:29:52 -0800 Subject: [PATCH 09/31] KVM: selftests: Limit dirty_log_test's s390x workaround to s390x s390 specific workaround causes the dirty-log mode of the test to dirty all guest memory on the first iteration, which is very slow when the test is run in a nested VM. Limit this workaround to s390x. Signed-off-by: Maxim Levitsky Link: https://lore.kernel.org/r/20250111003004.1235645-9-seanjc@google.com Signed-off-by: Sean Christopherson --- tools/testing/selftests/kvm/dirty_log_test.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c index 2aee047b8b1c..55a385499434 100644 --- a/tools/testing/selftests/kvm/dirty_log_test.c +++ b/tools/testing/selftests/kvm/dirty_log_test.c @@ -98,6 +98,7 @@ static void guest_code(void) uint64_t addr; int i; +#ifdef __s390x__ /* * On s390x, all pages of a 1M segment are initially marked as dirty * when a page of the segment is written to for the very first time. @@ -108,6 +109,7 @@ static void guest_code(void) addr = guest_test_virt_mem + i * guest_page_size; vcpu_arch_put_guest(*(uint64_t *)addr, READ_ONCE(iteration)); } +#endif while (true) { for (i = 0; i < TEST_PAGES_PER_LOOP; i++) { From f3629c0ef167aec0eb069cff2526cd54e966b5d8 Mon Sep 17 00:00:00 2001 From: Sean Christopherson Date: Fri, 10 Jan 2025 16:29:53 -0800 Subject: [PATCH 10/31] KVM: selftests: Honor "stop" request in dirty ring test Now that the vCPU doesn't dirty every page on the first iteration for architectures that support the dirty ring, honor vcpu_stop in the dirty ring's vCPU worker, i.e. stop when the main thread says "stop". This will allow plumbing vcpu_stop into the guest so that the vCPU doesn't need to periodically exit to userspace just to see if it should stop. Add a comment explaining that marking all pages as dirty is problematic for the dirty ring, as it results in the guest getting stuck on "ring full". This could be addressed by adding a GUEST_SYNC() in that initial loop, but it's not clear how that would interact with s390's behavior. Link: https://lore.kernel.org/r/20250111003004.1235645-10-seanjc@google.com Signed-off-by: Sean Christopherson --- tools/testing/selftests/kvm/dirty_log_test.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c index 55a385499434..8d31e275a23d 100644 --- a/tools/testing/selftests/kvm/dirty_log_test.c +++ b/tools/testing/selftests/kvm/dirty_log_test.c @@ -387,8 +387,7 @@ static void dirty_ring_after_vcpu_run(struct kvm_vcpu *vcpu) /* A ucall-sync or ring-full event is allowed */ if (get_ucall(vcpu, NULL) == UCALL_SYNC) { - /* We should allow this to continue */ - ; + vcpu_handle_sync_stop(); } else if (run->exit_reason == KVM_EXIT_DIRTY_RING_FULL) { /* Update the flag first before pause */ WRITE_ONCE(dirty_ring_vcpu_ring_full, true); @@ -697,6 +696,15 @@ static void run_test(enum vm_guest_mode mode, void *arg) #ifdef __s390x__ /* Align to 1M (segment size) */ guest_test_phys_mem = align_down(guest_test_phys_mem, 1 << 20); + + /* + * The workaround in guest_code() to write all pages prior to the first + * iteration isn't compatible with the dirty ring, as the dirty ring + * support relies on the vCPU to actually stop when vcpu_stop is set so + * that the vCPU doesn't hang waiting for the dirty ring to be emptied. + */ + TEST_ASSERT(host_log_mode != LOG_MODE_DIRTY_RING, + "Test needs to be updated to support s390 dirty ring"); #endif pr_info("guest physical test memory offset: 0x%lx\n", guest_test_phys_mem); From 0a818b3541af09ddaf412d125dc7315fc448851e Mon Sep 17 00:00:00 2001 From: Sean Christopherson Date: Fri, 10 Jan 2025 16:29:54 -0800 Subject: [PATCH 11/31] KVM: selftests: Keep dirty_log_test vCPU in guest until it needs to stop In the dirty_log_test guest code, exit to userspace only when the vCPU is explicitly told to stop. Periodically exiting just to check if a flag has been set is unnecessary, weirdly complex, and wastes time handling exits that could be used to dirty memory. Opportunistically convert 'i' to a uint64_t to guard against the unlikely scenario that guest_num_pages exceeds the storage of an int. Reviewed-by: Maxim Levitsky Link: https://lore.kernel.org/r/20250111003004.1235645-11-seanjc@google.com Signed-off-by: Sean Christopherson --- tools/testing/selftests/kvm/dirty_log_test.c | 43 ++++++++++---------- 1 file changed, 22 insertions(+), 21 deletions(-) diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c index 8d31e275a23d..40c8f5551c8e 100644 --- a/tools/testing/selftests/kvm/dirty_log_test.c +++ b/tools/testing/selftests/kvm/dirty_log_test.c @@ -31,9 +31,6 @@ /* Default guest test virtual memory offset */ #define DEFAULT_GUEST_TEST_MEM 0xc0000000 -/* How many pages to dirty for each guest loop */ -#define TEST_PAGES_PER_LOOP 1024 - /* How many host loops to run (one KVM_GET_DIRTY_LOG for each loop) */ #define TEST_HOST_LOOP_N 32UL @@ -75,6 +72,7 @@ static uint64_t host_page_size; static uint64_t guest_page_size; static uint64_t guest_num_pages; static uint64_t iteration; +static bool vcpu_stop; /* * Guest physical memory offset of the testing memory slot. @@ -96,9 +94,10 @@ static uint64_t guest_test_virt_mem = DEFAULT_GUEST_TEST_MEM; static void guest_code(void) { uint64_t addr; - int i; #ifdef __s390x__ + uint64_t i; + /* * On s390x, all pages of a 1M segment are initially marked as dirty * when a page of the segment is written to for the very first time. @@ -112,7 +111,7 @@ static void guest_code(void) #endif while (true) { - for (i = 0; i < TEST_PAGES_PER_LOOP; i++) { + while (!READ_ONCE(vcpu_stop)) { addr = guest_test_virt_mem; addr += (guest_random_u64(&guest_rng) % guest_num_pages) * guest_page_size; @@ -140,14 +139,7 @@ static uint64_t host_track_next_count; /* Whether dirty ring reset is requested, or finished */ static sem_t sem_vcpu_stop; static sem_t sem_vcpu_cont; -/* - * This is only set by main thread, and only cleared by vcpu thread. It is - * used to request vcpu thread to stop at the next GUEST_SYNC, since GUEST_SYNC - * is the only place that we'll guarantee both "dirty bit" and "dirty data" - * will match. E.g., SIG_IPI won't guarantee that if the vcpu is interrupted - * after setting dirty bit but before the data is written. - */ -static atomic_t vcpu_sync_stop_requested; + /* * This is updated by the vcpu thread to tell the host whether it's a * ring-full event. It should only be read until a sem_wait() of @@ -272,9 +264,7 @@ static void clear_log_collect_dirty_pages(struct kvm_vcpu *vcpu, int slot, /* Should only be called after a GUEST_SYNC */ static void vcpu_handle_sync_stop(void) { - if (atomic_read(&vcpu_sync_stop_requested)) { - /* It means main thread is sleeping waiting */ - atomic_set(&vcpu_sync_stop_requested, false); + if (READ_ONCE(vcpu_stop)) { sem_post(&sem_vcpu_stop); sem_wait(&sem_vcpu_cont); } @@ -801,11 +791,24 @@ static void run_test(enum vm_guest_mode mode, void *arg) } /* - * See vcpu_sync_stop_requested definition for details on why - * we need to stop vcpu when verify data. + * Stop the vCPU prior to collecting and verifying the dirty + * log. If the vCPU is allowed to run during collection, then + * pages that are written during this iteration may be missed, + * i.e. collected in the next iteration. And if the vCPU is + * writing memory during verification, pages that this thread + * sees as clean may be written with this iteration's value. */ - atomic_set(&vcpu_sync_stop_requested, true); + WRITE_ONCE(vcpu_stop, true); + sync_global_to_guest(vm, vcpu_stop); sem_wait(&sem_vcpu_stop); + + /* + * Clear vcpu_stop after the vCPU thread has acknowledge the + * stop request and is waiting, i.e. is definitely not running! + */ + WRITE_ONCE(vcpu_stop, false); + sync_global_to_guest(vm, vcpu_stop); + /* * NOTE: for dirty ring, it's possible that we didn't stop at * GUEST_SYNC but instead we stopped because ring is full; @@ -813,8 +816,6 @@ static void run_test(enum vm_guest_mode mode, void *arg) * the flush of the last page, and since we handle the last * page specially verification will succeed anyway. */ - assert(host_log_mode == LOG_MODE_DIRTY_RING || - atomic_read(&vcpu_sync_stop_requested) == false); vm_dirty_log_verify(mode, bmap); /* From 9a91f6542435ad94cbb8e5b64ca3d4cc31d3c12a Mon Sep 17 00:00:00 2001 From: Sean Christopherson Date: Fri, 10 Jan 2025 16:29:55 -0800 Subject: [PATCH 12/31] KVM: selftests: Post to sem_vcpu_stop if and only if vcpu_stop is true When running dirty_log_test using the dirty ring, post to sem_vcpu_stop only when the main thread has explicitly requested that the vCPU stop. Synchronizing the vCPU and main thread whenever the dirty ring happens to be full is unnecessary, as KVM's ABI is to actively prevent the vCPU from running until the ring is no longer full. I.e. attempting to run the vCPU will simply result in KVM_EXIT_DIRTY_RING_FULL without ever entering the guest. And if KVM doesn't exit, e.g. let's the vCPU dirty more pages, then that's a KVM bug worth finding. Posting to sem_vcpu_stop on ring full also makes it difficult to get the test logic right, e.g. it's easy to let the vCPU keep running when it shouldn't, as a ring full can essentially happen at any given time. Opportunistically rework the handling of dirty_ring_vcpu_ring_full to leave it set for the remainder of the iteration in order to simplify the surrounding logic. Link: https://lore.kernel.org/r/20250111003004.1235645-12-seanjc@google.com Signed-off-by: Sean Christopherson --- tools/testing/selftests/kvm/dirty_log_test.c | 18 ++++-------------- 1 file changed, 4 insertions(+), 14 deletions(-) diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c index 40c8f5551c8e..8544e8425f9c 100644 --- a/tools/testing/selftests/kvm/dirty_log_test.c +++ b/tools/testing/selftests/kvm/dirty_log_test.c @@ -379,12 +379,8 @@ static void dirty_ring_after_vcpu_run(struct kvm_vcpu *vcpu) if (get_ucall(vcpu, NULL) == UCALL_SYNC) { vcpu_handle_sync_stop(); } else if (run->exit_reason == KVM_EXIT_DIRTY_RING_FULL) { - /* Update the flag first before pause */ WRITE_ONCE(dirty_ring_vcpu_ring_full, true); - sem_post(&sem_vcpu_stop); - pr_info("Dirty ring full, waiting for it to be collected\n"); - sem_wait(&sem_vcpu_cont); - WRITE_ONCE(dirty_ring_vcpu_ring_full, false); + vcpu_handle_sync_stop(); } else { TEST_ASSERT(false, "Invalid guest sync status: " "exit_reason=%s", @@ -743,7 +739,6 @@ static void run_test(enum vm_guest_mode mode, void *arg) pthread_create(&vcpu_thread, NULL, vcpu_worker, vcpu); while (iteration < p->iterations) { - bool saw_dirty_ring_full = false; unsigned long i; dirty_ring_prev_iteration_last_page = dirty_ring_last_page; @@ -775,19 +770,12 @@ static void run_test(enum vm_guest_mode mode, void *arg) * the ring on every pass would make it unlikely the * vCPU would ever fill the fing). */ - if (READ_ONCE(dirty_ring_vcpu_ring_full)) - saw_dirty_ring_full = true; - if (i && !saw_dirty_ring_full) + if (i && !READ_ONCE(dirty_ring_vcpu_ring_full)) continue; log_mode_collect_dirty_pages(vcpu, TEST_MEM_SLOT_INDEX, bmap, host_num_pages, &ring_buf_idx); - - if (READ_ONCE(dirty_ring_vcpu_ring_full)) { - pr_info("Dirty ring emptied, restarting vCPU\n"); - sem_post(&sem_vcpu_cont); - } } /* @@ -829,6 +817,8 @@ static void run_test(enum vm_guest_mode mode, void *arg) WRITE_ONCE(host_quit, true); sync_global_to_guest(vm, iteration); + WRITE_ONCE(dirty_ring_vcpu_ring_full, false); + sem_post(&sem_vcpu_cont); } From c616f36a1002a1364f3b0ab5c938d796d6096837 Mon Sep 17 00:00:00 2001 From: Sean Christopherson Date: Fri, 10 Jan 2025 16:29:56 -0800 Subject: [PATCH 13/31] KVM: selftests: Use continue to handle all "pass" scenarios in dirty_log_test When verifying pages in dirty_log_test, immediately continue on all "pass" scenarios to make the logic consistent in how it handles pass vs. fail. No functional change intended. Reviewed-by: Maxim Levitsky Link: https://lore.kernel.org/r/20250111003004.1235645-13-seanjc@google.com Signed-off-by: Sean Christopherson --- tools/testing/selftests/kvm/dirty_log_test.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c index 8544e8425f9c..d7cf1840bd80 100644 --- a/tools/testing/selftests/kvm/dirty_log_test.c +++ b/tools/testing/selftests/kvm/dirty_log_test.c @@ -510,8 +510,6 @@ static void vm_dirty_log_verify(enum vm_guest_mode mode, unsigned long *bmap) } if (__test_and_clear_bit_le(page, bmap)) { - bool matched; - nr_dirty_pages++; /* @@ -519,9 +517,10 @@ static void vm_dirty_log_verify(enum vm_guest_mode mode, unsigned long *bmap) * the corresponding page should be either the * previous iteration number or the current one. */ - matched = (val == iteration || val == iteration - 1); + if (val == iteration || val == iteration - 1) + continue; - if (host_log_mode == LOG_MODE_DIRTY_RING && !matched) { + if (host_log_mode == LOG_MODE_DIRTY_RING) { if (val == iteration - 2 && min_iter <= iteration - 2) { /* * Short answer: this case is special @@ -567,10 +566,8 @@ static void vm_dirty_log_verify(enum vm_guest_mode mode, unsigned long *bmap) } } - TEST_ASSERT(matched, - "Set page %"PRIu64" value %"PRIu64 - " incorrect (iteration=%"PRIu64")", - page, val, iteration); + TEST_FAIL("Dirty page %lu value (%lu) != iteration (%lu)", + page, val, iteration); } else { nr_clean_pages++; /* From 24b9a2a613779cf5a0c6f7629cb7a70e6a769838 Mon Sep 17 00:00:00 2001 From: Sean Christopherson Date: Fri, 10 Jan 2025 16:29:57 -0800 Subject: [PATCH 14/31] KVM: selftests: Print (previous) last_page on dirty page value mismatch Print out the last dirty pages from the current and previous iteration on verification failures. In many cases, bugs (especially test bugs) occur on the edges, i.e. on or near the last pages, and being able to correlate failures with the last pages can aid in debug. Reviewed-by: Maxim Levitsky Link: https://lore.kernel.org/r/20250111003004.1235645-14-seanjc@google.com Signed-off-by: Sean Christopherson --- tools/testing/selftests/kvm/dirty_log_test.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c index d7cf1840bd80..fe8cc7f77e22 100644 --- a/tools/testing/selftests/kvm/dirty_log_test.c +++ b/tools/testing/selftests/kvm/dirty_log_test.c @@ -566,8 +566,10 @@ static void vm_dirty_log_verify(enum vm_guest_mode mode, unsigned long *bmap) } } - TEST_FAIL("Dirty page %lu value (%lu) != iteration (%lu)", - page, val, iteration); + TEST_FAIL("Dirty page %lu value (%lu) != iteration (%lu) " + "(last = %lu, prev_last = %lu)", + page, val, iteration, dirty_ring_last_page, + dirty_ring_prev_iteration_last_page); } else { nr_clean_pages++; /* @@ -590,9 +592,10 @@ static void vm_dirty_log_verify(enum vm_guest_mode mode, unsigned long *bmap) * value "iteration-1". */ TEST_ASSERT(val <= iteration, - "Clear page %"PRIu64" value %"PRIu64 - " incorrect (iteration=%"PRIu64")", - page, val, iteration); + "Clear page %lu value (%lu) > iteration (%lu) " + "(last = %lu, prev_last = %lu)", + page, val, iteration, dirty_ring_last_page, + dirty_ring_prev_iteration_last_page); if (val == iteration) { /* * This page is _just_ modified; it From d0bd72cb916072f88d9eda5ee2a7f85a999af404 Mon Sep 17 00:00:00 2001 From: Sean Christopherson Date: Fri, 10 Jan 2025 16:29:58 -0800 Subject: [PATCH 15/31] KVM: selftests: Collect *all* dirty entries in each dirty_log_test iteration Collect all dirty entries during each iteration of dirty_log_test by doing a final collection after the vCPU has been stopped. To deal with KVM's destructive approach to getting the dirty bitmaps, use a second bitmap for the post-stop collection. Collecting all entries that were dirtied during an iteration simplifies the verification logic *and* improves test coverage. - If a page is written during iteration X, but not seen as dirty until X+1, the test can get a false pass if the page is also written during X+1. - If a dirty page used a stale value from a previous iteration, the test would grant a false pass. - If a missed dirty log occurs in the last iteration, the test would fail to detect the issue. E.g. modifying mark_page_dirty_in_slot() to dirty an unwritten gfn: if (memslot && kvm_slot_dirty_track_enabled(memslot)) { unsigned long rel_gfn = gfn - memslot->base_gfn; u32 slot = (memslot->as_id << 16) | memslot->id; if (!vcpu->extra_dirty && gfn_to_memslot(kvm, gfn + 1) == memslot) { vcpu->extra_dirty = true; mark_page_dirty_in_slot(kvm, memslot, gfn + 1); } if (kvm->dirty_ring_size && vcpu) kvm_dirty_ring_push(vcpu, slot, rel_gfn); else if (memslot->dirty_bitmap) set_bit_le(rel_gfn, memslot->dirty_bitmap); } isn't detected with the current approach, even with an interval of 1ms (when running nested in a VM; bare metal would be even *less* likely to detect the bug due to the vCPU being able to dirty more memory). Whereas collecting all dirty entries consistently detects failures with an interval of 700ms or more (the longer interval means a higher probability of an actual write to the prematurely-dirtied page). Link: https://lore.kernel.org/r/20250111003004.1235645-15-seanjc@google.com Signed-off-by: Sean Christopherson --- tools/testing/selftests/kvm/dirty_log_test.c | 149 ++++++------------- 1 file changed, 45 insertions(+), 104 deletions(-) diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c index fe8cc7f77e22..3a4e411353d7 100644 --- a/tools/testing/selftests/kvm/dirty_log_test.c +++ b/tools/testing/selftests/kvm/dirty_log_test.c @@ -134,7 +134,6 @@ static uint64_t host_num_pages; /* For statistics only */ static uint64_t host_dirty_count; static uint64_t host_clear_count; -static uint64_t host_track_next_count; /* Whether dirty ring reset is requested, or finished */ static sem_t sem_vcpu_stop; @@ -422,15 +421,6 @@ struct log_mode { }, }; -/* - * We use this bitmap to track some pages that should have its dirty - * bit set in the _next_ iteration. For example, if we detected the - * page value changed to current iteration but at the same time the - * page bit is cleared in the latest bitmap, then the system must - * report that write in the next get dirty log call. - */ -static unsigned long *host_bmap_track; - static void log_modes_dump(void) { int i; @@ -491,79 +481,52 @@ static void *vcpu_worker(void *data) return NULL; } -static void vm_dirty_log_verify(enum vm_guest_mode mode, unsigned long *bmap) +static void vm_dirty_log_verify(enum vm_guest_mode mode, unsigned long **bmap) { uint64_t page, nr_dirty_pages = 0, nr_clean_pages = 0; uint64_t step = vm_num_host_pages(mode, 1); - uint64_t min_iter = 0; for (page = 0; page < host_num_pages; page += step) { uint64_t val = *(uint64_t *)(host_test_mem + page * host_page_size); + bool bmap0_dirty = __test_and_clear_bit_le(page, bmap[0]); - /* If this is a special page that we were tracking... */ - if (__test_and_clear_bit_le(page, host_bmap_track)) { - host_track_next_count++; - TEST_ASSERT(test_bit_le(page, bmap), - "Page %"PRIu64" should have its dirty bit " - "set in this iteration but it is missing", - page); - } - - if (__test_and_clear_bit_le(page, bmap)) { + /* + * Ensure both bitmaps are cleared, as a page can be written + * multiple times per iteration, i.e. can show up in both + * bitmaps, and the dirty ring is additive, i.e. doesn't purge + * bitmap entries from previous collections. + */ + if (__test_and_clear_bit_le(page, bmap[1]) || bmap0_dirty) { nr_dirty_pages++; /* - * If the bit is set, the value written onto - * the corresponding page should be either the - * previous iteration number or the current one. + * If the page is dirty, the value written to memory + * should be the current iteration number. */ - if (val == iteration || val == iteration - 1) + if (val == iteration) continue; if (host_log_mode == LOG_MODE_DIRTY_RING) { - if (val == iteration - 2 && min_iter <= iteration - 2) { - /* - * Short answer: this case is special - * only for dirty ring test where the - * page is the last page before a kvm - * dirty ring full in iteration N-2. - * - * Long answer: Assuming ring size R, - * one possible condition is: - * - * main thr vcpu thr - * -------- -------- - * iter=1 - * write 1 to page 0~(R-1) - * full, vmexit - * collect 0~(R-1) - * kick vcpu - * write 1 to (R-1)~(2R-2) - * full, vmexit - * iter=2 - * collect (R-1)~(2R-2) - * kick vcpu - * write 1 to (2R-2) - * (NOTE!!! "1" cached in cpu reg) - * write 2 to (2R-1)~(3R-3) - * full, vmexit - * iter=3 - * collect (2R-2)~(3R-3) - * (here if we read value on page - * "2R-2" is 1, while iter=3!!!) - * - * This however can only happen once per iteration. - */ - min_iter = iteration - 1; + /* + * The last page in the ring from this iteration + * or the previous can be written with the value + * from the previous iteration (relative to the + * last page's iteration), as the value to be + * written may be cached in a CPU register. + */ + if (page == dirty_ring_last_page || + page == dirty_ring_prev_iteration_last_page) continue; - } else if (page == dirty_ring_last_page || - page == dirty_ring_prev_iteration_last_page) { - /* - * Please refer to comments in - * dirty_ring_last_page. - */ - continue; - } + } else if (!val && iteration == 1 && bmap0_dirty) { + /* + * When testing get+clear, the dirty bitmap + * starts with all bits set, and so the first + * iteration can observe a "dirty" page that + * was never written, but only in the first + * bitmap (collecting the bitmap also clears + * all dirty pages). + */ + continue; } TEST_FAIL("Dirty page %lu value (%lu) != iteration (%lu) " @@ -574,36 +537,13 @@ static void vm_dirty_log_verify(enum vm_guest_mode mode, unsigned long *bmap) nr_clean_pages++; /* * If cleared, the value written can be any - * value smaller or equals to the iteration - * number. Note that the value can be exactly - * (iteration-1) if that write can happen - * like this: - * - * (1) increase loop count to "iteration-1" - * (2) write to page P happens (with value - * "iteration-1") - * (3) get dirty log for "iteration-1"; we'll - * see that page P bit is set (dirtied), - * and not set the bit in host_bmap_track - * (4) increase loop count to "iteration" - * (which is current iteration) - * (5) get dirty log for current iteration, - * we'll see that page P is cleared, with - * value "iteration-1". + * value smaller than the iteration number. */ - TEST_ASSERT(val <= iteration, - "Clear page %lu value (%lu) > iteration (%lu) " + TEST_ASSERT(val < iteration, + "Clear page %lu value (%lu) >= iteration (%lu) " "(last = %lu, prev_last = %lu)", page, val, iteration, dirty_ring_last_page, dirty_ring_prev_iteration_last_page); - if (val == iteration) { - /* - * This page is _just_ modified; it - * should report its dirtyness in the - * next run - */ - __set_bit_le(page, host_bmap_track); - } } } @@ -639,7 +579,7 @@ static void run_test(enum vm_guest_mode mode, void *arg) struct test_params *p = arg; struct kvm_vcpu *vcpu; struct kvm_vm *vm; - unsigned long *bmap; + unsigned long *bmap[2]; uint32_t ring_buf_idx = 0; int sem_val; @@ -695,8 +635,8 @@ static void run_test(enum vm_guest_mode mode, void *arg) pr_info("guest physical test memory offset: 0x%lx\n", guest_test_phys_mem); - bmap = bitmap_zalloc(host_num_pages); - host_bmap_track = bitmap_zalloc(host_num_pages); + bmap[0] = bitmap_zalloc(host_num_pages); + bmap[1] = bitmap_zalloc(host_num_pages); /* Add an extra memory slot for testing dirty logging */ vm_userspace_mem_region_add(vm, VM_MEM_SRC_ANONYMOUS, @@ -723,7 +663,6 @@ static void run_test(enum vm_guest_mode mode, void *arg) WRITE_ONCE(host_quit, false); host_dirty_count = 0; host_clear_count = 0; - host_track_next_count = 0; WRITE_ONCE(dirty_ring_vcpu_ring_full, false); /* @@ -774,7 +713,7 @@ static void run_test(enum vm_guest_mode mode, void *arg) continue; log_mode_collect_dirty_pages(vcpu, TEST_MEM_SLOT_INDEX, - bmap, host_num_pages, + bmap[0], host_num_pages, &ring_buf_idx); } @@ -804,6 +743,9 @@ static void run_test(enum vm_guest_mode mode, void *arg) * the flush of the last page, and since we handle the last * page specially verification will succeed anyway. */ + log_mode_collect_dirty_pages(vcpu, TEST_MEM_SLOT_INDEX, + bmap[1], host_num_pages, + &ring_buf_idx); vm_dirty_log_verify(mode, bmap); /* @@ -824,12 +766,11 @@ static void run_test(enum vm_guest_mode mode, void *arg) pthread_join(vcpu_thread, NULL); - pr_info("Total bits checked: dirty (%"PRIu64"), clear (%"PRIu64"), " - "track_next (%"PRIu64")\n", host_dirty_count, host_clear_count, - host_track_next_count); + pr_info("Total bits checked: dirty (%lu), clear (%lu)\n", + host_dirty_count, host_clear_count); - free(bmap); - free(host_bmap_track); + free(bmap[0]); + free(bmap[1]); kvm_vm_free(vm); } From 485e27ed208f0d9667608c0e3fa9440654e01399 Mon Sep 17 00:00:00 2001 From: Sean Christopherson Date: Fri, 10 Jan 2025 16:29:59 -0800 Subject: [PATCH 16/31] KVM: sefltests: Verify value of dirty_log_test last page isn't bogus Add a sanity check that a completely garbage value wasn't written to the last dirty page in the ring, e.g. that it doesn't contain the *next* iteration's value. Reviewed-by: Maxim Levitsky Link: https://lore.kernel.org/r/20250111003004.1235645-16-seanjc@google.com Signed-off-by: Sean Christopherson --- tools/testing/selftests/kvm/dirty_log_test.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c index 3a4e411353d7..500257b712e3 100644 --- a/tools/testing/selftests/kvm/dirty_log_test.c +++ b/tools/testing/selftests/kvm/dirty_log_test.c @@ -514,8 +514,9 @@ static void vm_dirty_log_verify(enum vm_guest_mode mode, unsigned long **bmap) * last page's iteration), as the value to be * written may be cached in a CPU register. */ - if (page == dirty_ring_last_page || - page == dirty_ring_prev_iteration_last_page) + if ((page == dirty_ring_last_page || + page == dirty_ring_prev_iteration_last_page) && + val < iteration) continue; } else if (!val && iteration == 1 && bmap0_dirty) { /* From 73eaa2aa14b73fde5a160785b49d99c975bc9609 Mon Sep 17 00:00:00 2001 From: Sean Christopherson Date: Fri, 10 Jan 2025 16:30:00 -0800 Subject: [PATCH 17/31] KVM: selftests: Ensure guest writes min number of pages in dirty_log_test Ensure the vCPU fully completes at least one write in each dirty_log_test iteration, as failure to dirty any pages complicates verification and forces the test to be overly conservative about possible values. E.g. verification needs to allow the last dirty page from a previous iteration to have *any* value, because the vCPU could get stuck for multiple iterations, which is unlikely but can happen in heavily overloaded and/or nested virtualization setups. Somewhat arbitrarily set the minimum to 0x100/256; high enough to be interesting, but not so high as to lead to pointlessly long runtimes. Opportunistically report the number of writes per iteration for debug purposes, and so that a human can sanity check the test. Due to each write targeting a random page, the number of dirty pages will likely be lower than the number of total writes, but it shouldn't be absurdly lower (which would suggest the pRNG is broken) Reported-by: Maxim Levitsky Reviewed-by: Maxim Levitsky Link: https://lore.kernel.org/r/20250111003004.1235645-17-seanjc@google.com Signed-off-by: Sean Christopherson --- tools/testing/selftests/kvm/dirty_log_test.c | 40 ++++++++++++++++++-- 1 file changed, 36 insertions(+), 4 deletions(-) diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c index 500257b712e3..c6b843ec8e0c 100644 --- a/tools/testing/selftests/kvm/dirty_log_test.c +++ b/tools/testing/selftests/kvm/dirty_log_test.c @@ -37,6 +37,12 @@ /* Interval for each host loop (ms) */ #define TEST_HOST_LOOP_INTERVAL 10UL +/* + * Ensure the vCPU is able to perform a reasonable number of writes in each + * iteration to provide a lower bound on coverage. + */ +#define TEST_MIN_WRITES_PER_ITERATION 0x100 + /* Dirty bitmaps are always little endian, so we need to swap on big endian */ #if defined(__s390x__) # define BITOP_LE_SWIZZLE ((BITS_PER_LONG-1) & ~0x7) @@ -72,6 +78,7 @@ static uint64_t host_page_size; static uint64_t guest_page_size; static uint64_t guest_num_pages; static uint64_t iteration; +static uint64_t nr_writes; static bool vcpu_stop; /* @@ -107,6 +114,7 @@ static void guest_code(void) for (i = 0; i < guest_num_pages; i++) { addr = guest_test_virt_mem + i * guest_page_size; vcpu_arch_put_guest(*(uint64_t *)addr, READ_ONCE(iteration)); + nr_writes++; } #endif @@ -118,6 +126,7 @@ static void guest_code(void) addr = align_down(addr, host_page_size); vcpu_arch_put_guest(*(uint64_t *)addr, READ_ONCE(iteration)); + nr_writes++; } GUEST_SYNC(1); @@ -548,8 +557,8 @@ static void vm_dirty_log_verify(enum vm_guest_mode mode, unsigned long **bmap) } } - pr_info("Iteration %2ld: dirty: %-6lu clean: %-6lu\n", - iteration, nr_dirty_pages, nr_clean_pages); + pr_info("Iteration %2ld: dirty: %-6lu clean: %-6lu writes: %-6lu\n", + iteration, nr_dirty_pages, nr_clean_pages, nr_writes); host_dirty_count += nr_dirty_pages; host_clear_count += nr_clean_pages; @@ -665,6 +674,8 @@ static void run_test(enum vm_guest_mode mode, void *arg) host_dirty_count = 0; host_clear_count = 0; WRITE_ONCE(dirty_ring_vcpu_ring_full, false); + WRITE_ONCE(nr_writes, 0); + sync_global_to_guest(vm, nr_writes); /* * Ensure the previous iteration didn't leave a dangling semaphore, i.e. @@ -683,10 +694,22 @@ static void run_test(enum vm_guest_mode mode, void *arg) dirty_ring_prev_iteration_last_page = dirty_ring_last_page; - /* Give the vcpu thread some time to dirty some pages */ - for (i = 0; i < p->interval; i++) { + /* + * Let the vCPU run beyond the configured interval until it has + * performed the minimum number of writes. This verifies the + * guest is making forward progress, e.g. isn't stuck because + * of a KVM bug, and puts a firm floor on test coverage. + */ + for (i = 0; i < p->interval || nr_writes < TEST_MIN_WRITES_PER_ITERATION; i++) { + /* + * Sleep in 1ms chunks to keep the interval math simple + * and so that the test doesn't run too far beyond the + * specified interval. + */ usleep(1000); + sync_global_from_guest(vm, nr_writes); + /* * Reap dirty pages while the guest is running so that * dirty ring full events are resolved, i.e. so that a @@ -737,6 +760,12 @@ static void run_test(enum vm_guest_mode mode, void *arg) WRITE_ONCE(vcpu_stop, false); sync_global_to_guest(vm, vcpu_stop); + /* + * Sync the number of writes performed before verification, the + * info will be printed along with the dirty/clean page counts. + */ + sync_global_from_guest(vm, nr_writes); + /* * NOTE: for dirty ring, it's possible that we didn't stop at * GUEST_SYNC but instead we stopped because ring is full; @@ -760,6 +789,9 @@ static void run_test(enum vm_guest_mode mode, void *arg) WRITE_ONCE(host_quit, true); sync_global_to_guest(vm, iteration); + WRITE_ONCE(nr_writes, 0); + sync_global_to_guest(vm, nr_writes); + WRITE_ONCE(dirty_ring_vcpu_ring_full, false); sem_post(&sem_vcpu_cont); From 2020d3b77a5a40a133e18fb4c870e9fe384405fd Mon Sep 17 00:00:00 2001 From: Sean Christopherson Date: Fri, 10 Jan 2025 16:30:01 -0800 Subject: [PATCH 18/31] KVM: selftests: Tighten checks around prev iter's last dirty page in ring Now that each iteration collects all dirty entries and ensures the guest *completes* at least one write, tighten the exemptions for the last dirty page of the previous iteration. Specifically, the only legal value (other than the current iteration) is N-1. Unlike the last page for the current iteration, the in-progress write from the previous iteration is guaranteed to have completed, otherwise the test would have hung. Reviewed-by: Maxim Levitsky Link: https://lore.kernel.org/r/20250111003004.1235645-18-seanjc@google.com Signed-off-by: Sean Christopherson --- tools/testing/selftests/kvm/dirty_log_test.c | 22 +++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c index c6b843ec8e0c..e9854b5a28f1 100644 --- a/tools/testing/selftests/kvm/dirty_log_test.c +++ b/tools/testing/selftests/kvm/dirty_log_test.c @@ -517,14 +517,22 @@ static void vm_dirty_log_verify(enum vm_guest_mode mode, unsigned long **bmap) if (host_log_mode == LOG_MODE_DIRTY_RING) { /* - * The last page in the ring from this iteration - * or the previous can be written with the value - * from the previous iteration (relative to the - * last page's iteration), as the value to be - * written may be cached in a CPU register. + * The last page in the ring from previous + * iteration can be written with the value + * from the previous iteration, as the value to + * be written may be cached in a CPU register. */ - if ((page == dirty_ring_last_page || - page == dirty_ring_prev_iteration_last_page) && + if (page == dirty_ring_prev_iteration_last_page && + val == iteration - 1) + continue; + + /* + * Any value from a previous iteration is legal + * for the last entry, as the write may not yet + * have retired, i.e. the page may hold whatever + * it had before this iteration started. + */ + if (page == dirty_ring_last_page && val < iteration) continue; } else if (!val && iteration == 1 && bmap0_dirty) { From 2680dcfb34e2c9ac88bff4e01d7bf366aa9976c0 Mon Sep 17 00:00:00 2001 From: Sean Christopherson Date: Fri, 10 Jan 2025 16:30:02 -0800 Subject: [PATCH 19/31] KVM: selftests: Set per-iteration variables at the start of each iteration Set the per-iteration variables at the start of each iteration instead of setting them before the loop, and at the end of each iteration. To ensure the vCPU doesn't race ahead before the first iteration, simply have the vCPU worker want for sem_vcpu_cont, which conveniently avoids the need to special case posting sem_vcpu_cont from the loop. Link: https://lore.kernel.org/r/20250111003004.1235645-19-seanjc@google.com Signed-off-by: Sean Christopherson --- tools/testing/selftests/kvm/dirty_log_test.c | 43 ++++++++------------ 1 file changed, 17 insertions(+), 26 deletions(-) diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c index e9854b5a28f1..40567257ebea 100644 --- a/tools/testing/selftests/kvm/dirty_log_test.c +++ b/tools/testing/selftests/kvm/dirty_log_test.c @@ -481,6 +481,8 @@ static void *vcpu_worker(void *data) { struct kvm_vcpu *vcpu = data; + sem_wait(&sem_vcpu_cont); + while (!READ_ONCE(host_quit)) { /* Let the guest dirty the random pages */ vcpu_run(vcpu); @@ -675,15 +677,9 @@ static void run_test(enum vm_guest_mode mode, void *arg) sync_global_to_guest(vm, guest_test_virt_mem); sync_global_to_guest(vm, guest_num_pages); - /* Start the iterations */ - iteration = 1; - sync_global_to_guest(vm, iteration); - WRITE_ONCE(host_quit, false); host_dirty_count = 0; host_clear_count = 0; - WRITE_ONCE(dirty_ring_vcpu_ring_full, false); - WRITE_ONCE(nr_writes, 0); - sync_global_to_guest(vm, nr_writes); + WRITE_ONCE(host_quit, false); /* * Ensure the previous iteration didn't leave a dangling semaphore, i.e. @@ -695,12 +691,22 @@ static void run_test(enum vm_guest_mode mode, void *arg) sem_getvalue(&sem_vcpu_cont, &sem_val); TEST_ASSERT_EQ(sem_val, 0); + TEST_ASSERT_EQ(vcpu_stop, false); + pthread_create(&vcpu_thread, NULL, vcpu_worker, vcpu); - while (iteration < p->iterations) { + for (iteration = 1; iteration < p->iterations; iteration++) { unsigned long i; + sync_global_to_guest(vm, iteration); + + WRITE_ONCE(nr_writes, 0); + sync_global_to_guest(vm, nr_writes); + dirty_ring_prev_iteration_last_page = dirty_ring_last_page; + WRITE_ONCE(dirty_ring_vcpu_ring_full, false); + + sem_post(&sem_vcpu_cont); /* * Let the vCPU run beyond the configured interval until it has @@ -785,26 +791,11 @@ static void run_test(enum vm_guest_mode mode, void *arg) bmap[1], host_num_pages, &ring_buf_idx); vm_dirty_log_verify(mode, bmap); - - /* - * Set host_quit before sem_vcpu_cont in the final iteration to - * ensure that the vCPU worker doesn't resume the guest. As - * above, the dirty ring test may stop and wait even when not - * explicitly request to do so, i.e. would hang waiting for a - * "continue" if it's allowed to resume the guest. - */ - if (++iteration == p->iterations) - WRITE_ONCE(host_quit, true); - sync_global_to_guest(vm, iteration); - - WRITE_ONCE(nr_writes, 0); - sync_global_to_guest(vm, nr_writes); - - WRITE_ONCE(dirty_ring_vcpu_ring_full, false); - - sem_post(&sem_vcpu_cont); } + WRITE_ONCE(host_quit, true); + sem_post(&sem_vcpu_cont); + pthread_join(vcpu_thread, NULL); pr_info("Total bits checked: dirty (%lu), clear (%lu)\n", From 7f225650e0991c7b9fdfc1524e0381ba61039730 Mon Sep 17 00:00:00 2001 From: Sean Christopherson Date: Fri, 10 Jan 2025 16:30:03 -0800 Subject: [PATCH 20/31] KVM: selftests: Fix an off-by-one in the number of dirty_log_test iterations Actually run all requested iterations, instead of iterations-1 (the count starts at '1' due to the need to avoid '0' as an in-memory value for a dirty page). Reviewed-by: Maxim Levitsky Link: https://lore.kernel.org/r/20250111003004.1235645-20-seanjc@google.com Signed-off-by: Sean Christopherson --- tools/testing/selftests/kvm/dirty_log_test.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c index 40567257ebea..79a7ee189f28 100644 --- a/tools/testing/selftests/kvm/dirty_log_test.c +++ b/tools/testing/selftests/kvm/dirty_log_test.c @@ -695,7 +695,7 @@ static void run_test(enum vm_guest_mode mode, void *arg) pthread_create(&vcpu_thread, NULL, vcpu_worker, vcpu); - for (iteration = 1; iteration < p->iterations; iteration++) { + for (iteration = 1; iteration <= p->iterations; iteration++) { unsigned long i; sync_global_to_guest(vm, iteration); From dae7d81e8d5819bb47d63918c98539d9666a45d1 Mon Sep 17 00:00:00 2001 From: Sean Christopherson Date: Fri, 10 Jan 2025 16:30:04 -0800 Subject: [PATCH 21/31] KVM: selftests: Allow running a single iteration of dirty_log_test Now that dirty_log_test doesn't require running multiple iterations to verify dirty pages, and actually runs the requested number of iterations, drop the requirement that the test run at least "3" (which was really "2" at the time the test was written) iterations. Link: https://lore.kernel.org/r/20250111003004.1235645-21-seanjc@google.com Signed-off-by: Sean Christopherson --- tools/testing/selftests/kvm/dirty_log_test.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c index 79a7ee189f28..23593d9eeba9 100644 --- a/tools/testing/selftests/kvm/dirty_log_test.c +++ b/tools/testing/selftests/kvm/dirty_log_test.c @@ -886,7 +886,7 @@ int main(int argc, char *argv[]) } } - TEST_ASSERT(p.iterations > 2, "Iterations must be greater than two"); + TEST_ASSERT(p.iterations > 0, "Iterations must be greater than zero"); TEST_ASSERT(p.interval > 0, "Interval must be greater than zero"); pr_info("Test iterations: %"PRIu64", interval: %"PRIu64" (ms)\n", From fd546aba1967bb05ddb569272f56abb75f604bd7 Mon Sep 17 00:00:00 2001 From: Sean Christopherson Date: Fri, 10 Jan 2025 16:50:41 -0800 Subject: [PATCH 22/31] KVM: selftests: Fix mostly theoretical leak of VM's binary stats FD When allocating and freeing a VM's cached binary stats info, check for a NULL descriptor, not a '0' file descriptor, as '0' is a legal FD. E.g. in the unlikely scenario the kernel installs the stats FD at entry '0', selftests would reallocate on the next __vm_get_stat() and/or fail to free the stats in kvm_vm_free(). Fixes: 83f6e109f562 ("KVM: selftests: Cache binary stats metadata for duration of test") Link: https://lore.kernel.org/r/20250111005049.1247555-2-seanjc@google.com Signed-off-by: Sean Christopherson --- tools/testing/selftests/kvm/lib/kvm_util.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c index 33fefeb3ca44..91d295ef5d02 100644 --- a/tools/testing/selftests/kvm/lib/kvm_util.c +++ b/tools/testing/selftests/kvm/lib/kvm_util.c @@ -749,7 +749,7 @@ void kvm_vm_free(struct kvm_vm *vmp) return; /* Free cached stats metadata and close FD */ - if (vmp->stats_fd) { + if (vmp->stats_desc) { free(vmp->stats_desc); close(vmp->stats_fd); } @@ -2218,7 +2218,7 @@ void __vm_get_stat(struct kvm_vm *vm, const char *stat_name, uint64_t *data, size_t size_desc; int i; - if (!vm->stats_fd) { + if (!vm->stats_desc) { vm->stats_fd = vm_get_stats_fd(vm); read_stats_header(vm->stats_fd, &vm->stats_header); vm->stats_desc = read_stats_descriptors(vm->stats_fd, From f7f232a01f3d26a5bd67a4309ea54640f625dbe5 Mon Sep 17 00:00:00 2001 From: Sean Christopherson Date: Fri, 10 Jan 2025 16:50:42 -0800 Subject: [PATCH 23/31] KVM: selftests: Close VM's binary stats FD when releasing VM Close/free a VM's binary stats cache when the VM is released, not when the VM is fully freed. When a VM is re-created, e.g. for state save/restore tests, the stats FD and descriptor points at the old, defunct VM. The FD is still valid, in that the underlying stats file won't be freed until the FD is closed, but reading stats will always pull information from the old VM. Note, this is a benign bug in the current code base as none of the tests that recreate VMs use binary stats. Fixes: 83f6e109f562 ("KVM: selftests: Cache binary stats metadata for duration of test") Link: https://lore.kernel.org/r/20250111005049.1247555-3-seanjc@google.com Signed-off-by: Sean Christopherson --- tools/testing/selftests/kvm/lib/kvm_util.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c index 91d295ef5d02..9138801ecb60 100644 --- a/tools/testing/selftests/kvm/lib/kvm_util.c +++ b/tools/testing/selftests/kvm/lib/kvm_util.c @@ -709,6 +709,15 @@ void kvm_vm_release(struct kvm_vm *vmp) ret = close(vmp->kvm_fd); TEST_ASSERT(!ret, __KVM_SYSCALL_ERROR("close()", ret)); + + /* Free cached stats metadata and close FD */ + if (vmp->stats_desc) { + free(vmp->stats_desc); + vmp->stats_desc = NULL; + + ret = close(vmp->stats_fd); + TEST_ASSERT(!ret, __KVM_SYSCALL_ERROR("close()", ret)); + } } static void __vm_mem_region_delete(struct kvm_vm *vm, @@ -748,12 +757,6 @@ void kvm_vm_free(struct kvm_vm *vmp) if (vmp == NULL) return; - /* Free cached stats metadata and close FD */ - if (vmp->stats_desc) { - free(vmp->stats_desc); - close(vmp->stats_fd); - } - /* Free userspace_mem_regions. */ hash_for_each_safe(vmp->regions.slot_hash, ctr, node, region, slot_node) __vm_mem_region_delete(vmp, region); From eead13d493af0c2d3b8025da4acb2a9ef854a26a Mon Sep 17 00:00:00 2001 From: Sean Christopherson Date: Fri, 10 Jan 2025 16:50:43 -0800 Subject: [PATCH 24/31] KVM: selftests: Assert that __vm_get_stat() actually finds a stat Fail the test if it attempts to read a stat that doesn't exist, e.g. due to a typo (hooray, strings), or because the test tried to get a stat for the wrong scope. As is, there's no indiciation of failure and @data is left untouched, e.g. holds '0' or random stack data in most cases. Fixes: 8448ec5993be ("KVM: selftests: Add NX huge pages test") Link: https://lore.kernel.org/r/20250111005049.1247555-4-seanjc@google.com [sean: fixup spelling mistake, courtesy of Colin Ian King] Signed-off-by: Sean Christopherson --- tools/testing/selftests/kvm/lib/kvm_util.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c index 9138801ecb60..e3cb3ee74491 100644 --- a/tools/testing/selftests/kvm/lib/kvm_util.c +++ b/tools/testing/selftests/kvm/lib/kvm_util.c @@ -2238,9 +2238,10 @@ void __vm_get_stat(struct kvm_vm *vm, const char *stat_name, uint64_t *data, read_stat_data(vm->stats_fd, &vm->stats_header, desc, data, max_elements); - - break; + return; } + + TEST_FAIL("Unable to find stat '%s'", stat_name); } __weak void kvm_arch_vm_post_create(struct kvm_vm *vm) From b0c3f5df92913de6b9936cb0707cf1c8cdad66a2 Mon Sep 17 00:00:00 2001 From: Sean Christopherson Date: Fri, 10 Jan 2025 16:50:44 -0800 Subject: [PATCH 25/31] KVM: selftests: Macrofy vm_get_stat() to auto-generate stat name string Turn vm_get_stat() into a macro that generates a string for the stat name, as opposed to taking a string. This will allow hardening stat usage in the future to generate errors on unknown stats at compile time. No functional change intended. Link: https://lore.kernel.org/r/20250111005049.1247555-5-seanjc@google.com Signed-off-by: Sean Christopherson --- tools/testing/selftests/kvm/include/kvm_util.h | 14 +++++++------- .../kvm/x86/dirty_log_page_splitting_test.c | 6 +++--- .../testing/selftests/kvm/x86/nx_huge_pages_test.c | 4 ++-- 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h index 4c4e5a847f67..044c2231431e 100644 --- a/tools/testing/selftests/kvm/include/kvm_util.h +++ b/tools/testing/selftests/kvm/include/kvm_util.h @@ -534,13 +534,13 @@ void read_stat_data(int stats_fd, struct kvm_stats_header *header, void __vm_get_stat(struct kvm_vm *vm, const char *stat_name, uint64_t *data, size_t max_elements); -static inline uint64_t vm_get_stat(struct kvm_vm *vm, const char *stat_name) -{ - uint64_t data; - - __vm_get_stat(vm, stat_name, &data, 1); - return data; -} +#define vm_get_stat(vm, stat) \ +({ \ + uint64_t data; \ + \ + __vm_get_stat(vm, #stat, &data, 1); \ + data; \ +}) void vm_create_irqchip(struct kvm_vm *vm); diff --git a/tools/testing/selftests/kvm/x86/dirty_log_page_splitting_test.c b/tools/testing/selftests/kvm/x86/dirty_log_page_splitting_test.c index 2929c067c207..b0d2b04a7ff2 100644 --- a/tools/testing/selftests/kvm/x86/dirty_log_page_splitting_test.c +++ b/tools/testing/selftests/kvm/x86/dirty_log_page_splitting_test.c @@ -41,9 +41,9 @@ struct kvm_page_stats { static void get_page_stats(struct kvm_vm *vm, struct kvm_page_stats *stats, const char *stage) { - stats->pages_4k = vm_get_stat(vm, "pages_4k"); - stats->pages_2m = vm_get_stat(vm, "pages_2m"); - stats->pages_1g = vm_get_stat(vm, "pages_1g"); + stats->pages_4k = vm_get_stat(vm, pages_4k); + stats->pages_2m = vm_get_stat(vm, pages_2m); + stats->pages_1g = vm_get_stat(vm, pages_1g); stats->hugepages = stats->pages_2m + stats->pages_1g; pr_debug("\nPage stats after %s: 4K: %ld 2M: %ld 1G: %ld huge: %ld\n", diff --git a/tools/testing/selftests/kvm/x86/nx_huge_pages_test.c b/tools/testing/selftests/kvm/x86/nx_huge_pages_test.c index e7efb2b35f8b..c0d84827f736 100644 --- a/tools/testing/selftests/kvm/x86/nx_huge_pages_test.c +++ b/tools/testing/selftests/kvm/x86/nx_huge_pages_test.c @@ -73,7 +73,7 @@ static void check_2m_page_count(struct kvm_vm *vm, int expected_pages_2m) { int actual_pages_2m; - actual_pages_2m = vm_get_stat(vm, "pages_2m"); + actual_pages_2m = vm_get_stat(vm, pages_2m); TEST_ASSERT(actual_pages_2m == expected_pages_2m, "Unexpected 2m page count. Expected %d, got %d", @@ -84,7 +84,7 @@ static void check_split_count(struct kvm_vm *vm, int expected_splits) { int actual_splits; - actual_splits = vm_get_stat(vm, "nx_lpage_splits"); + actual_splits = vm_get_stat(vm, nx_lpage_splits); TEST_ASSERT(actual_splits == expected_splits, "Unexpected NX huge page split count. Expected %d, got %d", From e65faf71bd54ac957aed3c2e81a964fea63f3e82 Mon Sep 17 00:00:00 2001 From: Sean Christopherson Date: Fri, 10 Jan 2025 16:50:45 -0800 Subject: [PATCH 26/31] KVM: selftests: Add struct and helpers to wrap binary stats cache Add a struct and helpers to manage the binary stats cache, which is currently used only for VM-scoped stats. This will allow expanding the selftests infrastructure to provide support for vCPU-scoped binary stats, which, except for the ioctl to get the stats FD are identical to VM-scoped stats. Defer converting __vm_get_stat() to a scope-agnostic helper to a future patch, as getting the stats FD from KVM needs to be moved elsewhere before it can be made completely scope-agnostic. Link: https://lore.kernel.org/r/20250111005049.1247555-6-seanjc@google.com Signed-off-by: Sean Christopherson --- .../testing/selftests/kvm/include/kvm_util.h | 11 +++-- tools/testing/selftests/kvm/lib/kvm_util.c | 47 +++++++++++-------- 2 files changed, 34 insertions(+), 24 deletions(-) diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h index 044c2231431e..9a64bab42f89 100644 --- a/tools/testing/selftests/kvm/include/kvm_util.h +++ b/tools/testing/selftests/kvm/include/kvm_util.h @@ -46,6 +46,12 @@ struct userspace_mem_region { struct hlist_node slot_node; }; +struct kvm_binary_stats { + int fd; + struct kvm_stats_header header; + struct kvm_stats_desc *desc; +}; + struct kvm_vcpu { struct list_head list; uint32_t id; @@ -99,10 +105,7 @@ struct kvm_vm { struct kvm_vm_arch arch; - /* Cache of information for binary stats interface */ - int stats_fd; - struct kvm_stats_header stats_header; - struct kvm_stats_desc *stats_desc; + struct kvm_binary_stats stats; /* * KVM region slots. These are the default memslots used by page diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c index e3cb3ee74491..6729268c8160 100644 --- a/tools/testing/selftests/kvm/lib/kvm_util.c +++ b/tools/testing/selftests/kvm/lib/kvm_util.c @@ -657,6 +657,20 @@ userspace_mem_region_find(struct kvm_vm *vm, uint64_t start, uint64_t end) return NULL; } +static void kvm_stats_release(struct kvm_binary_stats *stats) +{ + int ret; + + if (!stats->desc) + return; + + free(stats->desc); + stats->desc = NULL; + + ret = close(stats->fd); + TEST_ASSERT(!ret, __KVM_SYSCALL_ERROR("close()", ret)); +} + __weak void vcpu_arch_free(struct kvm_vcpu *vcpu) { @@ -711,13 +725,7 @@ void kvm_vm_release(struct kvm_vm *vmp) TEST_ASSERT(!ret, __KVM_SYSCALL_ERROR("close()", ret)); /* Free cached stats metadata and close FD */ - if (vmp->stats_desc) { - free(vmp->stats_desc); - vmp->stats_desc = NULL; - - ret = close(vmp->stats_fd); - TEST_ASSERT(!ret, __KVM_SYSCALL_ERROR("close()", ret)); - } + kvm_stats_release(&vmp->stats); } static void __vm_mem_region_delete(struct kvm_vm *vm, @@ -2214,34 +2222,33 @@ void read_stat_data(int stats_fd, struct kvm_stats_header *header, * * Read the data values of a specified stat from the binary stats interface. */ -void __vm_get_stat(struct kvm_vm *vm, const char *stat_name, uint64_t *data, +void __vm_get_stat(struct kvm_vm *vm, const char *name, uint64_t *data, size_t max_elements) { + struct kvm_binary_stats *stats = &vm->stats; struct kvm_stats_desc *desc; size_t size_desc; int i; - if (!vm->stats_desc) { - vm->stats_fd = vm_get_stats_fd(vm); - read_stats_header(vm->stats_fd, &vm->stats_header); - vm->stats_desc = read_stats_descriptors(vm->stats_fd, - &vm->stats_header); + if (!stats->desc) { + stats->fd = vm_get_stats_fd(vm); + read_stats_header(stats->fd, &stats->header); + stats->desc = read_stats_descriptors(stats->fd, &stats->header); } - size_desc = get_stats_descriptor_size(&vm->stats_header); + size_desc = get_stats_descriptor_size(&stats->header); - for (i = 0; i < vm->stats_header.num_desc; ++i) { - desc = (void *)vm->stats_desc + (i * size_desc); + for (i = 0; i < stats->header.num_desc; ++i) { + desc = (void *)stats->desc + (i * size_desc); - if (strcmp(desc->name, stat_name)) + if (strcmp(desc->name, name)) continue; - read_stat_data(vm->stats_fd, &vm->stats_header, desc, - data, max_elements); + read_stat_data(stats->fd, &stats->header, desc, data, max_elements); return; } - TEST_FAIL("Unable to find stat '%s'", stat_name); + TEST_FAIL("Unable to find stat '%s'", name); } __weak void kvm_arch_vm_post_create(struct kvm_vm *vm) From ea7179f99514f8119c6327d7c3b84a439a273533 Mon Sep 17 00:00:00 2001 From: Sean Christopherson Date: Fri, 10 Jan 2025 16:50:46 -0800 Subject: [PATCH 27/31] KVM: selftests: Get VM's binary stats FD when opening VM Get and cache a VM's binary stats FD when the VM is opened, as opposed to waiting until the stats are first used. Opening the stats FD outside of __vm_get_stat() will allow converting it to a scope-agnostic helper. Note, this doesn't interfere with kvm_binary_stats_test's testcase that verifies a stats FD can be used after its own VM's FD is closed, as the cached FD is also closed during kvm_vm_free(). Link: https://lore.kernel.org/r/20250111005049.1247555-7-seanjc@google.com Signed-off-by: Sean Christopherson --- tools/testing/selftests/kvm/lib/kvm_util.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c index 6729268c8160..8cacb7fd1ad9 100644 --- a/tools/testing/selftests/kvm/lib/kvm_util.c +++ b/tools/testing/selftests/kvm/lib/kvm_util.c @@ -196,6 +196,11 @@ static void vm_open(struct kvm_vm *vm) vm->fd = __kvm_ioctl(vm->kvm_fd, KVM_CREATE_VM, (void *)vm->type); TEST_ASSERT(vm->fd >= 0, KVM_IOCTL_ERROR(KVM_CREATE_VM, vm->fd)); + + if (kvm_has_cap(KVM_CAP_BINARY_STATS_FD)) + vm->stats.fd = vm_get_stats_fd(vm); + else + vm->stats.fd = -1; } const char *vm_guest_mode_string(uint32_t i) @@ -661,14 +666,17 @@ static void kvm_stats_release(struct kvm_binary_stats *stats) { int ret; - if (!stats->desc) + if (stats->fd < 0) return; - free(stats->desc); - stats->desc = NULL; + if (stats->desc) { + free(stats->desc); + stats->desc = NULL; + } ret = close(stats->fd); TEST_ASSERT(!ret, __KVM_SYSCALL_ERROR("close()", ret)); + stats->fd = -1; } __weak void vcpu_arch_free(struct kvm_vcpu *vcpu) @@ -2231,7 +2239,6 @@ void __vm_get_stat(struct kvm_vm *vm, const char *name, uint64_t *data, int i; if (!stats->desc) { - stats->fd = vm_get_stats_fd(vm); read_stats_header(stats->fd, &stats->header); stats->desc = read_stats_descriptors(stats->fd, &stats->header); } From 9b56532b8a593c3a3cbbd8b17fb87ffb4beee436 Mon Sep 17 00:00:00 2001 From: Sean Christopherson Date: Fri, 10 Jan 2025 16:50:47 -0800 Subject: [PATCH 28/31] KVM: selftests: Adjust number of files rlimit for all "standard" VMs Move the max vCPUs test's RLIMIT_NOFILE adjustments to common code, and use the new helper to adjust the resource limit for non-barebones VMs by default. x86's recalc_apic_map_test creates 512 vCPUs, and a future change will open the binary stats fd for all vCPUs, which will put the recalc APIC test above some distros' default limit of 1024. Link: https://lore.kernel.org/r/20250111005049.1247555-8-seanjc@google.com Signed-off-by: Sean Christopherson --- .../testing/selftests/kvm/include/kvm_util.h | 2 ++ .../selftests/kvm/kvm_create_max_vcpus.c | 28 +-------------- tools/testing/selftests/kvm/lib/kvm_util.c | 34 +++++++++++++++++++ 3 files changed, 37 insertions(+), 27 deletions(-) diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h index 9a64bab42f89..d4670b5962ab 100644 --- a/tools/testing/selftests/kvm/include/kvm_util.h +++ b/tools/testing/selftests/kvm/include/kvm_util.h @@ -966,6 +966,8 @@ static inline struct kvm_vm *vm_create_shape_with_one_vcpu(struct vm_shape shape struct kvm_vcpu *vm_recreate_with_one_vcpu(struct kvm_vm *vm); +void kvm_set_files_rlimit(uint32_t nr_vcpus); + void kvm_pin_this_task_to_pcpu(uint32_t pcpu); void kvm_print_vcpu_pinning_help(void); void kvm_parse_vcpu_pinning(const char *pcpus_string, uint32_t vcpu_to_pcpu[], diff --git a/tools/testing/selftests/kvm/kvm_create_max_vcpus.c b/tools/testing/selftests/kvm/kvm_create_max_vcpus.c index c78f34699f73..c5310736ed06 100644 --- a/tools/testing/selftests/kvm/kvm_create_max_vcpus.c +++ b/tools/testing/selftests/kvm/kvm_create_max_vcpus.c @@ -10,7 +10,6 @@ #include #include #include -#include #include "test_util.h" @@ -39,36 +38,11 @@ int main(int argc, char *argv[]) { int kvm_max_vcpu_id = kvm_check_cap(KVM_CAP_MAX_VCPU_ID); int kvm_max_vcpus = kvm_check_cap(KVM_CAP_MAX_VCPUS); - /* - * Number of file descriptors reqired, KVM_CAP_MAX_VCPUS for vCPU fds + - * an arbitrary number for everything else. - */ - int nr_fds_wanted = kvm_max_vcpus + 100; - struct rlimit rl; pr_info("KVM_CAP_MAX_VCPU_ID: %d\n", kvm_max_vcpu_id); pr_info("KVM_CAP_MAX_VCPUS: %d\n", kvm_max_vcpus); - /* - * Check that we're allowed to open nr_fds_wanted file descriptors and - * try raising the limits if needed. - */ - TEST_ASSERT(!getrlimit(RLIMIT_NOFILE, &rl), "getrlimit() failed!"); - - if (rl.rlim_cur < nr_fds_wanted) { - rl.rlim_cur = nr_fds_wanted; - if (rl.rlim_max < nr_fds_wanted) { - int old_rlim_max = rl.rlim_max; - rl.rlim_max = nr_fds_wanted; - - int r = setrlimit(RLIMIT_NOFILE, &rl); - __TEST_REQUIRE(r >= 0, - "RLIMIT_NOFILE hard limit is too low (%d, wanted %d)", - old_rlim_max, nr_fds_wanted); - } else { - TEST_ASSERT(!setrlimit(RLIMIT_NOFILE, &rl), "setrlimit() failed!"); - } - } + kvm_set_files_rlimit(kvm_max_vcpus); /* * Upstream KVM prior to 4.8 does not support KVM_CAP_MAX_VCPU_ID. diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c index 8cacb7fd1ad9..faeeb576be90 100644 --- a/tools/testing/selftests/kvm/lib/kvm_util.c +++ b/tools/testing/selftests/kvm/lib/kvm_util.c @@ -12,6 +12,7 @@ #include #include #include +#include #include #include #include @@ -411,6 +412,37 @@ static uint64_t vm_nr_pages_required(enum vm_guest_mode mode, return vm_adjust_num_guest_pages(mode, nr_pages); } +void kvm_set_files_rlimit(uint32_t nr_vcpus) +{ + /* + * Number of file descriptors required, nr_vpucs vCPU fds + an arbitrary + * number for everything else. + */ + int nr_fds_wanted = nr_vcpus + 100; + struct rlimit rl; + + /* + * Check that we're allowed to open nr_fds_wanted file descriptors and + * try raising the limits if needed. + */ + TEST_ASSERT(!getrlimit(RLIMIT_NOFILE, &rl), "getrlimit() failed!"); + + if (rl.rlim_cur < nr_fds_wanted) { + rl.rlim_cur = nr_fds_wanted; + if (rl.rlim_max < nr_fds_wanted) { + int old_rlim_max = rl.rlim_max; + + rl.rlim_max = nr_fds_wanted; + __TEST_REQUIRE(setrlimit(RLIMIT_NOFILE, &rl) >= 0, + "RLIMIT_NOFILE hard limit is too low (%d, wanted %d)", + old_rlim_max, nr_fds_wanted); + } else { + TEST_ASSERT(!setrlimit(RLIMIT_NOFILE, &rl), "setrlimit() failed!"); + } + } + +} + struct kvm_vm *__vm_create(struct vm_shape shape, uint32_t nr_runnable_vcpus, uint64_t nr_extra_pages) { @@ -420,6 +452,8 @@ struct kvm_vm *__vm_create(struct vm_shape shape, uint32_t nr_runnable_vcpus, struct kvm_vm *vm; int i; + kvm_set_files_rlimit(nr_runnable_vcpus); + pr_debug("%s: mode='%s' type='%d', pages='%ld'\n", __func__, vm_guest_mode_string(shape.mode), shape.type, nr_pages); From 16fc7cb406a5108182852229abe6804ecbad8d06 Mon Sep 17 00:00:00 2001 From: Sean Christopherson Date: Fri, 10 Jan 2025 16:50:48 -0800 Subject: [PATCH 29/31] KVM: selftests: Add infrastructure for getting vCPU binary stats Now that the binary stats cache infrastructure is largely scope agnostic, add support for vCPU-scoped stats. Like VM stats, open and cache the stats FD when the vCPU is created so that it's guaranteed to be valid when vcpu_get_stats() is invoked. Account for the extra per-vCPU file descriptor in kvm_set_files_rlimit(), so that tests that create large VMs don't run afoul of resource limits. To sanity check that the infrastructure actually works, and to get a bit of bonus coverage, add an assert in x86's xapic_ipi_test to verify that the number of HLTs executed by the test matches the number of HLT exits observed by KVM. Tested-by: Manali Shukla Link: https://lore.kernel.org/r/20250111005049.1247555-9-seanjc@google.com Signed-off-by: Sean Christopherson --- .../testing/selftests/kvm/include/kvm_util.h | 20 +++++++----- tools/testing/selftests/kvm/lib/kvm_util.c | 32 ++++++++----------- .../selftests/kvm/x86/xapic_ipi_test.c | 2 ++ 3 files changed, 27 insertions(+), 27 deletions(-) diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h index d4670b5962ab..373912464fb4 100644 --- a/tools/testing/selftests/kvm/include/kvm_util.h +++ b/tools/testing/selftests/kvm/include/kvm_util.h @@ -61,6 +61,7 @@ struct kvm_vcpu { #ifdef __x86_64__ struct kvm_cpuid2 *cpuid; #endif + struct kvm_binary_stats stats; struct kvm_dirty_gfn *dirty_gfns; uint32_t fetch_index; uint32_t dirty_gfns_count; @@ -534,17 +535,20 @@ void read_stat_data(int stats_fd, struct kvm_stats_header *header, struct kvm_stats_desc *desc, uint64_t *data, size_t max_elements); -void __vm_get_stat(struct kvm_vm *vm, const char *stat_name, uint64_t *data, - size_t max_elements); +void kvm_get_stat(struct kvm_binary_stats *stats, const char *name, + uint64_t *data, size_t max_elements); -#define vm_get_stat(vm, stat) \ -({ \ - uint64_t data; \ - \ - __vm_get_stat(vm, #stat, &data, 1); \ - data; \ +#define __get_stat(stats, stat) \ +({ \ + uint64_t data; \ + \ + kvm_get_stat(stats, #stat, &data, 1); \ + data; \ }) +#define vm_get_stat(vm, stat) __get_stat(&(vm)->stats, stat) +#define vcpu_get_stat(vcpu, stat) __get_stat(&(vcpu)->stats, stat) + void vm_create_irqchip(struct kvm_vm *vm); static inline int __vm_create_guest_memfd(struct kvm_vm *vm, uint64_t size, diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c index faeeb576be90..279ad8946040 100644 --- a/tools/testing/selftests/kvm/lib/kvm_util.c +++ b/tools/testing/selftests/kvm/lib/kvm_util.c @@ -415,10 +415,11 @@ static uint64_t vm_nr_pages_required(enum vm_guest_mode mode, void kvm_set_files_rlimit(uint32_t nr_vcpus) { /* - * Number of file descriptors required, nr_vpucs vCPU fds + an arbitrary - * number for everything else. + * Each vCPU will open two file descriptors: the vCPU itself and the + * vCPU's binary stats file descriptor. Add an arbitrary amount of + * buffer for all other files a test may open. */ - int nr_fds_wanted = nr_vcpus + 100; + int nr_fds_wanted = nr_vcpus * 2 + 100; struct rlimit rl; /* @@ -746,6 +747,8 @@ static void vm_vcpu_rm(struct kvm_vm *vm, struct kvm_vcpu *vcpu) ret = close(vcpu->fd); TEST_ASSERT(!ret, __KVM_SYSCALL_ERROR("close()", ret)); + kvm_stats_release(&vcpu->stats); + list_del(&vcpu->list); vcpu_arch_free(vcpu); @@ -1339,6 +1342,11 @@ struct kvm_vcpu *__vm_vcpu_add(struct kvm_vm *vm, uint32_t vcpu_id) TEST_ASSERT(vcpu->run != MAP_FAILED, __KVM_SYSCALL_ERROR("mmap()", (int)(unsigned long)MAP_FAILED)); + if (kvm_has_cap(KVM_CAP_BINARY_STATS_FD)) + vcpu->stats.fd = vcpu_get_stats_fd(vcpu); + else + vcpu->stats.fd = -1; + /* Add to linked-list of VCPUs. */ list_add(&vcpu->list, &vm->vcpus); @@ -2251,23 +2259,9 @@ void read_stat_data(int stats_fd, struct kvm_stats_header *header, desc->name, size, ret); } -/* - * Read the data of the named stat - * - * Input Args: - * vm - the VM for which the stat should be read - * stat_name - the name of the stat to read - * max_elements - the maximum number of 8-byte values to read into data - * - * Output Args: - * data - the buffer into which stat data should be read - * - * Read the data values of a specified stat from the binary stats interface. - */ -void __vm_get_stat(struct kvm_vm *vm, const char *name, uint64_t *data, - size_t max_elements) +void kvm_get_stat(struct kvm_binary_stats *stats, const char *name, + uint64_t *data, size_t max_elements) { - struct kvm_binary_stats *stats = &vm->stats; struct kvm_stats_desc *desc; size_t size_desc; int i; diff --git a/tools/testing/selftests/kvm/x86/xapic_ipi_test.c b/tools/testing/selftests/kvm/x86/xapic_ipi_test.c index a76078a08ff8..574a944763b7 100644 --- a/tools/testing/selftests/kvm/x86/xapic_ipi_test.c +++ b/tools/testing/selftests/kvm/x86/xapic_ipi_test.c @@ -465,6 +465,8 @@ int main(int argc, char *argv[]) cancel_join_vcpu_thread(threads[0], params[0].vcpu); cancel_join_vcpu_thread(threads[1], params[1].vcpu); + TEST_ASSERT_EQ(data->hlt_count, vcpu_get_stat(params[0].vcpu, halt_exits)); + fprintf(stderr, "Test successful after running for %d seconds.\n" "Sending vCPU sent %lu IPIs to halting vCPU\n" From 75418e222e30440ed9aa6d57b6aba474bcd8b4f1 Mon Sep 17 00:00:00 2001 From: Colin Ian King Date: Thu, 27 Feb 2025 22:08:19 +0000 Subject: [PATCH 30/31] KVM: selftests: Fix spelling mistake "UFFDIO_CONINUE" -> "UFFDIO_CONTINUE" There is a spelling mistake in a PER_PAGE_DEBUG debug message. Fix it. Signed-off-by: Colin Ian King Link: https://lore.kernel.org/r/20250227220819.656780-1-colin.i.king@gmail.com Signed-off-by: Sean Christopherson --- tools/testing/selftests/kvm/lib/userfaultfd_util.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/testing/selftests/kvm/lib/userfaultfd_util.c b/tools/testing/selftests/kvm/lib/userfaultfd_util.c index 7c9de8414462..5bde176cedd5 100644 --- a/tools/testing/selftests/kvm/lib/userfaultfd_util.c +++ b/tools/testing/selftests/kvm/lib/userfaultfd_util.c @@ -114,7 +114,7 @@ struct uffd_desc *uffd_setup_demand_paging(int uffd_mode, useconds_t delay, PER_PAGE_DEBUG("Userfaultfd %s mode, faults resolved with %s\n", is_minor ? "MINOR" : "MISSING", - is_minor ? "UFFDIO_CONINUE" : "UFFDIO_COPY"); + is_minor ? "UFFDIO_CONTINUE" : "UFFDIO_COPY"); uffd_desc = malloc(sizeof(struct uffd_desc)); TEST_ASSERT(uffd_desc, "Failed to malloc uffd descriptor"); From 62838fa5eade1b23d546e81e7aab6d4c92ec12f2 Mon Sep 17 00:00:00 2001 From: Sean Christopherson Date: Wed, 26 Feb 2025 15:18:09 -0800 Subject: [PATCH 31/31] KVM: selftests: Relax assertion on HLT exits if CPU supports Idle HLT If the CPU supports Idle HLT, which elides HLT VM-Exits if the vCPU has an unmasked pending IRQ or NMI, relax the xAPIC IPI test's assertion on the number of HLT exits to only require that the number of exits is less than or equal to the number of HLT instructions that were executed. I.e. don't fail the test if Idle HLT does what it's supposed to do. Note, unfortunately there's no way to determine if *KVM* supports Idle HLT, as this_cpu_has() checks raw CPU support, and kvm_cpu_has() checks what can be exposed to L1, i.e. the latter would check if KVM supports nested Idle HLT. But, since the assert is purely bonus coverage, checking for CPU support is good enough. Cc: Manali Shukla Tested-by: Manali Shukla Link: https://lore.kernel.org/r/20250226231809.3183093-1-seanjc@google.com Signed-off-by: Sean Christopherson --- tools/testing/selftests/kvm/include/x86/processor.h | 1 + tools/testing/selftests/kvm/x86/xapic_ipi_test.c | 13 ++++++++++++- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/tools/testing/selftests/kvm/include/x86/processor.h b/tools/testing/selftests/kvm/include/x86/processor.h index 9d365144ac3e..a968a56dcb6e 100644 --- a/tools/testing/selftests/kvm/include/x86/processor.h +++ b/tools/testing/selftests/kvm/include/x86/processor.h @@ -197,6 +197,7 @@ struct kvm_x86_cpu_feature { #define X86_FEATURE_PAUSEFILTER KVM_X86_CPU_FEATURE(0x8000000A, 0, EDX, 10) #define X86_FEATURE_PFTHRESHOLD KVM_X86_CPU_FEATURE(0x8000000A, 0, EDX, 12) #define X86_FEATURE_VGIF KVM_X86_CPU_FEATURE(0x8000000A, 0, EDX, 16) +#define X86_FEATURE_IDLE_HLT KVM_X86_CPU_FEATURE(0x8000000A, 0, EDX, 30) #define X86_FEATURE_SEV KVM_X86_CPU_FEATURE(0x8000001F, 0, EAX, 1) #define X86_FEATURE_SEV_ES KVM_X86_CPU_FEATURE(0x8000001F, 0, EAX, 3) diff --git a/tools/testing/selftests/kvm/x86/xapic_ipi_test.c b/tools/testing/selftests/kvm/x86/xapic_ipi_test.c index 574a944763b7..58d5c132ed8e 100644 --- a/tools/testing/selftests/kvm/x86/xapic_ipi_test.c +++ b/tools/testing/selftests/kvm/x86/xapic_ipi_test.c @@ -465,7 +465,18 @@ int main(int argc, char *argv[]) cancel_join_vcpu_thread(threads[0], params[0].vcpu); cancel_join_vcpu_thread(threads[1], params[1].vcpu); - TEST_ASSERT_EQ(data->hlt_count, vcpu_get_stat(params[0].vcpu, halt_exits)); + /* + * If the host support Idle HLT, i.e. KVM *might* be using Idle HLT, + * then the number of HLT exits may be less than the number of HLTs + * that were executed, as Idle HLT elides the exit if the vCPU has an + * unmasked, pending IRQ (or NMI). + */ + if (this_cpu_has(X86_FEATURE_IDLE_HLT)) + TEST_ASSERT(data->hlt_count >= vcpu_get_stat(params[0].vcpu, halt_exits), + "HLT insns = %lu, HLT exits = %lu", + data->hlt_count, vcpu_get_stat(params[0].vcpu, halt_exits)); + else + TEST_ASSERT_EQ(data->hlt_count, vcpu_get_stat(params[0].vcpu, halt_exits)); fprintf(stderr, "Test successful after running for %d seconds.\n"