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

["Zhang, Yang Z" <yang.z.zhang@xxxxxxxxx>]

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