Re: [PATCH] Remove a set operation for VCPU_KICK_SOFTIRQ when post interrupt to vm.

Re: [PATCH] Remove a set operation for VCPU_KICK_SOFTIRQ when post interrupt to vm.

[Liuqiming (John)]

I believe this also has something to do with a windows guest boot hang 
issue.

It randomly occured, when boot a guest has windows 2008 os and pv-driver 
installed.
The boot process hangs when wait xenstored replay event signal.

It can be reproduced after hundreds reboot using the xen staging branch.
But after I changed this code the hang issue can not reproduce.

Any Ideas?

On 2015/9/7 22:46, - wrote:
> From: liuqiming 00178499 <john.liuqiming@huawei.com>
>
> This set operation doesn't make any sense, and it will block later
> interrupt injected into vm (by vcpu_kick or deliver_posted_intr),
> which will cause a performance issue on CPU supporting posted-interrupt.
>
> Signed-off-by: Qiming Liu <john.liuqiming@huawei.com>
> Cc: Yang Zhang <yang.z.zhang@Intel.com>
> ---
>   xen/arch/x86/hvm/vmx/vmx.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index 2582cdd..9480b44 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -1681,7 +1681,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))
> +        if ( !test_bit(VCPU_KICK_SOFTIRQ, &softirq_pending(cpu))
>                && (cpu != smp_processor_id()) )
>               send_IPI_mask(cpumask_of(cpu), posted_intr_vector);
>       }

Re: [PATCH] Remove a set operation for VCPU_KICK_SOFTIRQ when post interrupt to vm.

[Jan Beulich]

>>> On 07.09.15 at 16:24, <john.liuqiming@huawei.com> wrote:
> I believe this also has something to do with a windows guest boot hang 
> issue.
> 
> It randomly occured, when boot a guest has windows 2008 os and pv-driver 
> installed.
> The boot process hangs when wait xenstored replay event signal.
> 
> It can be reproduced after hundreds reboot using the xen staging branch.
> But after I changed this code the hang issue can not reproduce.

The change below (which I don't think was ever posted to xen-devel)
does not make any sense, as it prohibits timely delivery of guest
interrupts. If there is an issue, I think you'd need to start with clearly
describing the issue you see and what you think it is being caused by.

Jan

> On 2015/9/7 22:46, - wrote:
>> From: liuqiming 00178499 <john.liuqiming@huawei.com>
>>
>> This set operation doesn't make any sense, and it will block later
>> interrupt injected into vm (by vcpu_kick or deliver_posted_intr),
>> which will cause a performance issue on CPU supporting posted-interrupt.
>>
>> Signed-off-by: Qiming Liu <john.liuqiming@huawei.com>
>> Cc: Yang Zhang <yang.z.zhang@Intel.com>
>> ---
>>   xen/arch/x86/hvm/vmx/vmx.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
>> index 2582cdd..9480b44 100644
>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>> @@ -1681,7 +1681,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))
>> +        if ( !test_bit(VCPU_KICK_SOFTIRQ, &softirq_pending(cpu))
>>                && (cpu != smp_processor_id()) )
>>               send_IPI_mask(cpumask_of(cpu), posted_intr_vector);
>>       }
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org 
> http://lists.xen.org/xen-devel 

Re: [PATCH] Remove a set operation for VCPU_KICK_SOFTIRQ when post interrupt to vm.

[Hanweidong (Randy)]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 2015年9月7日 22:46
> To: Liuqiming (John)
> Cc: Hanweidong (Randy); Zhangwei (FF); yang.z.zhang@Intel.com; xen-
> devel@lists.xenproject.org
> Subject: Re: [Xen-devel] [PATCH] Remove a set operation for
> VCPU_KICK_SOFTIRQ when post interrupt to vm.
> 
> >>> On 07.09.15 at 16:24, <john.liuqiming@huawei.com> wrote:
> > I believe this also has something to do with a windows guest boot
> hang
> > issue.
> >
> > It randomly occured, when boot a guest has windows 2008 os and pv-
> driver
> > installed.
> > The boot process hangs when wait xenstored replay event signal.
> >
> > It can be reproduced after hundreds reboot using the xen staging
> branch.
> > But after I changed this code the hang issue can not reproduce.
> 
> The change below (which I don't think was ever posted to xen-devel)
> does not make any sense, as it prohibits timely delivery of guest
> interrupts. If there is an issue, I think you'd need to start with
> clearly

This change won't prohibit timely delivery of guest interrupts, intead, it helps to deliver guest interrupt timely. Posted interrupt delivery doesn't kick cpu, so it should not set VCPU_KICK_SOFTIRQ bit, and doesn't care about if VCPU_KICK_SOFTIRQ is set or not. if VCPU_KICK_SOFTIRQ is set, next interrupt will not be delivered due to test_and_set_bit check. What's more, it also impacts vcpu_kick() to kick cpu (smp_send_event_check_cpu) when VCPU_KICK_SOFTIRQ is set.

Weidong


> describing the issue you see and what you think it is being caused by.
> 
> Jan
> 
> > On 2015/9/7 22:46, - wrote:
> >> From: liuqiming 00178499 <john.liuqiming@huawei.com>
> >>
> >> This set operation doesn't make any sense, and it will block later
> >> interrupt injected into vm (by vcpu_kick or deliver_posted_intr),
> >> which will cause a performance issue on CPU supporting posted-
> interrupt.
> >>
> >> Signed-off-by: Qiming Liu <john.liuqiming@huawei.com>
> >> Cc: Yang Zhang <yang.z.zhang@Intel.com>
> >> ---
> >>   xen/arch/x86/hvm/vmx/vmx.c | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> >> index 2582cdd..9480b44 100644
> >> --- a/xen/arch/x86/hvm/vmx/vmx.c
> >> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> >> @@ -1681,7 +1681,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))
> >> +        if ( !test_bit(VCPU_KICK_SOFTIRQ, &softirq_pending(cpu))
> >>                && (cpu != smp_processor_id()) )
> >>               send_IPI_mask(cpumask_of(cpu), posted_intr_vector);
> >>       }
> >
> >
> >
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > http://lists.xen.org/xen-devel
> 
> 

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

Re: [PATCH] Remove a set operation for VCPU_KICK_SOFTIRQ when post interrupt to vm.

[Zhang, Yang Z]

Hanweidong (Randy) wrote on 2015-09-08:
> 
> Jan Beulich wrote on ent: 2015年9月7日 22:46:
>> Subject: Re: [Xen-devel] [PATCH] Remove a set operation for
>> VCPU_KICK_SOFTIRQ when post interrupt to vm.
>> 
>>>>> On 07.09.15 at 16:24, <john.liuqiming@huawei.com> wrote:
>>> I believe this also has something to do with a windows guest boot hang
>>> issue.
>>> 
>>> It randomly occured, when boot a guest has windows 2008 os and pv-
>>> driver installed. The boot process hangs when wait xenstored replay
>>> event signal.
>>> 
>>> It can be reproduced after hundreds reboot using the xen staging
>>> branch. But after I changed this code the hang issue can not reproduce.
>> 
>> The change below (which I don't think was ever posted to xen-devel)
>> does not make any sense, as it prohibits timely delivery of guest
>> interrupts. If there is an issue, I think you'd need to start with
>> clearly
> 
> This change won't prohibit timely delivery of guest interrupts,
> intead, it helps to deliver guest interrupt timely. Posted interrupt
> delivery doesn't kick cpu, so it should not set VCPU_KICK_SOFTIRQ bit,
> and doesn't care about if VCPU_KICK_SOFTIRQ is set or not. if
> VCPU_KICK_SOFTIRQ is set, next interrupt will not be delivered due to
> test_and_set_bit check. What's more, it also impacts vcpu_kick() to
> kick cpu (smp_send_event_check_cpu) when VCPU_KICK_SOFTIRQ is set.

The patch seems wrong to me since the interrupt will lost in some corner cases with those changes. Can you explain more detail like why next interrupt will get lost if set the softirq here?

Best regards,
Yang


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

Re: [PATCH] Remove a set operation for VCPU_KICK_SOFTIRQ when post interrupt to vm.

[Liuqiming (John)]

Ok, I will try to explain, correct me if I got anything wrong:

The problem here is not interrupts lost but interrupts not delivered in 
time.

there are basically two path to inject an interrupt into VM  (or vCPU to 
another vCPU):
Path 1, the traditional way:
       1) set bit  in vlapic IRR field which represent an interrupt, 
then kick vcpu
       2) a VCPU_KICK_SOFTIRQ softirq raised
       3) if VCPU_KICK_SOFTIRQ bit not set, then set it, otherwise 
return and do nothing
       4) send an EVENT_CHECK_VECTOR IPI  to target vcpu
       5) target vcpu will VMEXIT due to EXIT_REASON_EXTERNAL_INTERRUPT
       6) the interrupt handler basically do nothing
       7) interrupt in IRR will be evaluated
       8) VCPU_KICK_SOFTIRQ will be cleared when do_softirq
       9) there will be an interrupt inject into vcpu when VMENTRY

Path 2, the Posted-interrupt way (current logic):
       1) set bit in posted-interrupt descriptor which represent an 
interrupt
       2) if VCPU_KICK_SOFTIRQ bit not set, then set it, otherwise 
return and do nothing
       3) send an POSTED_INTR_NOTIFICATION_VECTOR IPI to target vcpu
       4) if target vcpu in non-ROOT mode it will receive the interrupt 
immediately otherwise interrupt will be injected when VMENTRY

As the first operation in both path is setting a interrupt represent 
bit, so no interrupts will lost.

The issue is:
in path 2, the first interrupt will cause VCPU_KICK_SOFTIRQ set to 1,
and unless a VMEXIT occured or somewhere called do_softirq directly,
VCPU_KICK_SOFTIRQ will not cleared, that will make the later interrupts 
injection  ignored at step 2),
which will delay irq handler process in VM.

And because path 2 set VCPU_KICK_SOFTIRQ to 1, the kick vcpu logic in 
path 1 will also return in step 3),
which make this vcpu only can handle interrupt when some other reason 
cause VMEXIT.

On 2015/9/8 10:46, Zhang, Yang Z wrote:
> Hanweidong (Randy) wrote on 2015-09-08:
>> Jan Beulich wrote on ent: 2015年9月7日 22:46:
>>> Subject: Re: [Xen-devel] [PATCH] Remove a set operation for
>>> VCPU_KICK_SOFTIRQ when post interrupt to vm.
>>>
>>>>>> On 07.09.15 at 16:24, <john.liuqiming@huawei.com> wrote:
>>>> I believe this also has something to do with a windows guest boot hang
>>>> issue.
>>>>
>>>> It randomly occured, when boot a guest has windows 2008 os and pv-
>>>> driver installed. The boot process hangs when wait xenstored replay
>>>> event signal.
>>>>
>>>> It can be reproduced after hundreds reboot using the xen staging
>>>> branch. But after I changed this code the hang issue can not reproduce.
>>> The change below (which I don't think was ever posted to xen-devel)
>>> does not make any sense, as it prohibits timely delivery of guest
>>> interrupts. If there is an issue, I think you'd need to start with
>>> clearly
>> This change won't prohibit timely delivery of guest interrupts,
>> intead, it helps to deliver guest interrupt timely. Posted interrupt
>> delivery doesn't kick cpu, so it should not set VCPU_KICK_SOFTIRQ bit,
>> and doesn't care about if VCPU_KICK_SOFTIRQ is set or not. if
>> VCPU_KICK_SOFTIRQ is set, next interrupt will not be delivered due to
>> test_and_set_bit check. What's more, it also impacts vcpu_kick() to
>> kick cpu (smp_send_event_check_cpu) when VCPU_KICK_SOFTIRQ is set.
> The patch seems wrong to me since the interrupt will lost in some corner cases with those changes. Can you explain more detail like why next interrupt will get lost if set the softirq here?
>
> Best regards,
> Yang
>
>



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

Re: [PATCH] Remove a set operation for VCPU_KICK_SOFTIRQ when post interrupt to vm.

[Zhang, Yang Z]

Liuqiming (John) wrote on 2015-09-08:
> Ok, I will try to explain, correct me if I got anything wrong:
> 
> The problem here is not interrupts lost but interrupts not delivered in
> time.
> 
> there are basically two path to inject an interrupt into VM  (or vCPU to
> another vCPU):
> Path 1, the traditional way:
>        1) set bit  in vlapic IRR field which represent an interrupt,
>        then kick vcpu 2) a VCPU_KICK_SOFTIRQ softirq raised 3) if
>        VCPU_KICK_SOFTIRQ bit not set, then set it, otherwise return and
>        do nothing 4) send an EVENT_CHECK_VECTOR IPI  to target vcpu 5)
>        target vcpu will VMEXIT due to EXIT_REASON_EXTERNAL_INTERRUPT 6)
>        the interrupt handler basically do nothing 7) interrupt in IRR
>        will be evaluated 8) VCPU_KICK_SOFTIRQ will be cleared when
>        do_softirq 9) there will be an interrupt inject into vcpu when
>        VMENTRY
> Path 2, the Posted-interrupt way (current logic):
>        1) set bit in posted-interrupt descriptor which represent an
>        interrupt 2) if VCPU_KICK_SOFTIRQ bit not set, then set it,
>        otherwise return and do nothing 3) send an
>        POSTED_INTR_NOTIFICATION_VECTOR IPI to target vcpu 4) if target
>        vcpu in non-ROOT mode it will receive the interrupt
> immediately otherwise interrupt will be injected when VMENTRY
> 
> As the first operation in both path is setting a interrupt represent
> bit, so no interrupts will lost.
> 
> The issue is:
> in path 2, the first interrupt will cause VCPU_KICK_SOFTIRQ set to 1,
> and unless a VMEXIT occured or somewhere called do_softirq directly,
> VCPU_KICK_SOFTIRQ will not cleared, that will make the later interrupts
> injection  ignored at step 2),
> which will delay irq handler process in VM.
> 
> And because path 2 set VCPU_KICK_SOFTIRQ to 1, the kick vcpu logic in
> path 1 will also return in step 3),
> which make this vcpu only can handle interrupt when some other reason
> cause VMEXIT.

Nice catch! But without set the softirq, the interrupt delivery also will be delay. Look at the code in vmx_do_vmentry:

cli 
        <---------------the virtual interrupt occurs here will be delay. Because there is no softirq pending and the following posted interrupt may consumed by another running VCPU, so the target VCPU will not aware there is pending virtual interrupt before next vmexit.
cmp  %ecx,(%rdx,%rax,1)
jnz  .Lvmx_process_softirqs

I need more time to think it.

Best regards,
Yang

Re: [PATCH] Remove a set operation for VCPU_KICK_SOFTIRQ when post interrupt to vm.

[Jan Beulich]

>>> On 08.09.15 at 10:10, <yang.z.zhang@intel.com> wrote:
> Liuqiming (John) wrote on 2015-09-08:
>> Ok, I will try to explain, correct me if I got anything wrong:
>> 
>> The problem here is not interrupts lost but interrupts not delivered in
>> time.
>> 
>> there are basically two path to inject an interrupt into VM  (or vCPU to
>> another vCPU):
>> Path 1, the traditional way:
>>        1) set bit  in vlapic IRR field which represent an interrupt,
>>        then kick vcpu 2) a VCPU_KICK_SOFTIRQ softirq raised 3) if
>>        VCPU_KICK_SOFTIRQ bit not set, then set it, otherwise return and
>>        do nothing 4) send an EVENT_CHECK_VECTOR IPI  to target vcpu 5)
>>        target vcpu will VMEXIT due to EXIT_REASON_EXTERNAL_INTERRUPT 6)
>>        the interrupt handler basically do nothing 7) interrupt in IRR
>>        will be evaluated 8) VCPU_KICK_SOFTIRQ will be cleared when
>>        do_softirq 9) there will be an interrupt inject into vcpu when
>>        VMENTRY
>> Path 2, the Posted-interrupt way (current logic):
>>        1) set bit in posted-interrupt descriptor which represent an
>>        interrupt 2) if VCPU_KICK_SOFTIRQ bit not set, then set it,
>>        otherwise return and do nothing 3) send an
>>        POSTED_INTR_NOTIFICATION_VECTOR IPI to target vcpu 4) if target
>>        vcpu in non-ROOT mode it will receive the interrupt
>> immediately otherwise interrupt will be injected when VMENTRY
>> 
>> As the first operation in both path is setting a interrupt represent
>> bit, so no interrupts will lost.
>> 
>> The issue is:
>> in path 2, the first interrupt will cause VCPU_KICK_SOFTIRQ set to 1,
>> and unless a VMEXIT occured or somewhere called do_softirq directly,
>> VCPU_KICK_SOFTIRQ will not cleared, that will make the later interrupts
>> injection  ignored at step 2),
>> which will delay irq handler process in VM.
>> 
>> And because path 2 set VCPU_KICK_SOFTIRQ to 1, the kick vcpu logic in
>> path 1 will also return in step 3),
>> which make this vcpu only can handle interrupt when some other reason
>> cause VMEXIT.
> 
> Nice catch! But without set the softirq, the interrupt delivery also will be 
> delay.

Right. The patch as it stands, while improving performance, breaks
correctness (by - theoretically - deferring delivery of the interrupt
indefinitely). Yet there certainly is room for improvement, by finding
a correct condition for suppressing the softirq.

Jan

Re: [PATCH] Remove a set operation for VCPU_KICK_SOFTIRQ when post interrupt to vm.

[Zhang, Yang Z]

Zhang, Yang Z wrote on 2015-09-08:
> Liuqiming (John) wrote on 2015-09-08:
>> Ok, I will try to explain, correct me if I got anything wrong:
>> 
>> The problem here is not interrupts lost but interrupts not delivered
>> in time.
>> 
>> there are basically two path to inject an interrupt into VM  (or
>> vCPU to another vCPU):
>> Path 1, the traditional way:
>>        1) set bit  in vlapic IRR field which represent an interrupt,
>>        then kick vcpu 2) a VCPU_KICK_SOFTIRQ softirq raised 3) if
>>        VCPU_KICK_SOFTIRQ bit not set, then set it, otherwise return and
>>        do nothing 4) send an EVENT_CHECK_VECTOR IPI  to target vcpu 5)
>>        target vcpu will VMEXIT due to EXIT_REASON_EXTERNAL_INTERRUPT 6)
>>        the interrupt handler basically do nothing 7) interrupt in IRR
>>        will be evaluated 8) VCPU_KICK_SOFTIRQ will be cleared when
>>        do_softirq 9) there will be an interrupt inject into vcpu when
>>        VMENTRY Path 2, the Posted-interrupt way (current logic): 1) set
>>        bit in posted-interrupt descriptor which represent an interrupt
>>        2) if VCPU_KICK_SOFTIRQ bit not set, then set it, otherwise
>>        return and do nothing 3) send an POSTED_INTR_NOTIFICATION_VECTOR
>>        IPI to target vcpu 4) if target vcpu in non-ROOT mode it will
>>        receive the interrupt
>> immediately otherwise interrupt will be injected when VMENTRY
>> 
>> As the first operation in both path is setting a interrupt represent
>> bit, so no interrupts will lost.
>> 
>> The issue is:
>> in path 2, the first interrupt will cause VCPU_KICK_SOFTIRQ set to
>> 1, and unless a VMEXIT occured or somewhere called do_softirq
>> directly, VCPU_KICK_SOFTIRQ will not cleared, that will make the
>> later interrupts injection  ignored at step 2), which will delay irq
>> handler process in VM.
>> 
>> And because path 2 set VCPU_KICK_SOFTIRQ to 1, the kick vcpu logic
>> in path 1 will also return in step 3), which make this vcpu only can
>> handle interrupt when some other reason cause VMEXIT.
> 
> Nice catch! But without set the softirq, the interrupt delivery also will be delay.
> Look at the code in vmx_do_vmentry:
> 
> cli
>         <---------------the virtual interrupt occurs here will be
> delay. Because there is no softirq pending and the following posted
> interrupt may consumed by another running VCPU, so the target VCPU
> will not aware there is pending virtual interrupt before next vmexit.
> cmp  %ecx,(%rdx,%rax,1)
> jnz  .Lvmx_process_softirqs
> 
> I need more time to think it.

Hi Jan and Dario,

I have a quick check on current code. I am curious that is current Xen preemptive? The variable preempt_count is only checked in in_atomic() which never use in latest Xen. Also, when return from an interrupt handler, hypervisor didn't check whether reschedule is needed if the interrupt is occurred in kernel context.
ENTRY(ret_from_intr)
        GET_CURRENT(%rbx)
        testb $3,UREGS_cs(%rsp)
        jz    restore_all_xen              //call iret directly to restore previous context where interrupt occur if it is in kernel space.

If Xen isn't preemptive, the above case I mentioned should never happen since the VCPU still run in the same PCPU. Am I right?

Best regards,
Yang

Re: [PATCH] Remove a set operation for VCPU_KICK_SOFTIRQ when post interrupt to vm.

[Jan Beulich]

>>> "Zhang, Yang Z" <yang.z.zhang@intel.com> 09/18/15 2:29 AM >>>
>Zhang, Yang Z wrote on 2015-09-08:
>I have a quick check on current code. I am curious that is current Xen
> preemptive?

> Also, when return from an interrupt handler, hypervisor didn't check
> whether reschedule is needed if the interrupt is occurred in kernel context.
>ENTRY(ret_from_intr)
        >GET_CURRENT(%rbx)
        >testb $3,UREGS_cs(%rsp)
        >jz    restore_all_xen              //call iret directly to restore previous context where interrupt occur if it is in kernel space.

>If Xen isn't preemptive, the above case I mentioned should never happen since
> the VCPU still run in the same PCPU. Am I right?

I have to admit that I don't see the connection to preemptiveness: The reference
above is to a code section with interrupts disabled.

Jan

Re: [PATCH] Remove a set operation for VCPU_KICK_SOFTIRQ when post interrupt to vm.

[Zhang, Yang Z]

Jan Beulich wrote on 2015-09-18:
>>>> "Zhang, Yang Z" <yang.z.zhang@intel.com> 09/18/15 2:29 AM >>>
>> Zhang, Yang Z wrote on 2015-09-08:
>> I have a quick check on current code. I am curious that is current Xen
>> preemptive?
>> 
>> Also, when return from an interrupt handler, hypervisor didn't check
>> whether reschedule is needed if the interrupt is occurred in kernel context.
>> ENTRY(ret_from_intr)
>> GET_CURRENT(%rbx)
>> testb $3,UREGS_cs(%rsp)
>> jz    restore_all_xen              //call iret directly to restore
> previous context where interrupt occur if it is in kernel space.
> 
>> If Xen isn't preemptive, the above case I mentioned should never happen since
>> the VCPU still run in the same PCPU. Am I right?
> 
> I have to admit that I don't see the connection to preemptiveness: The
> reference above is to a code section with interrupts disabled.

Ok. Maybe I am asking a wrong question. My initial question is how Xen to support the preemption. For example, if VCPU is running in hypervisor (executing any code), can this VCPU be preempted immediately? If yes, how to achieve it because I didn't find any related code. I know if the PCPU get an SCHED_SOFTIRQ, the VCPU will do the schedule before do vmentry. But it only happens before do vmentry. Is it possible that VCPU been preempted at any time when running in hypervisor mode?

Best regards,
Yang

Re: [PATCH] Remove a set operation for VCPU_KICK_SOFTIRQ when post interrupt to vm.

[Andrew Cooper]

On 18/09/15 12:40, Zhang, Yang Z wrote:
> Jan Beulich wrote on 2015-09-18:
>>>>> "Zhang, Yang Z" <yang.z.zhang@intel.com> 09/18/15 2:29 AM >>>
>>> Zhang, Yang Z wrote on 2015-09-08:
>>> I have a quick check on current code. I am curious that is current Xen
>>> preemptive?
>>>
>>> Also, when return from an interrupt handler, hypervisor didn't check
>>> whether reschedule is needed if the interrupt is occurred in kernel context.
>>> ENTRY(ret_from_intr)
>>> GET_CURRENT(%rbx)
>>> testb $3,UREGS_cs(%rsp)
>>> jz    restore_all_xen              //call iret directly to restore
>> previous context where interrupt occur if it is in kernel space.
>>
>>> If Xen isn't preemptive, the above case I mentioned should never happen since
>>> the VCPU still run in the same PCPU. Am I right?
>> I have to admit that I don't see the connection to preemptiveness: The
>> reference above is to a code section with interrupts disabled.
> Ok. Maybe I am asking a wrong question. My initial question is how Xen to support the preemption. For example, if VCPU is running in hypervisor (executing any code), can this VCPU be preempted immediately? If yes, how to achieve it because I didn't find any related code. I know if the PCPU get an SCHED_SOFTIRQ, the VCPU will do the schedule before do vmentry. But it only happens before do vmentry. Is it possible that VCPU been preempted at any time when running in hypervisor mode?

All vcpus may be preempted at any time by the Xen scheduler.

However, paths through the hypervisor are synchronous, so a vcpu will
notice that it has been preempted on the return-to-guest path, and a
context switch will occur as appropriate.

~Andrew

Re: [PATCH] Remove a set operation for VCPU_KICK_SOFTIRQ when post interrupt to vm.

[Zhang, Yang Z]

Andrew Cooper wrote on 2015-09-18:
> On 18/09/15 12:40, Zhang, Yang Z wrote:
>> Jan Beulich wrote on 2015-09-18:
>>>>>> "Zhang, Yang Z" <yang.z.zhang@intel.com> 09/18/15 2:29 AM >>>
>>>> Zhang, Yang Z wrote on 2015-09-08:
>>>> I have a quick check on current code. I am curious that is current
>>>> Xen preemptive?
>>>> 
>>>> Also, when return from an interrupt handler, hypervisor didn't
>>>> check whether reschedule is needed if the interrupt is occurred in kernel context.
>>>> ENTRY(ret_from_intr)
>>>> GET_CURRENT(%rbx)
>>>> testb $3,UREGS_cs(%rsp)
>>>> jz    restore_all_xen              //call iret directly to restore
>>> previous context where interrupt occur if it is in kernel space.
>>> 
>>>> If Xen isn't preemptive, the above case I mentioned should never
>>>> happen since the VCPU still run in the same PCPU. Am I right?
>>> I have to admit that I don't see the connection to preemptiveness:
>>> The reference above is to a code section with interrupts disabled.
>> Ok. Maybe I am asking a wrong question. My initial question is how
>> Xen to
> support the preemption. For example, if VCPU is running in hypervisor
> (executing any code), can this VCPU be preempted immediately? If yes,
> how to achieve it because I didn't find any related code. I know if
> the PCPU get an SCHED_SOFTIRQ, the VCPU will do the schedule before do
> vmentry. But it only happens before do vmentry. Is it possible that
> VCPU been preempted at any time when running in hypervisor mode?
> 
> All vcpus may be preempted at any time by the Xen scheduler.
> 
> However, paths through the hypervisor are synchronous, so a vcpu will
> notice that it has been preempted on the return-to-guest path, and a
> context switch will occur as appropriate.

So the context switch only occurs on the return-to-guest patch or if the VCPU call schedule() by itself. It seems a little different from linux kernel which possible to preempt a task at any point.

Best regards,
Yang

Re: [PATCH] Remove a set operation for VCPU_KICK_SOFTIRQ when post interrupt to vm.

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 1403 bytes --]

On Fri, 2015-09-18 at 11:50 +0000, Zhang, Yang Z wrote:
> Andrew Cooper wrote on 2015-09-18:
> > On 18/09/15 12:40, Zhang, Yang Z wrote:

> > >  But it only happens before do vmentry. Is it possible that
> > VCPU been preempted at any time when running in hypervisor mode?
> > 
> > All vcpus may be preempted at any time by the Xen scheduler.
> > 
> > However, paths through the hypervisor are synchronous, so a vcpu
> > will
> > notice that it has been preempted on the return-to-guest path, and
> > a
> > context switch will occur as appropriate.
> 
> So the context switch only occurs on the return-to-guest patch or if
> the VCPU call schedule() by itself. It seems a little different from
> linux kernel which possible to preempt a task at any point.
> 
It is, indeed. Xen does not 'preempt itself', like Linux does, and you
also don't call schedule() directly (like Linux does).

In fact, any time you want to reschedule, you need to raise
SCHEDULE_SOFTIRQ, which is then checked and serviced in do_softirq()
(and, for instance, not in process_pending_softirq(), specifically to
avoid preemptions!).

Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH] Remove a set operation for VCPU_KICK_SOFTIRQ when post interrupt to vm.

[Zhang, Yang Z]

Zhang, Yang Z wrote on 2015-09-08:
> Liuqiming (John) wrote on 2015-09-08:
>> Ok, I will try to explain, correct me if I got anything wrong:
>> 
>> The problem here is not interrupts lost but interrupts not delivered
>> in time.
>> 
>> there are basically two path to inject an interrupt into VM  (or
>> vCPU to another vCPU):
>> Path 1, the traditional way:
>>        1) set bit  in vlapic IRR field which represent an interrupt,
>>        then kick vcpu 2) a VCPU_KICK_SOFTIRQ softirq raised 3) if
>>        VCPU_KICK_SOFTIRQ bit not set, then set it, otherwise return and
>>        do nothing 4) send an EVENT_CHECK_VECTOR IPI  to target vcpu 5)
>>        target vcpu will VMEXIT due to EXIT_REASON_EXTERNAL_INTERRUPT 6)
>>        the interrupt handler basically do nothing 7) interrupt in IRR
>>        will be evaluated 8) VCPU_KICK_SOFTIRQ will be cleared when
>>        do_softirq 9) there will be an interrupt inject into vcpu when
>>        VMENTRY Path 2, the Posted-interrupt way (current logic): 1) set
>>        bit in posted-interrupt descriptor which represent an interrupt
>>        2) if VCPU_KICK_SOFTIRQ bit not set, then set it, otherwise
>>        return and do nothing 3) send an POSTED_INTR_NOTIFICATION_VECTOR
>>        IPI to target vcpu 4) if target vcpu in non-ROOT mode it will
>>        receive the interrupt
>> immediately otherwise interrupt will be injected when VMENTRY
>> 
>> As the first operation in both path is setting a interrupt represent
>> bit, so no interrupts will lost.
>> 
>> The issue is:
>> in path 2, the first interrupt will cause VCPU_KICK_SOFTIRQ set to
>> 1, and unless a VMEXIT occured or somewhere called do_softirq
>> directly, VCPU_KICK_SOFTIRQ will not cleared, that will make the
>> later interrupts injection  ignored at step 2), which will delay irq
>> handler process in VM.
>> 
>> And because path 2 set VCPU_KICK_SOFTIRQ to 1, the kick vcpu logic
>> in path 1 will also return in step 3), which make this vcpu only can
>> handle interrupt when some other reason cause VMEXIT.
> 
> Nice catch! But without set the softirq, the interrupt delivery also will be delay.
> Look at the code in vmx_do_vmentry:
> 
> cli
>         <---------------the virtual interrupt occurs here will be
> delay. Because there is no softirq pending and the following posted
> interrupt may consumed by another running VCPU, so the target VCPU
> will not aware there is pending virtual interrupt before next vmexit.
> cmp  %ecx,(%rdx,%rax,1)
> jnz  .Lvmx_process_softirqs
> 
> I need more time to think it.

Hi Qiming,

Can you help to take a look at this patch? Also, if possible, please help to do a testing since I don't have machine to test it. Thanks very much.

diff --git a/xen/arch/x86/hvm/vmx/entry.S b/xen/arch/x86/hvm/vmx/entry.S
index 664ed83..1ebb5d0 100644
--- a/xen/arch/x86/hvm/vmx/entry.S
+++ b/xen/arch/x86/hvm/vmx/entry.S
@@ -77,6 +77,9 @@ UNLIKELY_START(ne, realmode)
         call vmx_enter_realmode
 UNLIKELY_END(realmode)
 
+        bt   $0,VCPU_vmx_pi_ctrl(%rbx)
+        jc   .Lvmx_do_vmentry
+
         mov  %rsp,%rdi
         call vmx_vmenter_helper
         mov  VCPU_hvm_guest_cr2(%rbx),%rax
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index e0e9e75..315ce65 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -1678,8 +1678,9 @@ 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 ( !test_bit(VCPU_KICK_SOFTIRQ, &softirq_pending(cpu))
+             && pi_test_on(&v->arch.hvm_vmx.pi_desc)
+             && (cpu != smp_processor_id()))
             send_IPI_mask(cpumask_of(cpu), posted_intr_vector);
     }
 }
diff --git a/xen/arch/x86/x86_64/asm-offsets.c b/xen/arch/x86/x86_64/asm-offsets.c
index 447c650..985725f 100644
--- a/xen/arch/x86/x86_64/asm-offsets.c
+++ b/xen/arch/x86/x86_64/asm-offsets.c
@@ -108,6 +108,7 @@ void __dummy__(void)
     OFFSET(VCPU_vmx_emulate, struct vcpu, arch.hvm_vmx.vmx_emulate);
     OFFSET(VCPU_vm86_seg_mask, struct vcpu, arch.hvm_vmx.vm86_segment_mask);
     OFFSET(VCPU_hvm_guest_cr2, struct vcpu, arch.hvm_vcpu.guest_cr[2]);
+    OFFSET(VCPU_vmx_pi_ctrl, struct vcpu, arch.hvm_vmx.pi_desc.control);
     BLANK();
 
     OFFSET(VCPU_nhvm_guestmode, struct vcpu, arch.hvm_vcpu.nvcpu.nv_guestmode);
diff --git a/xen/include/asm-x86/hvm/vmx/vmx.h b/xen/include/asm-x86/hvm/vmx/vmx.h
index 35f804a..157a4fe 100644
--- a/xen/include/asm-x86/hvm/vmx/vmx.h
+++ b/xen/include/asm-x86/hvm/vmx/vmx.h
@@ -116,6 +116,11 @@ static inline void pi_set_on(struct pi_desc *pi_desc)
     set_bit(POSTED_INTR_ON, &pi_desc->control);
 }
 
+static inline int pi_test_on(struct pi_desc *pi_desc)
+{
+    return test_bit(POSTED_INTR_ON, &pi_desc->control);
+}
+
 static inline int pi_test_and_clear_on(struct pi_desc *pi_desc)
 {
     return test_and_clear_bit(POSTED_INTR_ON, &pi_desc->control);


Best regards,
Yang

Re: [PATCH] Remove a set operation for VCPU_KICK_SOFTIRQ when post interrupt to vm.

[Jan Beulich]

>>> On 23.09.15 at 05:50, <yang.z.zhang@intel.com> wrote:
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -1678,8 +1678,9 @@ 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 ( !test_bit(VCPU_KICK_SOFTIRQ, &softirq_pending(cpu))
> +             && pi_test_on(&v->arch.hvm_vmx.pi_desc)
> +             && (cpu != smp_processor_id()))
>              send_IPI_mask(cpumask_of(cpu), posted_intr_vector);
>      }
>  }

So this still removes the setting of the softirq - how can that be
correct (namely in the cpu == smp_processor_id() case)? Did you
perhaps mean

        if ( pi_test_on(&v->arch.hvm_vmx.pi_desc)
             && !test_and_set_bit(VCPU_KICK_SOFTIRQ, &softirq_pending(cpu))
             && (cpu != smp_processor_id()))

?

Jan

Re: [PATCH] Remove a set operation for VCPU_KICK_SOFTIRQ when post interrupt to vm.

[George Dunlap]

On Wed, Sep 23, 2015 at 8:42 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 23.09.15 at 05:50, <yang.z.zhang@intel.com> wrote:
>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>> @@ -1678,8 +1678,9 @@ 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 ( !test_bit(VCPU_KICK_SOFTIRQ, &softirq_pending(cpu))
>> +             && pi_test_on(&v->arch.hvm_vmx.pi_desc)
>> +             && (cpu != smp_processor_id()))
>>              send_IPI_mask(cpumask_of(cpu), posted_intr_vector);
>>      }
>>  }
>
> So this still removes the setting of the softirq - how can that be
> correct (namely in the cpu == smp_processor_id() case)? Did you
> perhaps mean
>
>         if ( pi_test_on(&v->arch.hvm_vmx.pi_desc)
>              && !test_and_set_bit(VCPU_KICK_SOFTIRQ, &softirq_pending(cpu))
>              && (cpu != smp_processor_id()))

So the problem before was setting the SOFTIRQ for another cpu but then
never sending an interrupt?

Is there a reason why this code isn't using cpu_raise_softirq() here,
rather than manually doing the same thing (and doing it incorrectly,
apparently)?

 -George

Re: [PATCH] Remove a set operation for VCPU_KICK_SOFTIRQ when post interrupt to vm.

[Zhang, Yang Z]

Jan Beulich wrote on 2015-09-23:
>>>> On 23.09.15 at 05:50, <yang.z.zhang@intel.com> wrote:
>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>> @@ -1678,8 +1678,9 @@ 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 ( !test_bit(VCPU_KICK_SOFTIRQ, &softirq_pending(cpu)) +      
>>       && pi_test_on(&v->arch.hvm_vmx.pi_desc) +             && (cpu !=
>> smp_processor_id()))
>>              send_IPI_mask(cpumask_of(cpu), posted_intr_vector);
>>      }
>>  }
> 
> So this still removes the setting of the softirq - how can that be
> correct (namely in the cpu == smp_processor_id() case)? Did you
> perhaps mean

Why it will cause problem? The pending interrupt is covered by the check before vmentry: if the outstanding bit is setting, it will redo the vmentry. So even there is no softirq, the pending interrupt still can be injected to guest in time.

--- a/xen/arch/x86/hvm/vmx/entry.S
+++ b/xen/arch/x86/hvm/vmx/entry.S
@@ -77,6 +77,9 @@ UNLIKELY_START(ne, realmode)
         call vmx_enter_realmode
 UNLIKELY_END(realmode)
 
+        bt   $0,VCPU_vmx_pi_ctrl(%rbx)
+        jc   .Lvmx_do_vmentry
+
         mov  %rsp,%rdi
         call vmx_vmenter_helper
         mov  VCPU_hvm_guest_cr2(%rbx),%rax


Best regards,
Yang

Re: [PATCH] Remove a set operation for VCPU_KICK_SOFTIRQ when post interrupt to vm.

[Zhang, Yang Z]

George Dunlap wrote on 2015-09-23:
> On Wed, Sep 23, 2015 at 8:42 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 23.09.15 at 05:50, <yang.z.zhang@intel.com> wrote:
>>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>>> @@ -1678,8 +1678,9 @@ 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 ( !test_bit(VCPU_KICK_SOFTIRQ, &softirq_pending(cpu)) +   
>>>          && pi_test_on(&v->arch.hvm_vmx.pi_desc) +             && (cpu
>>> != smp_processor_id()))
>>>              send_IPI_mask(cpumask_of(cpu), posted_intr_vector);
>>>      }
>>>  }
>> 
>> So this still removes the setting of the softirq - how can that be
>> correct (namely in the cpu == smp_processor_id() case)? Did you
>> perhaps mean
>> 
>>         if ( pi_test_on(&v->arch.hvm_vmx.pi_desc)
>>              && !test_and_set_bit(VCPU_KICK_SOFTIRQ,
>>              &softirq_pending(cpu)) && (cpu != smp_processor_id()))
> 
> So the problem before was setting the SOFTIRQ for another cpu but then
> never sending an interrupt?

No, the problem is the setting SOFTIRQ doesn’t be cleared in time and cause the subsequent interrupt injection be delayed.

> 
> Is there a reason why this code isn't using cpu_raise_softirq() here,
> rather than manually doing the same thing (and doing it incorrectly, apparently)?

The vector is different which uses posted_intr_vector here not EVENT_CHECK_VECTOR.

> 
>  -George


Best regards,
Yang


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

Re: [PATCH] Remove a set operation for VCPU_KICK_SOFTIRQ when post interrupt to vm.

[Jan Beulich]

>>> On 23.09.15 at 11:08, <George.Dunlap@eu.citrix.com> wrote:
> On Wed, Sep 23, 2015 at 8:42 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 23.09.15 at 05:50, <yang.z.zhang@intel.com> wrote:
>>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>>> @@ -1678,8 +1678,9 @@ 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 ( !test_bit(VCPU_KICK_SOFTIRQ, &softirq_pending(cpu))
>>> +             && pi_test_on(&v->arch.hvm_vmx.pi_desc)
>>> +             && (cpu != smp_processor_id()))
>>>              send_IPI_mask(cpumask_of(cpu), posted_intr_vector);
>>>      }
>>>  }
>>
>> So this still removes the setting of the softirq - how can that be
>> correct (namely in the cpu == smp_processor_id() case)? Did you
>> perhaps mean
>>
>>         if ( pi_test_on(&v->arch.hvm_vmx.pi_desc)
>>              && !test_and_set_bit(VCPU_KICK_SOFTIRQ, &softirq_pending(cpu))
>>              && (cpu != smp_processor_id()))
> 
> So the problem before was setting the SOFTIRQ for another cpu but then
> never sending an interrupt?

No, the problem is a performance one, resulting from there being
too many softirqs (often, but not always pointlessly raised). At
least that's my recollection from earlier discussion here.

> Is there a reason why this code isn't using cpu_raise_softirq() here,
> rather than manually doing the same thing (and doing it incorrectly,
> apparently)?

I had asked the same question. The subtle difference being the vector
that gets used for the IPI.

Jan

Re: [PATCH] Remove a set operation for VCPU_KICK_SOFTIRQ when post interrupt to vm.

[Jan Beulich]

>>> On 23.09.15 at 11:15, <yang.z.zhang@intel.com> wrote:
> Jan Beulich wrote on 2015-09-23:
>>>>> On 23.09.15 at 05:50, <yang.z.zhang@intel.com> wrote:
>>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>>> @@ -1678,8 +1678,9 @@ 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 ( !test_bit(VCPU_KICK_SOFTIRQ, &softirq_pending(cpu)) +      
>>>       && pi_test_on(&v->arch.hvm_vmx.pi_desc) +             && (cpu !=
>>> smp_processor_id()))
>>>              send_IPI_mask(cpumask_of(cpu), posted_intr_vector);
>>>      }
>>>  }
>> 
>> So this still removes the setting of the softirq - how can that be
>> correct (namely in the cpu == smp_processor_id() case)? Did you
>> perhaps mean
> 
> Why it will cause problem? The pending interrupt is covered by the check 
> before vmentry: if the outstanding bit is setting, it will redo the vmentry. 
> So even there is no softirq, the pending interrupt still can be injected to 
> guest in time.

Then what's the point of checking whether that softirq is pending?
Couldn't the entire check, including the IPI send, then go away?

Apart from that I just noticed that your jump to .Lvmx_do_vmentry
is wrong: At that label interrupts have to be enabled. And I guess
the check would also better be moved ahead of the emulation and
realmode check (in which case you could as well branch to
.Lvmx_process_softirqs and avoid said interrupt enabling problem).

Jan

Re: [PATCH] Remove a set operation for VCPU_KICK_SOFTIRQ when post interrupt to vm.

[George Dunlap]

On Wed, Sep 23, 2015 at 10:18 AM, Zhang, Yang Z <yang.z.zhang@intel.com> wrote:
> George Dunlap wrote on 2015-09-23:
>> On Wed, Sep 23, 2015 at 8:42 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>> On 23.09.15 at 05:50, <yang.z.zhang@intel.com> wrote:
>>>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>>>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>>>> @@ -1678,8 +1678,9 @@ 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 ( !test_bit(VCPU_KICK_SOFTIRQ, &softirq_pending(cpu)) +
>>>>          && pi_test_on(&v->arch.hvm_vmx.pi_desc) +             && (cpu
>>>> != smp_processor_id()))
>>>>              send_IPI_mask(cpumask_of(cpu), posted_intr_vector);
>>>>      }
>>>>  }
>>>
>>> So this still removes the setting of the softirq - how can that be
>>> correct (namely in the cpu == smp_processor_id() case)? Did you
>>> perhaps mean
>>>
>>>         if ( pi_test_on(&v->arch.hvm_vmx.pi_desc)
>>>              && !test_and_set_bit(VCPU_KICK_SOFTIRQ,
>>>              &softirq_pending(cpu)) && (cpu != smp_processor_id()))
>>
>> So the problem before was setting the SOFTIRQ for another cpu but then
>> never sending an interrupt?
>
> No, the problem is the setting SOFTIRQ doesn’t be cleared in time and cause the subsequent interrupt injection be delayed.

Sorry, I misread the original patch.

>> Is there a reason why this code isn't using cpu_raise_softirq() here,
>> rather than manually doing the same thing (and doing it incorrectly, apparently)?
>
> The vector is different which uses posted_intr_vector here not EVENT_CHECK_VECTOR.

Got it, thanks.

 -George

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

Re: [PATCH] Remove a set operation for VCPU_KICK_SOFTIRQ when post interrupt to vm.

[Zhang, Yang Z]

Jan Beulich wrote on 2015-09-23:
>>>> On 23.09.15 at 11:15, <yang.z.zhang@intel.com> wrote:
>> Jan Beulich wrote on 2015-09-23:
>>>>>> On 23.09.15 at 05:50, <yang.z.zhang@intel.com> wrote:
>>>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>>>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>>>> @@ -1678,8 +1678,9 @@ 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 ( !test_bit(VCPU_KICK_SOFTIRQ, &softirq_pending(cpu)) +
>>>>       && pi_test_on(&v->arch.hvm_vmx.pi_desc) +             &&
> (cpu !=
>>>> smp_processor_id()))
>>>>              send_IPI_mask(cpumask_of(cpu), posted_intr_vector);
>>>>      }
>>>>  }
>>> 
>>> So this still removes the setting of the softirq - how can that be
>>> correct (namely in the cpu == smp_processor_id() case)? Did you
>>> perhaps mean
>> 
>> Why it will cause problem? The pending interrupt is covered by the
>> check before vmentry: if the outstanding bit is setting, it will redo
>> the vmentry. So even there is no softirq, the pending interrupt still
>> can be injected to guest in time.
> 
> Then what's the point of checking whether that softirq is pending?

There is no need to send the PI since the CPU needs to handle the softirq immediately if there is softirq pending. And we can let the next vmenty to inject pending interrupt .

> Couldn't the entire check, including the IPI send, then go away?

Yes, there is no correctness issue if the entire check is removed. But to avoid the needless PI, it's better to have the check there. Also, I don't think the IPI send can be avoided here. 

> 
> Apart from that I just noticed that your jump to .Lvmx_do_vmentry is
> wrong: At that label interrupts have to be enabled. And I guess the

You are right. I forget to enable the interrupt.

> check would also better be moved ahead of the emulation and realmode
> check (in which case you could as well branch to
> .Lvmx_process_softirqs and avoid said interrupt enabling problem).

If we put the check ahead of emulation and realmode check, it may call vmx_do_vmentry twice: one to pick up the interrupt and one to do emulation/realmode. To avoid it, I choice to put it behind them.

> 
> Jan


Best regards,
Yang

Re: [PATCH] Remove a set operation for VCPU_KICK_SOFTIRQ when post interrupt to vm.

[Hanweidong (Randy)]

> -----Original Message-----
> From: Zhang, Yang Z [mailto:yang.z.zhang@intel.com]
> Sent: 2015年9月23日 11:51
> To: Liuqiming (John); Hanweidong (Randy); Jan Beulich
> Cc: Zhangwei (FF); xen-devel@lists.xenproject.org
> Subject: RE: [Xen-devel] [PATCH] Remove a set operation for
> VCPU_KICK_SOFTIRQ when post interrupt to vm.
> 
> Zhang, Yang Z wrote on 2015-09-08:
> > Liuqiming (John) wrote on 2015-09-08:
> >> Ok, I will try to explain, correct me if I got anything wrong:
> >>
> >> The problem here is not interrupts lost but interrupts not delivered
> >> in time.
> >>
> >> there are basically two path to inject an interrupt into VM  (or
> >> vCPU to another vCPU):
> >> Path 1, the traditional way:
> >>        1) set bit  in vlapic IRR field which represent an interrupt,
> >>        then kick vcpu 2) a VCPU_KICK_SOFTIRQ softirq raised 3) if
> >>        VCPU_KICK_SOFTIRQ bit not set, then set it, otherwise return
> and
> >>        do nothing 4) send an EVENT_CHECK_VECTOR IPI  to target vcpu
> 5)
> >>        target vcpu will VMEXIT due to EXIT_REASON_EXTERNAL_INTERRUPT
> 6)
> >>        the interrupt handler basically do nothing 7) interrupt in
> IRR
> >>        will be evaluated 8) VCPU_KICK_SOFTIRQ will be cleared when
> >>        do_softirq 9) there will be an interrupt inject into vcpu
> when
> >>        VMENTRY Path 2, the Posted-interrupt way (current logic): 1)
> set
> >>        bit in posted-interrupt descriptor which represent an
> interrupt
> >>        2) if VCPU_KICK_SOFTIRQ bit not set, then set it, otherwise
> >>        return and do nothing 3) send an
> POSTED_INTR_NOTIFICATION_VECTOR
> >>        IPI to target vcpu 4) if target vcpu in non-ROOT mode it will
> >>        receive the interrupt
> >> immediately otherwise interrupt will be injected when VMENTRY
> >>
> >> As the first operation in both path is setting a interrupt represent
> >> bit, so no interrupts will lost.
> >>
> >> The issue is:
> >> in path 2, the first interrupt will cause VCPU_KICK_SOFTIRQ set to
> >> 1, and unless a VMEXIT occured or somewhere called do_softirq
> >> directly, VCPU_KICK_SOFTIRQ will not cleared, that will make the
> >> later interrupts injection  ignored at step 2), which will delay irq
> >> handler process in VM.
> >>
> >> And because path 2 set VCPU_KICK_SOFTIRQ to 1, the kick vcpu logic
> >> in path 1 will also return in step 3), which make this vcpu only can
> >> handle interrupt when some other reason cause VMEXIT.
> >
> > Nice catch! But without set the softirq, the interrupt delivery also
> will be delay.
> > Look at the code in vmx_do_vmentry:
> >
> > cli
> >         <---------------the virtual interrupt occurs here will be
> > delay. Because there is no softirq pending and the following posted
> > interrupt may consumed by another running VCPU, so the target VCPU
> > will not aware there is pending virtual interrupt before next vmexit.
> > cmp  %ecx,(%rdx,%rax,1)
> > jnz  .Lvmx_process_softirqs
> >
> > I need more time to think it.
> 
> Hi Qiming,
> 
> Can you help to take a look at this patch? Also, if possible, please
> help to do a testing since I don't have machine to test it. Thanks very
> much.
> 
> diff --git a/xen/arch/x86/hvm/vmx/entry.S
> b/xen/arch/x86/hvm/vmx/entry.S
> index 664ed83..1ebb5d0 100644
> --- a/xen/arch/x86/hvm/vmx/entry.S
> +++ b/xen/arch/x86/hvm/vmx/entry.S
> @@ -77,6 +77,9 @@ UNLIKELY_START(ne, realmode)
>          call vmx_enter_realmode
>  UNLIKELY_END(realmode)
> 
> +        bt   $0,VCPU_vmx_pi_ctrl(%rbx)
> +        jc   .Lvmx_do_vmentry
> +
>          mov  %rsp,%rdi
>          call vmx_vmenter_helper
>          mov  VCPU_hvm_guest_cr2(%rbx),%rax
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index e0e9e75..315ce65 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -1678,8 +1678,9 @@ 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 ( !test_bit(VCPU_KICK_SOFTIRQ, &softirq_pending(cpu))
> +             && pi_test_on(&v->arch.hvm_vmx.pi_desc)

Why need pi_test_on() here?

Weidong

> +             && (cpu != smp_processor_id()))
>              send_IPI_mask(cpumask_of(cpu), posted_intr_vector);
>      }
>  }
> diff --git a/xen/arch/x86/x86_64/asm-offsets.c
> b/xen/arch/x86/x86_64/asm-offsets.c
> index 447c650..985725f 100644
> --- a/xen/arch/x86/x86_64/asm-offsets.c
> +++ b/xen/arch/x86/x86_64/asm-offsets.c
> @@ -108,6 +108,7 @@ void __dummy__(void)
>      OFFSET(VCPU_vmx_emulate, struct vcpu, arch.hvm_vmx.vmx_emulate);
>      OFFSET(VCPU_vm86_seg_mask, struct vcpu,
> arch.hvm_vmx.vm86_segment_mask);
>      OFFSET(VCPU_hvm_guest_cr2, struct vcpu, arch.hvm_vcpu.guest_cr[2]);
> +    OFFSET(VCPU_vmx_pi_ctrl, struct vcpu,
> arch.hvm_vmx.pi_desc.control);
>      BLANK();
> 
>      OFFSET(VCPU_nhvm_guestmode, struct vcpu,
> arch.hvm_vcpu.nvcpu.nv_guestmode);
> diff --git a/xen/include/asm-x86/hvm/vmx/vmx.h b/xen/include/asm-
> x86/hvm/vmx/vmx.h
> index 35f804a..157a4fe 100644
> --- a/xen/include/asm-x86/hvm/vmx/vmx.h
> +++ b/xen/include/asm-x86/hvm/vmx/vmx.h
> @@ -116,6 +116,11 @@ static inline void pi_set_on(struct pi_desc
> *pi_desc)
>      set_bit(POSTED_INTR_ON, &pi_desc->control);
>  }
> 
> +static inline int pi_test_on(struct pi_desc *pi_desc)
> +{
> +    return test_bit(POSTED_INTR_ON, &pi_desc->control);
> +}
> +
>  static inline int pi_test_and_clear_on(struct pi_desc *pi_desc)
>  {
>      return test_and_clear_bit(POSTED_INTR_ON, &pi_desc->control);
> 
> 
> Best regards,
> Yang

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

Re: [PATCH] Remove a set operation for VCPU_KICK_SOFTIRQ when post interrupt to vm.

[Zhang, Yang Z]

Hanweidong (Randy) wrote on 2015-09-23:
> 
> 
> Zhang, Yang Z wrote on ent: 2015年9月23日 11:51:
>> VCPU_KICK_SOFTIRQ when post interrupt to vm.
>> 
>> Zhang, Yang Z wrote on 2015-09-08:
>>> Liuqiming (John) wrote on 2015-09-08:
>>>> Ok, I will try to explain, correct me if I got anything wrong:
>>>> 
>>>> The problem here is not interrupts lost but interrupts not
>>>> delivered in time.
>>>> 
>>>> there are basically two path to inject an interrupt into VM  (or
>>>> vCPU to another vCPU):
>>>> Path 1, the traditional way:
>>>>        1) set bit  in vlapic IRR field which represent an interrupt,
>>>>        then kick vcpu 2) a VCPU_KICK_SOFTIRQ softirq raised 3) if
>>>>        VCPU_KICK_SOFTIRQ bit not set, then set it, otherwise
>>>> return
>> and
>>>>        do nothing 4) send an EVENT_CHECK_VECTOR IPI  to target
> vcpu
>> 5)
>>>>        target vcpu will VMEXIT due to
>>>> EXIT_REASON_EXTERNAL_INTERRUPT
>> 6)
>>>>        the interrupt handler basically do nothing 7) interrupt in IRR
>>>>        will be evaluated 8) VCPU_KICK_SOFTIRQ will be cleared when
>>>>        do_softirq 9) there will be an interrupt inject into vcpu when
>>>>        VMENTRY Path 2, the Posted-interrupt way (current logic):
>>>> 1)
>> set
>>>>        bit in posted-interrupt descriptor which represent an
>>>>        interrupt 2) if VCPU_KICK_SOFTIRQ bit not set, then set it,
>>>>        otherwise return and do nothing 3) send an
>>>>        POSTED_INTR_NOTIFICATION_VECTOR IPI to target vcpu 4) if
>>>>        target vcpu in non-ROOT mode it will receive the interrupt
>>>> immediately otherwise interrupt will be injected when VMENTRY
>>>> 
>>>> As the first operation in both path is setting a interrupt
>>>> represent bit, so no interrupts will lost.
>>>> 
>>>> The issue is:
>>>> in path 2, the first interrupt will cause VCPU_KICK_SOFTIRQ set
>>>> to 1, and unless a VMEXIT occured or somewhere called do_softirq
>>>> directly, VCPU_KICK_SOFTIRQ will not cleared, that will make the
>>>> later interrupts injection  ignored at step 2), which will delay
>>>> irq handler process in VM.
>>>> 
>>>> And because path 2 set VCPU_KICK_SOFTIRQ to 1, the kick vcpu
>>>> logic in path 1 will also return in step 3), which make this vcpu
>>>> only can handle interrupt when some other reason cause VMEXIT.
>>> 
>>> Nice catch! But without set the softirq, the interrupt delivery also
>>> will be delay. Look at the code in vmx_do_vmentry:
>>> 
>>> cli
>>>         <---------------the virtual interrupt occurs here will be
>>> delay. Because there is no softirq pending and the following
>>> posted interrupt may consumed by another running VCPU, so the
>>> target VCPU will not aware there is pending virtual interrupt before next vmexit.
>>> cmp  %ecx,(%rdx,%rax,1)
>>> jnz  .Lvmx_process_softirqs
>>> 
>>> I need more time to think it.
>> 
>> Hi Qiming,
>> 
>> Can you help to take a look at this patch? Also, if possible, please
>> help to do a testing since I don't have machine to test it. Thanks
>> very much.
>> 
>> diff --git a/xen/arch/x86/hvm/vmx/entry.S
>> b/xen/arch/x86/hvm/vmx/entry.S index 664ed83..1ebb5d0 100644
>> --- a/xen/arch/x86/hvm/vmx/entry.S
>> +++ b/xen/arch/x86/hvm/vmx/entry.S
>> @@ -77,6 +77,9 @@ UNLIKELY_START(ne, realmode)
>>          call vmx_enter_realmode
>>  UNLIKELY_END(realmode)
>> +        bt   $0,VCPU_vmx_pi_ctrl(%rbx)
>> +        jc   .Lvmx_do_vmentry
>> +
>>          mov  %rsp,%rdi
>>          call vmx_vmenter_helper
>>          mov  VCPU_hvm_guest_cr2(%rbx),%rax diff --git
>> a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c index
>> e0e9e75..315ce65 100644
>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>> @@ -1678,8 +1678,9 @@ 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 ( !test_bit(VCPU_KICK_SOFTIRQ, &softirq_pending(cpu))
>> +             && pi_test_on(&v->arch.hvm_vmx.pi_desc)
> 
> Why need pi_test_on() here?

on bit is cleared means the interrupt is consumed by target VCPU already. So there is no need to send the PI.

Best regards,
Yang


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

Re: [PATCH] Remove a set operation for VCPU_KICK_SOFTIRQ when post interrupt to vm.

[Liuqiming (John)]

On 2015/9/23 21:41, Zhang, Yang Z wrote:
> Hanweidong (Randy) wrote on 2015-09-23:
>>
>> Zhang, Yang Z wrote on ent: 2015年9月23日 11:51:
>>> VCPU_KICK_SOFTIRQ when post interrupt to vm.
>>>
>>> Zhang, Yang Z wrote on 2015-09-08:
>>>> Liuqiming (John) wrote on 2015-09-08:
>>>>> Ok, I will try to explain, correct me if I got anything wrong:
>>>>>
>>>>> The problem here is not interrupts lost but interrupts not
>>>>> delivered in time.
>>>>>
>>>>> there are basically two path to inject an interrupt into VM  (or
>>>>> vCPU to another vCPU):
>>>>> Path 1, the traditional way:
>>>>>         1) set bit  in vlapic IRR field which represent an interrupt,
>>>>>         then kick vcpu 2) a VCPU_KICK_SOFTIRQ softirq raised 3) if
>>>>>         VCPU_KICK_SOFTIRQ bit not set, then set it, otherwise
>>>>> return
>>> and
>>>>>         do nothing 4) send an EVENT_CHECK_VECTOR IPI  to target
>> vcpu
>>> 5)
>>>>>         target vcpu will VMEXIT due to
>>>>> EXIT_REASON_EXTERNAL_INTERRUPT
>>> 6)
>>>>>         the interrupt handler basically do nothing 7) interrupt in IRR
>>>>>         will be evaluated 8) VCPU_KICK_SOFTIRQ will be cleared when
>>>>>         do_softirq 9) there will be an interrupt inject into vcpu when
>>>>>         VMENTRY Path 2, the Posted-interrupt way (current logic):
>>>>> 1)
>>> set
>>>>>         bit in posted-interrupt descriptor which represent an
>>>>>         interrupt 2) if VCPU_KICK_SOFTIRQ bit not set, then set it,
>>>>>         otherwise return and do nothing 3) send an
>>>>>         POSTED_INTR_NOTIFICATION_VECTOR IPI to target vcpu 4) if
>>>>>         target vcpu in non-ROOT mode it will receive the interrupt
>>>>> immediately otherwise interrupt will be injected when VMENTRY
>>>>>
>>>>> As the first operation in both path is setting a interrupt
>>>>> represent bit, so no interrupts will lost.
>>>>>
>>>>> The issue is:
>>>>> in path 2, the first interrupt will cause VCPU_KICK_SOFTIRQ set
>>>>> to 1, and unless a VMEXIT occured or somewhere called do_softirq
>>>>> directly, VCPU_KICK_SOFTIRQ will not cleared, that will make the
>>>>> later interrupts injection  ignored at step 2), which will delay
>>>>> irq handler process in VM.
>>>>>
>>>>> And because path 2 set VCPU_KICK_SOFTIRQ to 1, the kick vcpu
>>>>> logic in path 1 will also return in step 3), which make this vcpu
>>>>> only can handle interrupt when some other reason cause VMEXIT.
>>>> Nice catch! But without set the softirq, the interrupt delivery also
>>>> will be delay. Look at the code in vmx_do_vmentry:
>>>>
>>>> cli
>>>>          <---------------the virtual interrupt occurs here will be
>>>> delay. Because there is no softirq pending and the following
>>>> posted interrupt may consumed by another running VCPU, so the
>>>> target VCPU will not aware there is pending virtual interrupt before next vmexit.
>>>> cmp  %ecx,(%rdx,%rax,1)
>>>> jnz  .Lvmx_process_softirqs
>>>>
>>>> I need more time to think it.
>>> Hi Qiming,
>>>
>>> Can you help to take a look at this patch? Also, if possible, please
>>> help to do a testing since I don't have machine to test it. Thanks
>>> very much.
>>>
>>> diff --git a/xen/arch/x86/hvm/vmx/entry.S
>>> b/xen/arch/x86/hvm/vmx/entry.S index 664ed83..1ebb5d0 100644
>>> --- a/xen/arch/x86/hvm/vmx/entry.S
>>> +++ b/xen/arch/x86/hvm/vmx/entry.S
>>> @@ -77,6 +77,9 @@ UNLIKELY_START(ne, realmode)
>>>           call vmx_enter_realmode
>>>   UNLIKELY_END(realmode)
>>> +        bt   $0,VCPU_vmx_pi_ctrl(%rbx)
>>> +        jc   .Lvmx_do_vmentry
>>> +
>>>           mov  %rsp,%rdi
>>>           call vmx_vmenter_helper
>>>           mov  VCPU_hvm_guest_cr2(%rbx),%rax diff --git
>>> a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c index
>>> e0e9e75..315ce65 100644
>>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>>> @@ -1678,8 +1678,9 @@ 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 ( !test_bit(VCPU_KICK_SOFTIRQ, &softirq_pending(cpu))
>>> +             && pi_test_on(&v->arch.hvm_vmx.pi_desc)
>> Why need pi_test_on() here?
> on bit is cleared means the interrupt is consumed by target VCPU already. So there is no need to send the PI.
>
> Best regards,
> Yang
>
Hi Yang,

I had a question about the "outstanding-notification" bit 
(POSTED_INTR_ON)  handling.
It's not very clear in Intel manual. From what I learned, this bit is 
set by software
when send an interrupt to vcpu and cleared by hardware when interrupt 
delivered, right?

from the source code, there is a test_and_set op for this bit in 
function vmx_deliver_posted_intr,

else if ( !pi_test_and_set_on(&v->arch.hvm_vmx.pi_desc) )
     {
         __vmx_deliver_posted_interrupt(v);
         return;
     }

so when we enter __vmx_deliver_posted_interrupt to send ipi, this bit is 
already set on, the "pi_test_on" code is meaningless.

And another thought, the clear bit action performed only when you send a 
ipi to physical cpu or sync_pir_to_irr,
there has a chance the bit is set on and physical cpu not receive a ipi, 
for example

1) [CPU1] vcpu kicked and VCPU_KICK_SOFTIRQ is set
2) [CPU1] find the highest irr where call sync_pir_to_irr to clear 
POSTED_INTR_ON
3) [CPU2] a posted interrupt delivered to CPU1, it will set 
POSTED_INTR_ON on
4) [CPU2] in __vmx_deliver_posted_interrupt will test VCPU_KICK_SOFTIRQ 
on CPU1 which is set by 1), so no ipi sent

At this point, CPU1's POSTED_INTR_ON is on but not interrupt is 
received. and until next kick on CPU1,
CPU1 will not allow posted interrupt get in.

In my opinion, if we want best performance, it should just remove the 
VCPU_KICK_SOFTIRQ test in posted-interrupt function,
or at least we should change the order of test VCPU_KICK_SOFTIRQ and 
test POSTED_INTR_ON





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

Re: [PATCH] Remove a set operation for VCPU_KICK_SOFTIRQ when post interrupt to vm.

[Zhang, Yang Z]

Liuqiming (John) wrote on 2015-09-24:
> On 2015/9/23 21:41, Zhang, Yang Z wrote:
>> Hanweidong (Randy) wrote on 2015-09-23:
>>> 
>>> Zhang, Yang Z wrote on ent: 2015年9月23日 11:51:
>>>> VCPU_KICK_SOFTIRQ when post interrupt to vm.
>>>> 
>>>> Zhang, Yang Z wrote on 2015-09-08:
>>>>> Liuqiming (John) wrote on 2015-09-08:
>>>>>> Ok, I will try to explain, correct me if I got anything wrong:
>>>>>> 
>>>>>> The problem here is not interrupts lost but interrupts not
>>>>>> delivered in time.
>>>>>> 
>>>>>> there are basically two path to inject an interrupt into VM  (or
>>>>>> vCPU to another vCPU):
>>>>>> Path 1, the traditional way:
>>>>>>         1) set bit  in vlapic IRR field which represent an interrupt,
>>>>>>         then kick vcpu 2) a VCPU_KICK_SOFTIRQ softirq raised 3) if
>>>>>>         VCPU_KICK_SOFTIRQ bit not set, then set it, otherwise
>>>>>> return
>>>> and
>>>>>>         do nothing 4) send an EVENT_CHECK_VECTOR IPI  to target
>>> vcpu
>>>> 5)
>>>>>>         target vcpu will VMEXIT due to
>>>>>> EXIT_REASON_EXTERNAL_INTERRUPT
>>>> 6)
>>>>>>         the interrupt handler basically do nothing 7) interrupt in IRR
>>>>>>         will be evaluated 8) VCPU_KICK_SOFTIRQ will be cleared when
>>>>>>         do_softirq 9) there will be an interrupt inject into vcpu when
>>>>>>         VMENTRY Path 2, the Posted-interrupt way (current logic):
>>>>>> 1)
>>>> set
>>>>>>         bit in posted-interrupt descriptor which represent an
>>>>>>         interrupt 2) if VCPU_KICK_SOFTIRQ bit not set, then set it,
>>>>>>         otherwise return and do nothing 3) send an
>>>>>>         POSTED_INTR_NOTIFICATION_VECTOR IPI to target vcpu 4) if
>>>>>>         target vcpu in non-ROOT mode it will receive the
>>>>>> interrupt immediately otherwise interrupt will be injected when
>>>>>> VMENTRY
>>>>>> 
>>>>>> As the first operation in both path is setting a interrupt
>>>>>> represent bit, so no interrupts will lost.
>>>>>> 
>>>>>> The issue is:
>>>>>> in path 2, the first interrupt will cause VCPU_KICK_SOFTIRQ set
>>>>>> to 1, and unless a VMEXIT occured or somewhere called do_softirq
>>>>>> directly, VCPU_KICK_SOFTIRQ will not cleared, that will make the
>>>>>> later interrupts injection  ignored at step 2), which will delay
>>>>>> irq handler process in VM.
>>>>>> 
>>>>>> And because path 2 set VCPU_KICK_SOFTIRQ to 1, the kick vcpu
>>>>>> logic in path 1 will also return in step 3), which make this
>>>>>> vcpu only can handle interrupt when some other reason cause VMEXIT.
>>>>> Nice catch! But without set the softirq, the interrupt delivery
>>>>> also will be delay. Look at the code in vmx_do_vmentry:
>>>>> 
>>>>> cli
>>>>>          <---------------the virtual interrupt occurs here will
>>>>> be delay. Because there is no softirq pending and the following
>>>>> posted interrupt may consumed by another running VCPU, so the
>>>>> target VCPU will not aware there is pending virtual interrupt before next vmexit.
>>>>> cmp  %ecx,(%rdx,%rax,1)
>>>>> jnz  .Lvmx_process_softirqs
>>>>> 
>>>>> I need more time to think it.
>>>> Hi Qiming,
>>>> 
>>>> Can you help to take a look at this patch? Also, if possible,
>>>> please help to do a testing since I don't have machine to test it.
>>>> Thanks very much.
>>>> 
>>>> diff --git a/xen/arch/x86/hvm/vmx/entry.S
>>>> b/xen/arch/x86/hvm/vmx/entry.S index 664ed83..1ebb5d0 100644
>>>> --- a/xen/arch/x86/hvm/vmx/entry.S
>>>> +++ b/xen/arch/x86/hvm/vmx/entry.S
>>>> @@ -77,6 +77,9 @@ UNLIKELY_START(ne, realmode)
>>>>           call vmx_enter_realmode
>>>>   UNLIKELY_END(realmode)
>>>> +        bt   $0,VCPU_vmx_pi_ctrl(%rbx)
>>>> +        jc   .Lvmx_do_vmentry
>>>> +
>>>>           mov  %rsp,%rdi
>>>>           call vmx_vmenter_helper
>>>>           mov  VCPU_hvm_guest_cr2(%rbx),%rax diff --git
>>>> a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c index
>>>> e0e9e75..315ce65 100644
>>>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>>>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>>>> @@ -1678,8 +1678,9 @@ 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 ( !test_bit(VCPU_KICK_SOFTIRQ, &softirq_pending(cpu))
>>>> +             && pi_test_on(&v->arch.hvm_vmx.pi_desc)
>>> Why need pi_test_on() here?
>> on bit is cleared means the interrupt is consumed by target VCPU
>> already. So there is no need to send the PI.
>> 
>> Best regards,
>> Yang
>> 
> Hi Yang,
> 
> I had a question about the "outstanding-notification" bit
> (POSTED_INTR_ON)  handling.
> It's not very clear in Intel manual. From what I learned, this bit is
> set by software when send an interrupt to vcpu and cleared by hardware
> when interrupt delivered, right?
> 
> from the source code, there is a test_and_set op for this bit in
> function vmx_deliver_posted_intr,
> 
> else if ( !pi_test_and_set_on(&v->arch.hvm_vmx.pi_desc) )
>      {
>          __vmx_deliver_posted_interrupt(v);
>          return;
>      }
> so when we enter __vmx_deliver_posted_interrupt to send ipi, this bit
> is already set on, the "pi_test_on" code is meaningless.

The on bit will be cleared at any time.

> 
> And another thought, the clear bit action performed only when you send
> a ipi to physical cpu or sync_pir_to_irr, there has a chance the bit
> is set on and physical cpu not receive a ipi, for example
> 
> 1) [CPU1] vcpu kicked and VCPU_KICK_SOFTIRQ is set 2) [CPU1] find the
> highest irr where call sync_pir_to_irr to clear POSTED_INTR_ON 3) [CPU2]
> a posted interrupt delivered to CPU1, it will set POSTED_INTR_ON on 4)
> [CPU2] in __vmx_deliver_posted_interrupt will test VCPU_KICK_SOFTIRQ on
> CPU1 which is set by 1), so no ipi sent
> 
> At this point, CPU1's POSTED_INTR_ON is on but not interrupt is
> received. and until next kick on CPU1,
> CPU1 will not allow posted interrupt get in.

Why next kick? CPU1 is going to handle the KICK_SOFTIRQ immediately. After it completed, the following vmentry will pick up the pending interrupt. If you send the ipi unconditionally, actually it is received by hypervisor and the interrupt handler dose nothing. You still rely on the vmentry to pick up the interrupt.

> 
> In my opinion, if we want best performance, it should just remove the
> VCPU_KICK_SOFTIRQ test in posted-interrupt function, or at least we
> should change the order of test VCPU_KICK_SOFTIRQ and test POSTED_INTR_ON
> 
> 
>


Best regards,
Yang


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

Re: [PATCH] Remove a set operation for VCPU_KICK_SOFTIRQ when post interrupt to vm.

[Liuqiming (John)]

On 2015/9/24 11:25, Zhang, Yang Z wrote:
> Liuqiming (John) wrote on 2015-09-24:
>> On 2015/9/23 21:41, Zhang, Yang Z wrote:
>>> Hanweidong (Randy) wrote on 2015-09-23:
>>>> Zhang, Yang Z wrote on ent: 2015年9月23日 11:51:
>>>>> VCPU_KICK_SOFTIRQ when post interrupt to vm.
>>>>>
>>>>> Zhang, Yang Z wrote on 2015-09-08:
>>>>>> Liuqiming (John) wrote on 2015-09-08:
>>>>>>> Ok, I will try to explain, correct me if I got anything wrong:
>>>>>>>
>>>>>>> The problem here is not interrupts lost but interrupts not
>>>>>>> delivered in time.
>>>>>>>
>>>>>>> there are basically two path to inject an interrupt into VM  (or
>>>>>>> vCPU to another vCPU):
>>>>>>> Path 1, the traditional way:
>>>>>>>          1) set bit  in vlapic IRR field which represent an interrupt,
>>>>>>>          then kick vcpu 2) a VCPU_KICK_SOFTIRQ softirq raised 3) if
>>>>>>>          VCPU_KICK_SOFTIRQ bit not set, then set it, otherwise
>>>>>>> return
>>>>> and
>>>>>>>          do nothing 4) send an EVENT_CHECK_VECTOR IPI  to target
>>>> vcpu
>>>>> 5)
>>>>>>>          target vcpu will VMEXIT due to
>>>>>>> EXIT_REASON_EXTERNAL_INTERRUPT
>>>>> 6)
>>>>>>>          the interrupt handler basically do nothing 7) interrupt in IRR
>>>>>>>          will be evaluated 8) VCPU_KICK_SOFTIRQ will be cleared when
>>>>>>>          do_softirq 9) there will be an interrupt inject into vcpu when
>>>>>>>          VMENTRY Path 2, the Posted-interrupt way (current logic):
>>>>>>> 1)
>>>>> set
>>>>>>>          bit in posted-interrupt descriptor which represent an
>>>>>>>          interrupt 2) if VCPU_KICK_SOFTIRQ bit not set, then set it,
>>>>>>>          otherwise return and do nothing 3) send an
>>>>>>>          POSTED_INTR_NOTIFICATION_VECTOR IPI to target vcpu 4) if
>>>>>>>          target vcpu in non-ROOT mode it will receive the
>>>>>>> interrupt immediately otherwise interrupt will be injected when
>>>>>>> VMENTRY
>>>>>>>
>>>>>>> As the first operation in both path is setting a interrupt
>>>>>>> represent bit, so no interrupts will lost.
>>>>>>>
>>>>>>> The issue is:
>>>>>>> in path 2, the first interrupt will cause VCPU_KICK_SOFTIRQ set
>>>>>>> to 1, and unless a VMEXIT occured or somewhere called do_softirq
>>>>>>> directly, VCPU_KICK_SOFTIRQ will not cleared, that will make the
>>>>>>> later interrupts injection  ignored at step 2), which will delay
>>>>>>> irq handler process in VM.
>>>>>>>
>>>>>>> And because path 2 set VCPU_KICK_SOFTIRQ to 1, the kick vcpu
>>>>>>> logic in path 1 will also return in step 3), which make this
>>>>>>> vcpu only can handle interrupt when some other reason cause VMEXIT.
>>>>>> Nice catch! But without set the softirq, the interrupt delivery
>>>>>> also will be delay. Look at the code in vmx_do_vmentry:
>>>>>>
>>>>>> cli
>>>>>>           <---------------the virtual interrupt occurs here will
>>>>>> be delay. Because there is no softirq pending and the following
>>>>>> posted interrupt may consumed by another running VCPU, so the
>>>>>> target VCPU will not aware there is pending virtual interrupt before next vmexit.
>>>>>> cmp  %ecx,(%rdx,%rax,1)
>>>>>> jnz  .Lvmx_process_softirqs
>>>>>>
>>>>>> I need more time to think it.
>>>>> Hi Qiming,
>>>>>
>>>>> Can you help to take a look at this patch? Also, if possible,
>>>>> please help to do a testing since I don't have machine to test it.
>>>>> Thanks very much.
>>>>>
>>>>> diff --git a/xen/arch/x86/hvm/vmx/entry.S
>>>>> b/xen/arch/x86/hvm/vmx/entry.S index 664ed83..1ebb5d0 100644
>>>>> --- a/xen/arch/x86/hvm/vmx/entry.S
>>>>> +++ b/xen/arch/x86/hvm/vmx/entry.S
>>>>> @@ -77,6 +77,9 @@ UNLIKELY_START(ne, realmode)
>>>>>            call vmx_enter_realmode
>>>>>    UNLIKELY_END(realmode)
>>>>> +        bt   $0,VCPU_vmx_pi_ctrl(%rbx)
>>>>> +        jc   .Lvmx_do_vmentry
>>>>> +
>>>>>            mov  %rsp,%rdi
>>>>>            call vmx_vmenter_helper
>>>>>            mov  VCPU_hvm_guest_cr2(%rbx),%rax diff --git
>>>>> a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c index
>>>>> e0e9e75..315ce65 100644
>>>>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>>>>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>>>>> @@ -1678,8 +1678,9 @@ 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 ( !test_bit(VCPU_KICK_SOFTIRQ, &softirq_pending(cpu))
>>>>> +             && pi_test_on(&v->arch.hvm_vmx.pi_desc)
>>>> Why need pi_test_on() here?
>>> on bit is cleared means the interrupt is consumed by target VCPU
>>> already. So there is no need to send the PI.
>>>
>>> Best regards,
>>> Yang
>>>
>> Hi Yang,
>>
>> I had a question about the "outstanding-notification" bit
>> (POSTED_INTR_ON)  handling.
>> It's not very clear in Intel manual. From what I learned, this bit is
>> set by software when send an interrupt to vcpu and cleared by hardware
>> when interrupt delivered, right?
>>
>> from the source code, there is a test_and_set op for this bit in
>> function vmx_deliver_posted_intr,
>>
>> else if ( !pi_test_and_set_on(&v->arch.hvm_vmx.pi_desc) )
>>       {
>>           __vmx_deliver_posted_interrupt(v);
>>           return;
>>       }
>> so when we enter __vmx_deliver_posted_interrupt to send ipi, this bit
>> is already set on, the "pi_test_on" code is meaningless.
> The on bit will be cleared at any time.
That's true, just feel the logic is a little confusing,
the outside test(vmx_deliver_posted_intr) say: "the bit is not 1, lets 
set it to 1 and do the delivery"
the inner test(__vmx_deliver_posted_interrupt) say:"the bit is 1, let's 
do the delivery"
>> And another thought, the clear bit action performed only when you send
>> a ipi to physical cpu or sync_pir_to_irr, there has a chance the bit
>> is set on and physical cpu not receive a ipi, for example
>>
>> 1) [CPU1] vcpu kicked and VCPU_KICK_SOFTIRQ is set 2) [CPU1] find the
>> highest irr where call sync_pir_to_irr to clear POSTED_INTR_ON 3) [CPU2]
>> a posted interrupt delivered to CPU1, it will set POSTED_INTR_ON on 4)
>> [CPU2] in __vmx_deliver_posted_interrupt will test VCPU_KICK_SOFTIRQ on
>> CPU1 which is set by 1), so no ipi sent
>>
>> At this point, CPU1's POSTED_INTR_ON is on but not interrupt is
>> received. and until next kick on CPU1,
>> CPU1 will not allow posted interrupt get in.
> Why next kick? CPU1 is going to handle the KICK_SOFTIRQ immediately. After it completed, the following vmentry will pick up the pending interrupt. If you send the ipi unconditionally, actually it is received by hypervisor and the interrupt handler dose nothing. You still rely on the vmentry to pick up the interrupt.
My confusion is:  when vmentry, does physical CPU inject all interrupts 
on PIR into VM and clear POSTED_INTR_ON bit?
Or just inject the highest index of PIR and may leave POSTED_INTR_ON bit 
being set to 1?
>> In my opinion, if we want best performance, it should just remove the
>> VCPU_KICK_SOFTIRQ test in posted-interrupt function, or at least we
>> should change the order of test VCPU_KICK_SOFTIRQ and test POSTED_INTR_ON
>>
>>
>>
>
> Best regards,
> Yang
>
>



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

Re: [PATCH] Remove a set operation for VCPU_KICK_SOFTIRQ when post interrupt to vm.

[Zhang, Yang Z]

Liuqiming (John) wrote on 2015-09-24:
> 
> 
> On 2015/9/24 11:25, Zhang, Yang Z wrote:
>> Liuqiming (John) wrote on 2015-09-24:
>>> On 2015/9/23 21:41, Zhang, Yang Z wrote:
>>>> Hanweidong (Randy) wrote on 2015-09-23:
>>>>> Zhang, Yang Z wrote on ent: 2015年9月23日 11:51:
>>>>>> VCPU_KICK_SOFTIRQ when post interrupt to vm.
>>>>>> 
>>>>>> Zhang, Yang Z wrote on 2015-09-08:
>>>>>>> Liuqiming (John) wrote on 2015-09-08:
>>>>>>>> Ok, I will try to explain, correct me if I got anything wrong:
>>>>>>>> 
>>>>>>>> The problem here is not interrupts lost but interrupts not
>>>>>>>> delivered in time.
>>>>>>>> 
>>>>>>>> there are basically two path to inject an interrupt into VM  (or
>>>>>>>> vCPU to another vCPU):
>>>>>>>> Path 1, the traditional way:
>>>>>>>>          1) set bit  in vlapic IRR field which represent an interrupt,
>>>>>>>>          then kick vcpu 2) a VCPU_KICK_SOFTIRQ softirq raised 3) if
>>>>>>>>          VCPU_KICK_SOFTIRQ bit not set, then set it, otherwise
>>>>>>>> return
>>>>>> and
>>>>>>>>          do nothing 4) send an EVENT_CHECK_VECTOR IPI  to
> target
>>>>> vcpu
>>>>>> 5)
>>>>>>>>          target vcpu will VMEXIT due to
>>>>>>>> EXIT_REASON_EXTERNAL_INTERRUPT
>>>>>> 6)
>>>>>>>>          the interrupt handler basically do nothing 7) interrupt
>>>>>>>>          in IRR will be evaluated 8) VCPU_KICK_SOFTIRQ will be
>>>>>>>>          cleared when do_softirq 9) there will be an interrupt
>>>>>>>>          inject into vcpu when VMENTRY Path 2, the
>>>>>>>>          Posted-interrupt way (current
> logic):
>>>>>>>> 1)
>>>>>> set
>>>>>>>>          bit in posted-interrupt descriptor which represent an
>>>>>>>>          interrupt 2) if VCPU_KICK_SOFTIRQ bit not set, then set
>>>>>>>>          it, otherwise return and do nothing 3) send an
>>>>>>>>          POSTED_INTR_NOTIFICATION_VECTOR IPI to target vcpu 4) if
>>>>>>>>          target vcpu in non-ROOT mode it will receive the
>>>>>>>> interrupt immediately otherwise interrupt will be injected when
>>>>>>>> VMENTRY
>>>>>>>> 
>>>>>>>> As the first operation in both path is setting a interrupt
>>>>>>>> represent bit, so no interrupts will lost.
>>>>>>>> 
>>>>>>>> The issue is:
>>>>>>>> in path 2, the first interrupt will cause VCPU_KICK_SOFTIRQ set
>>>>>>>> to 1, and unless a VMEXIT occured or somewhere called do_softirq
>>>>>>>> directly, VCPU_KICK_SOFTIRQ will not cleared, that will make the
>>>>>>>> later interrupts injection  ignored at step 2), which will delay
>>>>>>>> irq handler process in VM.
>>>>>>>> 
>>>>>>>> And because path 2 set VCPU_KICK_SOFTIRQ to 1, the kick vcpu
>>>>>>>> logic in path 1 will also return in step 3), which make this
>>>>>>>> vcpu only can handle interrupt when some other reason cause
> VMEXIT.
>>>>>>> Nice catch! But without set the softirq, the interrupt delivery
>>>>>>> also will be delay. Look at the code in vmx_do_vmentry:
>>>>>>> 
>>>>>>> cli
>>>>>>>           <---------------the virtual interrupt occurs here will
>>>>>>> be delay. Because there is no softirq pending and the following
>>>>>>> posted interrupt may consumed by another running VCPU, so the
>>>>>>> target VCPU will not aware there is pending virtual interrupt
>>>>>>> before next vmexit. cmp  %ecx,(%rdx,%rax,1) jnz 
>>>>>>> .Lvmx_process_softirqs
>>>>>>> 
>>>>>>> I need more time to think it.
>>>>>> Hi Qiming,
>>>>>> 
>>>>>> Can you help to take a look at this patch? Also, if possible,
>>>>>> please help to do a testing since I don't have machine to test it.
>>>>>> Thanks very much.
>>>>>> 
>>>>>> diff --git a/xen/arch/x86/hvm/vmx/entry.S
>>>>>> b/xen/arch/x86/hvm/vmx/entry.S index 664ed83..1ebb5d0 100644
>>>>>> --- a/xen/arch/x86/hvm/vmx/entry.S
>>>>>> +++ b/xen/arch/x86/hvm/vmx/entry.S
>>>>>> @@ -77,6 +77,9 @@ UNLIKELY_START(ne, realmode)
>>>>>>            call vmx_enter_realmode
>>>>>>    UNLIKELY_END(realmode)
>>>>>> +        bt   $0,VCPU_vmx_pi_ctrl(%rbx)
>>>>>> +        jc   .Lvmx_do_vmentry
>>>>>> +
>>>>>>            mov  %rsp,%rdi
>>>>>>            call vmx_vmenter_helper
>>>>>>            mov  VCPU_hvm_guest_cr2(%rbx),%rax diff --git
>>>>>> a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c index
>>>>>> e0e9e75..315ce65 100644
>>>>>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>>>>>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>>>>>> @@ -1678,8 +1678,9 @@ 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 ( !test_bit(VCPU_KICK_SOFTIRQ, &softirq_pending(cpu))
>>>>>> +             && pi_test_on(&v->arch.hvm_vmx.pi_desc)
>>>>> Why need pi_test_on() here?
>>>> on bit is cleared means the interrupt is consumed by target VCPU
>>>> already. So there is no need to send the PI.
>>>> 
>>>> Best regards,
>>>> Yang
>>>> 
>>> Hi Yang,
>>> 
>>> I had a question about the "outstanding-notification" bit
>>> (POSTED_INTR_ON)  handling.
>>> It's not very clear in Intel manual. From what I learned, this bit is
>>> set by software when send an interrupt to vcpu and cleared by hardware
>>> when interrupt delivered, right?
>>> 
>>> from the source code, there is a test_and_set op for this bit in
>>> function vmx_deliver_posted_intr,
>>> 
>>> else if ( !pi_test_and_set_on(&v->arch.hvm_vmx.pi_desc) )
>>>       {
>>>           __vmx_deliver_posted_interrupt(v);
>>>           return;
>>>       }
>>> so when we enter __vmx_deliver_posted_interrupt to send ipi, this bit
>>> is already set on, the "pi_test_on" code is meaningless.
>> The on bit will be cleared at any time.
> That's true, just feel the logic is a little confusing,
> the outside test(vmx_deliver_posted_intr) say: "the bit is not 1, lets
> set it to 1 and do the delivery"
> the inner test(__vmx_deliver_posted_interrupt) say:"the bit is 1, let's
> do the delivery"
>>> And another thought, the clear bit action performed only when you send
>>> a ipi to physical cpu or sync_pir_to_irr, there has a chance the bit
>>> is set on and physical cpu not receive a ipi, for example
>>> 
>>> 1) [CPU1] vcpu kicked and VCPU_KICK_SOFTIRQ is set 2) [CPU1] find the
>>> highest irr where call sync_pir_to_irr to clear POSTED_INTR_ON 3) [CPU2]
>>> a posted interrupt delivered to CPU1, it will set POSTED_INTR_ON on 4)
>>> [CPU2] in __vmx_deliver_posted_interrupt will test VCPU_KICK_SOFTIRQ on
>>> CPU1 which is set by 1), so no ipi sent
>>> 
>>> At this point, CPU1's POSTED_INTR_ON is on but not interrupt is
>>> received. and until next kick on CPU1,
>>> CPU1 will not allow posted interrupt get in.
>> Why next kick? CPU1 is going to handle the KICK_SOFTIRQ immediately. After
> it completed, the following vmentry will pick up the pending interrupt.
> If you send the ipi unconditionally, actually it is received by
> hypervisor and the interrupt handler dose nothing. You still rely on the
> vmentry to pick up the interrupt. My confusion is:  when vmentry, does
> physical CPU inject all interrupts on PIR into VM and clear
> POSTED_INTR_ON bit? Or just inject the highest index of PIR and may
> leave POSTED_INTR_ON bit being set to 1?

I need to say your understanding is wrong. What we need to do is sync the pir to irr and let hardware or hypervisor to handle left part. So our focus is how to sync pir to irr correctly and timely. We don't care how the interrupt in PIR is injected to guest.

Best regards,
Yang


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

Re: [PATCH] Remove a set operation for VCPU_KICK_SOFTIRQ when post interrupt to vm.

[Zhang, Yang Z]

Zhang, Yang Z wrote on 2015-09-24:
> Liuqiming (John) wrote on 2015-09-24:
>> 
>> 
>> On 2015/9/24 11:25, Zhang, Yang Z wrote:
>> it completed, the following vmentry will pick up the pending interrupt.
>> If you send the ipi unconditionally, actually it is received by
>> hypervisor and the interrupt handler dose nothing. You still rely on
>> the vmentry to pick up the interrupt. My confusion is:  when
>> vmentry, does physical CPU inject all interrupts on PIR into VM and
>> clear POSTED_INTR_ON bit? Or just inject the highest index of PIR
>> and may leave POSTED_INTR_ON bit being set to 1?
> 
> I need to say your understanding is wrong. What we need to do is sync
> the pir to irr and let hardware or hypervisor to handle left part. So
> our focus is how to sync pir to irr correctly and timely. We don't
> care how the interrupt in PIR is injected to guest.

Hi Qiming, is there any other question with it? Also, do you have any chance to test it?

Best regards,
Yang

Re: [PATCH] Remove a set operation for VCPU_KICK_SOFTIRQ when post interrupt to vm.

[Liuqiming (John)]

On 2015/10/10 14:32, Zhang, Yang Z wrote:
> Zhang, Yang Z wrote on 2015-09-24:
>> Liuqiming (John) wrote on 2015-09-24:
>>>
>>> On 2015/9/24 11:25, Zhang, Yang Z wrote:
>>> it completed, the following vmentry will pick up the pending interrupt.
>>> If you send the ipi unconditionally, actually it is received by
>>> hypervisor and the interrupt handler dose nothing. You still rely on
>>> the vmentry to pick up the interrupt. My confusion is:  when
>>> vmentry, does physical CPU inject all interrupts on PIR into VM and
>>> clear POSTED_INTR_ON bit? Or just inject the highest index of PIR
>>> and may leave POSTED_INTR_ON bit being set to 1?
>> I need to say your understanding is wrong. What we need to do is sync
>> the pir to irr and let hardware or hypervisor to handle left part. So
>> our focus is how to sync pir to irr correctly and timely. We don't
>> care how the interrupt in PIR is injected to guest.
> Hi Qiming, is there any other question with it? Also, do you have any chance to test it?
>
> Best regards,
> Yang
   No other question.
   I have run some vm start-shutdown test, and haven't gotten a chance 
to test the performance.

Re: [PATCH] Remove a set operation for VCPU_KICK_SOFTIRQ when post interrupt to vm.

[Zhang, Yang Z]

Liuqiming (John) wrote on 2015-10-10:
> 
> On 2015/10/10 14:32, Zhang, Yang Z wrote:
>> Zhang, Yang Z wrote on 2015-09-24:
>>> Liuqiming (John) wrote on 2015-09-24:
>>>> 
>>>> On 2015/9/24 11:25, Zhang, Yang Z wrote:
>>>> it completed, the following vmentry will pick up the pending interrupt.
>>>> If you send the ipi unconditionally, actually it is received by
>>>> hypervisor and the interrupt handler dose nothing. You still rely
>>>> on the vmentry to pick up the interrupt. My confusion is:  when
>>>> vmentry, does physical CPU inject all interrupts on PIR into VM
>>>> and clear POSTED_INTR_ON bit? Or just inject the highest index of
>>>> PIR and may leave POSTED_INTR_ON bit being set to 1?
>>> I need to say your understanding is wrong. What we need to do is
>>> sync the pir to irr and let hardware or hypervisor to handle left
>>> part. So our focus is how to sync pir to irr correctly and timely.
>>> We don't care how the interrupt in PIR is injected to guest.
>> Hi Qiming, is there any other question with it? Also, do you have any
>> chance to test it?
>> 
>> Best regards,
>> Yang
>    No other question.
>    I have run some vm start-shutdown test, and haven't gotten a chance
> to test the performance.

Hi Qiming

Have your tested the performance? If it fixes your performance issue, I will send out a formal patch. 

btw, is it ok to add you in signed-by since the origin one is from you?

Best regards,
Yang