[SRU][Xenial][PATCH v3 3/4] KVM: VMX: avoid double list add with VT-d posted interrupts

[Stefan Bader]

On 19.01.21 00:22, Ioanna Alifieraki wrote:
> From: Paolo Bonzini <pbonzini at 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 at huawei.com>
> Cc: Gonglei <arei.gonglei at huawei.com>
> Cc: wangxin <wangxinxin.wang at huawei.com>
> Cc: Radim Krčmář <rkrcmar at redhat.com>
> Tested-by: Longpeng (Mike) <longpeng2 at huawei.com>
> Cc: stable at vger.kernel.org
> Signed-off-by: Paolo Bonzini <pbonzini at 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 at 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)
> 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20210121/23d9c70a/attachment-0001.sig>