| Message ID | 20090205170501.2a964172@cotte.boeblingen.de.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Am Thu, 5 Feb 2009 17:05:01 +0100 schrieb Carsten Otte <cotte@de.ibm.com>: > KVM common code should'nt try to create the same virtual cpu twice. > In case of s390, it crashes badly in kvm_arch_vcpu_create. This patch is broken, will refresh it. Please do _NOT_ apply (!) -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Feb 05, 2009 at 05:05:01PM +0100, Carsten Otte wrote: > KVM common code should'nt try to create the same virtual cpu twice. > In case of s390, it crashes badly in kvm_arch_vcpu_create. > > Reported-by: Mijo Safradin <mijo@linux.vnet.ibm.com> > Signed-off-by: Carsten Otte <cotte@de.ibm.com> > --- > virt/kvm/kvm_main.c | 3 +++ > 1 file changed, 3 insertions(+) > > Index: kvm/virt/kvm/kvm_main.c > =================================================================== > --- kvm.orig/virt/kvm/kvm_main.c > +++ kvm/virt/kvm/kvm_main.c > @@ -1605,6 +1605,9 @@ static int kvm_vm_ioctl_create_vcpu(stru > if (!valid_vcpu(n)) > return -EINVAL; > > + if (kvm->vcpus[i]) > + return -EEXIST; > + > vcpu = kvm_arch_vcpu_create(kvm, n); > if (IS_ERR(vcpu)) > return PTR_ERR(vcpu); Its confusing that there is the exact same check below, with kvm->lock held, and that both are needed since assignment happens under the lock. Can you also make it straightforward while fixing the bug please. Probably just hold it all the way through kvm_vm_ioctl_create_vcpu? Or is that not possible? -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Marcelo Tosatti wrote: > On Thu, Feb 05, 2009 at 05:05:01PM +0100, Carsten Otte wrote: > >> KVM common code should'nt try to create the same virtual cpu twice. >> In case of s390, it crashes badly in kvm_arch_vcpu_create. >> >> Reported-by: Mijo Safradin <mijo@linux.vnet.ibm.com> >> Signed-off-by: Carsten Otte <cotte@de.ibm.com> >> --- >> virt/kvm/kvm_main.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> Index: kvm/virt/kvm/kvm_main.c >> =================================================================== >> --- kvm.orig/virt/kvm/kvm_main.c >> +++ kvm/virt/kvm/kvm_main.c >> @@ -1605,6 +1605,9 @@ static int kvm_vm_ioctl_create_vcpu(stru >> if (!valid_vcpu(n)) >> return -EINVAL; >> >> + if (kvm->vcpus[i]) >> + return -EEXIST; >> + >> vcpu = kvm_arch_vcpu_create(kvm, n); >> if (IS_ERR(vcpu)) >> return PTR_ERR(vcpu); >> > > Its confusing that there is the exact same check below, with kvm->lock > held, and that both are needed since assignment happens under the lock. > Right, also the proposed fix still leaves a race. > Can you also make it straightforward while fixing the bug please. > > Probably just hold it all the way through kvm_vm_ioctl_create_vcpu? Or > is that not possible? > The original intent was that kvm_arch_vcpu_create() not "link in" the vcpu to any registers. That allows most of the vcpu creation to happen outside a lock. If it's not doable for s390 we can give this up, but I suggest checking if it's possible to keep things as is and modify s390's kvm_arch_vcpu_create() not to screw up instead.
Am Sun, 8 Feb 2009 04:26:16 -0200 schrieb Marcelo Tosatti <mtosatti@redhat.com>: > Its confusing that there is the exact same check below, with kvm->lock > held, and that both are needed since assignment happens under the lock. > > Can you also make it straightforward while fixing the bug please. > > Probably just hold it all the way through kvm_vm_ioctl_create_vcpu? Or > is that not possible? Okay, let me see what I can do to come up with a better fix. On s390, we have to link our sie control blocks (representing vcpu for the hardware) with our sca (representing virtual machine for the hardware). These are bidirectional pointers. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Index: kvm/virt/kvm/kvm_main.c =================================================================== --- kvm.orig/virt/kvm/kvm_main.c +++ kvm/virt/kvm/kvm_main.c @@ -1605,6 +1605,9 @@ static int kvm_vm_ioctl_create_vcpu(stru if (!valid_vcpu(n)) return -EINVAL; + if (kvm->vcpus[i]) + return -EEXIST; + vcpu = kvm_arch_vcpu_create(kvm, n); if (IS_ERR(vcpu)) return PTR_ERR(vcpu);
KVM common code should'nt try to create the same virtual cpu twice. In case of s390, it crashes badly in kvm_arch_vcpu_create. Reported-by: Mijo Safradin <mijo@linux.vnet.ibm.com> Signed-off-by: Carsten Otte <cotte@de.ibm.com> --- virt/kvm/kvm_main.c | 3 +++ 1 file changed, 3 insertions(+) -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html