[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v4] x86/apicv: Fix wrong IPI suppression during posted interrupt delivery
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@xxxxxxxxxxxxx https://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |