[PATCH 0/4] Preview: vcpu lookup by id changes

[PATCH 0/4] Preview: vcpu lookup by id changes

[Christian Borntraeger]

Paolo,

some more patches for review before I add them in my next pull request.
Can you review the common code changes and Ack/Nack?

Alex, Paul,
can you review the power changes and Ack/Nack? 

As I want to have patch 2 in 4.4, I splitted up Davids patch into smaller
chunks. David, can you double check that I did not mess up?
So I want to send patch 1 and patch 2 for 4.4. Patch 3 and Patch 4
are for kvm/next (4.5), but need patch 1 as well. 
So I will probably also add all patches into kvm/next by setting up
the git branches in a way that during 4.5 merge window kvm/next will
have my branch that will go via kvm/master as common ancestor.

Ok?

Christian

David Hildenbrand (4):
  KVM: Provide function for VCPU lookup by id
  KVM: s390: fix wrong lookup of VCPUs by array index
  KVM: use heuristic for fast VCPU lookup by id
  KVM: Use common function for VCPU lookup by id

 arch/powerpc/kvm/book3s_hv.c | 10 ++--------
 arch/s390/kvm/diag.c         | 11 +++--------
 arch/s390/kvm/sigp.c         |  8 ++------
 include/linux/kvm_host.h     | 16 ++++++++++++++++
 virt/kvm/kvm_main.c          | 12 +++++-------
 5 files changed, 28 insertions(+), 29 deletions(-)

-- 
2.3.0

[PATCH 1/4] KVM: Provide function for VCPU lookup by id

[Christian Borntraeger]

From: David Hildenbrand <dahi@linux.vnet.ibm.com>

Let's provide a function to lookup a VCPU by id.

Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
Reviewed-by: Dominik Dingel <dingel@linux.vnet.ibm.com>
Signed-off-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
[split patch from refactoring patch]
---
 include/linux/kvm_host.h | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 5706a21..b9f345f 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -460,6 +460,17 @@ static inline struct kvm_vcpu *kvm_get_vcpu(struct kvm *kvm, int i)
 	     (vcpup = kvm_get_vcpu(kvm, idx)) != NULL; \
 	     idx++)
 
+static inline struct kvm_vcpu *kvm_lookup_vcpu(struct kvm *kvm, int id)
+{
+	struct kvm_vcpu *vcpu;
+	int i;
+
+	kvm_for_each_vcpu(i, vcpu, kvm)
+		if (vcpu->vcpu_id == id)
+			return vcpu;
+	return NULL;
+}
+
 #define kvm_for_each_memslot(memslot, slots)	\
 	for (memslot = &slots->memslots[0];	\
 	      memslot < slots->memslots + KVM_MEM_SLOTS_NUM && memslot->npages;\
-- 
2.3.0

Re: [PATCH 1/4] KVM: Provide function for VCPU lookup by id

[Thomas Huth]

On 19/11/15 09:37, Christian Borntraeger wrote:
> From: David Hildenbrand <dahi@linux.vnet.ibm.com>
> 
> Let's provide a function to lookup a VCPU by id.
> 
> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Reviewed-by: Dominik Dingel <dingel@linux.vnet.ibm.com>
> Signed-off-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> [split patch from refactoring patch]
> ---
>  include/linux/kvm_host.h | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 5706a21..b9f345f 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -460,6 +460,17 @@ static inline struct kvm_vcpu *kvm_get_vcpu(struct kvm *kvm, int i)
>  	     (vcpup = kvm_get_vcpu(kvm, idx)) != NULL; \
>  	     idx++)
>  
> +static inline struct kvm_vcpu *kvm_lookup_vcpu(struct kvm *kvm, int id)
> +{
> +	struct kvm_vcpu *vcpu;
> +	int i;
> +
> +	kvm_for_each_vcpu(i, vcpu, kvm)
> +		if (vcpu->vcpu_id == id)
> +			return vcpu;
> +	return NULL;
> +}
> +

Any chance that you name this function differently? Otherwise we've got
two functions that sound very similar and also have similar prototypes:

