Re: [xen-unstable test] 104131: regressions - FAIL

[Chao Gao <chao.gao@intel.com>]

On Thu, Feb 09, 2017 at 08:51:46AM +0000, Xuquan (Quan Xu) wrote:
>On February 08, 2017 4:22 PM, Chao Gao wrote:
>>On Wed, Feb 08, 2017 at 10:15:28AM +0000, Xuquan (Quan Xu) wrote:
>>>On February 08, 2017 4:52 PM, Jan Beulich wrote:
>>>>>>> On 08.02.17 at 09:27, <xuquan8@huawei.com> wrote:
>>>>> Assumed vCPU is in guest_mode..
>>>>> When apicv is enabled, hypervisor calls vmx_deliver_posted_intr(),
>>>>> then
>>>>> __vmx_deliver_posted_interrupt() to deliver interrupt, but no vmexit
>>>>> (also no
>>>>> vcpu_kick() )..
>>>>> In __vmx_deliver_posted_interrupt(), it is __conditional__ to
>>>>> deliver posted interrupt. if posted interrupt is not delivered, the
>>>>> posted interrupt is pending until next VM entry -- by PIR to vIRR..
>>>>>
>>>>> one condition is :
>>>>> In __vmx_deliver_posted_interrupt(),  ' if (
>>>>> !test_and_set_bit(VCPU_KICK_SOFTIRQ, &softirq_pending(cpu))' ..
>>>>>
>>>>> Specifically, we did verify it by RES interrupt, which is used for
>>>>> smp_reschedule_interrupt..
>>>>> We even cost more time to deliver RES interrupt than no-apicv in
>>>>average..
>>>>>
>>>>> If RES interrupt (no. 1) is delivered by posted way (the vcpu is
>>>>> still guest_mode).. when tries to deliver next-coming RES interrupt
>>>>> (no. 2) by posted way, The next-coming RES interrupt (no. 2) is not
>>>>> delivered, as we set the VCPU_KICK_SOFTIRQ bit when we deliver RES
>>interrupt (no.
>>>>> 1)..
>>>>>
>>>>> Then the next-coming RES interrupt (no. 2) is pending until next VM
>>>>> entry -- by PIR to vIRR..
>>>>>
>>>>>
>>>>> We can fix it as below(I don't think this is a best one, it is
>>>>> better to set the VCPU_KICK_SOFTIRQ bit, but not test it):
>>>>>
>>>>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>>>>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>>>>> @@ -1846,7 +1846,7 @@ static void
>>>>__vmx_deliver_posted_interrupt(struct vcpu *v)
>>>>>      {
>>>>>          unsigned int cpu = v->processor;
>>>>>
>>>>> -        if ( !test_and_set_bit(VCPU_KICK_SOFTIRQ,
>>>>&softirq_pending(cpu))
>>>>> +        if ( !test_bit(VCPU_KICK_SOFTIRQ, &softirq_pending(cpu))
>>>>>               && (cpu != smp_processor_id()) )
>>>>>              send_IPI_mask(cpumask_of(cpu), posted_intr_vector);
>>>>>      }
>>>>
>>>>While I don't think I fully understand your description,
>>>
>>>Sorry!!
>>>
>>>>the line you change
>>>>here has always been puzzling me: If we were to raise a softirq here,
>>>>we ought to call cpu_raise_softirq() instead of partly open coding what it
>>does.
>>>>So I think not marking that softirq pending (but doing this
>>>>incompletely) is a valid change in any case.
>>>
>>>As comments in pi_notification_interrupt()  --
>>>xen/arch/x86/hvm/vmx/vmx.c ((((
>>>     *
>>>     * we need to set VCPU_KICK_SOFTIRQ for the current cpu, just like
>>>     * __vmx_deliver_posted_interrupt(). So the pending interrupt in
>>PIRR will
>>>     * be synced to vIRR before VM-Exit in time.
>>>     *
>>>))))
>>>
>>>I think setting VCPU_KICK_SOFTIRQ bit -- the pending interrupt in PIRR will
>>be synced to vIRR before VM-Exit in time.
>>>That's also why i said it is better to set the VCPU_KICK_SOFTIRQ bit, but
>>not test it..
>>>
>>
>>I think there is a typo. It should be "before VM-Entry in time". It set
>>VCPU_KICK_SOFTIRQ bit only to jump to vmx_do_vmentry again instead of
>>entering guest directly. Jumping to vmx_do_vmentry again can re-sync the
>>PIR to vIRR in vmx_intr_assist(). 
>
>impressive analysis..
>chao, could you show the related code?
>

In xen/arch/x86/vmx/entry.S, 
.Lvmx_do_vmentry:
	call vmx_intr_assist
	... 
	cli
	cmp %ecx,(%rdx,%rax,1)
	jnz .Lvmx_process_softirqs
and 
.Lvmx_process_softirqs:
	sti
	call do_softirq
	jmp .Lvmx_do_vmentry	

In vmx_intr_assist(), PIR is synced to vIRR. After vmx_intr_assist(), 
if some interrupts are posted in PIR, VCPU_KICK_SOFTIRQ is set 
in pi_nofitication_interrupt() and it will jump to vmx_process_softirqs
and jump to vmx_do_vmentry again.

Thanks
Chao

>Quan
>
>
>>In root-mode, cpu treat the pi notification
>>interrupt as normal interrupt, so cpu will run the interrupt handler
>>pi_notification_interrupt() instead of syncing PIR to vIRR automatically.
>>Receiving a pi notificatio interrupt means some interrupts have been
>>posted in PIR. Setting that bit is to deliver these new arrival interrupts
>>before this VM-entry.
>>
>

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