Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1658613 > unrolled thread

[PATCH CFT 0/4] VT-d PI fixes

Back to article view | Back to linux.kernel

Contents

  [PATCH CFT 0/4] VT-d PI fixes Paolo Bonzini <pbonzini@redhat.com> - 2017-06-06 13:00 +0200
    [PATCH 2/4] KVM: VMX: avoid double list add with VT-d posted interrupts Paolo Bonzini <pbonzini@redhat.com> - 2017-06-06 13:00 +0200
      Re: [PATCH 2/4] KVM: VMX: avoid double list add with VT-d posted  interrupts Paolo Bonzini <pbonzini@redhat.com> - 2017-06-06 14:40 +0200
        Re: [PATCH 2/4] KVM: VMX: avoid double list add with VT-d posted  interrupts "Longpeng (Mike)" <longpeng2@huawei.com> - 2017-06-06 14:50 +0200
      Re: [PATCH 2/4] KVM: VMX: avoid double list add with VT-d posted  interrupts "Longpeng (Mike)" <longpeng2@huawei.com> - 2017-06-06 14:40 +0200
    [PATCH 3/4] KVM: VMX: simplify and fix vmx_vcpu_pi_load Paolo Bonzini <pbonzini@redhat.com> - 2017-06-06 13:00 +0200
    RE: [PATCH CFT 0/4] VT-d PI fixes "Gonglei (Arei)" <arei.gonglei@huawei.com> - 2017-06-07 11:40 +0200
      Re: [PATCH CFT 0/4] VT-d PI fixes Paolo Bonzini <pbonzini@redhat.com> - 2017-06-07 16:40 +0200

#1658613 — [PATCH CFT 0/4] VT-d PI fixes

These should fix, or at least help, the kernel panic reported by Longpeng
with VT-d posted interrupts.

CONFIG_DEBUG_LIST reports a double add, meaning that pi_pre_block ran twice
without pi_post_block deleting the vCPU from the blocked_on_vcpu list.
The only possibility that I could think of is that this:

        if (!kvm_arch_has_assigned_device(vcpu->kvm) ||
                !irq_remapping_cap(IRQ_POSTING_CAP)  ||
                !kvm_vcpu_apicv_active(vcpu))
                return;

was false in pi_post_block.  In turn, I can only think of hot-unplug as
the cause of this imbalance, but maybe there is another way to reach it
just via repeated startup and shutdown.  Gonglei reported problems with
hot-unplug offlist too, so this is a start.

In any case, patch 2 replaces it with a check on vcpu->pre_pcpu.
A similar change is done in patch 3 to vmx_vcpu_pi_load.  I don't
have hardware easily accessible with VT-d PI, so these patches are
compile-tested only.  I apologize for any stupid mistakes.

The first three patches are meant for stable versions too.

Paolo

Paolo Bonzini (4):
  KVM: VMX: extract __pi_post_block
  KVM: VMX: avoid double list add with VT-d posted interrupts
  KVM: VMX: simplify and fix vmx_vcpu_pi_load
  KVM: VMX: simplify cmpxchg of PI descriptor control field

 arch/x86/kvm/vmx.c | 228 ++++++++++++++++++++++++++---------------------------
 1 file changed, 110 insertions(+), 118 deletions(-)

-- 
2.13.0

[toc] | [next] | [standalone]

#1658616 — [PATCH 2/4] KVM: VMX: avoid double list add with VT-d posted interrupts

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: Longpeng (Mike) <longpeng2@huawei.com>
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>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/vmx.c | 62 ++++++++++++++++++++++--------------------------------
 1 file changed, 25 insertions(+), 37 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 747d16525b45..0f4714fe4908 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -11236,10 +11236,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);
 
@@ -11256,14 +11257,10 @@ static void __pi_post_block(struct kvm_vcpu *vcpu)
 	} while (cmpxchg(&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;
 	}
 }
@@ -11283,7 +11280,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);
@@ -11293,34 +11289,20 @@ static int pi_pre_block(struct kvm_vcpu *vcpu)
 		!kvm_vcpu_apicv_active(vcpu))
 		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");
