| Message ID | 20210118232208.11178-4-ioanna-maria.alifieraki@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix corruption on blocked_vcpu_on_cpu list | expand |
On 19.01.21 00:22, Ioanna Alifieraki wrote: > From: Paolo Bonzini <pbonzini@redhat.com> > > BugLink: https://bugs.launchpad.net/bugs/1908428 > > In some cases, for example involving hot-unplug of assigned > devices, pi_post_block can forget to remove the vCPU from the > blocked_vcpu_list. When this happens, the next call to > pi_pre_block corrupts the list. > > Fix this in two ways. First, check vcpu->pre_pcpu in pi_pre_block > and WARN instead of adding the element twice in the list. Second, > always do the list removal in pi_post_block if vcpu->pre_pcpu is > set (not -1). > > The new code keeps interrupts disabled for the whole duration of > pi_pre_block/pi_post_block. This is not strictly necessary, but > easier to follow. For the same reason, PI.ON is checked only > after the cmpxchg, and to handle it we just call the post-block > code. This removes duplication of the list removal code. > > Cc: Huangweidong <weidong.huang@huawei.com> > Cc: Gonglei <arei.gonglei@huawei.com> > Cc: wangxin <wangxinxin.wang@huawei.com> > Cc: Radim Krčmář <rkrcmar@redhat.com> > Tested-by: Longpeng (Mike) <longpeng2@huawei.com> > Cc: stable@vger.kernel.org > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > (backported from commit 8b306e2f3c41939ea528e6174c88cfbfff893ce1) > [hunk 5 : resolve if-statement conflict.] I think this was "adjusting for context change (cmpxchg)". Generally anything where a hunk fails because the lines above or below do not quite match but the lines added or removed are identical to the original patch I would refer to as some form of context adjustments. > Signed-off-by: Ioanna Alifieraki <ioanna-maria.alifieraki@canonical.com> > --- > arch/x86/kvm/vmx.c | 61 ++++++++++++++++++++++-------------------------------- > 1 file changed, 25 insertions(+), 36 deletions(-) > > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > index 93d922a..a9d677c 100644 > --- a/arch/x86/kvm/vmx.c > +++ b/arch/x86/kvm/vmx.c > @@ -11150,10 +11150,11 @@ static void __pi_post_block(struct kvm_vcpu *vcpu) > struct pi_desc *pi_desc = vcpu_to_pi_desc(vcpu); > struct pi_desc old, new; > unsigned int dest; > - unsigned long flags; > > do { > old.control = new.control = pi_desc->control; > + WARN(old.nv != POSTED_INTR_WAKEUP_VECTOR, > + "Wakeup handler not enabled while the VCPU is blocked\n"); > > dest = cpu_physical_id(vcpu->cpu); > > @@ -11170,14 +11171,10 @@ static void __pi_post_block(struct kvm_vcpu *vcpu) > } while (cmpxchg64(&pi_desc->control, old.control, > new.control) != old.control); > > - if(vcpu->pre_pcpu != -1) { > - spin_lock_irqsave( > - &per_cpu(blocked_vcpu_on_cpu_lock, > - vcpu->pre_pcpu), flags); > + if (!WARN_ON_ONCE(vcpu->pre_pcpu == -1)) { > + spin_lock(&per_cpu(blocked_vcpu_on_cpu_lock, vcpu->pre_pcpu)); > list_del(&vcpu->blocked_vcpu_list); > - spin_unlock_irqrestore( > - &per_cpu(blocked_vcpu_on_cpu_lock, > - vcpu->pre_pcpu), flags); > + spin_unlock(&per_cpu(blocked_vcpu_on_cpu_lock, vcpu->pre_pcpu)); > vcpu->pre_pcpu = -1; > } > } > @@ -11197,7 +11194,6 @@ static void __pi_post_block(struct kvm_vcpu *vcpu) > */ > static int pi_pre_block(struct kvm_vcpu *vcpu) > { > - unsigned long flags; > unsigned int dest; > struct pi_desc old, new; > struct pi_desc *pi_desc = vcpu_to_pi_desc(vcpu); > @@ -11206,34 +11202,20 @@ static int pi_pre_block(struct kvm_vcpu *vcpu) > !irq_remapping_cap(IRQ_POSTING_CAP)) > return 0; > > - vcpu->pre_pcpu = vcpu->cpu; > - spin_lock_irqsave(&per_cpu(blocked_vcpu_on_cpu_lock, > - vcpu->pre_pcpu), flags); > - list_add_tail(&vcpu->blocked_vcpu_list, > - &per_cpu(blocked_vcpu_on_cpu, > - vcpu->pre_pcpu)); > - spin_unlock_irqrestore(&per_cpu(blocked_vcpu_on_cpu_lock, > - vcpu->pre_pcpu), flags); > + WARN_ON(irqs_disabled()); > + local_irq_disable(); > + if (!WARN_ON_ONCE(vcpu->pre_pcpu != -1)) { > + vcpu->pre_pcpu = vcpu->cpu; > + spin_lock(&per_cpu(blocked_vcpu_on_cpu_lock, vcpu->pre_pcpu)); > + list_add_tail(&vcpu->blocked_vcpu_list, > + &per_cpu(blocked_vcpu_on_cpu, > + vcpu->pre_pcpu)); > + spin_unlock(&per_cpu(blocked_vcpu_on_cpu_lock, vcpu->pre_pcpu)); > + } > > do { > old.control = new.control = pi_desc->control; > > - /* > - * We should not block the vCPU if > - * an interrupt is posted for it. > - */ > - if (pi_test_on(pi_desc) == 1) { > - spin_lock_irqsave(&per_cpu(blocked_vcpu_on_cpu_lock, > - vcpu->pre_pcpu), flags); > - list_del(&vcpu->blocked_vcpu_list); > - spin_unlock_irqrestore( > - &per_cpu(blocked_vcpu_on_cpu_lock, > - vcpu->pre_pcpu), flags); > - vcpu->pre_pcpu = -1; > - > - return 1; > - } > - > WARN((pi_desc->sn == 1), > "Warning: SN field of posted-interrupts " > "is set before blocking\n"); > @@ -11258,7 +11240,12 @@ static int pi_pre_block(struct kvm_vcpu *vcpu) > } while (cmpxchg64(&pi_desc->control, old.control, > new.control) != old.control); > > - return 0; > + /* We should not block the vCPU if an interrupt is posted for it. */ > + if (pi_test_on(pi_desc) == 1) > + __pi_post_block(vcpu); > + > + local_irq_enable(); > + return (vcpu->pre_pcpu == -1); > } > > static int vmx_pre_block(struct kvm_vcpu *vcpu) > @@ -11271,11 +11258,13 @@ static int vmx_pre_block(struct kvm_vcpu *vcpu) > > static void pi_post_block(struct kvm_vcpu *vcpu) > { > - if (!kvm_arch_has_assigned_device(vcpu->kvm) || > - !irq_remapping_cap(IRQ_POSTING_CAP)) > + if (vcpu->pre_pcpu == -1) > return; > > + WARN_ON(irqs_disabled()); > + local_irq_disable(); > __pi_post_block(vcpu); > + local_irq_enable(); > } > > static void vmx_post_block(struct kvm_vcpu *vcpu) >
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index 93d922a..a9d677c 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -11150,10 +11150,11 @@ static void __pi_post_block(struct kvm_vcpu *vcpu) struct pi_desc *pi_desc = vcpu_to_pi_desc(vcpu); struct pi_desc old, new; unsigned int dest; - unsigned long flags; do { old.control = new.control = pi_desc->control; + WARN(old.nv != POSTED_INTR_WAKEUP_VECTOR, + "Wakeup handler not enabled while the VCPU is blocked\n"); dest = cpu_physical_id(vcpu->cpu); @@ -11170,14 +11171,10 @@ static void __pi_post_block(struct kvm_vcpu *vcpu) } while (cmpxchg64(&pi_desc->control, old.control, new.control) != old.control); - if(vcpu->pre_pcpu != -1) { - spin_lock_irqsave( - &per_cpu(blocked_vcpu_on_cpu_lock, - vcpu->pre_pcpu), flags); + if (!WARN_ON_ONCE(vcpu->pre_pcpu == -1)) { + spin_lock(&per_cpu(blocked_vcpu_on_cpu_lock, vcpu->pre_pcpu)); list_del(&vcpu->blocked_vcpu_list); - spin_unlock_irqrestore( - &per_cpu(blocked_vcpu_on_cpu_lock, - vcpu->pre_pcpu), flags); + spin_unlock(&per_cpu(blocked_vcpu_on_cpu_lock, vcpu->pre_pcpu)); vcpu->pre_pcpu = -1; } } @@ -11197,7 +11194,6 @@ static void __pi_post_block(struct kvm_vcpu *vcpu) */ static int pi_pre_block(struct kvm_vcpu *vcpu) { - unsigned long flags; unsigned int dest; struct pi_desc old, new; struct pi_desc *pi_desc = vcpu_to_pi_desc(vcpu); @@ -11206,34 +11202,20 @@ static int pi_pre_block(struct kvm_vcpu *vcpu) !irq_remapping_cap(IRQ_POSTING_CAP)) return 0; - vcpu->pre_pcpu = vcpu->cpu; - spin_lock_irqsave(&per_cpu(blocked_vcpu_on_cpu_lock, - vcpu->pre_pcpu), flags); - list_add_tail(&vcpu->blocked_vcpu_list, - &per_cpu(blocked_vcpu_on_cpu, - vcpu->pre_pcpu)); - spin_unlock_irqrestore(&per_cpu(blocked_vcpu_on_cpu_lock, - vcpu->pre_pcpu), flags); + WARN_ON(irqs_disabled()); + local_irq_disable(); + if (!WARN_ON_ONCE(vcpu->pre_pcpu != -1)) { + vcpu->pre_pcpu = vcpu->cpu; + spin_lock(&per_cpu(blocked_vcpu_on_cpu_lock, vcpu->pre_pcpu)); + list_add_tail(&vcpu->blocked_vcpu_list, + &per_cpu(blocked_vcpu_on_cpu, + vcpu->pre_pcpu)); + spin_unlock(&per_cpu(blocked_vcpu_on_cpu_lock, vcpu->pre_pcpu)); + } do { old.control = new.control = pi_desc->control; - /* - * We should not block the vCPU if - * an interrupt is posted for it. - */ - if (pi_test_on(pi_desc) == 1) { - spin_lock_irqsave(&per_cpu(blocked_vcpu_on_cpu_lock, - vcpu->pre_pcpu), flags); - list_del(&vcpu->blocked_vcpu_list); - spin_unlock_irqrestore( - &per_cpu(blocked_vcpu_on_cpu_lock, - vcpu->pre_pcpu), flags); - vcpu->pre_pcpu = -1; - - return 1; - } - WARN((pi_desc->sn == 1), "Warning: SN field of posted-interrupts " "is set before blocking\n"); @@ -11258,7 +11240,12 @@ static int pi_pre_block(struct kvm_vcpu *vcpu) } while (cmpxchg64(&pi_desc->control, old.control, new.control) != old.control); - return 0; + /* We should not block the vCPU if an interrupt is posted for it. */ + if (pi_test_on(pi_desc) == 1) + __pi_post_block(vcpu); + + local_irq_enable(); + return (vcpu->pre_pcpu == -1); } static int vmx_pre_block(struct kvm_vcpu *vcpu) @@ -11271,11 +11258,13 @@ static int vmx_pre_block(struct kvm_vcpu *vcpu) static void pi_post_block(struct kvm_vcpu *vcpu) { - if (!kvm_arch_has_assigned_device(vcpu->kvm) || - !irq_remapping_cap(IRQ_POSTING_CAP)) + if (vcpu->pre_pcpu == -1) return; + WARN_ON(irqs_disabled()); + local_irq_disable(); __pi_post_block(vcpu); + local_irq_enable(); } static void vmx_post_block(struct kvm_vcpu *vcpu)