Re: [PATCH 2/7] Nested VMX: Allow to ack irq even virtual intr delivery is enabled

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 09/08/13 09:49, Yang Zhang wrote:
> From: Yang Zhang <yang.z.zhang@Intel.com>
>
> In some special cases, we want to ack irq regardless of virtual interrupt delivery.

Can you explain why and when we might want to force an ack?

>
> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
> ---
>  xen/arch/x86/hvm/irq.c           |    2 +-
>  xen/arch/x86/hvm/vlapic.c        |    4 ++--
>  xen/include/asm-x86/hvm/vlapic.h |    2 +-
>  3 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/irq.c b/xen/arch/x86/hvm/irq.c
> index 9eae5de..6a6fb68 100644
> --- a/xen/arch/x86/hvm/irq.c
> +++ b/xen/arch/x86/hvm/irq.c
> @@ -437,7 +437,7 @@ struct hvm_intack hvm_vcpu_ack_pending_irq(
>              intack.vector = (uint8_t)vector;
>          break;
>      case hvm_intsrc_lapic:
> -        if ( !vlapic_ack_pending_irq(v, intack.vector) )
> +        if ( !vlapic_ack_pending_irq(v, intack.vector, 0) )
>              intack = hvm_intack_none;
>          break;
>      case hvm_intsrc_vector:
> diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
> index 7a154f9..20a36a0 100644
> --- a/xen/arch/x86/hvm/vlapic.c
> +++ b/xen/arch/x86/hvm/vlapic.c
> @@ -1048,11 +1048,11 @@ int vlapic_has_pending_irq(struct vcpu *v)
>      return irr;
>  }
>  
> -int vlapic_ack_pending_irq(struct vcpu *v, int vector)
> +int vlapic_ack_pending_irq(struct vcpu *v, int vector, int force_ack)
>  {
>      struct vlapic *vlapic = vcpu_vlapic(v);
>  
> -    if ( vlapic_virtual_intr_delivery_enabled() )
> +    if ( vlapic_virtual_intr_delivery_enabled() && !force_ack )
>          return 1;

The logic in this entire function seems quite backwards.  It
unconditionally returns 1 in all cases, and its sole callsite is in an
if statement.

Something like:

void vlapic_ack_pending_irq(struct vcpu *v, uint8_t vector, bool_t
force_ack)
{
    struct vlapic *vlapic = vcpu_vlapic(v);

    if ( force_ack || !vlapic_virtual_intr_delivery_enabled() )
    {
        vlapic_set_vector(vector, &vlapic->regs->data[APIC_ISR]);
        vlapic_clear_irr(vector, vlapic);
    }
}

Seems substantially clearer.

~Andrew

>  
>      vlapic_set_vector(vector, &vlapic->regs->data[APIC_ISR]);
> diff --git a/xen/include/asm-x86/hvm/vlapic.h b/xen/include/asm-x86/hvm/vlapic.h
> index 021a5f2..d8c9511 100644
> --- a/xen/include/asm-x86/hvm/vlapic.h
> +++ b/xen/include/asm-x86/hvm/vlapic.h
> @@ -96,7 +96,7 @@ bool_t is_vlapic_lvtpc_enabled(struct vlapic *vlapic);
>  void vlapic_set_irq(struct vlapic *vlapic, uint8_t vec, uint8_t trig);
>  
>  int vlapic_has_pending_irq(struct vcpu *v);
> -int vlapic_ack_pending_irq(struct vcpu *v, int vector);
> +int vlapic_ack_pending_irq(struct vcpu *v, int vector, int force_ack);
>  
>  int  vlapic_init(struct vcpu *v);
>  void vlapic_destroy(struct vcpu *v);