diff mbox series

[V2,07/11] KVM: selftests: Consolidate boilerplate code in get_ucall()

Message ID 20220801201109.825284-8-pgonda@google.com (mailing list archive)
State New, archived
Headers show
Series KVM: selftests: Add simple SEV test | expand

Commit Message

Peter Gonda Aug. 1, 2022, 8:11 p.m. UTC
From: Sean Christopherson <seanjc@google.com>

Consolidate the actual copying of a ucall struct from guest=>host into
the common get_ucall().  Return a host virtual address instead of a guest
virtual address even though the addr_gva2hva() part could be moved to
get_ucall() too.  Conceptually, get_ucall() is invoked from the host and
should return a host virtual address (and returning NULL for "nothing to
see here" is far superior to returning 0).

Use pointer shenanigans instead of an unnecessary bounce buffer when the
caller of get_ucall() provides a valid pointer.

Signed-off-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Peter Gonda <pgonda@google.com>
---
 .../selftests/kvm/include/ucall_common.h      |  8 ++------
 .../testing/selftests/kvm/lib/aarch64/ucall.c | 13 +++----------
 tools/testing/selftests/kvm/lib/riscv/ucall.c | 19 +++----------------
 tools/testing/selftests/kvm/lib/s390x/ucall.c | 16 +++-------------
 .../testing/selftests/kvm/lib/ucall_common.c  | 19 +++++++++++++++++++
 .../testing/selftests/kvm/lib/x86_64/ucall.c  | 16 +++-------------
 6 files changed, 33 insertions(+), 58 deletions(-)

Comments

Andrew Jones Aug. 2, 2022, 8:37 a.m. UTC | #1
On Mon, Aug 01, 2022 at 01:11:05PM -0700, Peter Gonda wrote:
> From: Sean Christopherson <seanjc@google.com>
> 
> Consolidate the actual copying of a ucall struct from guest=>host into
> the common get_ucall().  Return a host virtual address instead of a guest
> virtual address even though the addr_gva2hva() part could be moved to
> get_ucall() too.  Conceptually, get_ucall() is invoked from the host and
> should return a host virtual address (and returning NULL for "nothing to
> see here" is far superior to returning 0).
> 
> Use pointer shenanigans instead of an unnecessary bounce buffer when the
> caller of get_ucall() provides a valid pointer.
> 
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Peter Gonda <pgonda@google.com>
> ---
>  .../selftests/kvm/include/ucall_common.h      |  8 ++------
>  .../testing/selftests/kvm/lib/aarch64/ucall.c | 13 +++----------
>  tools/testing/selftests/kvm/lib/riscv/ucall.c | 19 +++----------------
>  tools/testing/selftests/kvm/lib/s390x/ucall.c | 16 +++-------------
>  .../testing/selftests/kvm/lib/ucall_common.c  | 19 +++++++++++++++++++
>  .../testing/selftests/kvm/lib/x86_64/ucall.c  | 16 +++-------------
>  6 files changed, 33 insertions(+), 58 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/include/ucall_common.h b/tools/testing/selftests/kvm/include/ucall_common.h
> index 5a85f5318bbe..c1bc8e33ef3f 100644
> --- a/tools/testing/selftests/kvm/include/ucall_common.h
> +++ b/tools/testing/selftests/kvm/include/ucall_common.h
> @@ -27,9 +27,10 @@ struct ucall {
>  void ucall_arch_init(struct kvm_vm *vm, void *arg);
>  void ucall_arch_uninit(struct kvm_vm *vm);
>  void ucall_arch_do_ucall(vm_vaddr_t uc);
> -uint64_t ucall_arch_get_ucall(struct kvm_vcpu *vcpu, struct ucall *uc);
> +uint64_t ucall_arch_get_ucall(struct kvm_vcpu *vcpu);

This should now return a vm_vaddr_t instead of a uint64_t.

>  
>  void ucall(uint64_t cmd, int nargs, ...);
> +uint64_t get_ucall(struct kvm_vcpu *vcpu, struct ucall *uc);
>  
>  static inline void ucall_init(struct kvm_vm *vm, void *arg)
>  {
> @@ -41,11 +42,6 @@ static inline void ucall_uninit(struct kvm_vm *vm)
>  	ucall_arch_uninit(vm);
>  }
>  
> -static inline uint64_t get_ucall(struct kvm_vcpu *vcpu, struct ucall *uc)
> -{
> -	return ucall_arch_get_ucall(vcpu, uc);
> -}
> -
>  #define GUEST_SYNC_ARGS(stage, arg1, arg2, arg3, arg4)	\
>  				ucall(UCALL_SYNC, 6, "hello", stage, arg1, arg2, arg3, arg4)
>  #define GUEST_SYNC(stage)	ucall(UCALL_SYNC, 2, "hello", stage)
> diff --git a/tools/testing/selftests/kvm/lib/aarch64/ucall.c b/tools/testing/selftests/kvm/lib/aarch64/ucall.c
> index 1c81a6a5c1f2..d2f099caa9ab 100644
> --- a/tools/testing/selftests/kvm/lib/aarch64/ucall.c
> +++ b/tools/testing/selftests/kvm/lib/aarch64/ucall.c
> @@ -78,24 +78,17 @@ void ucall_arch_do_ucall(vm_vaddr_t uc)
>  uint64_t ucall_arch_get_ucall(struct kvm_vcpu *vcpu, struct ucall *uc)

Need to drop uc from the parameters.

>  {
>  	struct kvm_run *run = vcpu->run;
> -	struct ucall ucall = {};
> -
> -	if (uc)
> -		memset(uc, 0, sizeof(*uc));
>  
>  	if (run->exit_reason == KVM_EXIT_MMIO &&
>  	    run->mmio.phys_addr == (uint64_t)ucall_exit_mmio_addr) {
> -		vm_vaddr_t gva;
> +		uint64_t ucall_addr;
>  
>  		TEST_ASSERT(run->mmio.is_write && run->mmio.len == 8,
>  			    "Unexpected ucall exit mmio address access");
>  		memcpy(&gva, run->mmio.data, sizeof(gva));

We just dropped the gva variable, so this can't compile. We don't want
to return a uint64_t anyway, though, so we shouldn't replace gva, we
should return it.

> -		memcpy(&ucall, addr_gva2hva(vcpu->vm, gva), sizeof(ucall));
>  
> -		vcpu_run_complete_io(vcpu);
> -		if (uc)
> -			memcpy(uc, &ucall, sizeof(ucall));
> +		return ucall_addr;
>  	}
>  
> -	return ucall.cmd;
> +	return 0;
>  }
> diff --git a/tools/testing/selftests/kvm/lib/riscv/ucall.c b/tools/testing/selftests/kvm/lib/riscv/ucall.c
> index b1598f418c1f..3f000d0b705f 100644
> --- a/tools/testing/selftests/kvm/lib/riscv/ucall.c
> +++ b/tools/testing/selftests/kvm/lib/riscv/ucall.c
> @@ -51,27 +51,15 @@ void ucall_arch_do_ucall(vm_vaddr_t uc)
>  		  uc, 0, 0, 0, 0, 0);
>  }
>  
> -uint64_t ucall_arch_get_ucall(struct kvm_vcpu *vcpu, struct ucall *uc)
> +uint64_t ucall_arch_get_ucall(struct kvm_vcpu *vcpu)
>  {
>  	struct kvm_run *run = vcpu->run;
> -	struct ucall ucall = {};
> -
> -	if (uc)
> -		memset(uc, 0, sizeof(*uc));
>  
>  	if (run->exit_reason == KVM_EXIT_RISCV_SBI &&
>  	    run->riscv_sbi.extension_id == KVM_RISCV_SELFTESTS_SBI_EXT) {
>  		switch (run->riscv_sbi.function_id) {
>  		case KVM_RISCV_SELFTESTS_SBI_UCALL:
> -			memcpy(&ucall,
> -			       addr_gva2hva(vcpu->vm, run->riscv_sbi.args[0]),
> -			       sizeof(ucall));
> -
> -			vcpu_run_complete_io(vcpu);
> -			if (uc)
> -				memcpy(uc, &ucall, sizeof(ucall));
> -
> -			break;
> +			return vcpu->vm, run->riscv_sbi.args[0];

The "vcpu->vm," needs to go away.

>  		case KVM_RISCV_SELFTESTS_SBI_UNEXP:
>  			vcpu_dump(stderr, vcpu, 2);
>  			TEST_ASSERT(0, "Unexpected trap taken by guest");
> @@ -80,6 +68,5 @@ uint64_t ucall_arch_get_ucall(struct kvm_vcpu *vcpu, struct ucall *uc)
>  			break;
>  		}
>  	}
> -
> -	return ucall.cmd;
> +	return 0;
>  }
> diff --git a/tools/testing/selftests/kvm/lib/s390x/ucall.c b/tools/testing/selftests/kvm/lib/s390x/ucall.c
> index 114cb4af295f..f7a5a7eb4aa8 100644
> --- a/tools/testing/selftests/kvm/lib/s390x/ucall.c
> +++ b/tools/testing/selftests/kvm/lib/s390x/ucall.c
> @@ -20,13 +20,9 @@ void ucall_arch_do_ucall(vm_vaddr_t uc)
>  	asm volatile ("diag 0,%0,0x501" : : "a"(uc) : "memory");
>  }
>  
> -uint64_t ucall_arch_get_ucall(struct kvm_vcpu *vcpu, struct ucall *uc)
> +uint64_t ucall_arch_get_ucall(struct kvm_vcpu *vcpu)
>  {
>  	struct kvm_run *run = vcpu->run;
> -	struct ucall ucall = {};
> -
> -	if (uc)
> -		memset(uc, 0, sizeof(*uc));
>  
>  	if (run->exit_reason == KVM_EXIT_S390_SIEIC &&
>  	    run->s390_sieic.icptcode == 4 &&
> @@ -34,13 +30,7 @@ uint64_t ucall_arch_get_ucall(struct kvm_vcpu *vcpu, struct ucall *uc)
>  	    (run->s390_sieic.ipb >> 16) == 0x501) {
>  		int reg = run->s390_sieic.ipa & 0xf;
>  
> -		memcpy(&ucall, addr_gva2hva(vcpu->vm, run->s.regs.gprs[reg]),
> -		       sizeof(ucall));
> -
> -		vcpu_run_complete_io(vcpu);
> -		if (uc)
> -			memcpy(uc, &ucall, sizeof(ucall));
> +		return run->s.regs.gprs[reg];
>  	}
> -
> -	return ucall.cmd;
> +	return 0;
>  }
> diff --git a/tools/testing/selftests/kvm/lib/ucall_common.c b/tools/testing/selftests/kvm/lib/ucall_common.c
> index 749ffdf23855..a060252bab40 100644
> --- a/tools/testing/selftests/kvm/lib/ucall_common.c
> +++ b/tools/testing/selftests/kvm/lib/ucall_common.c
> @@ -18,3 +18,22 @@ void ucall(uint64_t cmd, int nargs, ...)
>  
>  	ucall_arch_do_ucall((vm_vaddr_t)&uc);
>  }
> +
> +uint64_t get_ucall(struct kvm_vcpu *vcpu, struct ucall *uc)
> +{
> +	struct ucall ucall;
> +	void *addr;
> +
> +	if (!uc)
> +		uc = &ucall;
> +
> +	addr = addr_gva2hva(vcpu->vm, ucall_arch_get_ucall(vcpu));
> +	if (addr) {
> +		memcpy(uc, addr, sizeof(*uc));
> +		vcpu_run_complete_io(vcpu);
> +	} else {
> +		memset(uc, 0, sizeof(*uc));
> +	}
> +
> +	return uc->cmd;
> +}
> diff --git a/tools/testing/selftests/kvm/lib/x86_64/ucall.c b/tools/testing/selftests/kvm/lib/x86_64/ucall.c
> index 9f532dba1003..24746120a593 100644
> --- a/tools/testing/selftests/kvm/lib/x86_64/ucall.c
> +++ b/tools/testing/selftests/kvm/lib/x86_64/ucall.c
> @@ -22,25 +22,15 @@ void ucall_arch_do_ucall(vm_vaddr_t uc)
>  		: : [port] "d" (UCALL_PIO_PORT), "D" (uc) : "rax", "memory");
>  }
>  
> -uint64_t ucall_arch_get_ucall(struct kvm_vcpu *vcpu, struct ucall *uc)
> +uint64_t ucall_arch_get_ucall(struct kvm_vcpu *vcpu)
>  {
>  	struct kvm_run *run = vcpu->run;
> -	struct ucall ucall = {};
> -
> -	if (uc)
> -		memset(uc, 0, sizeof(*uc));
>  
>  	if (run->exit_reason == KVM_EXIT_IO && run->io.port == UCALL_PIO_PORT) {
>  		struct kvm_regs regs;
>  
>  		vcpu_regs_get(vcpu, &regs);
> -		memcpy(&ucall, addr_gva2hva(vcpu->vm, (vm_vaddr_t)regs.rdi),
> -		       sizeof(ucall));
> -
> -		vcpu_run_complete_io(vcpu);
> -		if (uc)
> -			memcpy(uc, &ucall, sizeof(ucall));
> +		return regs.rdi;
>  	}
> -
> -	return ucall.cmd;
> +	return 0;
>  }
> -- 
> 2.37.1.455.g008518b4e5-goog
>

Thanks,
drew
Andrew Jones Aug. 2, 2022, 9:54 a.m. UTC | #2
On Mon, Aug 01, 2022 at 01:11:05PM -0700, Peter Gonda wrote:
> From: Sean Christopherson <seanjc@google.com>
> 
> Consolidate the actual copying of a ucall struct from guest=>host into
> the common get_ucall().  Return a host virtual address instead of a guest
> virtual address even though the addr_gva2hva() part could be moved to
> get_ucall() too.  Conceptually, get_ucall() is invoked from the host and
> should return a host virtual address (and returning NULL for "nothing to
> see here" is far superior to returning 0).

The code does not match this description anymore, now that
ucall_arch_get_ucall() returns a gva (as a uint64_t), but the description
is good, the code is wrong. Please restore the spirit of Sean's patch
(particularly because it still says it's from Sean...)

Thanks,
drew

> 
> Use pointer shenanigans instead of an unnecessary bounce buffer when the
> caller of get_ucall() provides a valid pointer.
> 
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Peter Gonda <pgonda@google.com>
> ---
>  .../selftests/kvm/include/ucall_common.h      |  8 ++------
>  .../testing/selftests/kvm/lib/aarch64/ucall.c | 13 +++----------
>  tools/testing/selftests/kvm/lib/riscv/ucall.c | 19 +++----------------
>  tools/testing/selftests/kvm/lib/s390x/ucall.c | 16 +++-------------
>  .../testing/selftests/kvm/lib/ucall_common.c  | 19 +++++++++++++++++++
>  .../testing/selftests/kvm/lib/x86_64/ucall.c  | 16 +++-------------
>  6 files changed, 33 insertions(+), 58 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/include/ucall_common.h b/tools/testing/selftests/kvm/include/ucall_common.h
> index 5a85f5318bbe..c1bc8e33ef3f 100644
> --- a/tools/testing/selftests/kvm/include/ucall_common.h
> +++ b/tools/testing/selftests/kvm/include/ucall_common.h
> @@ -27,9 +27,10 @@ struct ucall {
>  void ucall_arch_init(struct kvm_vm *vm, void *arg);
>  void ucall_arch_uninit(struct kvm_vm *vm);
>  void ucall_arch_do_ucall(vm_vaddr_t uc);
> -uint64_t ucall_arch_get_ucall(struct kvm_vcpu *vcpu, struct ucall *uc);
> +uint64_t ucall_arch_get_ucall(struct kvm_vcpu *vcpu);
>  
>  void ucall(uint64_t cmd, int nargs, ...);
> +uint64_t get_ucall(struct kvm_vcpu *vcpu, struct ucall *uc);
>  
>  static inline void ucall_init(struct kvm_vm *vm, void *arg)
>  {
> @@ -41,11 +42,6 @@ static inline void ucall_uninit(struct kvm_vm *vm)
>  	ucall_arch_uninit(vm);
>  }
>  
> -static inline uint64_t get_ucall(struct kvm_vcpu *vcpu, struct ucall *uc)
> -{
> -	return ucall_arch_get_ucall(vcpu, uc);
> -}
> -
>  #define GUEST_SYNC_ARGS(stage, arg1, arg2, arg3, arg4)	\
>  				ucall(UCALL_SYNC, 6, "hello", stage, arg1, arg2, arg3, arg4)
>  #define GUEST_SYNC(stage)	ucall(UCALL_SYNC, 2, "hello", stage)
> diff --git a/tools/testing/selftests/kvm/lib/aarch64/ucall.c b/tools/testing/selftests/kvm/lib/aarch64/ucall.c
> index 1c81a6a5c1f2..d2f099caa9ab 100644
> --- a/tools/testing/selftests/kvm/lib/aarch64/ucall.c
> +++ b/tools/testing/selftests/kvm/lib/aarch64/ucall.c
> @@ -78,24 +78,17 @@ void ucall_arch_do_ucall(vm_vaddr_t uc)
>  uint64_t ucall_arch_get_ucall(struct kvm_vcpu *vcpu, struct ucall *uc)
>  {
>  	struct kvm_run *run = vcpu->run;
> -	struct ucall ucall = {};
> -
> -	if (uc)
> -		memset(uc, 0, sizeof(*uc));
>  
>  	if (run->exit_reason == KVM_EXIT_MMIO &&
>  	    run->mmio.phys_addr == (uint64_t)ucall_exit_mmio_addr) {
> -		vm_vaddr_t gva;
> +		uint64_t ucall_addr;
>  
>  		TEST_ASSERT(run->mmio.is_write && run->mmio.len == 8,
>  			    "Unexpected ucall exit mmio address access");
>  		memcpy(&gva, run->mmio.data, sizeof(gva));
> -		memcpy(&ucall, addr_gva2hva(vcpu->vm, gva), sizeof(ucall));
>  
> -		vcpu_run_complete_io(vcpu);
> -		if (uc)
> -			memcpy(uc, &ucall, sizeof(ucall));
> +		return ucall_addr;
>  	}
>  
> -	return ucall.cmd;
> +	return 0;
>  }
> diff --git a/tools/testing/selftests/kvm/lib/riscv/ucall.c b/tools/testing/selftests/kvm/lib/riscv/ucall.c
> index b1598f418c1f..3f000d0b705f 100644
> --- a/tools/testing/selftests/kvm/lib/riscv/ucall.c
> +++ b/tools/testing/selftests/kvm/lib/riscv/ucall.c
> @@ -51,27 +51,15 @@ void ucall_arch_do_ucall(vm_vaddr_t uc)
>  		  uc, 0, 0, 0, 0, 0);
>  }
>  
> -uint64_t ucall_arch_get_ucall(struct kvm_vcpu *vcpu, struct ucall *uc)
> +uint64_t ucall_arch_get_ucall(struct kvm_vcpu *vcpu)
>  {
>  	struct kvm_run *run = vcpu->run;
> -	struct ucall ucall = {};
> -
> -	if (uc)
> -		memset(uc, 0, sizeof(*uc));
>  
>  	if (run->exit_reason == KVM_EXIT_RISCV_SBI &&
>  	    run->riscv_sbi.extension_id == KVM_RISCV_SELFTESTS_SBI_EXT) {
>  		switch (run->riscv_sbi.function_id) {
>  		case KVM_RISCV_SELFTESTS_SBI_UCALL:
> -			memcpy(&ucall,
> -			       addr_gva2hva(vcpu->vm, run->riscv_sbi.args[0]),
> -			       sizeof(ucall));
> -
> -			vcpu_run_complete_io(vcpu);
> -			if (uc)
> -				memcpy(uc, &ucall, sizeof(ucall));
> -
> -			break;
> +			return vcpu->vm, run->riscv_sbi.args[0];
>  		case KVM_RISCV_SELFTESTS_SBI_UNEXP:
>  			vcpu_dump(stderr, vcpu, 2);
>  			TEST_ASSERT(0, "Unexpected trap taken by guest");
> @@ -80,6 +68,5 @@ uint64_t ucall_arch_get_ucall(struct kvm_vcpu *vcpu, struct ucall *uc)
>  			break;
>  		}
>  	}
> -
> -	return ucall.cmd;
> +	return 0;
>  }
> diff --git a/tools/testing/selftests/kvm/lib/s390x/ucall.c b/tools/testing/selftests/kvm/lib/s390x/ucall.c
> index 114cb4af295f..f7a5a7eb4aa8 100644
> --- a/tools/testing/selftests/kvm/lib/s390x/ucall.c
> +++ b/tools/testing/selftests/kvm/lib/s390x/ucall.c
> @@ -20,13 +20,9 @@ void ucall_arch_do_ucall(vm_vaddr_t uc)
>  	asm volatile ("diag 0,%0,0x501" : : "a"(uc) : "memory");
>  }
>  
> -uint64_t ucall_arch_get_ucall(struct kvm_vcpu *vcpu, struct ucall *uc)
> +uint64_t ucall_arch_get_ucall(struct kvm_vcpu *vcpu)
>  {
>  	struct kvm_run *run = vcpu->run;
> -	struct ucall ucall = {};
> -
> -	if (uc)
> -		memset(uc, 0, sizeof(*uc));
>  
>  	if (run->exit_reason == KVM_EXIT_S390_SIEIC &&
>  	    run->s390_sieic.icptcode == 4 &&
> @@ -34,13 +30,7 @@ uint64_t ucall_arch_get_ucall(struct kvm_vcpu *vcpu, struct ucall *uc)
>  	    (run->s390_sieic.ipb >> 16) == 0x501) {
>  		int reg = run->s390_sieic.ipa & 0xf;
>  
> -		memcpy(&ucall, addr_gva2hva(vcpu->vm, run->s.regs.gprs[reg]),
> -		       sizeof(ucall));
> -
> -		vcpu_run_complete_io(vcpu);
> -		if (uc)
> -			memcpy(uc, &ucall, sizeof(ucall));
> +		return run->s.regs.gprs[reg];
>  	}
> -
> -	return ucall.cmd;
> +	return 0;
>  }
> diff --git a/tools/testing/selftests/kvm/lib/ucall_common.c b/tools/testing/selftests/kvm/lib/ucall_common.c
> index 749ffdf23855..a060252bab40 100644
> --- a/tools/testing/selftests/kvm/lib/ucall_common.c
> +++ b/tools/testing/selftests/kvm/lib/ucall_common.c
> @@ -18,3 +18,22 @@ void ucall(uint64_t cmd, int nargs, ...)
>  
>  	ucall_arch_do_ucall((vm_vaddr_t)&uc);
>  }
> +
> +uint64_t get_ucall(struct kvm_vcpu *vcpu, struct ucall *uc)
> +{
> +	struct ucall ucall;
> +	void *addr;
> +
> +	if (!uc)
> +		uc = &ucall;
> +
> +	addr = addr_gva2hva(vcpu->vm, ucall_arch_get_ucall(vcpu));
> +	if (addr) {
> +		memcpy(uc, addr, sizeof(*uc));
> +		vcpu_run_complete_io(vcpu);
> +	} else {
> +		memset(uc, 0, sizeof(*uc));
> +	}
> +
> +	return uc->cmd;
> +}
> diff --git a/tools/testing/selftests/kvm/lib/x86_64/ucall.c b/tools/testing/selftests/kvm/lib/x86_64/ucall.c
> index 9f532dba1003..24746120a593 100644
> --- a/tools/testing/selftests/kvm/lib/x86_64/ucall.c
> +++ b/tools/testing/selftests/kvm/lib/x86_64/ucall.c
> @@ -22,25 +22,15 @@ void ucall_arch_do_ucall(vm_vaddr_t uc)
>  		: : [port] "d" (UCALL_PIO_PORT), "D" (uc) : "rax", "memory");
>  }
>  
> -uint64_t ucall_arch_get_ucall(struct kvm_vcpu *vcpu, struct ucall *uc)
> +uint64_t ucall_arch_get_ucall(struct kvm_vcpu *vcpu)
>  {
>  	struct kvm_run *run = vcpu->run;
> -	struct ucall ucall = {};
> -
> -	if (uc)
> -		memset(uc, 0, sizeof(*uc));
>  
>  	if (run->exit_reason == KVM_EXIT_IO && run->io.port == UCALL_PIO_PORT) {
>  		struct kvm_regs regs;
>  
>  		vcpu_regs_get(vcpu, &regs);
> -		memcpy(&ucall, addr_gva2hva(vcpu->vm, (vm_vaddr_t)regs.rdi),
> -		       sizeof(ucall));
> -
> -		vcpu_run_complete_io(vcpu);
> -		if (uc)
> -			memcpy(uc, &ucall, sizeof(ucall));
> +		return regs.rdi;
>  	}
> -
> -	return ucall.cmd;
> +	return 0;
>  }
> -- 
> 2.37.1.455.g008518b4e5-goog
>
Peter Gonda Aug. 2, 2022, 2:46 p.m. UTC | #3
On Tue, Aug 2, 2022 at 3:54 AM Andrew Jones <andrew.jones@linux.dev> wrote:
>
> On Mon, Aug 01, 2022 at 01:11:05PM -0700, Peter Gonda wrote:
> > From: Sean Christopherson <seanjc@google.com>
> >
> > Consolidate the actual copying of a ucall struct from guest=>host into
> > the common get_ucall().  Return a host virtual address instead of a guest
> > virtual address even though the addr_gva2hva() part could be moved to
> > get_ucall() too.  Conceptually, get_ucall() is invoked from the host and
> > should return a host virtual address (and returning NULL for "nothing to
> > see here" is far superior to returning 0).
>
> The code does not match this description anymore, now that
> ucall_arch_get_ucall() returns a gva (as a uint64_t), but the description
> is good, the code is wrong. Please restore the spirit of Sean's patch
> (particularly because it still says it's from Sean...)

As discussed in the encrypted ucall patch I'll get this fixed up to
return gva from ucall_arch_get_ucall() and return a vm_addr_t.

>
> Thanks,
> drew
>
> >
> > Use pointer shenanigans instead of an unnecessary bounce buffer when the
> > caller of get_ucall() provides a valid pointer.
> >
> > Signed-off-by: Sean Christopherson <seanjc@google.com>
> > Signed-off-by: Peter Gonda <pgonda@google.com>
> > ---
> >  .../selftests/kvm/include/ucall_common.h      |  8 ++------
> >  .../testing/selftests/kvm/lib/aarch64/ucall.c | 13 +++----------
> >  tools/testing/selftests/kvm/lib/riscv/ucall.c | 19 +++----------------
> >  tools/testing/selftests/kvm/lib/s390x/ucall.c | 16 +++-------------
> >  .../testing/selftests/kvm/lib/ucall_common.c  | 19 +++++++++++++++++++
> >  .../testing/selftests/kvm/lib/x86_64/ucall.c  | 16 +++-------------
> >  6 files changed, 33 insertions(+), 58 deletions(-)
> >
> > diff --git a/tools/testing/selftests/kvm/include/ucall_common.h b/tools/testing/selftests/kvm/include/ucall_common.h
> > index 5a85f5318bbe..c1bc8e33ef3f 100644
> > --- a/tools/testing/selftests/kvm/include/ucall_common.h
> > +++ b/tools/testing/selftests/kvm/include/ucall_common.h
> > @@ -27,9 +27,10 @@ struct ucall {
> >  void ucall_arch_init(struct kvm_vm *vm, void *arg);
> >  void ucall_arch_uninit(struct kvm_vm *vm);
> >  void ucall_arch_do_ucall(vm_vaddr_t uc);
> > -uint64_t ucall_arch_get_ucall(struct kvm_vcpu *vcpu, struct ucall *uc);
> > +uint64_t ucall_arch_get_ucall(struct kvm_vcpu *vcpu);
> >
> >  void ucall(uint64_t cmd, int nargs, ...);
> > +uint64_t get_ucall(struct kvm_vcpu *vcpu, struct ucall *uc);
> >
> >  static inline void ucall_init(struct kvm_vm *vm, void *arg)
> >  {
> > @@ -41,11 +42,6 @@ static inline void ucall_uninit(struct kvm_vm *vm)
> >       ucall_arch_uninit(vm);
> >  }
> >
> > -static inline uint64_t get_ucall(struct kvm_vcpu *vcpu, struct ucall *uc)
> > -{
> > -     return ucall_arch_get_ucall(vcpu, uc);
> > -}
> > -
> >  #define GUEST_SYNC_ARGS(stage, arg1, arg2, arg3, arg4)       \
> >                               ucall(UCALL_SYNC, 6, "hello", stage, arg1, arg2, arg3, arg4)
> >  #define GUEST_SYNC(stage)    ucall(UCALL_SYNC, 2, "hello", stage)
> > diff --git a/tools/testing/selftests/kvm/lib/aarch64/ucall.c b/tools/testing/selftests/kvm/lib/aarch64/ucall.c
> > index 1c81a6a5c1f2..d2f099caa9ab 100644
> > --- a/tools/testing/selftests/kvm/lib/aarch64/ucall.c
> > +++ b/tools/testing/selftests/kvm/lib/aarch64/ucall.c
> > @@ -78,24 +78,17 @@ void ucall_arch_do_ucall(vm_vaddr_t uc)
> >  uint64_t ucall_arch_get_ucall(struct kvm_vcpu *vcpu, struct ucall *uc)
> >  {
> >       struct kvm_run *run = vcpu->run;
> > -     struct ucall ucall = {};
> > -
> > -     if (uc)
> > -             memset(uc, 0, sizeof(*uc));
> >
> >       if (run->exit_reason == KVM_EXIT_MMIO &&
> >           run->mmio.phys_addr == (uint64_t)ucall_exit_mmio_addr) {
> > -             vm_vaddr_t gva;
> > +             uint64_t ucall_addr;
> >
> >               TEST_ASSERT(run->mmio.is_write && run->mmio.len == 8,
> >                           "Unexpected ucall exit mmio address access");
> >               memcpy(&gva, run->mmio.data, sizeof(gva));
> > -             memcpy(&ucall, addr_gva2hva(vcpu->vm, gva), sizeof(ucall));
> >
> > -             vcpu_run_complete_io(vcpu);
> > -             if (uc)
> > -                     memcpy(uc, &ucall, sizeof(ucall));
> > +             return ucall_addr;
> >       }
> >
> > -     return ucall.cmd;
> > +     return 0;
> >  }
> > diff --git a/tools/testing/selftests/kvm/lib/riscv/ucall.c b/tools/testing/selftests/kvm/lib/riscv/ucall.c
> > index b1598f418c1f..3f000d0b705f 100644
> > --- a/tools/testing/selftests/kvm/lib/riscv/ucall.c
> > +++ b/tools/testing/selftests/kvm/lib/riscv/ucall.c
> > @@ -51,27 +51,15 @@ void ucall_arch_do_ucall(vm_vaddr_t uc)
> >                 uc, 0, 0, 0, 0, 0);
> >  }
> >
> > -uint64_t ucall_arch_get_ucall(struct kvm_vcpu *vcpu, struct ucall *uc)
> > +uint64_t ucall_arch_get_ucall(struct kvm_vcpu *vcpu)
> >  {
> >       struct kvm_run *run = vcpu->run;
> > -     struct ucall ucall = {};
> > -
> > -     if (uc)
> > -             memset(uc, 0, sizeof(*uc));
> >
> >       if (run->exit_reason == KVM_EXIT_RISCV_SBI &&
> >           run->riscv_sbi.extension_id == KVM_RISCV_SELFTESTS_SBI_EXT) {
> >               switch (run->riscv_sbi.function_id) {
> >               case KVM_RISCV_SELFTESTS_SBI_UCALL:
> > -                     memcpy(&ucall,
> > -                            addr_gva2hva(vcpu->vm, run->riscv_sbi.args[0]),
> > -                            sizeof(ucall));
> > -
> > -                     vcpu_run_complete_io(vcpu);
> > -                     if (uc)
> > -                             memcpy(uc, &ucall, sizeof(ucall));
> > -
> > -                     break;
> > +                     return vcpu->vm, run->riscv_sbi.args[0];
> >               case KVM_RISCV_SELFTESTS_SBI_UNEXP:
> >                       vcpu_dump(stderr, vcpu, 2);
> >                       TEST_ASSERT(0, "Unexpected trap taken by guest");
> > @@ -80,6 +68,5 @@ uint64_t ucall_arch_get_ucall(struct kvm_vcpu *vcpu, struct ucall *uc)
> >                       break;
> >               }
> >       }
> > -
> > -     return ucall.cmd;
> > +     return 0;
> >  }
> > diff --git a/tools/testing/selftests/kvm/lib/s390x/ucall.c b/tools/testing/selftests/kvm/lib/s390x/ucall.c
> > index 114cb4af295f..f7a5a7eb4aa8 100644
> > --- a/tools/testing/selftests/kvm/lib/s390x/ucall.c
> > +++ b/tools/testing/selftests/kvm/lib/s390x/ucall.c
> > @@ -20,13 +20,9 @@ void ucall_arch_do_ucall(vm_vaddr_t uc)
> >       asm volatile ("diag 0,%0,0x501" : : "a"(uc) : "memory");
> >  }
> >
> > -uint64_t ucall_arch_get_ucall(struct kvm_vcpu *vcpu, struct ucall *uc)
> > +uint64_t ucall_arch_get_ucall(struct kvm_vcpu *vcpu)
> >  {
> >       struct kvm_run *run = vcpu->run;
> > -     struct ucall ucall = {};
> > -
> > -     if (uc)
> > -             memset(uc, 0, sizeof(*uc));
> >
> >       if (run->exit_reason == KVM_EXIT_S390_SIEIC &&
> >           run->s390_sieic.icptcode == 4 &&
> > @@ -34,13 +30,7 @@ uint64_t ucall_arch_get_ucall(struct kvm_vcpu *vcpu, struct ucall *uc)
> >           (run->s390_sieic.ipb >> 16) == 0x501) {
> >               int reg = run->s390_sieic.ipa & 0xf;
> >
> > -             memcpy(&ucall, addr_gva2hva(vcpu->vm, run->s.regs.gprs[reg]),
> > -                    sizeof(ucall));
> > -
> > -             vcpu_run_complete_io(vcpu);
> > -             if (uc)
> > -                     memcpy(uc, &ucall, sizeof(ucall));
> > +             return run->s.regs.gprs[reg];
> >       }
> > -
> > -     return ucall.cmd;
> > +     return 0;
> >  }
> > diff --git a/tools/testing/selftests/kvm/lib/ucall_common.c b/tools/testing/selftests/kvm/lib/ucall_common.c
> > index 749ffdf23855..a060252bab40 100644
> > --- a/tools/testing/selftests/kvm/lib/ucall_common.c
> > +++ b/tools/testing/selftests/kvm/lib/ucall_common.c
> > @@ -18,3 +18,22 @@ void ucall(uint64_t cmd, int nargs, ...)
> >
> >       ucall_arch_do_ucall((vm_vaddr_t)&uc);
> >  }
> > +
> > +uint64_t get_ucall(struct kvm_vcpu *vcpu, struct ucall *uc)
> > +{
> > +     struct ucall ucall;
> > +     void *addr;
> > +
> > +     if (!uc)
> > +             uc = &ucall;
> > +
> > +     addr = addr_gva2hva(vcpu->vm, ucall_arch_get_ucall(vcpu));
> > +     if (addr) {
> > +             memcpy(uc, addr, sizeof(*uc));
> > +             vcpu_run_complete_io(vcpu);
> > +     } else {
> > +             memset(uc, 0, sizeof(*uc));
> > +     }
> > +
> > +     return uc->cmd;
> > +}
> > diff --git a/tools/testing/selftests/kvm/lib/x86_64/ucall.c b/tools/testing/selftests/kvm/lib/x86_64/ucall.c
> > index 9f532dba1003..24746120a593 100644
> > --- a/tools/testing/selftests/kvm/lib/x86_64/ucall.c
> > +++ b/tools/testing/selftests/kvm/lib/x86_64/ucall.c
> > @@ -22,25 +22,15 @@ void ucall_arch_do_ucall(vm_vaddr_t uc)
> >               : : [port] "d" (UCALL_PIO_PORT), "D" (uc) : "rax", "memory");
> >  }
> >
> > -uint64_t ucall_arch_get_ucall(struct kvm_vcpu *vcpu, struct ucall *uc)
> > +uint64_t ucall_arch_get_ucall(struct kvm_vcpu *vcpu)
> >  {
> >       struct kvm_run *run = vcpu->run;
> > -     struct ucall ucall = {};
> > -
> > -     if (uc)
> > -             memset(uc, 0, sizeof(*uc));
> >
> >       if (run->exit_reason == KVM_EXIT_IO && run->io.port == UCALL_PIO_PORT) {
> >               struct kvm_regs regs;
> >
> >               vcpu_regs_get(vcpu, &regs);
> > -             memcpy(&ucall, addr_gva2hva(vcpu->vm, (vm_vaddr_t)regs.rdi),
> > -                    sizeof(ucall));
> > -
> > -             vcpu_run_complete_io(vcpu);
> > -             if (uc)
> > -                     memcpy(uc, &ucall, sizeof(ucall));
> > +             return regs.rdi;
> >       }
> > -
> > -     return ucall.cmd;
> > +     return 0;
> >  }
> > --
> > 2.37.1.455.g008518b4e5-goog
> >
diff mbox series

Patch

diff --git a/tools/testing/selftests/kvm/include/ucall_common.h b/tools/testing/selftests/kvm/include/ucall_common.h
index 5a85f5318bbe..c1bc8e33ef3f 100644
--- a/tools/testing/selftests/kvm/include/ucall_common.h
+++ b/tools/testing/selftests/kvm/include/ucall_common.h
@@ -27,9 +27,10 @@  struct ucall {
 void ucall_arch_init(struct kvm_vm *vm, void *arg);
 void ucall_arch_uninit(struct kvm_vm *vm);
 void ucall_arch_do_ucall(vm_vaddr_t uc);
-uint64_t ucall_arch_get_ucall(struct kvm_vcpu *vcpu, struct ucall *uc);
+uint64_t ucall_arch_get_ucall(struct kvm_vcpu *vcpu);
 
 void ucall(uint64_t cmd, int nargs, ...);
+uint64_t get_ucall(struct kvm_vcpu *vcpu, struct ucall *uc);
 
 static inline void ucall_init(struct kvm_vm *vm, void *arg)
 {
@@ -41,11 +42,6 @@  static inline void ucall_uninit(struct kvm_vm *vm)
 	ucall_arch_uninit(vm);
 }
 
-static inline uint64_t get_ucall(struct kvm_vcpu *vcpu, struct ucall *uc)
-{
-	return ucall_arch_get_ucall(vcpu, uc);
-}
-
 #define GUEST_SYNC_ARGS(stage, arg1, arg2, arg3, arg4)	\
 				ucall(UCALL_SYNC, 6, "hello", stage, arg1, arg2, arg3, arg4)
 #define GUEST_SYNC(stage)	ucall(UCALL_SYNC, 2, "hello", stage)
diff --git a/tools/testing/selftests/kvm/lib/aarch64/ucall.c b/tools/testing/selftests/kvm/lib/aarch64/ucall.c
index 1c81a6a5c1f2..d2f099caa9ab 100644
--- a/tools/testing/selftests/kvm/lib/aarch64/ucall.c
+++ b/tools/testing/selftests/kvm/lib/aarch64/ucall.c
@@ -78,24 +78,17 @@  void ucall_arch_do_ucall(vm_vaddr_t uc)
 uint64_t ucall_arch_get_ucall(struct kvm_vcpu *vcpu, struct ucall *uc)
 {
 	struct kvm_run *run = vcpu->run;
-	struct ucall ucall = {};
-
-	if (uc)
-		memset(uc, 0, sizeof(*uc));
 
 	if (run->exit_reason == KVM_EXIT_MMIO &&
 	    run->mmio.phys_addr == (uint64_t)ucall_exit_mmio_addr) {
-		vm_vaddr_t gva;
+		uint64_t ucall_addr;
 
 		TEST_ASSERT(run->mmio.is_write && run->mmio.len == 8,
 			    "Unexpected ucall exit mmio address access");
 		memcpy(&gva, run->mmio.data, sizeof(gva));
-		memcpy(&ucall, addr_gva2hva(vcpu->vm, gva), sizeof(ucall));
 
-		vcpu_run_complete_io(vcpu);
-		if (uc)
-			memcpy(uc, &ucall, sizeof(ucall));
+		return ucall_addr;
 	}
 
-	return ucall.cmd;
+	return 0;
 }
diff --git a/tools/testing/selftests/kvm/lib/riscv/ucall.c b/tools/testing/selftests/kvm/lib/riscv/ucall.c
index b1598f418c1f..3f000d0b705f 100644
--- a/tools/testing/selftests/kvm/lib/riscv/ucall.c
+++ b/tools/testing/selftests/kvm/lib/riscv/ucall.c
@@ -51,27 +51,15 @@  void ucall_arch_do_ucall(vm_vaddr_t uc)
 		  uc, 0, 0, 0, 0, 0);
 }
 
-uint64_t ucall_arch_get_ucall(struct kvm_vcpu *vcpu, struct ucall *uc)
+uint64_t ucall_arch_get_ucall(struct kvm_vcpu *vcpu)
 {
 	struct kvm_run *run = vcpu->run;
-	struct ucall ucall = {};
-
-	if (uc)
-		memset(uc, 0, sizeof(*uc));
 
 	if (run->exit_reason == KVM_EXIT_RISCV_SBI &&
 	    run->riscv_sbi.extension_id == KVM_RISCV_SELFTESTS_SBI_EXT) {
 		switch (run->riscv_sbi.function_id) {
 		case KVM_RISCV_SELFTESTS_SBI_UCALL:
-			memcpy(&ucall,
-			       addr_gva2hva(vcpu->vm, run->riscv_sbi.args[0]),
-			       sizeof(ucall));
-
-			vcpu_run_complete_io(vcpu);
-			if (uc)
-				memcpy(uc, &ucall, sizeof(ucall));
-
-			break;
+			return vcpu->vm, run->riscv_sbi.args[0];
 		case KVM_RISCV_SELFTESTS_SBI_UNEXP:
 			vcpu_dump(stderr, vcpu, 2);
 			TEST_ASSERT(0, "Unexpected trap taken by guest");
@@ -80,6 +68,5 @@  uint64_t ucall_arch_get_ucall(struct kvm_vcpu *vcpu, struct ucall *uc)
 			break;
 		}
 	}
-
-	return ucall.cmd;
+	return 0;
 }
diff --git a/tools/testing/selftests/kvm/lib/s390x/ucall.c b/tools/testing/selftests/kvm/lib/s390x/ucall.c
index 114cb4af295f..f7a5a7eb4aa8 100644
--- a/tools/testing/selftests/kvm/lib/s390x/ucall.c
+++ b/tools/testing/selftests/kvm/lib/s390x/ucall.c
@@ -20,13 +20,9 @@  void ucall_arch_do_ucall(vm_vaddr_t uc)
 	asm volatile ("diag 0,%0,0x501" : : "a"(uc) : "memory");
 }
 
-uint64_t ucall_arch_get_ucall(struct kvm_vcpu *vcpu, struct ucall *uc)
+uint64_t ucall_arch_get_ucall(struct kvm_vcpu *vcpu)
 {
 	struct kvm_run *run = vcpu->run;
-	struct ucall ucall = {};
-
-	if (uc)
-		memset(uc, 0, sizeof(*uc));
 
 	if (run->exit_reason == KVM_EXIT_S390_SIEIC &&
 	    run->s390_sieic.icptcode == 4 &&
@@ -34,13 +30,7 @@  uint64_t ucall_arch_get_ucall(struct kvm_vcpu *vcpu, struct ucall *uc)
 	    (run->s390_sieic.ipb >> 16) == 0x501) {
 		int reg = run->s390_sieic.ipa & 0xf;
 
-		memcpy(&ucall, addr_gva2hva(vcpu->vm, run->s.regs.gprs[reg]),
-		       sizeof(ucall));
-
-		vcpu_run_complete_io(vcpu);
-		if (uc)
-			memcpy(uc, &ucall, sizeof(ucall));
+		return run->s.regs.gprs[reg];
 	}
-
-	return ucall.cmd;
+	return 0;
 }
