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

["Longpeng (Mike)" <longpeng2@huawei.com>]

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)