- kvm_get_vcpu(struct kvm *kvm, int i)
- kvm_lookup_vcpu(struct kvm *kvm, int id)

I'm pretty sure this will cause confusion in the future!
==> Could you maybe name the new function something like
"kvm_lookup_vcpu_by_id" or "kvm_get_vcpu_by_id" instead?

Also a short comment before the function would be nice to make the
reader aware of the difference (e.g. when hot-plugging) between the
vcpu-id and array-id.

Otherwise: +1 for finally having a proper common function for this!

 Thomas

Re: [PATCH 1/4] KVM: Provide function for VCPU lookup by id

[David Hildenbrand]

> 
> Any chance that you name this function differently? Otherwise we've got
> two functions that sound very similar and also have similar prototypes:
> 
> - kvm_get_vcpu(struct kvm *kvm, int i)
> - kvm_lookup_vcpu(struct kvm *kvm, int id)
> 
> I'm pretty sure this will cause confusion in the future!
> ==> Could you maybe name the new function something like
> "kvm_lookup_vcpu_by_id" or "kvm_get_vcpu_by_id" instead?

Had that in a previous version but decided to name it that way ... hmm ...
other opinions?

> 
> Also a short comment before the function would be nice to make the
> reader aware of the difference (e.g. when hot-plugging) between the
> vcpu-id and array-id.

Also not sure as the header directly includes the (for my eyes ;) ) easy code.
I would agree for a pure prototype. Other opinions?

> 
> Otherwise: +1 for finally having a proper common function for this!
> 

Thanks for having a look Thomas!

David

Re: [PATCH 1/4] KVM: Provide function for VCPU lookup by id

[Paolo Bonzini]

On 19/11/2015 13:55, David Hildenbrand wrote:
>> > 
>> > I'm pretty sure this will cause confusion in the future!
>> > ==> Could you maybe name the new function something like
>> > "kvm_lookup_vcpu_by_id" or "kvm_get_vcpu_by_id" instead?
> Had that in a previous version but decided to name it that way ... hmm ...
> other opinions?

Having _by_id at the end of the name makes sense indeed.

Paolo

Re: [PATCH 1/4] KVM: Provide function for VCPU lookup by id

[Christian Borntraeger]

On 11/19/2015 02:13 PM, Paolo Bonzini wrote:
> 
> 
> On 19/11/2015 13:55, David Hildenbrand wrote:
>>>>
>>>> I'm pretty sure this will cause confusion in the future!
>>>> ==> Could you maybe name the new function something like
>>>> "kvm_lookup_vcpu_by_id" or "kvm_get_vcpu_by_id" instead?
>> Had that in a previous version but decided to name it that way ... hmm ...
>> other opinions?
> 
> Having _by_id at the end of the name makes sense indeed.

David,
I will fix up the function name myself to kvm_get_vcpu_by_id. Ok?

Re: [PATCH 1/4] KVM: Provide function for VCPU lookup by id

[David Hildenbrand]

> On 11/19/2015 02:13 PM, Paolo Bonzini wrote:
> > 
> > 
> > On 19/11/2015 13:55, David Hildenbrand wrote:
> >>>>
> >>>> I'm pretty sure this will cause confusion in the future!
> >>>> ==> Could you maybe name the new function something like
> >>>> "kvm_lookup_vcpu_by_id" or "kvm_get_vcpu_by_id" instead?
> >> Had that in a previous version but decided to name it that way ... hmm ...
> >> other opinions?
> > 
> > Having _by_id at the end of the name makes sense indeed.
> 
> David,
> I will fix up the function name myself to kvm_get_vcpu_by_id. Ok?

Sure, thanks a lot!

David

[PATCH 2/4] KVM: s390: fix wrong lookup of VCPUs by array index

[Christian Borntraeger]

From: David Hildenbrand <dahi@linux.vnet.ibm.com>

For now, VCPUs were always created sequentially with incrementing
VCPU ids. Therefore, the index in the VCPUs array matched the id.

