Re: [Xen-devel] [ PATCH 2/2] xen: enable Virtual-interrupt delivery

["Li, Jiongxi" <jiongxi.li@xxxxxxxxx>]

Sorry for the late response.

> -----Original Message-----
> From: Keir Fraser [mailto:keir.xen@xxxxxxxxx]
> Sent: Friday, August 31, 2012 5:58 PM
> To: Li, Jiongxi; xen-devel@xxxxxxxxxxxxx
> Subject: Re: [Xen-devel] [ PATCH 2/2] xen: enable Virtual-interrupt delivery
> 
> On 31/08/2012 10:30, "Li, Jiongxi" <jiongxi.li@xxxxxxxxx> wrote:
> 
> > Virtual interrupt delivery avoids Xen to inject vAPIC interrupts
> > manually, which is fully taken care of by the hardware. This needs
> > some special awareness into existing interrupr injection path:
> > For pending interrupt from vLAPIC, instead of direct injection, we may
> > need update architecture specific indicators before resuming to guest.
> > Before returning to guest, RVI should be updated if any pending IRRs
> > EOI exit bitmap controls whether an EOI write should cause VM-Exit. If
> > set, a trap-like induced EOI VM-Exit is triggered. The approach here
> > is to manipulate EOI exit bitmap based on value of TMR. Level
> > triggered irq requires a hook in vLAPIC EOI write, so that vIOAPIC EOI
> > is triggered and emulated
> 
> Thanks. A couple of quick comments below. This will need some careful review
> from a couple of us, I expect.
> 
>  -- Keir
> 
> > Signed-off-by: Yang Zhang <yang.z.zhang@xxxxxxxxx>
> > Signed-off-by: Jiongxi Li <jiongxi.li@xxxxxxxxx>
> >
> > diff -r cb821c24ca74 xen/arch/x86/hvm/irq.c
> > --- a/xen/arch/x86/hvm/irq.c  Fri Aug 31 09:30:38 2012 +0800
> > +++ b/xen/arch/x86/hvm/irq.c         Fri Aug 31 09:49:39 2012 +0800
> > @@ -452,7 +452,11 @@ struct hvm_intack hvm_vcpu_ack_pending_i
> >
> >  int hvm_local_events_need_delivery(struct vcpu *v) {
> > -    struct hvm_intack intack = hvm_vcpu_has_pending_irq(v);
> > +    struct hvm_intack intack;
> > +
> > +    pt_update_irq(v);
> 
> Why would this change be needed for vAPIC?
When vcpu is scheduled out, there will be periodic timer interrupt pending for 
injection. Every VMExit is a chance to inject the pending periodic timer 
interrupt on vmx_intr_assist. In no virtual interrupt delivery case, although 
injected timer interrupt is edge trigger, EOI always induces VMExit, pending 
periodic timer can be kept injected till there is no pending left. But in 
virtual interrupt delivery case, only level trigger interrupt can induce 
VMExit, there is much less chance for injecting pending timer interrupt through 
VMExit. 
Adding pt_update_irq here is another code path to inject pending periodic 
timer, but it can't guarantee every pending timer interrupt can be injected. 
Another way is to make every EOI of pending timer interrupt induce VMExit, but 
it may obey the spirit of virtual interrupt delivery - reducing VMExit.
We have been evaluating that.
> 
> > +    intack = hvm_vcpu_has_pending_irq(v);
> >
> >      if ( likely(intack.source == hvm_intsrc_none) )
> >          return 0;
> > diff -r cb821c24ca74 xen/arch/x86/hvm/vlapic.c
> > --- a/xen/arch/x86/hvm/vlapic.c      Fri Aug 31 09:30:38 2012 +0800
> > +++ b/xen/arch/x86/hvm/vlapic.c   Fri Aug 31 09:49:39 2012 +0800
> > @@ -143,7 +143,16 @@ static int vlapic_find_highest_irr(struc int
> > vlapic_set_irq(struct vlapic *vlapic, uint8_t vec, uint8_t trig) {
> >      if ( trig )
> > +    {
> >          vlapic_set_vector(vec, &vlapic->regs->data[APIC_TMR]);
> > +        if ( cpu_has_vmx_virtual_intr_delivery )
> > +            vmx_set_eoi_exit_bitmap(vlapic_vcpu(vlapic), vec);
> > +    }
> > +    else
> > +    {
> > +        if ( cpu_has_vmx_virtual_intr_delivery )
> > +            vmx_clear_eoi_exit_bitmap(vlapic_vcpu(vlapic), vec);
> > +    }
> >
> >      /* We may need to wake up target vcpu, besides set pending bit here
> */
> >      return !vlapic_test_and_set_irr(vec, vlapic); @@ -410,6 +419,22
> > @@ void vlapic_EOI_set(struct vlapic *vlapi
> >      hvm_dpci_msi_eoi(current->domain, vector); }
> >
> > +/*
> > + * When "Virtual Interrupt Delivery" is enabled, this function is
> > +used
> > + * to handle EOI-induced VM exit
> > + */
> > +void vlapic_handle_EOI_induced_exit(struct vlapic *vlapic, int
> > +vector) {
> > +    ASSERT(cpu_has_vmx_virtual_intr_delivery);
> > +
> > +    if ( vlapic_test_and_clear_vector(vector,
> > + &vlapic->regs->data[APIC_TMR])
> > )
> > +    {
> > +        vioapic_update_EOI(vlapic_domain(vlapic), vector);
> > +    }
> 
> No need for braces for single-line statement.
OK.
> 
> > +    hvm_dpci_msi_eoi(current->domain, vector); }
> > +
> > int vlapic_ipi(
> >      struct vlapic *vlapic, uint32_t icr_low, uint32_t icr_high) { @@
> > -1014,6 +1039,9 @@ int vlapic_has_pending_irq(struct vcpu *
> >      if ( irr == -1 )
> >          return -1;
> >
> > +    if ( cpu_has_vmx_virtual_intr_delivery )
> > +        return irr;
> 
> Why is it correct to ignore ISR here? I guess vAPIC deals with interrupt 
> window
> automatically, maybe?
In delivery of virtual interrupt and VEOI updates, VISR[vector] is updated 
automatically by hardware, software doesn't need to care much about it
> 
> >      isr = vlapic_find_highest_isr(vlapic);
> >      isr = (isr != -1) ? isr : 0;
> >      if ( (isr & 0xf0) >= (irr & 0xf0) ) @@ -1026,6 +1054,9 @@ int
> > vlapic_ack_pending_irq(struct vcpu * {
> >      struct vlapic *vlapic = vcpu_vlapic(v);
> >
> > +    if ( cpu_has_vmx_virtual_intr_delivery )
> > +        return 1;
> > +
> >     vlapic_set_vector(vector, &vlapic->regs->data[APIC_ISR]);
> >      vlapic_clear_irr(vector, vlapic);
> >
> 


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