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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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 



[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux