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

[Keir Fraser <keir.xen@gmail.com>]

On 31/08/2012 10:30, "Li, Jiongxi" <jiongxi.li@intel.com> 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@intel.com>
> Signed-off-by: Jiongxi Li <jiongxi.li@intel.com>
> 
> 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?

> +    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.

> +    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?

>      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);
>