Re: [Xen-devel] [PATCH v4] x86/apicv: Fix wrong IPI suppression during posted interrupt delivery

[Chao Gao <chao.gao@xxxxxxxxx>]

On Mon, Mar 06, 2017 at 03:53:44AM +0000, Xuquan (Quan Xu) wrote:
>On March 03, 2017 10:36 AM, Chao Gao wrote:
>>+    /*
>>+     * Just like vcpu_kick(), nothing is needed for the following two cases:
>>+     * 1. The target vCPU is not running, meaning it is blocked or runnable.
>>+     * 2. The target vCPU is the current vCPU and we're in non-interrupt
>>+     * context.
>>+     */
>>     if ( running && (in_irq() || (v != current)) )
>>     {
>>+        /*
>>+         * Note: Only two cases will reach here:
>>+         * 1. The target vCPU is running on other pCPU.
>>+         * 2. The target vCPU is the current vCPU.
>>+         *
>>+         * Note2: Don't worry the v->processor may change. The vCPU
>>being
>>+         * moved to another processor is guaranteed to sync PIR to vIRR,
>>+         * due to the involved scheduling cycle.
>>+         */
>>         unsigned int cpu = v->processor;
>>
>>-        if ( !test_and_set_bit(VCPU_KICK_SOFTIRQ, &softirq_pending(cpu))
>>-             && (cpu != smp_processor_id()) )
>>+        /*
>>+         * For case 1, we send an IPI to the pCPU. When an IPI arrives, the
>
>
>I almost agree to your comments!!
>You really mentioned 2 times 'two cases'.. so 'For case 1' here is a little 
>ambiguous..
>I am not sure what does it refer to, the first 'case 1' or the second one?
>

Indeed. It refers to the second one. Do you have any suggestion to make it 
better?

>
>From here to ...
>>+         * target vCPU maybe is running in non-root mode, running in root
>>+         * mode, runnable or blocked. If the target vCPU is running in
>>+         * non-root mode, the hardware will sync PIR to vIRR for
>>+         * 'posted_intr_vector' is special to the pCPU. If the target vCPU is
>>+         * running in root-mode, the interrupt handler starts to run.
>>+         * Considering an IPI may arrive in the window between the call to
>>+         * vmx_intr_assist() and interrupts getting disabled, the interrupt
>>+         * handler should raise a softirq to ensure events will be delivered
>>+         * in time. If the target vCPU is runnable, it will sync PIR to
>>+         * vIRR next time it is chose to run. In this case, a IPI and a
>>+         * softirq is sent to a wrong vCPU which will not have any adverse
>>+         * effect. If the target vCPU is blocked, since vcpu_block() checks
>>+         * whether there is an event to be delivered through
>>+         * local_events_need_delivery() just after blocking, the vCPU must
>>+         * have synced PIR to vIRR.
>
>... here. This looks an explanation to the first 'two cases' -- """nothing is 
>needed for the following two cases"""
>If so, you'd better move it next to the first 'two cases'..
>

The current status of the target vCPU separates the first two cases. The status 
of the target vCPU when
the IPI arrives the target pCPU separates the second one.

>
>
>-Quan
>
>> Similarly, there is a IPI and a softirq
>>+         * sent to a wrong vCPU.
>>+         */
>To me, this is a little confusing.. also " a IPI and a softirq is sent to a 
>wrong vCPU which will not have any adverse effect. "...  what about dropping 
>it?
>
>And some typo:
>             's/maybe is/is maybe/'
>             's/a IPI/an IPI/'
>

Really sorry for that. Considering it has been merged, I will send a fix fix :) 
patch later.

Thanks,
Chao
>
>
>
>>+        if ( cpu != smp_processor_id() )
>>             send_IPI_mask(cpumask_of(cpu), posted_intr_vector);
>>+        /*
>>+         * For case 2, raising a softirq ensures PIR will be synced to vIRR.
>>+         * As any softirq will do, as an optimization we only raise one if
>>+         * none is pending already.
>>+         */
>>+        else if ( !softirq_pending(cpu) )
>>+            raise_softirq(VCPU_KICK_SOFTIRQ);
>>     }
>> }
>>
>>@@ -2281,13 +2321,9 @@ const struct hvm_function_table * __init
>>start_vmx(void)
>>
>>     if ( cpu_has_vmx_posted_intr_processing )
>>     {
>>+        alloc_direct_apic_vector(&posted_intr_vector,
>>+ pi_notification_interrupt);
>>         if ( iommu_intpost )
>>-        {
>>-            alloc_direct_apic_vector(&posted_intr_vector,
>>pi_notification_interrupt);
>>             alloc_direct_apic_vector(&pi_wakeup_vector,
>>pi_wakeup_interrupt);
>>-        }
>>-        else
>>-            alloc_direct_apic_vector(&posted_intr_vector,
>>event_check_interrupt);
>>     }
>>     else
>>     {
>>--
>>1.8.3.1
>

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel