[PATCH] KVM: PPC: Book3S HV: Replace kvmppc_find_vcpu() with kvm_get_vcpu()

[PATCH] KVM: PPC: Book3S HV: Replace kvmppc_find_vcpu() with kvm_get_vcpu()

[Thomas Huth]

Both functions are doing the same thing - looking up the struct
kvm_vcpu pointer for a given vCPU ID. So there's no need for the
kvmppc_find_vcpu() function, simply use the common function instead.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 arch/powerpc/kvm/book3s_hv.c | 22 +++-------------------
 1 file changed, 3 insertions(+), 19 deletions(-)

diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
index 48d3c5d..78e62cf 100644
--- a/arch/powerpc/kvm/book3s_hv.c
+++ b/arch/powerpc/kvm/book3s_hv.c
@@ -292,22 +292,6 @@ void kvmppc_dump_regs(struct kvm_vcpu *vcpu)
 	       vcpu->arch.last_inst);
 }
 
-struct kvm_vcpu *kvmppc_find_vcpu(struct kvm *kvm, int id)
-{
-	int r;
-	struct kvm_vcpu *v, *ret = NULL;
-
-	mutex_lock(&kvm->lock);
-	kvm_for_each_vcpu(r, v, kvm) {
-		if (v->vcpu_id == id) {
-			ret = v;
-			break;
-		}
-	}
-	mutex_unlock(&kvm->lock);
-	return ret;
-}
-
 static void init_vpa(struct kvm_vcpu *vcpu, struct lppaca *vpa)
 {
 	vpa->__old_status |= LPPACA_OLD_SHARED_PROC;
@@ -358,7 +342,7 @@ static unsigned long do_h_register_vpa(struct kvm_vcpu *vcpu,
 	int subfunc;
 	struct kvmppc_vpa *vpap;
 
-	tvcpu = kvmppc_find_vcpu(kvm, vcpuid);
+	tvcpu = kvm_get_vcpu(kvm, vcpuid);
 	if (!tvcpu)
 		return H_PARAMETER;
 
@@ -678,7 +662,7 @@ int kvmppc_pseries_do_hcall(struct kvm_vcpu *vcpu)
 		break;
 	case H_PROD:
 		target = kvmppc_get_gpr(vcpu, 4);
-		tvcpu = kvmppc_find_vcpu(vcpu->kvm, target);
+		tvcpu = kvm_get_vcpu(vcpu->kvm, target);
 		if (!tvcpu) {
 			ret = H_PARAMETER;
 			break;
@@ -696,7 +680,7 @@ int kvmppc_pseries_do_hcall(struct kvm_vcpu *vcpu)
 		target = kvmppc_get_gpr(vcpu, 4);
 		if (target == -1)
 			break;
-		tvcpu = kvmppc_find_vcpu(vcpu->kvm, target);
+		tvcpu = kvm_get_vcpu(vcpu->kvm, target);
 		if (!tvcpu) {
 			ret = H_PARAMETER;
 			break;
-- 
1.8.3.1

[PATCH] KVM: PPC: Book3S HV: Replace kvmppc_find_vcpu() with kvm_get_vcpu()

[Thomas Huth]

Both functions are doing the same thing - looking up the struct
kvm_vcpu pointer for a given vCPU ID. So there's no need for the
kvmppc_find_vcpu() function, simply use the common function instead.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 arch/powerpc/kvm/book3s_hv.c | 22 +++-------------------
 1 file changed, 3 insertions(+), 19 deletions(-)

diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
index 48d3c5d..78e62cf 100644
--- a/arch/powerpc/kvm/book3s_hv.c
+++ b/arch/powerpc/kvm/book3s_hv.c
@@ -292,22 +292,6 @@ void kvmppc_dump_regs(struct kvm_vcpu *vcpu)
 	       vcpu->arch.last_inst);
 }
 
-struct kvm_vcpu *kvmppc_find_vcpu(struct kvm *kvm, int id)
-{
-	int r;
-	struct kvm_vcpu *v, *ret = NULL;
-
-	mutex_lock(&kvm->lock);
-	kvm_for_each_vcpu(r, v, kvm) {
-		if (v->vcpu_id = id) {
-			ret = v;
-			break;
-		}
-	}
-	mutex_unlock(&kvm->lock);
-	return ret;
-}
-
 static void init_vpa(struct kvm_vcpu *vcpu, struct lppaca *vpa)
 {
 	vpa->__old_status |= LPPACA_OLD_SHARED_PROC;
@@ -358,7 +342,7 @@ static unsigned long do_h_register_vpa(struct kvm_vcpu *vcpu,
 	int subfunc;
 	struct kvmppc_vpa *vpap;
 
-	tvcpu = kvmppc_find_vcpu(kvm, vcpuid);
+	tvcpu = kvm_get_vcpu(kvm, vcpuid);
 	if (!tvcpu)
 		return H_PARAMETER;
 
@@ -678,7 +662,7 @@ int kvmppc_pseries_do_hcall(struct kvm_vcpu *vcpu)
 		break;
 	case H_PROD:
 		target = kvmppc_get_gpr(vcpu, 4);
-		tvcpu = kvmppc_find_vcpu(vcpu->kvm, target);
+		tvcpu = kvm_get_vcpu(vcpu->kvm, target);
 		if (!tvcpu) {
 			ret = H_PARAMETER;
 			break;
@@ -696,7 +680,7 @@ int kvmppc_pseries_do_hcall(struct kvm_vcpu *vcpu)
 		target = kvmppc_get_gpr(vcpu, 4);
 		if (target = -1)
 			break;
-		tvcpu = kvmppc_find_vcpu(vcpu->kvm, target);
+		tvcpu = kvm_get_vcpu(vcpu->kvm, target);
 		if (!tvcpu) {
 			ret = H_PARAMETER;
 			break;
-- 
1.8.3.1

Re: [PATCH] KVM: PPC: Book3S HV: Replace kvmppc_find_vcpu() with kvm_get_vcpu()

[Paul Mackerras]

On Thu, May 07, 2015 at 05:49:39PM +0200, Thomas Huth wrote:
> Both functions are doing the same thing - looking up the struct
> kvm_vcpu pointer for a given vCPU ID.

No, kvm_get_vcpu(n) returns the n'th vcpu created, which is often
but not necessarily the same as the vcpu with id n.  In fact, due to
the way we do threading in HV KVM, it's quite common for userspace to
create vcpus with ids 0, 8, 16, ..., which means that the n'th vcpu
created does not have id n (except of course for vcpu 0).

So, NAK.

Paul.

Re: [PATCH] KVM: PPC: Book3S HV: Replace kvmppc_find_vcpu() with kvm_get_vcpu()

[Paul Mackerras]

On Thu, May 07, 2015 at 05:49:39PM +0200, Thomas Huth wrote:
> Both functions are doing the same thing - looking up the struct
> kvm_vcpu pointer for a given vCPU ID.

No, kvm_get_vcpu(n) returns the n'th vcpu created, which is often
but not necessarily the same as the vcpu with id n.  In fact, due to
the way we do threading in HV KVM, it's quite common for userspace to
create vcpus with ids 0, 8, 16, ..., which means that the n'th vcpu
created does not have id n (except of course for vcpu 0).

So, NAK.

Paul.

Re: [PATCH] KVM: PPC: Book3S HV: Replace kvmppc_find_vcpu() with kvm_get_vcpu()

[Thomas Huth]

On Fri, 8 May 2015 09:07:18 +1000
Paul Mackerras <paulus@samba.org> wrote:

> On Thu, May 07, 2015 at 05:49:39PM +0200, Thomas Huth wrote:
> > Both functions are doing the same thing - looking up the struct
> > kvm_vcpu pointer for a given vCPU ID.
> 
> No, kvm_get_vcpu(n) returns the n'th vcpu created, which is often
> but not necessarily the same as the vcpu with id n.  In fact, due to
> the way we do threading in HV KVM, it's quite common for userspace to
> create vcpus with ids 0, 8, 16, ..., which means that the n'th vcpu
> created does not have id n (except of course for vcpu 0).

Ah, ok, thanks a lot for the explanation ... I thought that KVM would
always put the VCPUs at the position of their ID into the array, but
looking at kvm_vm_ioctl_create_vcpu(), you're right, it's using the
amount of online CPUs as index instead:

	kvm->vcpus[atomic_read(&kvm->online_vcpus)] = vcpu;

So never mind, and sorry for the nuisance!

 Thomas

Re: [PATCH] KVM: PPC: Book3S HV: Replace kvmppc_find_vcpu() with kvm_get_vcpu()

[Thomas Huth]

On Fri, 8 May 2015 09:07:18 +1000
Paul Mackerras <paulus@samba.org> wrote:

> On Thu, May 07, 2015 at 05:49:39PM +0200, Thomas Huth wrote:
> > Both functions are doing the same thing - looking up the struct
> > kvm_vcpu pointer for a given vCPU ID.
> 
> No, kvm_get_vcpu(n) returns the n'th vcpu created, which is often
> but not necessarily the same as the vcpu with id n.  In fact, due to
> the way we do threading in HV KVM, it's quite common for userspace to
> create vcpus with ids 0, 8, 16, ..., which means that the n'th vcpu
> created does not have id n (except of course for vcpu 0).

Ah, ok, thanks a lot for the explanation ... I thought that KVM would
always put the VCPUs at the position of their ID into the array, but
looking at kvm_vm_ioctl_create_vcpu(), you're right, it's using the
amount of online CPUs as index instead:

	kvm->vcpus[atomic_read(&kvm->online_vcpus)] = vcpu;

So never mind, and sorry for the nuisance!

 Thomas

Re: [PATCH] KVM: PPC: Book3S HV: Replace kvmppc_find_vcpu() with kvm_get_vcpu()

[Michael Ellerman]

On Fri, 2015-05-08 at 10:19 +0200, Thomas Huth wrote:
> On Fri, 8 May 2015 09:07:18 +1000
> Paul Mackerras <paulus@samba.org> wrote:
> 
> > On Thu, May 07, 2015 at 05:49:39PM +0200, Thomas Huth wrote:
> > > Both functions are doing the same thing - looking up the struct
> > > kvm_vcpu pointer for a given vCPU ID.
> > 
> > No, kvm_get_vcpu(n) returns the n'th vcpu created, which is often
> > but not necessarily the same as the vcpu with id n.  In fact, due to
> > the way we do threading in HV KVM, it's quite common for userspace to
> > create vcpus with ids 0, 8, 16, ..., which means that the n'th vcpu
> > created does not have id n (except of course for vcpu 0).

A patch adding a comment explaining that subtlety would be welcome from anyone
who sent it :)

cheers

Re: [PATCH] KVM: PPC: Book3S HV: Replace kvmppc_find_vcpu() with kvm_get_vcpu()

[Michael Ellerman]

On Fri, 2015-05-08 at 10:19 +0200, Thomas Huth wrote:
> On Fri, 8 May 2015 09:07:18 +1000
> Paul Mackerras <paulus@samba.org> wrote:
> 
> > On Thu, May 07, 2015 at 05:49:39PM +0200, Thomas Huth wrote:
> > > Both functions are doing the same thing - looking up the struct
> > > kvm_vcpu pointer for a given vCPU ID.
> > 
> > No, kvm_get_vcpu(n) returns the n'th vcpu created, which is often
> > but not necessarily the same as the vcpu with id n.  In fact, due to
> > the way we do threading in HV KVM, it's quite common for userspace to
> > create vcpus with ids 0, 8, 16, ..., which means that the n'th vcpu
> > created does not have id n (except of course for vcpu 0).

A patch adding a comment explaining that subtlety would be welcome from anyone
who sent it :)

cheers