linux-kernel - Re: [PATCH 11/20] KVM: selftests: Post to sem_vcpu_stop if and only if vcpu

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <336741c9f992bb340aa075f65578a0d4a68b0193.camel@redhat.com>
Date: Tue, 17 Dec 2024 19:01:32 -0500
From: Maxim Levitsky <mlevitsk@...hat.com>
To: Sean Christopherson <seanjc@...gle.com>, Paolo Bonzini
 <pbonzini@...hat.com>
Cc: kvm@...r.kernel.org, linux-kernel@...r.kernel.org, Peter Xu
	 <peterx@...hat.com>
Subject: Re: [PATCH 11/20] KVM: selftests: Post to sem_vcpu_stop if and only
 if vcpu_stop is true

On Fri, 2024-12-13 at 17:07 -0800, Sean Christopherson wrote:
> 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.

This is probably a good idea to do sometimes, but this can also reduce coverage because
now the vCPU will pointlessly enter and exit when dirty log is full.

Best regards,
	Maxim Levitsky


> 
> 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.
> 
> Signed-off-by: Sean Christopherson <seanjc@...gle.com>
> ---
>  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);
>  	}
>