As sequential creation might change with cpu hotplug, let's use
the correct lookup function to find a VCPU by id, not array index.

Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
Signed-off-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
[split stable/non-stable parts]
Cc: stable@vger.kernel.org # c3853a8: KVM: Provide function for VCPU lookup by id
---
 arch/s390/kvm/sigp.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/arch/s390/kvm/sigp.c b/arch/s390/kvm/sigp.c
index da690b6..081dbd0 100644
--- a/arch/s390/kvm/sigp.c
+++ b/arch/s390/kvm/sigp.c
@@ -291,12 +291,8 @@ static int handle_sigp_dst(struct kvm_vcpu *vcpu, u8 order_code,
 			   u16 cpu_addr, u32 parameter, u64 *status_reg)
 {
 	int rc;
-	struct kvm_vcpu *dst_vcpu;
+	struct kvm_vcpu *dst_vcpu = kvm_lookup_vcpu(vcpu->kvm, cpu_addr);
 
-	if (cpu_addr >= KVM_MAX_VCPUS)
-		return SIGP_CC_NOT_OPERATIONAL;
-
-	dst_vcpu = kvm_get_vcpu(vcpu->kvm, cpu_addr);
 	if (!dst_vcpu)
 		return SIGP_CC_NOT_OPERATIONAL;
 
@@ -478,7 +474,7 @@ int kvm_s390_handle_sigp_pei(struct kvm_vcpu *vcpu)
 	trace_kvm_s390_handle_sigp_pei(vcpu, order_code, cpu_addr);
 
 	if (order_code == SIGP_EXTERNAL_CALL) {
-		dest_vcpu = kvm_get_vcpu(vcpu->kvm, cpu_addr);
+		dest_vcpu = kvm_lookup_vcpu(vcpu->kvm, cpu_addr);
 		BUG_ON(dest_vcpu == NULL);
 
 		kvm_s390_vcpu_wakeup(dest_vcpu);
-- 
2.3.0

Re: [PATCH 2/4] KVM: s390: fix wrong lookup of VCPUs by array index

[Christian Borntraeger]

Sigh.
Seems that my mail script got confused by the # after stable. So please
strip down the cc list.


On 11/19/2015 09:37 AM, Christian Borntraeger wrote:
> From: David Hildenbrand <dahi@linux.vnet.ibm.com>
> 
> For now, VCPUs were always created sequentially with incrementing
> VCPU ids. Therefore, the index in the VCPUs array matched the id.
> 
> As sequential creation might change with cpu hotplug, let's use
> the correct lookup function to find a VCPU by id, not array index.
> 
> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Signed-off-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> [split stable/non-stable parts]
> Cc: stable@vger.kernel.org # c3853a8: KVM: Provide function for VCPU lookup by id
> ---
>  arch/s390/kvm/sigp.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/s390/kvm/sigp.c b/arch/s390/kvm/sigp.c
> index da690b6..081dbd0 100644
> --- a/arch/s390/kvm/sigp.c
> +++ b/arch/s390/kvm/sigp.c
> @@ -291,12 +291,8 @@ static int handle_sigp_dst(struct kvm_vcpu *vcpu, u8 order_code,
>  			   u16 cpu_addr, u32 parameter, u64 *status_reg)
>  {
>  	int rc;
> -	struct kvm_vcpu *dst_vcpu;
> +	struct kvm_vcpu *dst_vcpu = kvm_lookup_vcpu(vcpu->kvm, cpu_addr);
> 
> -	if (cpu_addr >= KVM_MAX_VCPUS)
> -		return SIGP_CC_NOT_OPERATIONAL;
> -
> -	dst_vcpu = kvm_get_vcpu(vcpu->kvm, cpu_addr);
>  	if (!dst_vcpu)
>  		return SIGP_CC_NOT_OPERATIONAL;
> 
> @@ -478,7 +474,7 @@ int kvm_s390_handle_sigp_pei(struct kvm_vcpu *vcpu)
>  	trace_kvm_s390_handle_sigp_pei(vcpu, order_code, cpu_addr);
> 
>  	if (order_code == SIGP_EXTERNAL_CALL) {
> -		dest_vcpu = kvm_get_vcpu(vcpu->kvm, cpu_addr);
> +		dest_vcpu = kvm_lookup_vcpu(vcpu->kvm, cpu_addr);
>  		BUG_ON(dest_vcpu == NULL);
> 
>  		kvm_s390_vcpu_wakeup(dest_vcpu);
> 

[PATCH 3/4] KVM: use heuristic for fast VCPU lookup by id

[Christian Borntraeger]

From: David Hildenbrand <dahi@linux.vnet.ibm.com>

Usually, VCPU ids match the array index. So let's try a fast
lookup first before falling back to the slow iteration.

Suggested-by: Christian Borntraeger <borntraeger@de.ibm.com>
Reviewed-by: Dominik Dingel <dingel@linux.vnet.ibm.com>
Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
Signed-off-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 include/linux/kvm_host.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index b9f345f..7821137 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -465,6 +465,11 @@ static inline struct kvm_vcpu *kvm_lookup_vcpu(struct kvm *kvm, int id)
 	struct kvm_vcpu *vcpu;
 	int i;
 
+	if (id < 0 || id >= KVM_MAX_VCPUS)
+		return NULL;
+	vcpu = kvm_get_vcpu(kvm, id);
+	if (vcpu && vcpu->vcpu_id == id)
+		return vcpu;
 	kvm_for_each_vcpu(i, vcpu, kvm)
 		if (vcpu->vcpu_id == id)
 			return vcpu;
-- 
2.3.0

[PATCH 4/4] KVM: Use common function for VCPU lookup by id

[Christian Borntraeger]

From: David Hildenbrand <dahi@linux.vnet.ibm.com>

Let's reuse the new common function for VPCU lookup by id.

Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
Reviewed-by: Dominik Dingel <dingel@linux.vnet.ibm.com>
Signed-off-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
[split out the new function into a separate patch]
---
 arch/powerpc/kvm/book3s_hv.c | 10 ++--------
 arch/s390/kvm/diag.c         | 11 +++--------
 virt/kvm/kvm_main.c          | 12 +++++-------
 3 files changed, 10 insertions(+), 23 deletions(-)

diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
index 54b45b7..904b3b0 100644
--- a/arch/powerpc/kvm/book3s_hv.c
+++ b/arch/powerpc/kvm/book3s_hv.c
@@ -308,16 +308,10 @@ static void kvmppc_dump_regs(struct kvm_vcpu *vcpu)
 
 static struct kvm_vcpu *kvmppc_find_vcpu(struct kvm *kvm, int id)
 {
-	int r;
-	struct kvm_vcpu *v, *ret = NULL;
+	struct kvm_vcpu *ret;
 
 	mutex_lock(&kvm->lock);
-	kvm_for_each_vcpu(r, v, kvm) {
-		if (v->vcpu_id == id) {
-			ret = v;
-			break;
-		}
-	}
+	ret = kvm_lookup_vcpu(kvm, id);
 	mutex_unlock(&kvm->lock);
 	return ret;
 }
diff --git a/arch/s390/kvm/diag.c b/arch/s390/kvm/diag.c
index 5fbfb88..aaa7cc0 100644
--- a/arch/s390/kvm/diag.c
+++ b/arch/s390/kvm/diag.c
@@ -155,10 +155,8 @@ static int __diag_time_slice_end(struct kvm_vcpu *vcpu)
 
 static int __diag_time_slice_end_directed(struct kvm_vcpu *vcpu)
 {
-	struct kvm *kvm = vcpu->kvm;
 	struct kvm_vcpu *tcpu;
 	int tid;
-	int i;
 
 	tid = vcpu->run->s.regs.gprs[(vcpu->arch.sie_block->ipa & 0xf0) >> 4];
 	vcpu->stat.diagnose_9c++;
@@ -167,12 +165,9 @@ static int __diag_time_slice_end_directed(struct kvm_vcpu *vcpu)
 	if (tid == vcpu->vcpu_id)
 		return 0;
 
-	kvm_for_each_vcpu(i, tcpu, kvm)
-		if (tcpu->vcpu_id == tid) {
-			kvm_vcpu_yield_to(tcpu);
-			break;
-		}
-
+	tcpu = kvm_lookup_vcpu(vcpu->kvm, tid);
+	if (tcpu)
+		kvm_vcpu_yield_to(tcpu);
 	return 0;
 }
 
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 484079e..244c9b4 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -2257,7 +2257,7 @@ static int create_vcpu_fd(struct kvm_vcpu *vcpu)
 static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, u32 id)
 {
 	int r;
-	struct kvm_vcpu *vcpu, *v;
+	struct kvm_vcpu *vcpu;
 
 	if (id >= KVM_MAX_VCPUS)
 		return -EINVAL;
@@ -2281,12 +2281,10 @@ static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, u32 id)
 		r = -EINVAL;
 		goto unlock_vcpu_destroy;
 	}
-
-	kvm_for_each_vcpu(r, v, kvm)
-		if (v->vcpu_id == id) {
-			r = -EEXIST;
-			goto unlock_vcpu_destroy;
-		}
+	if (kvm_lookup_vcpu(kvm, id)) {
+		r = -EEXIST;
+		goto unlock_vcpu_destroy;
+	}
 
 	BUG_ON(kvm->vcpus[atomic_read(&kvm->online_vcpus)]);
 
-- 
2.3.0

Re: [PATCH 0/4] Preview: vcpu lookup by id changes

[Paolo Bonzini]

On 19/11/2015 09:37, Christian Borntraeger wrote:
> 
> some more patches for review before I add them in my next pull request.
> Can you review the common code changes and Ack/Nack?
> 
> Alex, Paul,
> can you review the power changes and Ack/Nack? 
> 
> As I want to have patch 2 in 4.4, I splitted up Davids patch into smaller
> chunks. David, can you double check that I did not mess up?
> So I want to send patch 1 and patch 2 for 4.4. Patch 3 and Patch 4
> are for kvm/next (4.5), but need patch 1 as well. 
> So I will probably also add all patches into kvm/next by setting up
> the git branches in a way that during 4.5 merge window kvm/next will
> have my branch that will go via kvm/master as common ancestor.
> 
> Ok?

Patches and plan both look good!

I can apply patch 1 and 2 now to kvm/master.  By the time kvm/next forks
from Linus's tree (sometime next week, since kvm/queue is already
largish) they will be in.

Thanks,

Re: [PATCH 0/4] Preview: vcpu lookup by id changes

[Christian Borntraeger]

On 11/19/2015 10:38 AM, Paolo Bonzini wrote:
> 
> 
> On 19/11/2015 09:37, Christian Borntraeger wrote:
>>
>> some more patches for review before I add them in my next pull request.
>> Can you review the common code changes and Ack/Nack?
>>
>> Alex, Paul,
>> can you review the power changes and Ack/Nack? 
>>
>> As I want to have patch 2 in 4.4, I splitted up Davids patch into smaller
>> chunks. David, can you double check that I did not mess up?
>> So I want to send patch 1 and patch 2 for 4.4. Patch 3 and Patch 4
>> are for kvm/next (4.5), but need patch 1 as well. 
>> So I will probably also add all patches into kvm/next by setting up
>> the git branches in a way that during 4.5 merge window kvm/next will
>> have my branch that will go via kvm/master as common ancestor.
>>
>> Ok?
> 
> Patches and plan both look good!
> 
> I can apply patch 1 and 2 now to kvm/master.  By the time kvm/next forks
> from Linus's tree (sometime next week, since kvm/queue is already
> largish) they will be in.

I have 2or 3 more patches for 4.4 and I will prepare a pull request today. So
either take them or wait for my request. Whatever works best for you.

Re: [PATCH 0/4] Preview: vcpu lookup by id changes

[Paolo Bonzini]

On 19/11/2015 10:51, Christian Borntraeger wrote:
> > I can apply patch 1 and 2 now to kvm/master.  By the time kvm/next forks
> > from Linus's tree (sometime next week, since kvm/queue is already
> > largish) they will be in.
> 
> I have 2or 3 more patches for 4.4 and I will prepare a pull request today. So
> either take them or wait for my request. Whatever works best for you.

I'll wait then.  ARM folks also have a couple patches for me. :)

Paolo