| Message ID | CANRm+CwC=duTn7oPykbE+ixnLUBQwt8KMMHkCQWZOS9i2Mzz0A@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> Thanks, this is beautiful enough. :) >>> >>> Hmm, the combination of 6c7caebc26c5 ("KVM: introduce >>> kvm->created_vcpus", 2016-06-16) and 4c5ea0a9cd02 ("locking/static_key: >>> Fix concurrent static_key_slow_inc()", 2016-06-24) should have fixed it >>> for good. >>> >>> Is the ENABLE_CAP necessary to reproduce? Then, the bug is simply that >>> the ENABLE_CAP should have failed without an irqchip (the >>> KVM_CREATE_IRQCHIP in turn must have failed with EINVAL). >> >> ENABLE_CAP is necessary to reproduce. > > Now I see what Paolo means, how about something like below: > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 51ccfe0..7ec22e2 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -3337,7 +3337,10 @@ static int kvm_vcpu_ioctl_enable_cap(struct > kvm_vcpu *vcpu, > > switch (cap->cap) { > case KVM_CAP_HYPERV_SYNIC: > - return kvm_hv_activate_synic(vcpu); > + if (!irqchip_in_kernel(vcpu->kvm)) > + return -EINVAL; > + else You can simply drop the else and return directly. Can't really say if this is the right fix, my first thought was that a request has been set although it should never have been set for that VCPU. Maybe that is an effect of synic being activated (because synic code unconditionally later on sets the request). Fixing the cause of the request seems better than fixing up the result.
On 03/01/2017 13:06, David Hildenbrand wrote: >> >> switch (cap->cap) { >> case KVM_CAP_HYPERV_SYNIC: >> - return kvm_hv_activate_synic(vcpu); >> + if (!irqchip_in_kernel(vcpu->kvm)) >> + return -EINVAL; >> + else > > You can simply drop the else and return directly. > > Can't really say if this is the right fix, my first thought was that > a request has been set although it should never have been set for > that VCPU. Maybe that is an effect of synic being activated > (because synic code unconditionally later on sets the request). > > Fixing the cause of the request seems better than fixing up the result. Yes, I agree. Wanpeng's second patch is fine. Paolo -- 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
2017-01-04 1:23 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>: > > > On 03/01/2017 13:06, David Hildenbrand wrote: >>> >>> switch (cap->cap) { >>> case KVM_CAP_HYPERV_SYNIC: >>> - return kvm_hv_activate_synic(vcpu); >>> + if (!irqchip_in_kernel(vcpu->kvm)) >>> + return -EINVAL; >>> + else >> >> You can simply drop the else and return directly. >> >> Can't really say if this is the right fix, my first thought was that >> a request has been set although it should never have been set for >> that VCPU. Maybe that is an effect of synic being activated >> (because synic code unconditionally later on sets the request). >> >> Fixing the cause of the request seems better than fixing up the result. > > Yes, I agree. Wanpeng's second patch is fine. Thanks Paolo, I will send out a formal one soon. Regards, Wanpeng Li -- 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
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 51ccfe0..7ec22e2 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -3337,7 +3337,10 @@ static int kvm_vcpu_ioctl_enable_cap(struct kvm_vcpu *vcpu, switch (cap->cap) { case KVM_CAP_HYPERV_SYNIC: - return kvm_hv_activate_synic(vcpu); + if (!irqchip_in_kernel(vcpu->kvm)) + return -EINVAL; + else + return kvm_hv_activate_synic(vcpu); default: return -EINVAL;