From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH 2/7] Nested VMX: Allow to ack irq even virtual intr delivery is enabled Date: Fri, 9 Aug 2013 11:28:07 +0100 Message-ID: <5204C437.9050107@citrix.com> References: <1376038175-18571-1-git-send-email-yang.z.zhang@intel.com> <1376038175-18571-3-git-send-email-yang.z.zhang@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1376038175-18571-3-git-send-email-yang.z.zhang@intel.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Yang Zhang Cc: xen-devel@lists.xensource.com, keir.xen@gmail.com, JBeulich@suse.com List-Id: xen-devel@lists.xenproject.org On 09/08/13 09:49, Yang Zhang wrote: > From: Yang Zhang > > 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 > --- > 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);