@@ -11345,7 +11327,12 @@ static int pi_pre_block(struct kvm_vcpu *vcpu)
 	} while (cmpxchg(&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)
@@ -11361,12 +11348,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)  ||
-		!kvm_vcpu_apicv_active(vcpu))
+	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)
-- 
2.13.0

[toc] | [prev] | [next] | [standalone]

#1658733 — Re: [PATCH 2/4] KVM: VMX: avoid double list add with VT-d posted interrupts

On 06/06/2017 14:30, Longpeng (Mike) wrote:
> 
> 
> On 2017/6/6 18:57, Paolo Bonzini wrote:
> 
>> 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: Longpeng (Mike) <longpeng2@huawei.com>
>> 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>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  arch/x86/kvm/vmx.c | 62 ++++++++++++++++++++++--------------------------------
>>  1 file changed, 25 insertions(+), 37 deletions(-)
>>
> 
> 
> [...]
> 
> 
>> @@ -11256,14 +11257,10 @@ static void __pi_post_block(struct kvm_vcpu *vcpu)
>>  	} while (cmpxchg(&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));
> 
> 
> Hi Paolo,
> 
> spin_lock_irqsave() will disable kernel preempt, but spin_lock() won't. is there
> some potential problems ?

Hi,

This function (and pi_pre_block too's part where it takes the spin lock)
runs with interrupts disabled now.

Thanks,

Paolo

> Regards,
> Longpeng(Mike)
> 
>>  		vcpu->pre_pcpu = -1;
>>  	}
>>  }
>> @@ -11283,7 +11280,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);
>> @@ -11293,34 +11289,20 @@ static int pi_pre_block(struct kvm_vcpu *vcpu)
>>  		!kvm_vcpu_apicv_active(vcpu))
>>  		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");
>> @@ -11345,7 +11327,12 @@ static int pi_pre_block(struct kvm_vcpu *vcpu)
>>  	} while (cmpxchg(&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)
>> @@ -11361,12 +11348,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)  ||
>> -		!kvm_vcpu_apicv_active(vcpu))
>> +	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)
> 
> 

[toc] | [prev] | [next] | [standalone]

#1658743 — Re: [PATCH 2/4] KVM: VMX: avoid double list add with VT-d posted interrupts

On 2017/6/6 20:35, Paolo Bonzini wrote:

> 
> 
> On 06/06/2017 14:30, Longpeng (Mike) wrote:
>>
>>
>> On 2017/6/6 18:57, Paolo Bonzini wrote:
>>
>>> 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: Longpeng (Mike) <longpeng2@huawei.com>
>>> 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>
>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>> ---
>>>  arch/x86/kvm/vmx.c | 62 ++++++++++++++++++++++--------------------------------
>>>  1 file changed, 25 insertions(+), 37 deletions(-)
>>>
>>
>>
>> [...]
>>
>>
>>> @@ -11256,14 +11257,10 @@ static void __pi_post_block(struct kvm_vcpu *vcpu)
>>>  	} while (cmpxchg(&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));
>>
>>
>> Hi Paolo,
>>
>> spin_lock_irqsave() will disable kernel preempt, but spin_lock() won't. is there
>> some potential problems ?
> 
> Hi,
> 
> This function (and pi_pre_block too's part where it takes the spin lock)
> runs with interrupts disabled now.
> 


Oh, yes, please forgive my foolish.

We'll continue to find why the list is corrupt when repeat poweron/shutdown

Thanks.

> Thanks,
> 
> Paolo
> 
>> Regards,
>> Longpeng(Mike)
>>
>>>  		vcpu->pre_pcpu = -1;
>>>  	}
>>>  }
>>> @@ -11283,7 +11280,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);
>>> @@ -11293,34 +11289,20 @@ static int pi_pre_block(struct kvm_vcpu *vcpu)
>>>  		!kvm_vcpu_apicv_active(vcpu))
>>>  		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");
>>> @@ -11345,7 +11327,12 @@ static int pi_pre_block(struct kvm_vcpu *vcpu)
>>>  	} while (cmpxchg(&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)
>>> @@ -11361,12 +11348,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)  ||
>>> -		!kvm_vcpu_apicv_active(vcpu))
>>> +	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)
>>
>>
> 
> .
> 


-- 
Regards,
Longpeng(Mike)

[toc] | [prev] | [next] | [standalone]

#1658740 — Re: [PATCH 2/4] KVM: VMX: avoid double list add with VT-d posted interrupts

On 2017/6/6 18:57, Paolo Bonzini wrote:

> 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: Longpeng (Mike) <longpeng2@huawei.com>
> 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>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  arch/x86/kvm/vmx.c | 62 ++++++++++++++++++++++--------------------------------
>  1 file changed, 25 insertions(+), 37 deletions(-)
> 


[...]


> @@ -11256,14 +11257,10 @@ static void __pi_post_block(struct kvm_vcpu *vcpu)
>  	} while (cmpxchg(&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));


Hi Paolo,

spin_lock_irqsave() will disable kernel preempt, but spin_lock() won't. is there
some potential problems ?

Regards,
Longpeng(Mike)

>  		vcpu->pre_pcpu = -1;
>  	}
>  }
> @@ -11283,7 +11280,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);
> @@ -11293,34 +11289,20 @@ static int pi_pre_block(struct kvm_vcpu *vcpu)
>  		!kvm_vcpu_apicv_active(vcpu))
>  		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");
> @@ -11345,7 +11327,12 @@ static int pi_pre_block(struct kvm_vcpu *vcpu)
>  	} while (cmpxchg(&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)
> @@ -11361,12 +11348,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)  ||
> -		!kvm_vcpu_apicv_active(vcpu))
> +	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)


-- 
Regards,
Longpeng(Mike)

[toc] | [prev] | [next] | [standalone]

#1658620 — [PATCH 3/4] KVM: VMX: simplify and fix vmx_vcpu_pi_load

The simplify part: do not touch pi_desc.nv, we can set it when the
VCPU is first created.  Likewise, pi_desc.sn is only handled by
vmx_vcpu_pi_load, do not touch it in __pi_post_block.

The fix part: do not check kvm_arch_has_assigned_device, instead
check the SN bit to figure out whether vmx_vcpu_pi_put ran before.
This matches what the previous patch did in pi_post_block.

Cc: Longpeng (Mike) <longpeng2@huawei.com>
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>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/vmx.c | 68 ++++++++++++++++++++++++++++--------------------------
 1 file changed, 35 insertions(+), 33 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 0f4714fe4908..81047f373747 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -2184,43 +2184,41 @@ static void vmx_vcpu_pi_load(struct kvm_vcpu *vcpu, int cpu)
 	struct pi_desc old, new;
 	unsigned int dest;
 
-	if (!kvm_arch_has_assigned_device(vcpu->kvm) ||
-		!irq_remapping_cap(IRQ_POSTING_CAP)  ||
-		!kvm_vcpu_apicv_active(vcpu))
+	/*
+	 * In case of hot-plug or hot-unplug, we may have to undo
+	 * vmx_vcpu_pi_put even if there is no assigned device.  And we
+	 * always keep PI.NDST up to date for simplicity: it makes the
+	 * code easier, and CPU migration is not a fast path.
+	 */
+	if (!pi_test_sn(pi_desc) && vcpu->cpu == cpu)
+		return;
+
+	/*
+	 * First handle the simple case where no cmpxchg is necessary; just
+	 * allow posting non-urgent interrupts.
+	 *
+	 * If the 'nv' field is POSTED_INTR_WAKEUP_VECTOR, do not change
+	 * PI.NDST: pi_post_block will do it for us and the wakeup_handler
+	 * expects the VCPU to be on the blocked_vcpu_list that matches
+	 * PI.NDST.
+	 */
+	if (pi_desc->nv == POSTED_INTR_WAKEUP_VECTOR ||
+	    vcpu->cpu == cpu) {
+		pi_clear_sn(pi_desc);
 		return;
+	}
 
+	/* The full case.  */
 	do {
 		old.control = new.control = pi_desc->control;
 
-		/*
-		 * If 'nv' field is POSTED_INTR_WAKEUP_VECTOR, there
-		 * are two possible cases:
-		 * 1. After running 'pre_block', context switch
-		 *    happened. For this case, 'sn' was set in
-		 *    vmx_vcpu_put(), so we need to clear it here.
-		 * 2. After running 'pre_block', we were blocked,
-		 *    and woken up by some other guy. For this case,
-		 *    we don't need to do anything, 'pi_post_block'
-		 *    will do everything for us. However, we cannot
-		 *    check whether it is case #1 or case #2 here
-		 *    (maybe, not needed), so we also clear sn here,
-		 *    I think it is not a big deal.
-		 */
-		if (pi_desc->nv != POSTED_INTR_WAKEUP_VECTOR) {
-			if (vcpu->cpu != cpu) {
-				dest = cpu_physical_id(cpu);
-
-				if (x2apic_enabled())
-					new.ndst = dest;
-				else
-					new.ndst = (dest << 8) & 0xFF00;
-			}
+		dest = cpu_physical_id(cpu);
 
-			/* set 'NV' to 'notification vector' */
-			new.nv = POSTED_INTR_VECTOR;
-		}
+		if (x2apic_enabled())
+			new.ndst = dest;
+		else
+			new.ndst = (dest << 8) & 0xFF00;
 
-		/* Allow posting non-urgent interrupts */
 		new.sn = 0;
 	} while (cmpxchg(&pi_desc->control, old.control,
 			new.control) != old.control);
@@ -9259,6 +9257,13 @@ static struct kvm_vcpu *vmx_create_vcpu(struct kvm *kvm, unsigned int id)
 
 	vmx->msr_ia32_feature_control_valid_bits = FEATURE_CONTROL_LOCKED;
 
+	/*
+	 * Enforce invariant: pi_desc.nv is always either POSTED_INTR_VECTOR
+	 * or POSTED_INTR_WAKEUP_VECTOR.
+	 */
+	vmx->pi_desc.nv = POSTED_INTR_VECTOR;
+	vmx->pi_desc.sn = 1;
+
 	return &vmx->vcpu;
 
 free_vmcs:
@@ -11249,9 +11254,6 @@ static void __pi_post_block(struct kvm_vcpu *vcpu)
 		else
 			new.ndst = (dest << 8) & 0xFF00;
 
-		/* Allow posting non-urgent interrupts */
-		new.sn = 0;
-
 		/* set 'NV' to 'notification vector' */
 		new.nv = POSTED_INTR_VECTOR;
 	} while (cmpxchg(&pi_desc->control, old.control,
-- 
2.13.0

[toc] | [prev] | [next] | [standalone]

#1659603

> -----Original Message-----
> From: Paolo Bonzini [mailto:paolo.bonzini@gmail.com] On Behalf Of Paolo
> Bonzini
> Sent: Tuesday, June 06, 2017 6:57 PM
> To: linux-kernel@vger.kernel.org; kvm@vger.kernel.org
> Cc: longpeng; Huangweidong (C); Gonglei (Arei); wangxin (U); Radim Krčmář
> Subject: [PATCH CFT 0/4] VT-d PI fixes
> 
> These should fix, or at least help, the kernel panic reported by Longpeng
> with VT-d posted interrupts.
> 
> CONFIG_DEBUG_LIST reports a double add, meaning that pi_pre_block ran
> twice
> without pi_post_block deleting the vCPU from the blocked_on_vcpu list.
> The only possibility that I could think of is that this:
> 
>         if (!kvm_arch_has_assigned_device(vcpu->kvm) ||
>                 !irq_remapping_cap(IRQ_POSTING_CAP)  ||
>                 !kvm_vcpu_apicv_active(vcpu))
>                 return;
> 
> was false in pi_post_block.  In turn, I can only think of hot-unplug as
> the cause of this imbalance, but maybe there is another way to reach it
> just via repeated startup and shutdown.  Gonglei reported problems with
> hot-unplug offlist too, so this is a start.
> 
> In any case, patch 2 replaces it with a check on vcpu->pre_pcpu.
> A similar change is done in patch 3 to vmx_vcpu_pi_load.  I don't
> have hardware easily accessible with VT-d PI, so these patches are
> compile-tested only.  I apologize for any stupid mistakes.
> 
Hi Paolo,

We are testing your patch, but maybe need some time to report
the results because it's not an inevitable problem.

Meanwhile we also try to find a possible scenario of non-hotplugging to
explain the double-add warnings.

We found that some other VMs start failed before the kernel painc:

 2017-06-02T12:27:49.972583Z qemu-kvm: -device vfio-pci,host=0b:10.4,id=hostdev0,bus=pci.0,addr=0x5: vfio: error getting device 0000:
0b:10.4 from group 97: No such device
Verify all devices in group 97 are bound to vfio-<bus> or pci-stub and not already in use
2017-06-02T12:27:49.975925Z qemu-kvm: -device vfio-pci,host=0b:10.4,id=hostdev0,bus=pci.0,addr=0x5: vfio: failed to get device 0000:
0b:10.4
2017-06-02T12:27:51.246385Z qemu-kvm: -device vfio-pci,host=0b:10.4,id=hostdev0,bus=pci.0,addr=0x5: Device initialization failed
2017-06-02 12:27:53.628: shutting down, reason=crashed
2017-06-02 12:30:48.723: shutting down, reason=failed

But we don't think those failure will cause the unequal of kvm->arch.assigned_device_count
between pi_pre_block and pi_post_block. Am I right?

Thanks,
-Gonglei

[toc] | [prev] | [next] | [standalone]

#1659857

On 07/06/2017 11:33, Gonglei (Arei) wrote:
> 
>> -----Original Message-----
>> From: Paolo Bonzini [mailto:paolo.bonzini@gmail.com] On Behalf Of Paolo
>> Bonzini
>> Sent: Tuesday, June 06, 2017 6:57 PM
>> To: linux-kernel@vger.kernel.org; kvm@vger.kernel.org
>> Cc: longpeng; Huangweidong (C); Gonglei (Arei); wangxin (U); Radim Krčmář
>> Subject: [PATCH CFT 0/4] VT-d PI fixes
>>
>> These should fix, or at least help, the kernel panic reported by Longpeng
>> with VT-d posted interrupts.
>>
>> CONFIG_DEBUG_LIST reports a double add, meaning that pi_pre_block ran
>> twice
>> without pi_post_block deleting the vCPU from the blocked_on_vcpu list.
>> The only possibility that I could think of is that this:
>>
>>         if (!kvm_arch_has_assigned_device(vcpu->kvm) ||
>>                 !irq_remapping_cap(IRQ_POSTING_CAP)  ||
>>                 !kvm_vcpu_apicv_active(vcpu))
>>                 return;
>>
>> was false in pi_post_block.  In turn, I can only think of hot-unplug as
>> the cause of this imbalance, but maybe there is another way to reach it
>> just via repeated startup and shutdown.  Gonglei reported problems with
>> hot-unplug offlist too, so this is a start.
>>
>> In any case, patch 2 replaces it with a check on vcpu->pre_pcpu.
>> A similar change is done in patch 3 to vmx_vcpu_pi_load.  I don't
>> have hardware easily accessible with VT-d PI, so these patches are
>> compile-tested only.  I apologize for any stupid mistakes.
>>
> Hi Paolo,
> 
> We are testing your patch, but maybe need some time to report
> the results because it's not an inevitable problem.

Of course!  I guess it should run for at least a couple days before
deeming it fixed.  If you didn't find any immediate showstopper bugs,
that's already good. :)

Paolo

> Meanwhile we also try to find a possible scenario of non-hotplugging to
> explain the double-add warnings.
> 
> We found that some other VMs start failed before the kernel painc:
> 
>  2017-06-02T12:27:49.972583Z qemu-kvm: -device vfio-pci,host=0b:10.4,id=hostdev0,bus=pci.0,addr=0x5: vfio: error getting device 0000:
> 0b:10.4 from group 97: No such device
> Verify all devices in group 97 are bound to vfio-<bus> or pci-stub and not already in use
> 2017-06-02T12:27:49.975925Z qemu-kvm: -device vfio-pci,host=0b:10.4,id=hostdev0,bus=pci.0,addr=0x5: vfio: failed to get device 0000:
> 0b:10.4
> 2017-06-02T12:27:51.246385Z qemu-kvm: -device vfio-pci,host=0b:10.4,id=hostdev0,bus=pci.0,addr=0x5: Device initialization failed
> 2017-06-02 12:27:53.628: shutting down, reason=crashed
> 2017-06-02 12:30:48.723: shutting down, reason=failed
> 
> But we don't think those failure will cause the unequal of kvm->arch.assigned_device_count
> between pi_pre_block and pi_post_block. Am I right?
> 
> Thanks,
> -Gonglei
> 
> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web