| Message ID | 1488508543-30295-1-git-send-email-chao.gao@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 >
On March 03, 2017 10:36 AM, Chao Gao wrote: >__vmx_deliver_posted_interrupt() wrongly used a softirq bit to decide >whether to suppress an IPI. Its logic was: the first time an IPI was sent, we set >the softirq bit. Next time, we would check that softirq bit before sending >another IPI. If the 1st IPI arrived at the pCPU which was in non-root mode, the >hardware would consume the IPI and sync PIR to vIRR. >During the process, no one (both hardware and software) will clear the softirq >bit. As a result, the following IPI would be wrongly suppressed. > >This patch discards the suppression check, always sending an IPI. >The softirq also need to be raised. But there is a little change. >This patch moves the place where we raise a softirq for 'cpu != >smp_processor_id()' case to the IPI interrupt handler. >Namely, don't raise a softirq for this case and set the interrupt handler to >pi_notification_interrupt()(in which a softirq is raised) regardless of VT-d PI >enabled or not. The only difference is when an IPI arrives at the pCPU which is >happened in non-root mode, the code will not raise a useless softirq since the >IPI is consumed by hardware rather than raise a softirq unconditionally. > >Signed-off-by: Quan Xu <xuquan8@huawei.com> >Signed-off-by: Chao Gao <chao.gao@intel.com> >Acked-by: Kevin Tian <kevin.tian@intel.com> >--- >v4: >- Replace patch v3 title "Enhance posted-interrupt processing" with the >current title. >- Desciption changes >- Fix a compiler error > >--- > xen/arch/x86/hvm/vmx/vmx.c | 50 >+++++++++++++++++++++++++++++++++++++++------- > 1 file changed, 43 insertions(+), 7 deletions(-) > >diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c >index 5b1717d..1a0e130 100644 >--- a/xen/arch/x86/hvm/vmx/vmx.c >+++ b/xen/arch/x86/hvm/vmx/vmx.c >@@ -1842,13 +1842,53 @@ static void >__vmx_deliver_posted_interrupt(struct vcpu *v) > bool_t running = v->is_running; > > vcpu_unblock(v); >+ /* >+ * 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? 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'.. -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/' >+ 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
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 >>
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c index 5b1717d..1a0e130 100644 --- a/xen/arch/x86/hvm/vmx/vmx.c +++ b/xen/arch/x86/hvm/vmx/vmx.c @@ -1842,13 +1842,53 @@ static void __vmx_deliver_posted_interrupt(struct vcpu *v) bool_t running = v->is_running; vcpu_unblock(v); + /* + * 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 + * 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. Similarly, there is a IPI and a softirq + * sent to a wrong vCPU. + */ + 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 {