diff mbox series

[1/3] selftests: kvm/evmcs_test: complete I/O before migrating guest state

Message ID 1555001308-36016-2-git-send-email-pbonzini@redhat.com (mailing list archive)
State New, archived
Headers show
Series kvm: selftests fixes and a new smm_test | expand

Commit Message

Paolo Bonzini April 11, 2019, 4:48 p.m. UTC 
Starting state migration after an IO exit without first completing IO
may result in test failures.  We already have two tests that need this
(this patch in fact fixes evmcs_test, similar to what was fixed for
state_test in commit 0f73bbc851ed, "KVM: selftests: complete IO before
migrating guest state", 2019-03-13) and a third is coming.  So, move the
code to vcpu_save_state, and while at it do not access register state
until after I/O is complete.
---
 tools/testing/selftests/kvm/lib/kvm_util.c         |  5 +++++
 tools/testing/selftests/kvm/lib/x86_64/processor.c |  8 ++++++++
 tools/testing/selftests/kvm/x86_64/evmcs_test.c    |  5 +++--
 tools/testing/selftests/kvm/x86_64/state_test.c    | 15 +--------------
 4 files changed, 17 insertions(+), 16 deletions(-)

Comments

Andrew Jones April 16, 2019, 1:39 p.m. UTC | #1 
On Thu, Apr 11, 2019 at 06:48:26PM +0200, Paolo Bonzini wrote:
> Starting state migration after an IO exit without first completing IO
> may result in test failures.  We already have two tests that need this
> (this patch in fact fixes evmcs_test, similar to what was fixed for
> state_test in commit 0f73bbc851ed, "KVM: selftests: complete IO before
> migrating guest state", 2019-03-13) and a third is coming.  So, move the
> code to vcpu_save_state, and while at it do not access register state
> until after I/O is complete.
> ---
>  tools/testing/selftests/kvm/lib/kvm_util.c         |  5 +++++
>  tools/testing/selftests/kvm/lib/x86_64/processor.c |  8 ++++++++
>  tools/testing/selftests/kvm/x86_64/evmcs_test.c    |  5 +++--
>  tools/testing/selftests/kvm/x86_64/state_test.c    | 15 +--------------
>  4 files changed, 17 insertions(+), 16 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
> index efa0aad8b3c6..4ca96b228e46 100644
> --- a/tools/testing/selftests/kvm/lib/kvm_util.c
> +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
> @@ -91,6 +91,11 @@ static void vm_open(struct kvm_vm *vm, int perm, unsigned long type)
>  	if (vm->kvm_fd < 0)
>  		exit(KSFT_SKIP);
>  
> +	if (!kvm_check_cap(KVM_CAP_IMMEDIATE_EXIT)) {
> +		fprintf(stderr, "immediate_exit not available, skipping test\n");
> +		exit(KSFT_SKIP);
> +	}
> +
>  	vm->fd = ioctl(vm->kvm_fd, KVM_CREATE_VM, type);
>  	TEST_ASSERT(vm->fd >= 0, "KVM_CREATE_VM ioctl failed, "
>  		"rc: %i errno: %i", vm->fd, errno);
> diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c
> index f28127f4a3af..b363c9611bd6 100644
> --- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
> +++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
> @@ -1030,6 +1030,14 @@ struct kvm_x86_state *vcpu_save_state(struct kvm_vm *vm, uint32_t vcpuid)
>  			    nested_size, sizeof(state->nested_));
>  	}
>  
> +	/*
> +	 * When KVM exits to userspace with KVM_EXIT_IO, KVM guarantees
> +	 * guest state is consistent only after userspace re-enters the
> +	 * kernel with KVM_RUN.  Complete IO prior to migrating state
> +	 * to a new VM.
> +	 */
> +	vcpu_run_complete_io(vm, vcpuid);
> +

Since the need for IO completion also affects MMIO exits and there's no
reason to believe that vcpu_save_state() will always be called after
an IO exit, then shouldn't we instead put this in get_ucall()?

diff --git a/tools/testing/selftests/kvm/lib/ucall.c b/tools/testing/selftests/kvm/lib/ucall.c
index a2ab38be2f47..e8c6f2741ce7 100644
--- a/tools/testing/selftests/kvm/lib/ucall.c
+++ b/tools/testing/selftests/kvm/lib/ucall.c
@@ -132,6 +132,7 @@ uint64_t get_ucall(struct kvm_vm *vm, uint32_t vcpu_id, struct ucall *uc)
        if (ucall_type == UCALL_PIO && run->exit_reason == KVM_EXIT_IO &&
            run->io.port == UCALL_PIO_PORT) {
                struct kvm_regs regs;
+               vcpu_run_complete_io(vm, vcpu_id);
                vcpu_regs_get(vm, vcpu_id, &regs);
                memcpy(uc, addr_gva2hva(vm, (vm_vaddr_t)regs.rdi), sizeof(*uc));
                return uc->cmd;
@@ -144,6 +145,7 @@ uint64_t get_ucall(struct kvm_vm *vm, uint32_t vcpu_id, struct ucall *uc)
                            "Unexpected ucall exit mmio address access");
                gva = *(vm_vaddr_t *)run->mmio.data;
                memcpy(uc, addr_gva2hva(vm, gva), sizeof(*uc));
+               vcpu_run_complete_io(vm, vcpu_id);
        }
 
        return uc->cmd;



>  	nmsrs = kvm_get_num_msrs(vm);
>  	list = malloc(sizeof(*list) + nmsrs * sizeof(list->indices[0]));
>  	list->nmsrs = nmsrs;
> diff --git a/tools/testing/selftests/kvm/x86_64/evmcs_test.c b/tools/testing/selftests/kvm/x86_64/evmcs_test.c
> index c49c2a28b0eb..36669684eca5 100644
> --- a/tools/testing/selftests/kvm/x86_64/evmcs_test.c
> +++ b/tools/testing/selftests/kvm/x86_64/evmcs_test.c
> @@ -123,8 +123,6 @@ int main(int argc, char *argv[])
>  			    stage, run->exit_reason,
>  			    exit_reason_str(run->exit_reason));
>  
> -		memset(&regs1, 0, sizeof(regs1));
> -		vcpu_regs_get(vm, VCPU_ID, &regs1);
>  		switch (get_ucall(vm, VCPU_ID, &uc)) {
>  		case UCALL_ABORT:
>  			TEST_ASSERT(false, "%s at %s:%d", (const char *)uc.args[0],
> @@ -144,6 +142,9 @@ int main(int argc, char *argv[])
>  			    stage, (ulong)uc.args[1]);
>  
>  		state = vcpu_save_state(vm, VCPU_ID);
> +		memset(&regs1, 0, sizeof(regs1));
> +		vcpu_regs_get(vm, VCPU_ID, &regs1);
> +

We would still need this change to get the vcpu_regs_get() below the
get_ucall().

>  		kvm_vm_release(vm);
>  
>  		/* Restore state in a new VM.  */
> diff --git a/tools/testing/selftests/kvm/x86_64/state_test.c b/tools/testing/selftests/kvm/x86_64/state_test.c
> index 30f75856cf39..e0a3c0204b7c 100644
> --- a/tools/testing/selftests/kvm/x86_64/state_test.c
> +++ b/tools/testing/selftests/kvm/x86_64/state_test.c
> @@ -134,11 +134,6 @@ int main(int argc, char *argv[])
>  
>  	struct kvm_cpuid_entry2 *entry = kvm_get_supported_cpuid_entry(1);
>  
> -	if (!kvm_check_cap(KVM_CAP_IMMEDIATE_EXIT)) {
> -		fprintf(stderr, "immediate_exit not available, skipping test\n");
> -		exit(KSFT_SKIP);
> -	}
> -
>  	/* Create VM */
>  	vm = vm_create_default(VCPU_ID, 0, guest_code);
>  	vcpu_set_cpuid(vm, VCPU_ID, kvm_get_supported_cpuid());
> @@ -179,18 +174,10 @@ int main(int argc, char *argv[])
>  			    uc.args[1] == stage, "Unexpected register values vmexit #%lx, got %lx",
>  			    stage, (ulong)uc.args[1]);
>  
> -		/*
> -		 * When KVM exits to userspace with KVM_EXIT_IO, KVM guarantees
> -		 * guest state is consistent only after userspace re-enters the
> -		 * kernel with KVM_RUN.  Complete IO prior to migrating state
> -		 * to a new VM.
> -		 */
> -		vcpu_run_complete_io(vm, VCPU_ID);
> -
> +		state = vcpu_save_state(vm, VCPU_ID);
>  		memset(&regs1, 0, sizeof(regs1));
>  		vcpu_regs_get(vm, VCPU_ID, &regs1);
>  
> -		state = vcpu_save_state(vm, VCPU_ID);
>  		kvm_vm_release(vm);
>  
>  		/* Restore state in a new VM.  */
> -- 
> 1.8.3.1
> 
>

Thanks,
drew
diff mbox series

Patch

diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
index efa0aad8b3c6..4ca96b228e46 100644
--- a/tools/testing/selftests/kvm/lib/kvm_util.c
+++ b/tools/testing/selftests/kvm/lib/kvm_util.c
@@ -91,6 +91,11 @@  static void vm_open(struct kvm_vm *vm, int perm, unsigned long type)
 	if (vm->kvm_fd < 0)
 		exit(KSFT_SKIP);
 
+	if (!kvm_check_cap(KVM_CAP_IMMEDIATE_EXIT)) {
+		fprintf(stderr, "immediate_exit not available, skipping test\n");
+		exit(KSFT_SKIP);
+	}
+
 	vm->fd = ioctl(vm->kvm_fd, KVM_CREATE_VM, type);
 	TEST_ASSERT(vm->fd >= 0, "KVM_CREATE_VM ioctl failed, "
 		"rc: %i errno: %i", vm->fd, errno);
diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c
index f28127f4a3af..b363c9611bd6 100644
--- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
+++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
@@ -1030,6 +1030,14 @@  struct kvm_x86_state *vcpu_save_state(struct kvm_vm *vm, uint32_t vcpuid)
 			    nested_size, sizeof(state->nested_));
 	}
 
+	/*
+	 * When KVM exits to userspace with KVM_EXIT_IO, KVM guarantees
+	 * guest state is consistent only after userspace re-enters the
+	 * kernel with KVM_RUN.  Complete IO prior to migrating state
+	 * to a new VM.
+	 */
+	vcpu_run_complete_io(vm, vcpuid);
+
 	nmsrs = kvm_get_num_msrs(vm);
 	list = malloc(sizeof(*list) + nmsrs * sizeof(list->indices[0]));
 	list->nmsrs = nmsrs;
diff --git a/tools/testing/selftests/kvm/x86_64/evmcs_test.c b/tools/testing/selftests/kvm/x86_64/evmcs_test.c
index c49c2a28b0eb..36669684eca5 100644
--- a/tools/testing/selftests/kvm/x86_64/evmcs_test.c
+++ b/tools/testing/selftests/kvm/x86_64/evmcs_test.c
@@ -123,8 +123,6 @@  int main(int argc, char *argv[])
 			    stage, run->exit_reason,
 			    exit_reason_str(run->exit_reason));
 
-		memset(&regs1, 0, sizeof(regs1));
-		vcpu_regs_get(vm, VCPU_ID, &regs1);
 		switch (get_ucall(vm, VCPU_ID, &uc)) {
 		case UCALL_ABORT:
 			TEST_ASSERT(false, "%s at %s:%d", (const char *)uc.args[0],
@@ -144,6 +142,9 @@  int main(int argc, char *argv[])
 			    stage, (ulong)uc.args[1]);
 
 		state = vcpu_save_state(vm, VCPU_ID);
+		memset(&regs1, 0, sizeof(regs1));
+		vcpu_regs_get(vm, VCPU_ID, &regs1);
+
 		kvm_vm_release(vm);
 
 		/* Restore state in a new VM.  */
diff --git a/tools/testing/selftests/kvm/x86_64/state_test.c b/tools/testing/selftests/kvm/x86_64/state_test.c
index 30f75856cf39..e0a3c0204b7c 100644
--- a/tools/testing/selftests/kvm/x86_64/state_test.c
+++ b/tools/testing/selftests/kvm/x86_64/state_test.c
@@ -134,11 +134,6 @@  int main(int argc, char *argv[])
 
 	struct kvm_cpuid_entry2 *entry = kvm_get_supported_cpuid_entry(1);
 
-	if (!kvm_check_cap(KVM_CAP_IMMEDIATE_EXIT)) {
-		fprintf(stderr, "immediate_exit not available, skipping test\n");
-		exit(KSFT_SKIP);
-	}
-
 	/* Create VM */
 	vm = vm_create_default(VCPU_ID, 0, guest_code);
 	vcpu_set_cpuid(vm, VCPU_ID, kvm_get_supported_cpuid());
@@ -179,18 +174,10 @@  int main(int argc, char *argv[])
 			    uc.args[1] == stage, "Unexpected register values vmexit #%lx, got %lx",
 			    stage, (ulong)uc.args[1]);
 
-		/*
-		 * When KVM exits to userspace with KVM_EXIT_IO, KVM guarantees
-		 * guest state is consistent only after userspace re-enters the
-		 * kernel with KVM_RUN.  Complete IO prior to migrating state
-		 * to a new VM.
-		 */
-		vcpu_run_complete_io(vm, VCPU_ID);
-
+		state = vcpu_save_state(vm, VCPU_ID);
 		memset(&regs1, 0, sizeof(regs1));
 		vcpu_regs_get(vm, VCPU_ID, &regs1);
 
-		state = vcpu_save_state(vm, VCPU_ID);
 		kvm_vm_release(vm);
 
 		/* Restore state in a new VM.  */