Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1420833 > unrolled thread
| Started by | Paolo Bonzini <pbonzini@redhat.com> |
|---|---|
| First post | 2016-06-13 15:30 +0200 |
| Last post | 2016-06-16 10:10 +0200 |
| Articles | 8 — 3 participants |
Back to article view | Back to linux.kernel
[PATCH 0/3] KVM: do not use kvm->online_vcpus to check "has one VCPU been created?" Paolo Bonzini <pbonzini@redhat.com> - 2016-06-13 15:30 +0200
Re: [PATCH 0/3] KVM: do not use kvm->online_vcpus to check "has one VCPU been created?" Christian Borntraeger <borntraeger@de.ibm.com> - 2016-06-13 15:40 +0200
Re: [PATCH 0/3] KVM: do not use kvm->online_vcpus to check "has one VCPU been created?" Cornelia Huck <cornelia.huck@de.ibm.com> - 2016-06-13 16:50 +0200
Re: [PATCH 0/3] KVM: do not use kvm->online_vcpus to check "has one VCPU been created?" Paolo Bonzini <pbonzini@redhat.com> - 2016-06-16 00:10 +0200
Re: [PATCH 0/3] KVM: do not use kvm->online_vcpus to check "has one VCPU been created?" Christian Borntraeger <borntraeger@de.ibm.com> - 2016-06-16 09:40 +0200
Re: [PATCH 0/3] KVM: do not use kvm->online_vcpus to check "has one VCPU been created?" Cornelia Huck <cornelia.huck@de.ibm.com> - 2016-06-16 10:00 +0200
Re: [PATCH 0/3] KVM: do not use kvm->online_vcpus to check "has one VCPU been created?" Paolo Bonzini <pbonzini@redhat.com> - 2016-06-16 10:30 +0200
Re: [PATCH 0/3] KVM: do not use kvm->online_vcpus to check "has one VCPU been created?" Paolo Bonzini <pbonzini@redhat.com> - 2016-06-16 10:10 +0200
| From | Paolo Bonzini <pbonzini@redhat.com> |
|---|---|
| Date | 2016-06-13 15:30 +0200 |
| Subject | [PATCH 0/3] KVM: do not use kvm->online_vcpus to check "has one VCPU been created?" |
| Message-ID | <rJA6d-6PL-13@gated-at.bofh.it> |
kvm->online_vcpus is only updated at the very end of KVM_CREATE_VCPU. Thus, it can be racy to use it as a check for "has any VCPU been created already?". While x86 detects the race through kvm_vcpu_compatible, it is kinda hackish and s390 does not do anything similar. Provide a better fix for both. Paolo Bonzini (3): KVM: introduce created_vcpus KVM: remove kvm_vcpu_compatible KVM: s390: use created_vcpus arch/s390/kvm/kvm-s390.c | 10 +++++----- arch/x86/kvm/Kconfig | 1 - arch/x86/kvm/x86.c | 11 +++-------- include/linux/kvm_host.h | 14 ++++++++------ virt/kvm/Kconfig | 3 --- virt/kvm/kvm_main.c | 27 +++++++++++++++++---------- 6 files changed, 33 insertions(+), 33 deletions(-) -- 1.8.3.1
[toc] | [next] | [standalone]
| From | Christian Borntraeger <borntraeger@de.ibm.com> |
|---|---|
| Date | 2016-06-13 15:40 +0200 |
| Subject | Re: [PATCH 0/3] KVM: do not use kvm->online_vcpus to check "has one VCPU been created?" |
| Message-ID | <rJAfU-6Tq-39@gated-at.bofh.it> |
| In reply to | #1420833 |
On 06/13/2016 03:25 PM, Paolo Bonzini wrote: > kvm->online_vcpus is only updated at the very end of KVM_CREATE_VCPU. > Thus, it can be racy to use it as a check for "has any VCPU been created > already?". While x86 detects the race through kvm_vcpu_compatible, > it is kinda hackish and s390 does not do anything similar. Provide a > better fix for both. > > Paolo Bonzini (3): > KVM: introduce created_vcpus > KVM: remove kvm_vcpu_compatible > KVM: s390: use created_vcpus > > arch/s390/kvm/kvm-s390.c | 10 +++++----- > arch/x86/kvm/Kconfig | 1 - > arch/x86/kvm/x86.c | 11 +++-------- > include/linux/kvm_host.h | 14 ++++++++------ > virt/kvm/Kconfig | 3 --- > virt/kvm/kvm_main.c | 27 +++++++++++++++++---------- > 6 files changed, 33 insertions(+), 33 deletions(-) > From a quick look this looks "the right thing". Will do a proper review later this day or tomorrow.
[toc] | [prev] | [next] | [standalone]
| From | Cornelia Huck <cornelia.huck@de.ibm.com> |
|---|---|
| Date | 2016-06-13 16:50 +0200 |
| Subject | Re: [PATCH 0/3] KVM: do not use kvm->online_vcpus to check "has one VCPU been created?" |
| Message-ID | <rJBlE-7AX-23@gated-at.bofh.it> |
| In reply to | #1420833 |
On Mon, 13 Jun 2016 15:25:29 +0200 Paolo Bonzini <pbonzini@redhat.com> wrote: > kvm->online_vcpus is only updated at the very end of KVM_CREATE_VCPU. > Thus, it can be racy to use it as a check for "has any VCPU been created > already?". While x86 detects the race through kvm_vcpu_compatible, > it is kinda hackish and s390 does not do anything similar. Provide a > better fix for both. > > Paolo Bonzini (3): > KVM: introduce created_vcpus > KVM: remove kvm_vcpu_compatible > KVM: s390: use created_vcpus > > arch/s390/kvm/kvm-s390.c | 10 +++++----- > arch/x86/kvm/Kconfig | 1 - > arch/x86/kvm/x86.c | 11 +++-------- > include/linux/kvm_host.h | 14 ++++++++------ > virt/kvm/Kconfig | 3 --- > virt/kvm/kvm_main.c | 27 +++++++++++++++++---------- > 6 files changed, 33 insertions(+), 33 deletions(-) > Looks like a sane approach, only two inversions in the s390 patch :)
[toc] | [prev] | [next] | [standalone]
| From | Paolo Bonzini <pbonzini@redhat.com> |
|---|---|
| Date | 2016-06-16 00:10 +0200 |
| Subject | Re: [PATCH 0/3] KVM: do not use kvm->online_vcpus to check "has one VCPU been created?" |
| Message-ID | <rKray-8lC-5@gated-at.bofh.it> |
| In reply to | #1420947 |
On 13/06/2016 16:44, Cornelia Huck wrote: >> > arch/s390/kvm/kvm-s390.c | 10 +++++----- >> > arch/x86/kvm/Kconfig | 1 - >> > arch/x86/kvm/x86.c | 11 +++-------- >> > include/linux/kvm_host.h | 14 ++++++++------ >> > virt/kvm/Kconfig | 3 --- >> > virt/kvm/kvm_main.c | 27 +++++++++++++++++---------- >> > 6 files changed, 33 insertions(+), 33 deletions(-) >> > > Looks like a sane approach, only two inversions in the s390 patch :) So it's okay to push patch 3 to kvm/next? Thanks, Paolo
[toc] | [prev] | [next] | [standalone]
| From | Christian Borntraeger <borntraeger@de.ibm.com> |
|---|---|
| Date | 2016-06-16 09:40 +0200 |
| Subject | Re: [PATCH 0/3] KVM: do not use kvm->online_vcpus to check "has one VCPU been created?" |
| Message-ID | <rKA49-5o9-13@gated-at.bofh.it> |
| In reply to | #1423512 |
On 06/16/2016 12:00 AM, Paolo Bonzini wrote: > > > On 13/06/2016 16:44, Cornelia Huck wrote: >>>> arch/s390/kvm/kvm-s390.c | 10 +++++----- >>>> arch/x86/kvm/Kconfig | 1 - >>>> arch/x86/kvm/x86.c | 11 +++-------- >>>> include/linux/kvm_host.h | 14 ++++++++------ >>>> virt/kvm/Kconfig | 3 --- >>>> virt/kvm/kvm_main.c | 27 +++++++++++++++++---------- >>>> 6 files changed, 33 insertions(+), 33 deletions(-) >>>> >> Looks like a sane approach, only two inversions in the s390 patch :) > > So it's okay to push patch 3 to kvm/next? With the 2 fixes that Conny requested, yes. With that fixed up, Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
[toc] | [prev] | [next] | [standalone]
| From | Cornelia Huck <cornelia.huck@de.ibm.com> |
|---|---|
| Date | 2016-06-16 10:00 +0200 |
| Subject | Re: [PATCH 0/3] KVM: do not use kvm->online_vcpus to check "has one VCPU been created?" |
| Message-ID | <rKAnw-5uL-31@gated-at.bofh.it> |
| In reply to | #1423751 |
On Thu, 16 Jun 2016 09:30:34 +0200 Christian Borntraeger <borntraeger@de.ibm.com> wrote: > On 06/16/2016 12:00 AM, Paolo Bonzini wrote: > > > > > > On 13/06/2016 16:44, Cornelia Huck wrote: > >>>> arch/s390/kvm/kvm-s390.c | 10 +++++----- > >>>> arch/x86/kvm/Kconfig | 1 - > >>>> arch/x86/kvm/x86.c | 11 +++-------- > >>>> include/linux/kvm_host.h | 14 ++++++++------ > >>>> virt/kvm/Kconfig | 3 --- > >>>> virt/kvm/kvm_main.c | 27 +++++++++++++++++---------- > >>>> 6 files changed, 33 insertions(+), 33 deletions(-) > >>>> > >> Looks like a sane approach, only two inversions in the s390 patch :) > > > > So it's okay to push patch 3 to kvm/next? > > > With the 2 fixes that Conny requested, yes. I had been waiting for a v2 ;) > > With that fixed up, > Acked-by: Christian Borntraeger <borntraeger@de.ibm.com> > In that case, you can also add my Acked-by: Cornelia Huck <cornelia.huck@de.ibm.com> to the s390 patch.
[toc] | [prev] | [next] | [standalone]
| From | Paolo Bonzini <pbonzini@redhat.com> |
|---|---|
| Date | 2016-06-16 10:30 +0200 |
| Subject | Re: [PATCH 0/3] KVM: do not use kvm->online_vcpus to check "has one VCPU been created?" |
| Message-ID | <rKAQx-5Ue-5@gated-at.bofh.it> |
| In reply to | #1423766 |
On 16/06/2016 09:59, Cornelia Huck wrote:
> On Thu, 16 Jun 2016 09:30:34 +0200
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
>
>> On 06/16/2016 12:00 AM, Paolo Bonzini wrote:
>>>
>>>
>>> On 13/06/2016 16:44, Cornelia Huck wrote:
>>>>>> arch/s390/kvm/kvm-s390.c | 10 +++++-----
>>>>>> arch/x86/kvm/Kconfig | 1 -
>>>>>> arch/x86/kvm/x86.c | 11 +++--------
>>>>>> include/linux/kvm_host.h | 14 ++++++++------
>>>>>> virt/kvm/Kconfig | 3 ---
>>>>>> virt/kvm/kvm_main.c | 27 +++++++++++++++++----------
>>>>>> 6 files changed, 33 insertions(+), 33 deletions(-)
>>>>>>
>>>> Looks like a sane approach, only two inversions in the s390 patch :)
>>>
>>> So it's okay to push patch 3 to kvm/next?
>>
>>
>> With the 2 fixes that Conny requested, yes.
>
> I had been waiting for a v2 ;)
This is the patch I've pushed:
-------------------- 8< ---------------------
From a03825bbd0c39feeba605912cdbc28e79e4e01e1 Mon Sep 17 00:00:00 2001
From: Paolo Bonzini <pbonzini@redhat.com>
Date: Mon, 13 Jun 2016 14:50:04 +0200
Subject: [PATCH] KVM: s390: use kvm->created_vcpus
The new created_vcpus field avoids possible races between enabling
capabilities and creating VCPUs.
Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 49c60393a15c..0dcf9b8fc12c 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -422,7 +422,7 @@ static int kvm_vm_ioctl_enable_cap(struct kvm *kvm, struct kvm_enable_cap *cap)
break;
case KVM_CAP_S390_VECTOR_REGISTERS:
mutex_lock(&kvm->lock);
- if (atomic_read(&kvm->online_vcpus)) {
+ if (kvm->created_vcpus) {
r = -EBUSY;
} else if (MACHINE_HAS_VX) {
set_kvm_facility(kvm->arch.model.fac_mask, 129);
@@ -437,7 +437,7 @@ static int kvm_vm_ioctl_enable_cap(struct kvm *kvm, struct kvm_enable_cap *cap)
case KVM_CAP_S390_RI:
r = -EINVAL;
mutex_lock(&kvm->lock);
- if (atomic_read(&kvm->online_vcpus)) {
+ if (kvm->created_vcpus) {
r = -EBUSY;
} else if (test_facility(64)) {
set_kvm_facility(kvm->arch.model.fac_mask, 64);
@@ -492,7 +492,7 @@ static int kvm_s390_set_mem_control(struct kvm *kvm, struct kvm_device_attr *att
ret = -EBUSY;
VM_EVENT(kvm, 3, "%s", "ENABLE: CMMA support");
mutex_lock(&kvm->lock);
- if (atomic_read(&kvm->online_vcpus) == 0) {
+ if (!kvm->created_vcpus) {
kvm->arch.use_cmma = 1;
ret = 0;
}
@@ -536,7 +536,7 @@ static int kvm_s390_set_mem_control(struct kvm *kvm, struct kvm_device_attr *att
ret = -EBUSY;
mutex_lock(&kvm->lock);
- if (atomic_read(&kvm->online_vcpus) == 0) {
+ if (!kvm->created_vcpus) {
/* gmap_alloc will round the limit up */
struct gmap *new = gmap_alloc(current->mm, new_limit);
@@ -713,7 +713,7 @@ static int kvm_s390_set_processor(struct kvm *kvm, struct kvm_device_attr *attr)
int ret = 0;
mutex_lock(&kvm->lock);
- if (atomic_read(&kvm->online_vcpus)) {
+ if (kvm->created_vcpus) {
ret = -EBUSY;
goto out;
}
[toc] | [prev] | [next] | [standalone]
| From | Paolo Bonzini <pbonzini@redhat.com> |
|---|---|
| Date | 2016-06-16 10:10 +0200 |
| Subject | Re: [PATCH 0/3] KVM: do not use kvm->online_vcpus to check "has one VCPU been created?" |
| Message-ID | <rKAxc-5MU-17@gated-at.bofh.it> |
| In reply to | #1423751 |
On 16/06/2016 09:30, Christian Borntraeger wrote: > On 06/16/2016 12:00 AM, Paolo Bonzini wrote: >> >> >> On 13/06/2016 16:44, Cornelia Huck wrote: >>>>> arch/s390/kvm/kvm-s390.c | 10 +++++----- >>>>> arch/x86/kvm/Kconfig | 1 - >>>>> arch/x86/kvm/x86.c | 11 +++-------- >>>>> include/linux/kvm_host.h | 14 ++++++++------ >>>>> virt/kvm/Kconfig | 3 --- >>>>> virt/kvm/kvm_main.c | 27 +++++++++++++++++---------- >>>>> 6 files changed, 33 insertions(+), 33 deletions(-) >>>>> >>> Looks like a sane approach, only two inversions in the s390 patch :) >> >> So it's okay to push patch 3 to kvm/next? > > > With the 2 fixes that Conny requested, yes. > > With that fixed up, > Acked-by: Christian Borntraeger <borntraeger@de.ibm.com> Yes, of course. Pushed, thanks! Paolo
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web