diff --git a/tools/testing/selftests/kvm/lib/ucall_common.c b/tools/testing/selftests/kvm/lib/ucall_common.c
index 749ffdf23855..a060252bab40 100644
--- a/tools/testing/selftests/kvm/lib/ucall_common.c
+++ b/tools/testing/selftests/kvm/lib/ucall_common.c
@@ -18,3 +18,22 @@  void ucall(uint64_t cmd, int nargs, ...)
 
 	ucall_arch_do_ucall((vm_vaddr_t)&uc);
 }
+
+uint64_t get_ucall(struct kvm_vcpu *vcpu, struct ucall *uc)
+{
+	struct ucall ucall;
+	void *addr;
+
+	if (!uc)
+		uc = &ucall;
+
+	addr = addr_gva2hva(vcpu->vm, ucall_arch_get_ucall(vcpu));
+	if (addr) {
+		memcpy(uc, addr, sizeof(*uc));
+		vcpu_run_complete_io(vcpu);
+	} else {
+		memset(uc, 0, sizeof(*uc));
+	}
+
+	return uc->cmd;
+}
diff --git a/tools/testing/selftests/kvm/lib/x86_64/ucall.c b/tools/testing/selftests/kvm/lib/x86_64/ucall.c
index 9f532dba1003..24746120a593 100644
--- a/tools/testing/selftests/kvm/lib/x86_64/ucall.c
+++ b/tools/testing/selftests/kvm/lib/x86_64/ucall.c
@@ -22,25 +22,15 @@  void ucall_arch_do_ucall(vm_vaddr_t uc)
 		: : [port] "d" (UCALL_PIO_PORT), "D" (uc) : "rax", "memory");
 }
 
-uint64_t ucall_arch_get_ucall(struct kvm_vcpu *vcpu, struct ucall *uc)
+uint64_t ucall_arch_get_ucall(struct kvm_vcpu *vcpu)
 {
 	struct kvm_run *run = vcpu->run;
-	struct ucall ucall = {};
-
-	if (uc)
-		memset(uc, 0, sizeof(*uc));
 
 	if (run->exit_reason == KVM_EXIT_IO && run->io.port == UCALL_PIO_PORT) {
 		struct kvm_regs regs;
 
 		vcpu_regs_get(vcpu, &regs);
-		memcpy(&ucall, addr_gva2hva(vcpu->vm, (vm_vaddr_t)regs.rdi),
-		       sizeof(ucall));
-
-		vcpu_run_complete_io(vcpu);
-		if (uc)
-			memcpy(uc, &ucall, sizeof(ucall));
+		return regs.rdi;
 	}
-
-	return ucall.cmd;
+	return 0;
 }