On Fri, 2024-12-13 at 17:07 -0800, Sean Christopherson wrote:
> 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.
>
> Reported-by: Maxim Levitsky <mlevitsk@xxxxxxxxxx>
> Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> ---
> tools/testing/selftests/kvm/dirty_log_test.c | 30 ++++++++++++++++++--
> 1 file changed, 28 insertions(+), 2 deletions(-)
>
> diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c
> index 500257b712e3..8eb51597f762 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);
> @@ -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
> @@ -760,6 +783,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);
This makes sense.
Reviewed-by: Maxim Levitsky <mlevitsk@xxxxxxxxxx>
Best regards,
Maxim Levitsky
