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

["Xuquan (Quan Xu)" <xuquan8@huawei.com>]

On March 06, 2017 6:50 AM, Chao Gao wrote:
>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.
>

Since this patch have been merged, ignore all of my comments. :) .. 
Sometimes, code comment and patch description are much harder than code modification..
I think there are more and more people who are keeping an eye on your 'vt-d pi' patch set..
Keep moving -- 'Jia You'... :)

-Quan





>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@lists.xen.org
https://lists.xen.org/xen-devel