| Message ID | E0A769A898ADB6449596C41F51EF62C6AE6E44@SZXEMI506-MBX.china.huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Feb 17, 2017 at 09:37:45AM +0000, Xuquan (Quan Xu) wrote: >From a589074281cc22a30ed75a5bccba60e83d2312a6 Mon Sep 17 00:00:00 2001 >From: Quan Xu <xuquan8@huawei.com> >Date: Sat, 18 Feb 2017 09:27:37 +0800 >Subject: [PATCH] x86/apicv: enhance posted-interrupt processing > >If guest is already in non-root mode, an posted interrupt will >be directly delivered to guest (leaving softirq being set w/o >actually incurring a VM-Exit - breaking desired softirq behavior). >Then further posted interrupts will skip the IPI, stay in PIR and >not noted until another VM-Exit happens. > >Remove the softirq set. Actually since it's an optimization for >less IPIs, check softirq_pending(cpu) directly instead of sticking >to one bit only. > >Signed-off-by: Quan Xu <xuquan8@huawei.com> >--- > xen/arch/x86/hvm/vmx/vmx.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > >diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c >index 61925cf..3887c32 100644 >--- a/xen/arch/x86/hvm/vmx/vmx.c >+++ b/xen/arch/x86/hvm/vmx/vmx.c >@@ -1846,8 +1846,7 @@ static void __vmx_deliver_posted_interrupt(struct vcpu *v) > { > unsigned int cpu = v->processor; > >- if ( !test_and_set_bit(VCPU_KICK_SOFTIRQ, &softirq_pending(cpu)) >- && (cpu != smp_processor_id()) ) >+ if ( !softirq_pending(cpu) && (cpu != smp_processor_id()) ) HI, Quan. Is there a situation that we need set VCPU_KICK_SOFTIRQ. For example, after vmx_intr_assist(), a interrupt happened and its handler called this function to deliver interrupt to current vcpu. In that case, the interrupt would not be injected to guest before this VM-entry for we don't generate a softirq and don't send a self-IPI to current vcpu. Thanks, Chao > send_IPI_mask(cpumask_of(cpu), posted_intr_vector); > } > } >-- >1.8.3.1
>>> On 17.02.17 at 09:49, <chao.gao@intel.com> wrote: > On Fri, Feb 17, 2017 at 09:37:45AM +0000, Xuquan (Quan Xu) wrote: >>From a589074281cc22a30ed75a5bccba60e83d2312a6 Mon Sep 17 00:00:00 2001 >>From: Quan Xu <xuquan8@huawei.com> >>Date: Sat, 18 Feb 2017 09:27:37 +0800 >>Subject: [PATCH] x86/apicv: enhance posted-interrupt processing >> >>If guest is already in non-root mode, an posted interrupt will >>be directly delivered to guest (leaving softirq being set w/o >>actually incurring a VM-Exit - breaking desired softirq behavior). >>Then further posted interrupts will skip the IPI, stay in PIR and >>not noted until another VM-Exit happens. >> >>Remove the softirq set. Actually since it's an optimization for >>less IPIs, check softirq_pending(cpu) directly instead of sticking >>to one bit only. >> >>Signed-off-by: Quan Xu <xuquan8@huawei.com> >>--- >> xen/arch/x86/hvm/vmx/vmx.c | 3 +-- >> 1 file changed, 1 insertion(+), 2 deletions(-) >> >>diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c >>index 61925cf..3887c32 100644 >>--- a/xen/arch/x86/hvm/vmx/vmx.c >>+++ b/xen/arch/x86/hvm/vmx/vmx.c >>@@ -1846,8 +1846,7 @@ static void __vmx_deliver_posted_interrupt(struct vcpu *v) >> { >> unsigned int cpu = v->processor; >> >>- if ( !test_and_set_bit(VCPU_KICK_SOFTIRQ, &softirq_pending(cpu)) >>- && (cpu != smp_processor_id()) ) >>+ if ( !softirq_pending(cpu) && (cpu != smp_processor_id()) ) > HI, Quan. > Is there a situation that we need set VCPU_KICK_SOFTIRQ. For example, > after vmx_intr_assist(), a interrupt happened and its handler called this > function to deliver interrupt to current vcpu. In that case, the interrupt > would not be injected to guest before this VM-entry for we don't generate a > softirq and don't send a self-IPI to current vcpu. Good point, I think we indeed want to retain the old behavior (but in a not open coded fashion) for the cpu == smp_processor_id() case. Jan
On Mon, Feb 20, 2017 at 11:25:29AM +0000, Xuquan (Quan Xu) wrote: >On February 18, 2017 12:33 AM, Jan Beulich wrote: >>>>> On 17.02.17 at 09:49, <chao.gao@intel.com> wrote: >>> On Fri, Feb 17, 2017 at 09:37:45AM +0000, Xuquan (Quan Xu) wrote: >>>>From a589074281cc22a30ed75a5bccba60e83d2312a6 Mon Sep 17 >>00:00:00 2001 >>>>From: Quan Xu <xuquan8@huawei.com> >>>>Date: Sat, 18 Feb 2017 09:27:37 +0800 >>>>Subject: [PATCH] x86/apicv: enhance posted-interrupt processing >>>> >>>>If guest is already in non-root mode, an posted interrupt will be >>>>directly delivered to guest (leaving softirq being set w/o actually >>>>incurring a VM-Exit - breaking desired softirq behavior). >>>>Then further posted interrupts will skip the IPI, stay in PIR and not >>>>noted until another VM-Exit happens. >>>> >>>>Remove the softirq set. Actually since it's an optimization for less >>>>IPIs, check softirq_pending(cpu) directly instead of sticking to one >>>>bit only. >>>> >>>>Signed-off-by: Quan Xu <xuquan8@huawei.com> >>>>--- >>>> xen/arch/x86/hvm/vmx/vmx.c | 3 +-- >>>> 1 file changed, 1 insertion(+), 2 deletions(-) >>>> >>>>diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c >>>>index 61925cf..3887c32 100644 >>>>--- a/xen/arch/x86/hvm/vmx/vmx.c >>>>+++ b/xen/arch/x86/hvm/vmx/vmx.c >>>>@@ -1846,8 +1846,7 @@ static void >>__vmx_deliver_posted_interrupt(struct vcpu *v) >>>> { >>>> unsigned int cpu = v->processor; >>>> >>>>- if ( !test_and_set_bit(VCPU_KICK_SOFTIRQ, >>&softirq_pending(cpu)) >>>>- && (cpu != smp_processor_id()) ) >>>>+ if ( !softirq_pending(cpu) && (cpu != smp_processor_id()) ) >>> HI, Quan. >>> Is there a situation that we need set VCPU_KICK_SOFTIRQ. For example, >>> after vmx_intr_assist(), a interrupt happened and its handler called >>> this function to deliver interrupt to current vcpu. In that case, the >>> interrupt would not be injected to guest before this VM-entry for we >>> don't generate a softirq and don't send a self-IPI to current vcpu. >> > >Chao, __iiuc__, your question may be from the comments of xen/arch/x86/hvm/vmx/vmx.c :: pi_notification_interrupt() .. >IF VT-d PI is enabled, > VCPU_KICK_SOFTIRQ bit is set by ' raise_softirq(VCPU_KICK_SOFTIRQ)', at the end of pi_notification_interrupt().. >Else > Is it possible for your case? > If vcpu is in root mode and is to do VM-entry, it has synced PIR to vIRR. Now a interrupt (e.g. PMU_APIC_VECTOR) happens. Thus it goes following the path pmu_apic_interrupt->vpmu_do_interrupt->vlapic_set_irq(assume it will inject a interrupt to current vcpu) -> vmx_deliver_posted_intr( set one bit in PIR )-> __vmx_deliver_posted_interrupt Assuming that there is no softirq pending, the code after change doesn't generate a IPI for (cpu == smp_processor_id()). In this case, this interrupt would not be injected to guest before this VM-entry. Although there are many assumption in the explaination, I think it may be possible. Thanks, Chao >I hope Kevin could help us to check/correct it. > > >>Good point, I think we indeed want to retain the old behavior (but in a not >>open coded fashion) for the cpu == smp_processor_id() case. >> >__iiuc__ >for the cpu == smp_processor_id() case, the vCPUs (cpu = v->processor) pending to this cpu, are indeed not in guest-mode.. > >Quan >
On February 18, 2017 12:33 AM, Jan Beulich wrote: >>>> On 17.02.17 at 09:49, <chao.gao@intel.com> wrote: >> On Fri, Feb 17, 2017 at 09:37:45AM +0000, Xuquan (Quan Xu) wrote: >>>From a589074281cc22a30ed75a5bccba60e83d2312a6 Mon Sep 17 >00:00:00 2001 >>>From: Quan Xu <xuquan8@huawei.com> >>>Date: Sat, 18 Feb 2017 09:27:37 +0800 >>>Subject: [PATCH] x86/apicv: enhance posted-interrupt processing >>> >>>If guest is already in non-root mode, an posted interrupt will be >>>directly delivered to guest (leaving softirq being set w/o actually >>>incurring a VM-Exit - breaking desired softirq behavior). >>>Then further posted interrupts will skip the IPI, stay in PIR and not >>>noted until another VM-Exit happens. >>> >>>Remove the softirq set. Actually since it's an optimization for less >>>IPIs, check softirq_pending(cpu) directly instead of sticking to one >>>bit only. >>> >>>Signed-off-by: Quan Xu <xuquan8@huawei.com> >>>--- >>> xen/arch/x86/hvm/vmx/vmx.c | 3 +-- >>> 1 file changed, 1 insertion(+), 2 deletions(-) >>> >>>diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c >>>index 61925cf..3887c32 100644 >>>--- a/xen/arch/x86/hvm/vmx/vmx.c >>>+++ b/xen/arch/x86/hvm/vmx/vmx.c >>>@@ -1846,8 +1846,7 @@ static void >__vmx_deliver_posted_interrupt(struct vcpu *v) >>> { >>> unsigned int cpu = v->processor; >>> >>>- if ( !test_and_set_bit(VCPU_KICK_SOFTIRQ, >&softirq_pending(cpu)) >>>- && (cpu != smp_processor_id()) ) >>>+ if ( !softirq_pending(cpu) && (cpu != smp_processor_id()) ) >> HI, Quan. >> Is there a situation that we need set VCPU_KICK_SOFTIRQ. For example, >> after vmx_intr_assist(), a interrupt happened and its handler called >> this function to deliver interrupt to current vcpu. In that case, the >> interrupt would not be injected to guest before this VM-entry for we >> don't generate a softirq and don't send a self-IPI to current vcpu. > Chao, __iiuc__, your question may be from the comments of xen/arch/x86/hvm/vmx/vmx.c :: pi_notification_interrupt() .. IF VT-d PI is enabled, VCPU_KICK_SOFTIRQ bit is set by ' raise_softirq(VCPU_KICK_SOFTIRQ)', at the end of pi_notification_interrupt().. Else Is it possible for your case? I hope Kevin could help us to check/correct it. >Good point, I think we indeed want to retain the old behavior (but in a not >open coded fashion) for the cpu == smp_processor_id() case. > __iiuc__ for the cpu == smp_processor_id() case, the vCPUs (cpu = v->processor) pending to this cpu, are indeed not in guest-mode.. Quan
On Tue, Feb 21, 2017 at 04:11:53AM +0000, Xuquan (Quan Xu) wrote: >On February 21, 2017 11:07 AM, Tian, Kevin wrote: >>> From: Xuquan (Quan Xu) [mailto:xuquan8@huawei.com] >>> Sent: Tuesday, February 21, 2017 10:49 AM >>> >>Chao, __iiuc__, your question may be from the comments of >>> >xen/arch/x86/hvm/vmx/vmx.c :: pi_notification_interrupt() .. >>> >>IF VT-d PI is enabled, >>> >> VCPU_KICK_SOFTIRQ bit is set by ' >>> >>raise_softirq(VCPU_KICK_SOFTIRQ)', >>> >at the end of pi_notification_interrupt().. >>> >>Else >>> >> Is it possible for your case? >>> >> >>> >If vcpu is in root mode and is to do VM-entry, it has synced PIR to vIRR. >>> >Now a interrupt (e.g. PMU_APIC_VECTOR) happens. Thus it goes >>> >following the path >>> >pmu_apic_interrupt->vpmu_do_interrupt->vlapic_set_irq(assume >>> >it will inject a interrupt to current vcpu) >>> >-> vmx_deliver_posted_intr( set one bit in PIR )-> >>> >-> __vmx_deliver_posted_interrupt >>> >Assuming that there is no softirq pending, the code after change >>> >doesn't generate a IPI for (cpu == smp_processor_id()). In this case, >>> >this interrupt would not be injected to guest before this VM-entry. >>> >Although there are many assumption in the explaination, I think it >>> >may be possible. >>> > >>> >>> So far, I agree to add VCPU_KICK_SOFTIRQ bit in a nice way.. >>> Even we have checked whether the vCPU is running or not at the >>> beginning of __vmx_deliver_posted_interrupt(), we can't grantee the >>> vcpu is still in guest mode at the point to call this IPI.. >>> as in an extreme case, at the point to call this IPI, all of vCPUs are >>> in root-mode, the posted-interrupt won't be delivered.. >> >>I don't understand of your concern of whether guest is in guest mode here. > >If the vCPUs are in root-mode, whatever we do we can't deliver posted interrupt >Successfully. > >... > >>The purpose of this function is not to guarantee posted-interrupt is always >>used (cannot unless you pause remote cpu). It's just a best effort. > >... > >the best effort, you mentioned here, __iiuc__, we will sync PIRR to vIRR >before vmentry.. > >if we don't set VCPU_KICK_SOFTIRQ bit, the pending interrupt in PIRR will be not synced to vIRR before vm-entry in time. >In an extreme case, if there is only one interrupt pending interrupt in PIRR (no other caller to set VCPU_KICK_SOFTIRQ bit), >The pending interrupt in PIRR will never be delivered to guest later.. > >... > >> If target >>vcpu is in root mode, then this IPI causes a real interrupt on remote cpu as >>notification (then the handler pi_notification_interrupt you copied earlier >>will jump in to help). >> >> >... >The posted_intr_vector handler is not always pi_notification_interrupt. If the vt-d PI is not enabled, the handler >is event_check_interrupt.. >The VCPU_KICK_SOFTIRQ bit is set in pi_notification_interrupt , but not event_check_interrupt.. > OK. you are right. I think we can resolve it by replacing event_check_interrupt with pi_notification_interrupt. A remote vcpu receives event check notification, the vcpu doesn't know where it is. Maybe it hasn't execute the code that handles event, or it has already execute the code. So if the vcpu wants to handle the event, it's better to jump back to the code that handles event. I guess why the current event_check_interrupt doesn't set the softirq flag is it assumes the flag has been set by the source vcpu ( by the line you remove ). Also, for the case that the target vcpu is current vcpu, the vcpu also doesn't know about where it is. It is better to set a softirq if there is no softirq. Thanks, Chao > > >-I'd be very sorry if my description is still not clear.. >Quan
On February 20, 2017 4:24 PM, Chao Gao wrote: >On Mon, Feb 20, 2017 at 11:25:29AM +0000, Xuquan (Quan Xu) wrote: >>On February 18, 2017 12:33 AM, Jan Beulich wrote: >>>>>> On 17.02.17 at 09:49, <chao.gao@intel.com> wrote: >>>> On Fri, Feb 17, 2017 at 09:37:45AM +0000, Xuquan (Quan Xu) wrote: >>>>>From a589074281cc22a30ed75a5bccba60e83d2312a6 Mon Sep 17 >>>00:00:00 2001 >>>>>From: Quan Xu <xuquan8@huawei.com> >>>>>Date: Sat, 18 Feb 2017 09:27:37 +0800 >>>>>Subject: [PATCH] x86/apicv: enhance posted-interrupt processing >>>>> >>>>>If guest is already in non-root mode, an posted interrupt will be >>>>>directly delivered to guest (leaving softirq being set w/o actually >>>>>incurring a VM-Exit - breaking desired softirq behavior). >>>>>Then further posted interrupts will skip the IPI, stay in PIR and >>>>>not noted until another VM-Exit happens. >>>>> >>>>>Remove the softirq set. Actually since it's an optimization for less >>>>>IPIs, check softirq_pending(cpu) directly instead of sticking to one >>>>>bit only. >>>>> >>>>>Signed-off-by: Quan Xu <xuquan8@huawei.com> >>>>>--- >>>>> xen/arch/x86/hvm/vmx/vmx.c | 3 +-- >>>>> 1 file changed, 1 insertion(+), 2 deletions(-) >>>>> >>>>>diff --git a/xen/arch/x86/hvm/vmx/vmx.c >b/xen/arch/x86/hvm/vmx/vmx.c >>>>>index 61925cf..3887c32 100644 >>>>>--- a/xen/arch/x86/hvm/vmx/vmx.c >>>>>+++ b/xen/arch/x86/hvm/vmx/vmx.c >>>>>@@ -1846,8 +1846,7 @@ static void >>>__vmx_deliver_posted_interrupt(struct vcpu *v) >>>>> { >>>>> unsigned int cpu = v->processor; >>>>> >>>>>- if ( !test_and_set_bit(VCPU_KICK_SOFTIRQ, >>>&softirq_pending(cpu)) >>>>>- && (cpu != smp_processor_id()) ) >>>>>+ if ( !softirq_pending(cpu) && (cpu != smp_processor_id()) ) >>>> HI, Quan. >>>> Is there a situation that we need set VCPU_KICK_SOFTIRQ. For >>>> example, after vmx_intr_assist(), a interrupt happened and its >>>> handler called this function to deliver interrupt to current vcpu. >>>> In that case, the interrupt would not be injected to guest before >>>> this VM-entry for we don't generate a softirq and don't send a self-IPI >to current vcpu. >>> >> >>Chao, __iiuc__, your question may be from the comments of >xen/arch/x86/hvm/vmx/vmx.c :: pi_notification_interrupt() .. >>IF VT-d PI is enabled, >> VCPU_KICK_SOFTIRQ bit is set by ' raise_softirq(VCPU_KICK_SOFTIRQ)', >at the end of pi_notification_interrupt().. >>Else >> Is it possible for your case? >> >If vcpu is in root mode and is to do VM-entry, it has synced PIR to vIRR. >Now a interrupt (e.g. PMU_APIC_VECTOR) happens. Thus it goes following >the path pmu_apic_interrupt->vpmu_do_interrupt->vlapic_set_irq(assume >it will inject a interrupt to current vcpu) >-> vmx_deliver_posted_intr( set one bit in PIR )-> >-> __vmx_deliver_posted_interrupt >Assuming that there is no softirq pending, the code after change doesn't >generate a IPI for (cpu == smp_processor_id()). In this case, this interrupt >would not be injected to guest before this VM-entry. >Although there are many assumption in the explaination, I think it may be >possible. > So far, I agree to add VCPU_KICK_SOFTIRQ bit in a nice way.. Even we have checked whether the vCPU is running or not at the beginning of __vmx_deliver_posted_interrupt(), we can't grantee the vcpu is still in guest mode at the point to call this IPI.. as in an extreme case, at the point to call this IPI, all of vCPUs are in root-mode, the posted-interrupt won't be delivered.. btw, we better drop these local variables('running' / 'cpu') in __vmx_deliver_posted_interrupt(), as we may check a stale status.. for the cpu == smp_processor_id() case, as mentioned as below, the vCPUs (cpu = v->processor) pending to this cpu, are indeed not in guest-mode.. could you explain more? Quan >Thanks, >Chao >>I hope Kevin could help us to check/correct it. >> >> >>>Good point, I think we indeed want to retain the old behavior (but in >>>a not open coded fashion) for the cpu == smp_processor_id() case. >>> >>__iiuc__ >>for the cpu == smp_processor_id() case, the vCPUs (cpu = v->processor) >pending to this cpu, are indeed not in guest-mode.. >> >>Quan >>
> From: Xuquan (Quan Xu) [mailto:xuquan8@huawei.com] > Sent: Tuesday, February 21, 2017 10:49 AM > > On February 20, 2017 4:24 PM, Chao Gao wrote: > >On Mon, Feb 20, 2017 at 11:25:29AM +0000, Xuquan (Quan Xu) wrote: > >>On February 18, 2017 12:33 AM, Jan Beulich wrote: > >>>>>> On 17.02.17 at 09:49, <chao.gao@intel.com> wrote: > >>>> On Fri, Feb 17, 2017 at 09:37:45AM +0000, Xuquan (Quan Xu) wrote: > >>>>>From a589074281cc22a30ed75a5bccba60e83d2312a6 Mon Sep 17 > >>>00:00:00 2001 > >>>>>From: Quan Xu <xuquan8@huawei.com> > >>>>>Date: Sat, 18 Feb 2017 09:27:37 +0800 > >>>>>Subject: [PATCH] x86/apicv: enhance posted-interrupt processing > >>>>> > >>>>>If guest is already in non-root mode, an posted interrupt will be > >>>>>directly delivered to guest (leaving softirq being set w/o actually > >>>>>incurring a VM-Exit - breaking desired softirq behavior). > >>>>>Then further posted interrupts will skip the IPI, stay in PIR and > >>>>>not noted until another VM-Exit happens. > >>>>> > >>>>>Remove the softirq set. Actually since it's an optimization for less > >>>>>IPIs, check softirq_pending(cpu) directly instead of sticking to one > >>>>>bit only. > >>>>> > >>>>>Signed-off-by: Quan Xu <xuquan8@huawei.com> > >>>>>--- > >>>>> xen/arch/x86/hvm/vmx/vmx.c | 3 +-- > >>>>> 1 file changed, 1 insertion(+), 2 deletions(-) > >>>>> > >>>>>diff --git a/xen/arch/x86/hvm/vmx/vmx.c > >b/xen/arch/x86/hvm/vmx/vmx.c > >>>>>index 61925cf..3887c32 100644 > >>>>>--- a/xen/arch/x86/hvm/vmx/vmx.c > >>>>>+++ b/xen/arch/x86/hvm/vmx/vmx.c > >>>>>@@ -1846,8 +1846,7 @@ static void > >>>__vmx_deliver_posted_interrupt(struct vcpu *v) > >>>>> { > >>>>> unsigned int cpu = v->processor; > >>>>> > >>>>>- if ( !test_and_set_bit(VCPU_KICK_SOFTIRQ, > >>>&softirq_pending(cpu)) > >>>>>- && (cpu != smp_processor_id()) ) > >>>>>+ if ( !softirq_pending(cpu) && (cpu != smp_processor_id()) ) > >>>> HI, Quan. > >>>> Is there a situation that we need set VCPU_KICK_SOFTIRQ. For > >>>> example, after vmx_intr_assist(), a interrupt happened and its > >>>> handler called this function to deliver interrupt to current vcpu. > >>>> In that case, the interrupt would not be injected to guest before > >>>> this VM-entry for we don't generate a softirq and don't send a self-IPI > >to current vcpu. > >>> > >> > >>Chao, __iiuc__, your question may be from the comments of > >xen/arch/x86/hvm/vmx/vmx.c :: pi_notification_interrupt() .. > >>IF VT-d PI is enabled, > >> VCPU_KICK_SOFTIRQ bit is set by ' raise_softirq(VCPU_KICK_SOFTIRQ)', > >at the end of pi_notification_interrupt().. > >>Else > >> Is it possible for your case? > >> > >If vcpu is in root mode and is to do VM-entry, it has synced PIR to vIRR. > >Now a interrupt (e.g. PMU_APIC_VECTOR) happens. Thus it goes following > >the path pmu_apic_interrupt->vpmu_do_interrupt->vlapic_set_irq(assume > >it will inject a interrupt to current vcpu) > >-> vmx_deliver_posted_intr( set one bit in PIR )-> > >-> __vmx_deliver_posted_interrupt > >Assuming that there is no softirq pending, the code after change doesn't > >generate a IPI for (cpu == smp_processor_id()). In this case, this interrupt > >would not be injected to guest before this VM-entry. > >Although there are many assumption in the explaination, I think it may be > >possible. > > > > So far, I agree to add VCPU_KICK_SOFTIRQ bit in a nice way.. > Even we have checked whether the vCPU is running or not at the beginning of > __vmx_deliver_posted_interrupt(), > we can't grantee the vcpu is still in guest mode at the point to call this IPI.. > as in an extreme case, at the point to call this IPI, all of vCPUs are in root-mode, the > posted-interrupt > won't be delivered.. I don't understand of your concern of whether guest is in guest mode here. The purpose of this function is not to guarantee posted-interrupt is always used (cannot unless you pause remote cpu). It's just a best effort. If target vcpu is in root mode, then this IPI causes a real interrupt on remote cpu as notification (then the handler pi_notification_interrupt you copied earlier will jump in to help). Thanks Kevin
On February 21, 2017 11:07 AM, Tian, Kevin wrote: >> From: Xuquan (Quan Xu) [mailto:xuquan8@huawei.com] >> Sent: Tuesday, February 21, 2017 10:49 AM >> >> On February 20, 2017 4:24 PM, Chao Gao wrote: >> >On Mon, Feb 20, 2017 at 11:25:29AM +0000, Xuquan (Quan Xu) wrote: >> >>On February 18, 2017 12:33 AM, Jan Beulich wrote: >> >>>>>> On 17.02.17 at 09:49, <chao.gao@intel.com> wrote: >> >>>> On Fri, Feb 17, 2017 at 09:37:45AM +0000, Xuquan (Quan Xu) wrote: >> >>>>>From a589074281cc22a30ed75a5bccba60e83d2312a6 Mon Sep 17 >> >>>00:00:00 2001 >> >>>>>From: Quan Xu <xuquan8@huawei.com> >> >>>>>Date: Sat, 18 Feb 2017 09:27:37 +0800 >> >>>>>Subject: [PATCH] x86/apicv: enhance posted-interrupt processing >> >>>>> >> >>>>>If guest is already in non-root mode, an posted interrupt will be >> >>>>>directly delivered to guest (leaving softirq being set w/o >> >>>>>actually incurring a VM-Exit - breaking desired softirq behavior). >> >>>>>Then further posted interrupts will skip the IPI, stay in PIR and >> >>>>>not noted until another VM-Exit happens. >> >>>>> >> >>>>>Remove the softirq set. Actually since it's an optimization for >> >>>>>less IPIs, check softirq_pending(cpu) directly instead of >> >>>>>sticking to one bit only. >> >>>>> >> >>>>>Signed-off-by: Quan Xu <xuquan8@huawei.com> >> >>>>>--- >> >>>>> xen/arch/x86/hvm/vmx/vmx.c | 3 +-- >> >>>>> 1 file changed, 1 insertion(+), 2 deletions(-) >> >>>>> >> >>>>>diff --git a/xen/arch/x86/hvm/vmx/vmx.c >> >b/xen/arch/x86/hvm/vmx/vmx.c >> >>>>>index 61925cf..3887c32 100644 >> >>>>>--- a/xen/arch/x86/hvm/vmx/vmx.c >> >>>>>+++ b/xen/arch/x86/hvm/vmx/vmx.c >> >>>>>@@ -1846,8 +1846,7 @@ static void >> >>>__vmx_deliver_posted_interrupt(struct vcpu *v) >> >>>>> { >> >>>>> unsigned int cpu = v->processor; >> >>>>> >> >>>>>- if ( !test_and_set_bit(VCPU_KICK_SOFTIRQ, >> >>>&softirq_pending(cpu)) >> >>>>>- && (cpu != smp_processor_id()) ) >> >>>>>+ if ( !softirq_pending(cpu) && (cpu != >> >>>>>+ smp_processor_id()) ) >> >>>> HI, Quan. >> >>>> Is there a situation that we need set VCPU_KICK_SOFTIRQ. For >> >>>> example, after vmx_intr_assist(), a interrupt happened and its >> >>>> handler called this function to deliver interrupt to current vcpu. >> >>>> In that case, the interrupt would not be injected to guest before >> >>>> this VM-entry for we don't generate a softirq and don't send a >> >>>> self-IPI >> >to current vcpu. >> >>> >> >> >> >>Chao, __iiuc__, your question may be from the comments of >> >xen/arch/x86/hvm/vmx/vmx.c :: pi_notification_interrupt() .. >> >>IF VT-d PI is enabled, >> >> VCPU_KICK_SOFTIRQ bit is set by ' >> >>raise_softirq(VCPU_KICK_SOFTIRQ)', >> >at the end of pi_notification_interrupt().. >> >>Else >> >> Is it possible for your case? >> >> >> >If vcpu is in root mode and is to do VM-entry, it has synced PIR to vIRR. >> >Now a interrupt (e.g. PMU_APIC_VECTOR) happens. Thus it goes >> >following the path >> >pmu_apic_interrupt->vpmu_do_interrupt->vlapic_set_irq(assume >> >it will inject a interrupt to current vcpu) >> >-> vmx_deliver_posted_intr( set one bit in PIR )-> >> >-> __vmx_deliver_posted_interrupt >> >Assuming that there is no softirq pending, the code after change >> >doesn't generate a IPI for (cpu == smp_processor_id()). In this case, >> >this interrupt would not be injected to guest before this VM-entry. >> >Although there are many assumption in the explaination, I think it >> >may be possible. >> > >> >> So far, I agree to add VCPU_KICK_SOFTIRQ bit in a nice way.. >> Even we have checked whether the vCPU is running or not at the >> beginning of __vmx_deliver_posted_interrupt(), we can't grantee the >> vcpu is still in guest mode at the point to call this IPI.. >> as in an extreme case, at the point to call this IPI, all of vCPUs are >> in root-mode, the posted-interrupt won't be delivered.. > >I don't understand of your concern of whether guest is in guest mode here. If the vCPUs are in root-mode, whatever we do we can't deliver posted interrupt Successfully. ... >The purpose of this function is not to guarantee posted-interrupt is always >used (cannot unless you pause remote cpu). It's just a best effort. ... the best effort, you mentioned here, __iiuc__, we will sync PIRR to vIRR before vmentry.. if we don't set VCPU_KICK_SOFTIRQ bit, the pending interrupt in PIRR will be not synced to vIRR before vm-entry in time. In an extreme case, if there is only one interrupt pending interrupt in PIRR (no other caller to set VCPU_KICK_SOFTIRQ bit), The pending interrupt in PIRR will never be delivered to guest later.. ... > If target >vcpu is in root mode, then this IPI causes a real interrupt on remote cpu as >notification (then the handler pi_notification_interrupt you copied earlier >will jump in to help). > > ... The posted_intr_vector handler is not always pi_notification_interrupt. If the vt-d PI is not enabled, the handler is event_check_interrupt.. The VCPU_KICK_SOFTIRQ bit is set in pi_notification_interrupt , but not event_check_interrupt.. -I'd be very sorry if my description is still not clear.. Quan
On February 21, 2017 5:55 AM, Chao Gao wrote: >On Tue, Feb 21, 2017 at 04:11:53AM +0000, Xuquan (Quan Xu) wrote: >>On February 21, 2017 11:07 AM, Tian, Kevin wrote: >>>> From: Xuquan (Quan Xu) [mailto:xuquan8@huawei.com] >>>> Sent: Tuesday, February 21, 2017 10:49 AM >>>> >>Chao, __iiuc__, your question may be from the comments of >>>> >xen/arch/x86/hvm/vmx/vmx.c :: pi_notification_interrupt() .. >>>> >>IF VT-d PI is enabled, >>>> >> VCPU_KICK_SOFTIRQ bit is set by ' >>>> >>raise_softirq(VCPU_KICK_SOFTIRQ)', >>>> >at the end of pi_notification_interrupt().. >>>> >>Else >>>> >> Is it possible for your case? >>>> >> >>>> >If vcpu is in root mode and is to do VM-entry, it has synced PIR to >vIRR. >>>> >Now a interrupt (e.g. PMU_APIC_VECTOR) happens. Thus it goes >>>> >following the path >>>> >pmu_apic_interrupt->vpmu_do_interrupt->vlapic_set_irq(assume >>>> >it will inject a interrupt to current vcpu) >>>> >-> vmx_deliver_posted_intr( set one bit in PIR )-> >>>> >-> __vmx_deliver_posted_interrupt >>>> >Assuming that there is no softirq pending, the code after change >>>> >doesn't generate a IPI for (cpu == smp_processor_id()). In this >>>> >case, this interrupt would not be injected to guest before this >VM-entry. >>>> >Although there are many assumption in the explaination, I think it >>>> >may be possible. >>>> > >>>> >>>> So far, I agree to add VCPU_KICK_SOFTIRQ bit in a nice way.. >>>> Even we have checked whether the vCPU is running or not at the >>>> beginning of __vmx_deliver_posted_interrupt(), we can't grantee the >>>> vcpu is still in guest mode at the point to call this IPI.. >>>> as in an extreme case, at the point to call this IPI, all of vCPUs >>>> are in root-mode, the posted-interrupt won't be delivered.. >>> >>>I don't understand of your concern of whether guest is in guest mode >here. >> >>If the vCPUs are in root-mode, whatever we do we can't deliver posted >>interrupt Successfully. >> >>... >> >>>The purpose of this function is not to guarantee posted-interrupt is >>>always used (cannot unless you pause remote cpu). It's just a best effort. >> >>... >> >>the best effort, you mentioned here, __iiuc__, we will sync PIRR to >>vIRR before vmentry.. >> >>if we don't set VCPU_KICK_SOFTIRQ bit, the pending interrupt in PIRR will >be not synced to vIRR before vm-entry in time. >>In an extreme case, if there is only one interrupt pending interrupt in >>PIRR (no other caller to set VCPU_KICK_SOFTIRQ bit), The pending >interrupt in PIRR will never be delivered to guest later.. >> >>... >> >>> If target >>>vcpu is in root mode, then this IPI causes a real interrupt on remote >>>cpu as notification (then the handler pi_notification_interrupt you >>>copied earlier will jump in to help). >>> >>> >>... >>The posted_intr_vector handler is not always pi_notification_interrupt. >>If the vt-d PI is not enabled, the handler is event_check_interrupt.. >>The VCPU_KICK_SOFTIRQ bit is set in pi_notification_interrupt , but not >event_check_interrupt.. >> >OK. you are right. I think we can resolve it by replacing >event_check_interrupt with pi_notification_interrupt. Sounds good to me. Note that there is more ' perfc_incr(ipis); ' in event_check_interrupt().. I don't know what's the purpose.. >A remote vcpu receives event check notification, the vcpu doesn't know >where it is. Maybe it hasn't execute the code that handles event, or it has >already execute the code. So if the vcpu wants to handle the event, it's >better to jump back to the code that handles event. I guess why the current >event_check_interrupt doesn't set the softirq flag is it assumes the flag has >been set by the source vcpu ( by the line you remove ). > >Also, for the case that the target vcpu is current vcpu, the vcpu also doesn't >know about where it is. >It is better to set a softirq if there is no softirq. > A little confused, but the ipi here is to pCPU, instead of vCPU.. Quan >Thanks, >Chao >> >> >>-I'd be very sorry if my description is still not clear.. >>Quan
>>> On 21.02.17 at 07:19, <xuquan8@huawei.com> wrote: > On February 21, 2017 5:55 AM, Chao Gao wrote: >>On Tue, Feb 21, 2017 at 04:11:53AM +0000, Xuquan (Quan Xu) wrote: >>>The posted_intr_vector handler is not always pi_notification_interrupt. >>>If the vt-d PI is not enabled, the handler is event_check_interrupt.. >>>The VCPU_KICK_SOFTIRQ bit is set in pi_notification_interrupt , but not >>event_check_interrupt.. >>> >>OK. you are right. I think we can resolve it by replacing >>event_check_interrupt with pi_notification_interrupt. > > Sounds good to me. Note that there is more ' perfc_incr(ipis); ' in > event_check_interrupt().. I don't know what's the purpose.. That's a performance counter getting incremented (if such counters are enabled in the first place). Jan
> From: Xuquan (Quan Xu) [mailto:xuquan8@huawei.com] > Sent: Tuesday, February 21, 2017 12:12 PM > > On February 21, 2017 11:07 AM, Tian, Kevin wrote: > >> From: Xuquan (Quan Xu) [mailto:xuquan8@huawei.com] > >> Sent: Tuesday, February 21, 2017 10:49 AM > >> > >> On February 20, 2017 4:24 PM, Chao Gao wrote: > >> >On Mon, Feb 20, 2017 at 11:25:29AM +0000, Xuquan (Quan Xu) wrote: > >> >>On February 18, 2017 12:33 AM, Jan Beulich wrote: > >> >>>>>> On 17.02.17 at 09:49, <chao.gao@intel.com> wrote: > >> >>>> On Fri, Feb 17, 2017 at 09:37:45AM +0000, Xuquan (Quan Xu) wrote: > >> >>>>>From a589074281cc22a30ed75a5bccba60e83d2312a6 Mon Sep 17 > >> >>>00:00:00 2001 > >> >>>>>From: Quan Xu <xuquan8@huawei.com> > >> >>>>>Date: Sat, 18 Feb 2017 09:27:37 +0800 > >> >>>>>Subject: [PATCH] x86/apicv: enhance posted-interrupt processing > >> >>>>> > >> >>>>>If guest is already in non-root mode, an posted interrupt will be > >> >>>>>directly delivered to guest (leaving softirq being set w/o > >> >>>>>actually incurring a VM-Exit - breaking desired softirq behavior). > >> >>>>>Then further posted interrupts will skip the IPI, stay in PIR and > >> >>>>>not noted until another VM-Exit happens. > >> >>>>> > >> >>>>>Remove the softirq set. Actually since it's an optimization for > >> >>>>>less IPIs, check softirq_pending(cpu) directly instead of > >> >>>>>sticking to one bit only. > >> >>>>> > >> >>>>>Signed-off-by: Quan Xu <xuquan8@huawei.com> > >> >>>>>--- > >> >>>>> xen/arch/x86/hvm/vmx/vmx.c | 3 +-- > >> >>>>> 1 file changed, 1 insertion(+), 2 deletions(-) > >> >>>>> > >> >>>>>diff --git a/xen/arch/x86/hvm/vmx/vmx.c > >> >b/xen/arch/x86/hvm/vmx/vmx.c > >> >>>>>index 61925cf..3887c32 100644 > >> >>>>>--- a/xen/arch/x86/hvm/vmx/vmx.c > >> >>>>>+++ b/xen/arch/x86/hvm/vmx/vmx.c > >> >>>>>@@ -1846,8 +1846,7 @@ static void > >> >>>__vmx_deliver_posted_interrupt(struct vcpu *v) > >> >>>>> { > >> >>>>> unsigned int cpu = v->processor; > >> >>>>> > >> >>>>>- if ( !test_and_set_bit(VCPU_KICK_SOFTIRQ, > >> >>>&softirq_pending(cpu)) > >> >>>>>- && (cpu != smp_processor_id()) ) > >> >>>>>+ if ( !softirq_pending(cpu) && (cpu != > >> >>>>>+ smp_processor_id()) ) > >> >>>> HI, Quan. > >> >>>> Is there a situation that we need set VCPU_KICK_SOFTIRQ. For > >> >>>> example, after vmx_intr_assist(), a interrupt happened and its > >> >>>> handler called this function to deliver interrupt to current vcpu. > >> >>>> In that case, the interrupt would not be injected to guest before > >> >>>> this VM-entry for we don't generate a softirq and don't send a > >> >>>> self-IPI > >> >to current vcpu. > >> >>> > >> >> > >> >>Chao, __iiuc__, your question may be from the comments of > >> >xen/arch/x86/hvm/vmx/vmx.c :: pi_notification_interrupt() .. > >> >>IF VT-d PI is enabled, > >> >> VCPU_KICK_SOFTIRQ bit is set by ' > >> >>raise_softirq(VCPU_KICK_SOFTIRQ)', > >> >at the end of pi_notification_interrupt().. > >> >>Else > >> >> Is it possible for your case? > >> >> > >> >If vcpu is in root mode and is to do VM-entry, it has synced PIR to vIRR. > >> >Now a interrupt (e.g. PMU_APIC_VECTOR) happens. Thus it goes > >> >following the path > >> >pmu_apic_interrupt->vpmu_do_interrupt->vlapic_set_irq(assume > >> >it will inject a interrupt to current vcpu) > >> >-> vmx_deliver_posted_intr( set one bit in PIR )-> > >> >-> __vmx_deliver_posted_interrupt > >> >Assuming that there is no softirq pending, the code after change > >> >doesn't generate a IPI for (cpu == smp_processor_id()). In this case, > >> >this interrupt would not be injected to guest before this VM-entry. > >> >Although there are many assumption in the explaination, I think it > >> >may be possible. > >> > > >> > >> So far, I agree to add VCPU_KICK_SOFTIRQ bit in a nice way.. > >> Even we have checked whether the vCPU is running or not at the > >> beginning of __vmx_deliver_posted_interrupt(), we can't grantee the > >> vcpu is still in guest mode at the point to call this IPI.. > >> as in an extreme case, at the point to call this IPI, all of vCPUs are > >> in root-mode, the posted-interrupt won't be delivered.. > > > >I don't understand of your concern of whether guest is in guest mode here. > > If the vCPUs are in root-mode, whatever we do we can't deliver posted interrupt > Successfully. The point is not to 100% guarantee this IPI is posted when target vcpu is in non-root mode. The goal is to kick vcpu about pending interrupts. If it happens to be in non-root mode (in most time), then posted way is triggered. Otherwise a physical IPI triggered in root mode with corresponding interrupt handler to mark out this situation. > > ... > > >The purpose of this function is not to guarantee posted-interrupt is always > >used (cannot unless you pause remote cpu). It's just a best effort. > > ... > > the best effort, you mentioned here, __iiuc__, we will sync PIRR to vIRR > before vmentry.. > > if we don't set VCPU_KICK_SOFTIRQ bit, the pending interrupt in PIRR will be not synced > to vIRR before vm-entry in time. > In an extreme case, if there is only one interrupt pending interrupt in PIRR (no other caller > to set VCPU_KICK_SOFTIRQ bit), > The pending interrupt in PIRR will never be delivered to guest later.. > > ... > > > If target > >vcpu is in root mode, then this IPI causes a real interrupt on remote cpu as > >notification (then the handler pi_notification_interrupt you copied earlier > >will jump in to help). > > > > > ... > The posted_intr_vector handler is not always pi_notification_interrupt. If the vt-d PI is not > enabled, the handler > is event_check_interrupt.. > The VCPU_KICK_SOFTIRQ bit is set in pi_notification_interrupt , but not > event_check_interrupt.. > > As Chao explained, likely event_check_interrupt is used in pair with original delivery logic. Now since delivery path is changed, we should move to pi_notification_interrupt always. Thanks Kevin
On Thu, Feb 23, 2017 at 11:55:15AM +0000, Xuquan (Quan Xu) wrote: >On February 23, 2017 7:01 PM, Jan Beulich wrote: >>>>> On 23.02.17 at 11:53, <xuquan8@huawei.com> wrote: >>> On February 23, 2017 5:59 PM, Jan Beulich wrote: >>>>>>> On 23.02.17 at 10:28, <xuquan8@huawei.com> wrote: >>>>> On February 18, 2017 12:33 AM, Jan Beulich wrote: >>>>>>>>> On 17.02.17 at 09:49, <chao.gao@intel.com> wrote: >>>>>>>>diff --git a/xen/arch/x86/hvm/vmx/vmx.c >>>>b/xen/arch/x86/hvm/vmx/vmx.c >>>>>>>>index 61925cf..3887c32 100644 >>>>>>>>--- a/xen/arch/x86/hvm/vmx/vmx.c >>>>>>>>+++ b/xen/arch/x86/hvm/vmx/vmx.c >>>>>>>>@@ -1846,8 +1846,7 @@ static void >>>>>>__vmx_deliver_posted_interrupt(struct vcpu *v) >>>>>>>> { >>>>>>>> unsigned int cpu = v->processor; >>>>>>>> >>>>>>>>- if ( !test_and_set_bit(VCPU_KICK_SOFTIRQ, >>>>>>&softirq_pending(cpu)) >>>>>>>>- && (cpu != smp_processor_id()) ) >>>>>>>>+ if ( !softirq_pending(cpu) && (cpu != smp_processor_id()) >>>>>>>>+ ) >>>>> >>>>> Jan, >>>>> could you help me present the definition of ' smp_processor_id()' >>and ' >>>>> current' in __vmx_deliver_posted_interrupt() ? thanks.. >>>> >>>>I'm afraid I don't understand the request. >>> >>> IOW, >>> which vcpu does the 'current' refer to? >>> which cpu does the ' smp_processor_id()' refer to? >> >>current: currently running vCPU > >in a SMP machine, are there more than one currently running vCPU? >I think so, the condition "if ( running && (in_irq() || (v != current)) )", in __vmx_deliver_posted_interrupt() looks strange -- when vCPU is running, why to check ' v != current '.. > > >>smp_processor_id(): processor ID of the CPU we're running on >> >I think if vcpu is running, ' cpu != smp_processor_id() ' should be true. > I am afraid it is not right. when the 'current' wants to deliver posted interrupt to 'current', the vcpu is running and cpu == smp_processor_id(). > >I think we could simplify __vmx_deliver_posted_interrupt(): > > 1. set VCPU_KICK_SOFTIRQ bit of v->processor. Do we need set VCPU_KICK_SOFTIRQ bit when vcpu is not running? IMO, if vcpu is not running, just unblock (or, vcpu_kick since when vcpu is not running, vcpu_kick is the same with vcpu_unblock )is enough. > 2. IF vcpu is running: > - send_IPI Do we need send self-IPI when the target vcpu is 'current'? IMO, set VCPU_KICK_SOFTIQR is enough. > ELSE > - vcpu_kick > > >Thank you for your patience.. >Quan > >
On February 18, 2017 12:33 AM, Jan Beulich wrote: >>>> On 17.02.17 at 09:49, <chao.gao@intel.com> wrote: >> On Fri, Feb 17, 2017 at 09:37:45AM +0000, Xuquan (Quan Xu) wrote: >>>From a589074281cc22a30ed75a5bccba60e83d2312a6 Mon Sep 17 >00:00:00 2001 >>>From: Quan Xu <xuquan8@huawei.com> >>>Date: Sat, 18 Feb 2017 09:27:37 +0800 >>>Subject: [PATCH] x86/apicv: enhance posted-interrupt processing >>> >>>If guest is already in non-root mode, an posted interrupt will be >>>directly delivered to guest (leaving softirq being set w/o actually >>>incurring a VM-Exit - breaking desired softirq behavior). >>>Then further posted interrupts will skip the IPI, stay in PIR and not >>>noted until another VM-Exit happens. >>> >>>Remove the softirq set. Actually since it's an optimization for less >>>IPIs, check softirq_pending(cpu) directly instead of sticking to one >>>bit only. >>> >>>Signed-off-by: Quan Xu <xuquan8@huawei.com> >>>--- >>> xen/arch/x86/hvm/vmx/vmx.c | 3 +-- >>> 1 file changed, 1 insertion(+), 2 deletions(-) >>> >>>diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c >>>index 61925cf..3887c32 100644 >>>--- a/xen/arch/x86/hvm/vmx/vmx.c >>>+++ b/xen/arch/x86/hvm/vmx/vmx.c >>>@@ -1846,8 +1846,7 @@ static void >__vmx_deliver_posted_interrupt(struct vcpu *v) >>> { >>> unsigned int cpu = v->processor; >>> >>>- if ( !test_and_set_bit(VCPU_KICK_SOFTIRQ, >&softirq_pending(cpu)) >>>- && (cpu != smp_processor_id()) ) >>>+ if ( !softirq_pending(cpu) && (cpu != smp_processor_id()) ) Jan, could you help me present the definition of ' smp_processor_id()' and ' current' in __vmx_deliver_posted_interrupt() ? thanks.. Quan >> HI, Quan. >> Is there a situation that we need set VCPU_KICK_SOFTIRQ. For example, >> after vmx_intr_assist(), a interrupt happened and its handler called >> this function to deliver interrupt to current vcpu. In that case, the >> interrupt would not be injected to guest before this VM-entry for we >> don't generate a softirq and don't send a self-IPI to current vcpu. > >Good point, I think we indeed want to retain the old behavior (but in a not >open coded fashion) for the cpu == smp_processor_id() case. > >Jan
>>> On 23.02.17 at 10:28, <xuquan8@huawei.com> wrote: > On February 18, 2017 12:33 AM, Jan Beulich wrote: >>>>> On 17.02.17 at 09:49, <chao.gao@intel.com> wrote: >>> On Fri, Feb 17, 2017 at 09:37:45AM +0000, Xuquan (Quan Xu) wrote: >>>>From a589074281cc22a30ed75a5bccba60e83d2312a6 Mon Sep 17 >>00:00:00 2001 >>>>From: Quan Xu <xuquan8@huawei.com> >>>>Date: Sat, 18 Feb 2017 09:27:37 +0800 >>>>Subject: [PATCH] x86/apicv: enhance posted-interrupt processing >>>> >>>>If guest is already in non-root mode, an posted interrupt will be >>>>directly delivered to guest (leaving softirq being set w/o actually >>>>incurring a VM-Exit - breaking desired softirq behavior). >>>>Then further posted interrupts will skip the IPI, stay in PIR and not >>>>noted until another VM-Exit happens. >>>> >>>>Remove the softirq set. Actually since it's an optimization for less >>>>IPIs, check softirq_pending(cpu) directly instead of sticking to one >>>>bit only. >>>> >>>>Signed-off-by: Quan Xu <xuquan8@huawei.com> >>>>--- >>>> xen/arch/x86/hvm/vmx/vmx.c | 3 +-- >>>> 1 file changed, 1 insertion(+), 2 deletions(-) >>>> >>>>diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c >>>>index 61925cf..3887c32 100644 >>>>--- a/xen/arch/x86/hvm/vmx/vmx.c >>>>+++ b/xen/arch/x86/hvm/vmx/vmx.c >>>>@@ -1846,8 +1846,7 @@ static void >>__vmx_deliver_posted_interrupt(struct vcpu *v) >>>> { >>>> unsigned int cpu = v->processor; >>>> >>>>- if ( !test_and_set_bit(VCPU_KICK_SOFTIRQ, >>&softirq_pending(cpu)) >>>>- && (cpu != smp_processor_id()) ) >>>>+ if ( !softirq_pending(cpu) && (cpu != smp_processor_id()) ) > > Jan, > could you help me present the definition of ' smp_processor_id()' and ' > current' in __vmx_deliver_posted_interrupt() ? thanks.. I'm afraid I don't understand the request. Jan
On February 23, 2017 5:59 PM, Jan Beulich wrote: >>>> On 23.02.17 at 10:28, <xuquan8@huawei.com> wrote: >> On February 18, 2017 12:33 AM, Jan Beulich wrote: >>>>>> On 17.02.17 at 09:49, <chao.gao@intel.com> wrote: >>>> On Fri, Feb 17, 2017 at 09:37:45AM +0000, Xuquan (Quan Xu) wrote: >>>>>From a589074281cc22a30ed75a5bccba60e83d2312a6 Mon Sep 17 >>>00:00:00 2001 >>>>>From: Quan Xu <xuquan8@huawei.com> >>>>>Date: Sat, 18 Feb 2017 09:27:37 +0800 >>>>>Subject: [PATCH] x86/apicv: enhance posted-interrupt processing >>>>> >>>>>If guest is already in non-root mode, an posted interrupt will be >>>>>directly delivered to guest (leaving softirq being set w/o actually >>>>>incurring a VM-Exit - breaking desired softirq behavior). >>>>>Then further posted interrupts will skip the IPI, stay in PIR and >>>>>not noted until another VM-Exit happens. >>>>> >>>>>Remove the softirq set. Actually since it's an optimization for less >>>>>IPIs, check softirq_pending(cpu) directly instead of sticking to one >>>>>bit only. >>>>> >>>>>Signed-off-by: Quan Xu <xuquan8@huawei.com> >>>>>--- >>>>> xen/arch/x86/hvm/vmx/vmx.c | 3 +-- >>>>> 1 file changed, 1 insertion(+), 2 deletions(-) >>>>> >>>>>diff --git a/xen/arch/x86/hvm/vmx/vmx.c >b/xen/arch/x86/hvm/vmx/vmx.c >>>>>index 61925cf..3887c32 100644 >>>>>--- a/xen/arch/x86/hvm/vmx/vmx.c >>>>>+++ b/xen/arch/x86/hvm/vmx/vmx.c >>>>>@@ -1846,8 +1846,7 @@ static void >>>__vmx_deliver_posted_interrupt(struct vcpu *v) >>>>> { >>>>> unsigned int cpu = v->processor; >>>>> >>>>>- if ( !test_and_set_bit(VCPU_KICK_SOFTIRQ, >>>&softirq_pending(cpu)) >>>>>- && (cpu != smp_processor_id()) ) >>>>>+ if ( !softirq_pending(cpu) && (cpu != smp_processor_id()) ) >> >> Jan, >> could you help me present the definition of ' smp_processor_id()' and ' >> current' in __vmx_deliver_posted_interrupt() ? thanks.. > >I'm afraid I don't understand the request. IOW, which vcpu does the 'current' refer to? which cpu does the ' smp_processor_id()' refer to? Quan
>>> On 23.02.17 at 11:53, <xuquan8@huawei.com> wrote: > On February 23, 2017 5:59 PM, Jan Beulich wrote: >>>>> On 23.02.17 at 10:28, <xuquan8@huawei.com> wrote: >>> On February 18, 2017 12:33 AM, Jan Beulich wrote: >>>>>>> On 17.02.17 at 09:49, <chao.gao@intel.com> wrote: >>>>> On Fri, Feb 17, 2017 at 09:37:45AM +0000, Xuquan (Quan Xu) wrote: >>>>>>From a589074281cc22a30ed75a5bccba60e83d2312a6 Mon Sep 17 >>>>00:00:00 2001 >>>>>>From: Quan Xu <xuquan8@huawei.com> >>>>>>Date: Sat, 18 Feb 2017 09:27:37 +0800 >>>>>>Subject: [PATCH] x86/apicv: enhance posted-interrupt processing >>>>>> >>>>>>If guest is already in non-root mode, an posted interrupt will be >>>>>>directly delivered to guest (leaving softirq being set w/o actually >>>>>>incurring a VM-Exit - breaking desired softirq behavior). >>>>>>Then further posted interrupts will skip the IPI, stay in PIR and >>>>>>not noted until another VM-Exit happens. >>>>>> >>>>>>Remove the softirq set. Actually since it's an optimization for less >>>>>>IPIs, check softirq_pending(cpu) directly instead of sticking to one >>>>>>bit only. >>>>>> >>>>>>Signed-off-by: Quan Xu <xuquan8@huawei.com> >>>>>>--- >>>>>> xen/arch/x86/hvm/vmx/vmx.c | 3 +-- >>>>>> 1 file changed, 1 insertion(+), 2 deletions(-) >>>>>> >>>>>>diff --git a/xen/arch/x86/hvm/vmx/vmx.c >>b/xen/arch/x86/hvm/vmx/vmx.c >>>>>>index 61925cf..3887c32 100644 >>>>>>--- a/xen/arch/x86/hvm/vmx/vmx.c >>>>>>+++ b/xen/arch/x86/hvm/vmx/vmx.c >>>>>>@@ -1846,8 +1846,7 @@ static void >>>>__vmx_deliver_posted_interrupt(struct vcpu *v) >>>>>> { >>>>>> unsigned int cpu = v->processor; >>>>>> >>>>>>- if ( !test_and_set_bit(VCPU_KICK_SOFTIRQ, >>>>&softirq_pending(cpu)) >>>>>>- && (cpu != smp_processor_id()) ) >>>>>>+ if ( !softirq_pending(cpu) && (cpu != smp_processor_id()) ) >>> >>> Jan, >>> could you help me present the definition of ' smp_processor_id()' and ' >>> current' in __vmx_deliver_posted_interrupt() ? thanks.. >> >>I'm afraid I don't understand the request. > > IOW, > which vcpu does the 'current' refer to? > which cpu does the ' smp_processor_id()' refer to? current: currently running vCPU smp_processor_id(): processor ID of the CPU we're running on Jan
On February 23, 2017 7:01 PM, Jan Beulich wrote: >>>> On 23.02.17 at 11:53, <xuquan8@huawei.com> wrote: >> On February 23, 2017 5:59 PM, Jan Beulich wrote: >>>>>> On 23.02.17 at 10:28, <xuquan8@huawei.com> wrote: >>>> On February 18, 2017 12:33 AM, Jan Beulich wrote: >>>>>>>> On 17.02.17 at 09:49, <chao.gao@intel.com> wrote: >>>>>>>diff --git a/xen/arch/x86/hvm/vmx/vmx.c >>>b/xen/arch/x86/hvm/vmx/vmx.c >>>>>>>index 61925cf..3887c32 100644 >>>>>>>--- a/xen/arch/x86/hvm/vmx/vmx.c >>>>>>>+++ b/xen/arch/x86/hvm/vmx/vmx.c >>>>>>>@@ -1846,8 +1846,7 @@ static void >>>>>__vmx_deliver_posted_interrupt(struct vcpu *v) >>>>>>> { >>>>>>> unsigned int cpu = v->processor; >>>>>>> >>>>>>>- if ( !test_and_set_bit(VCPU_KICK_SOFTIRQ, >>>>>&softirq_pending(cpu)) >>>>>>>- && (cpu != smp_processor_id()) ) >>>>>>>+ if ( !softirq_pending(cpu) && (cpu != smp_processor_id()) >>>>>>>+ ) >>>> >>>> Jan, >>>> could you help me present the definition of ' smp_processor_id()' >and ' >>>> current' in __vmx_deliver_posted_interrupt() ? thanks.. >>> >>>I'm afraid I don't understand the request. >> >> IOW, >> which vcpu does the 'current' refer to? >> which cpu does the ' smp_processor_id()' refer to? > >current: currently running vCPU in a SMP machine, are there more than one currently running vCPU? I think so, the condition "if ( running && (in_irq() || (v != current)) )", in __vmx_deliver_posted_interrupt() looks strange -- when vCPU is running, why to check ' v != current '.. >smp_processor_id(): processor ID of the CPU we're running on > I think if vcpu is running, ' cpu != smp_processor_id() ' should be true. I think we could simplify __vmx_deliver_posted_interrupt(): 1. set VCPU_KICK_SOFTIRQ bit of v->processor. 2. IF vcpu is running: - send_IPI ELSE - vcpu_kick Thank you for your patience.. Quan
>>> On 23.02.17 at 12:55, <xuquan8@huawei.com> wrote: > On February 23, 2017 7:01 PM, Jan Beulich wrote: >>>>> On 23.02.17 at 11:53, <xuquan8@huawei.com> wrote: >>> IOW, >>> which vcpu does the 'current' refer to? >>> which cpu does the ' smp_processor_id()' refer to? >> >>current: currently running vCPU > > in a SMP machine, are there more than one currently running vCPU? Of course. "current" obviously is the one running on the CPU we're on. > I think so, the condition "if ( running && (in_irq() || (v != current)) )", in > __vmx_deliver_posted_interrupt() looks strange -- when vCPU is running, why to > check ' v != current '.. > > >>smp_processor_id(): processor ID of the CPU we're running on >> > I think if vcpu is running, ' cpu != smp_processor_id() ' should be true. > > > I think we could simplify __vmx_deliver_posted_interrupt(): > > 1. set VCPU_KICK_SOFTIRQ bit of v->processor. > 2. IF vcpu is running: > - send_IPI > ELSE > - vcpu_kick If this can be done in a race free manner (after all, the running state of a vCPU running on another pCPU may change at any time) ... Jan
On February 23, 2017 8:27 PM, Jan Beulich wrote: >>>> On 23.02.17 at 12:55, <xuquan8@huawei.com> wrote: >> On February 23, 2017 7:01 PM, Jan Beulich wrote: >>>>>> On 23.02.17 at 11:53, <xuquan8@huawei.com> wrote: >>>> IOW, >>>> which vcpu does the 'current' refer to? >>>> which cpu does the ' smp_processor_id()' refer to? >>> >>>current: currently running vCPU >> >> in a SMP machine, are there more than one currently running vCPU? > >Of course. "current" obviously is the one running on the CPU we're on. > Jan, Another question, it 'd be the _main_gap_.. If the pCPU runs __vmx_deliver_posted_interrupt() here, in a hyperver-context, _iiuc_, all of vCPUs , waiting in the run queue, are scheduled out.. So the 'current' is NULL, as there is no running vCPU.. correct me!! Quan >> I think so, the condition "if ( running && (in_irq() || (v != >> current)) )", in >> __vmx_deliver_posted_interrupt() looks strange -- when vCPU is >> running, why to check ' v != current '.. >> >> >>>smp_processor_id(): processor ID of the CPU we're running on >>> >> I think if vcpu is running, ' cpu != smp_processor_id() ' should be true. >> >> >> I think we could simplify __vmx_deliver_posted_interrupt(): >> >> 1. set VCPU_KICK_SOFTIRQ bit of v->processor. >> 2. IF vcpu is running: >> - send_IPI >> ELSE >> - vcpu_kick > >If this can be done in a race free manner (after all, the running state of a vCPU >running on another pCPU may change at any >time) ... > >Jan
>>> On 24.02.17 at 09:02, <xuquan8@huawei.com> wrote: > Another question, it 'd be the _main_gap_.. > If the pCPU runs __vmx_deliver_posted_interrupt() here, in a > hyperver-context, > _iiuc_, all of vCPUs , waiting in the run queue, are scheduled out.. So the > 'current' is NULL, as there is no running vCPU.. Excuse me, but these are fundamentals. "current" can never be NULL. If no guest vCPU is running, the idle vCPU for the pCPU would be. And switching to hypervisor context _does not_ mean the current vCPU changes. In particular, vlapic_set_irq() (which is a few call stack layers up from __vmx_deliver_posted_interrupt()) can run in various contexts afaics. Jan
On February 23, 2017 4:38 PM, Chao Gao wrote: >On Thu, Feb 23, 2017 at 11:55:15AM +0000, Xuquan (Quan Xu) wrote: >>On February 23, 2017 7:01 PM, Jan Beulich wrote: >>>>>> On 23.02.17 at 11:53, <xuquan8@huawei.com> wrote: >>>> On February 23, 2017 5:59 PM, Jan Beulich wrote: >>>>>>>> On 23.02.17 at 10:28, <xuquan8@huawei.com> wrote: >>>>>> On February 18, 2017 12:33 AM, Jan Beulich wrote: >>>>>>>>>> On 17.02.17 at 09:49, <chao.gao@intel.com> wrote: >>>>>>>>>diff --git a/xen/arch/x86/hvm/vmx/vmx.c >>>>>b/xen/arch/x86/hvm/vmx/vmx.c >>>>>>>>>index 61925cf..3887c32 100644 >>>>>>>>>--- a/xen/arch/x86/hvm/vmx/vmx.c >>>>>>>>>+++ b/xen/arch/x86/hvm/vmx/vmx.c >>>>>>>>>@@ -1846,8 +1846,7 @@ static void >>>>>>>__vmx_deliver_posted_interrupt(struct vcpu *v) >>>>>>>>> { >>>>>>>>> unsigned int cpu = v->processor; >>>>>>>>> >>>>>>>>>- if ( !test_and_set_bit(VCPU_KICK_SOFTIRQ, >>>>>>>&softirq_pending(cpu)) >>>>>>>>>- && (cpu != smp_processor_id()) ) >>>>>>>>>+ if ( !softirq_pending(cpu) && (cpu != >>>>>>>>>+ smp_processor_id()) >>>>>>>>>+ ) >>>>>> >>>>>> Jan, >>>>>> could you help me present the definition of ' smp_processor_id()' >>>and ' >>>>>> current' in __vmx_deliver_posted_interrupt() ? thanks.. >>>>> >>>>>I'm afraid I don't understand the request. >>>> >>>> IOW, >>>> which vcpu does the 'current' refer to? >>>> which cpu does the ' smp_processor_id()' refer to? >>> >>>current: currently running vCPU >> >>in a SMP machine, are there more than one currently running vCPU? >>I think so, the condition "if ( running && (in_irq() || (v != current)) )", in >__vmx_deliver_posted_interrupt() looks strange -- when vCPU is running, >why to check ' v != current '.. >> >> >>>smp_processor_id(): processor ID of the CPU we're running on >>> >>I think if vcpu is running, ' cpu != smp_processor_id() ' should be true. >> > >I am afraid it is not right. when the 'current' wants to deliver posted interrupt >to 'current', the vcpu is running and cpu == smp_processor_id(). > >> >>I think we could simplify __vmx_deliver_posted_interrupt(): >> >> 1. set VCPU_KICK_SOFTIRQ bit of v->processor. > >Do we need set VCPU_KICK_SOFTIRQ bit when vcpu is not running? IMO, if >vcpu is not running, just unblock (or, vcpu_kick since when vcpu is not >running, vcpu_kick is the same with vcpu_unblock )is enough. > >> 2. IF vcpu is running: >> - send_IPI > >Do we need send self-IPI when the target vcpu is 'current'? IMO, set >VCPU_KICK_SOFTIQR is enough. > >> ELSE >> - vcpu_kick >> >> Chao, I almost agree to your comments.. I will send out v2 later.. I appreciate your(KT, JB and you) comments that help me understand posted interrupt.. Quan
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c index 61925cf..3887c32 100644 --- a/xen/arch/x86/hvm/vmx/vmx.c +++ b/xen/arch/x86/hvm/vmx/vmx.c @@ -1846,8 +1846,7 @@ static void __vmx_deliver_posted_interrupt(struct vcpu *v) { unsigned int cpu = v->processor; - if ( !test_and_set_bit(VCPU_KICK_SOFTIRQ, &softirq_pending(cpu)) - && (cpu != smp_processor_id()) ) + if ( !softirq_pending(cpu) && (cpu != smp_processor_id()) ) send_IPI_mask(cpumask_of(cpu), posted_intr_vector); } }