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

["Hanweidong (Randy)" <hanweidong@xxxxxxxxxx>]

> -----Original Message-----
> From: Zhang, Yang Z [mailto:yang.z.zhang@xxxxxxxxx]
> Sent: 2015å9æ23æ 11:51
> To: Liuqiming (John); Hanweidong (Randy); Jan Beulich
> Cc: Zhangwei (FF); xen-devel@xxxxxxxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel