From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH-4.5 2/4] xen/arm: support HW interrupts in gic_set_lr Date: Fri, 07 Feb 2014 22:31:00 +0000 Message-ID: <52F55EA4.2060703@linaro.org> References: <1391799378-31664-2-git-send-email-stefano.stabellini@eu.citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1391799378-31664-2-git-send-email-stefano.stabellini@eu.citrix.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: Stefano Stabellini , xen-devel@lists.xensource.com Cc: julien.grall@citrix.com, Ian.Campbell@citrix.com List-Id: xen-devel@lists.xenproject.org Hi Stefano, On 07/02/14 18:56, Stefano Stabellini wrote: > If the irq to be injected is an hardware irq (p->desc != NULL), set > GICH_LR_HW. If you set the GICH_LR_HW, I think you should remove the EOI of physical interrupt in maintenance IRQ in this patch. Otherwise we will EOI twice and from the documentation the behavior is unpredicatable. > Also add a struct vcpu* parameter to gic_set_lr. > Signed-off-by: Stefano Stabellini > --- > xen/arch/arm/gic.c | 28 ++++++++++++++++------------ > 1 file changed, 16 insertions(+), 12 deletions(-) > > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c > index acf7195..215b679 100644 > --- a/xen/arch/arm/gic.c > +++ b/xen/arch/arm/gic.c > @@ -618,20 +618,24 @@ int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new) > return rc; > } > > -static inline void gic_set_lr(int lr, unsigned int virtual_irq, > +static inline void gic_set_lr(struct vcpu *v, int lr, unsigned int irq, > unsigned int state, unsigned int priority) > { > - int maintenance_int = GICH_LR_MAINTENANCE_IRQ; > - struct pending_irq *p = irq_to_pending(current, virtual_irq); > + struct pending_irq *p = irq_to_pending(v, irq); > > BUG_ON(lr >= nr_lrs); > BUG_ON(lr < 0); > BUG_ON(state & ~(GICH_LR_STATE_MASK< > - GICH[GICH_LR + lr] = state | > - maintenance_int | > - ((priority >> 3) << GICH_LR_PRIORITY_SHIFT) | > - ((virtual_irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT); > + if ( p->desc != NULL ) > + GICH[GICH_LR + lr] = GICH_LR_HW | state | GICH_LR_MAINTENANCE_IRQ | > + ((priority >> 3) << GICH_LR_PRIORITY_SHIFT) | > + ((irq & GICH_LR_PHYSICAL_MASK) << GICH_LR_PHYSICAL_SHIFT) | We should not assume that the physical IRQ == virtual IRQ. You should use p->desc->irq > + ((irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT); > + else > + GICH[GICH_LR + lr] = state | GICH_LR_MAINTENANCE_IRQ | > + ((priority >> 3) << GICH_LR_PRIORITY_SHIFT) | > + ((irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT); The final result of virtual IRQ is a subset of the physical IRQ. Can you factor the code? -- Julien Grall