DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com A727FB3104
Subject: Re: [PATCH 2/4] KVM: VMX: avoid double list add with VT-d posted
 interrupts
To: "Longpeng (Mike)" <longpeng2@huawei.com>
Cc: linux-kernel@vger.kernel.org, kvm@vger.kernel.org,
        Huangweidong <weidong.huang@huawei.com>,
        Gonglei <arei.gonglei@huawei.com>,
        wangxin <wangxinxin.wang@huawei.com>,
        =?UTF-8?B?UmFkaW0gS3LEjW3DocWZ?= <rkrcmar@redhat.com>
References: <20170606105707.23207-1-pbonzini@redhat.com>
 <20170606105707.23207-3-pbonzini@redhat.com> <597AA21B.8030108@huawei.com>
From: Paolo Bonzini <pbonzini@redhat.com>
Message-ID: <711c9dab-6f4f-06f4-7799-cbc960ee7101@redhat.com>
Date: Fri, 28 Jul 2017 08:28:06 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.2.1
MIME-Version: 1.0
In-Reply-To: <597AA21B.8030108@huawei.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2670
Lines: 68

On 28/07/2017 04:31, Longpeng (Mike) wrote:
> Hi Paolo,
> 
> 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(-)
>>
>> 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)) {
> 
> 
> __pi_post_block is only called by pi_post_block/pi_pre_block now, it seems that
> both of them would make sure "vcpu->pre_pcpu != -1" before __pi_post_block is
> called, so maybe the above check is useless, right?

It's because a WARN is better than a double-add.  And even if the caller
broke the invariant you'd have to do the cmpxchg loop above to make
things not break too much.

Paolo