From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [RFC PATCH v3 13/18] xen/arm: ITS: Add irq descriptors for LPIs Date: Mon, 29 Jun 2015 14:11:50 +0100 Message-ID: <55914416.6040704@citrix.com> References: <1434974517-12136-1-git-send-email-vijay.kilari@gmail.com> <1434974517-12136-14-git-send-email-vijay.kilari@gmail.com> <1435582703.32500.317.camel@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1435582703.32500.317.camel@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: Ian Campbell , vijay.kilari@gmail.com Cc: stefano.stabellini@eu.citrix.com, Prasun.Kapoor@caviumnetworks.com, vijaya.kumar@caviumnetworks.com, tim@xen.org, xen-devel@lists.xen.org, stefano.stabellini@citrix.com, manish.jaggi@caviumnetworks.com List-Id: xen-devel@lists.xenproject.org On 29/06/15 13:58, Ian Campbell wrote: > On Mon, 2015-06-22 at 17:31 +0530, vijay.kilari@gmail.com wrote: >> From: Vijaya Kumar K >> >> Add irq descriptors for LPIs and route > > This seems to also do interrupt injection for LPIs and more. Please > check that your commit messages are accurately describing the contents > of the patch. If it turns into a long list of unrelated sounding things > then that might suggest the patch should be split up. > >> Signed-off-by: Vijaya Kumar K >> --- >> xen/arch/arm/gic-v3.c | 8 +++- >> xen/arch/arm/gic.c | 17 +++++++- >> xen/arch/arm/irq.c | 38 +++++++++++++---- >> xen/arch/arm/vgic-v3-its.c | 9 +++++ >> xen/arch/arm/vgic.c | 90 ++++++++++++++++++++++++++++++++++++++--- >> xen/include/asm-arm/domain.h | 2 + >> xen/include/asm-arm/gic-its.h | 6 +++ >> xen/include/asm-arm/gic.h | 3 ++ >> xen/include/asm-arm/vgic.h | 1 + >> 9 files changed, 157 insertions(+), 17 deletions(-) >> >> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c >> index 737646c..793f2f0 100644 >> --- a/xen/arch/arm/gic-v3.c >> +++ b/xen/arch/arm/gic-v3.c >> @@ -899,9 +899,13 @@ static void gicv3_update_lr(int lr, const struct pending_irq *p, >> >> val = (((uint64_t)state & 0x3) << GICH_LR_STATE_SHIFT) | grp; >> val |= ((uint64_t)p->priority & 0xff) << GICH_LR_PRIORITY_SHIFT; >> - val |= ((uint64_t)p->irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT; >> >> - if ( p->desc != NULL ) >> + if ( is_lpi(p->irq) ) >> + val |= ((uint64_t)p->irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT; >> + else >> + val |= ((uint64_t)p->irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT; > > Is there supposed to be something different between these two cases? (Or > am I missing it?) > >> + >> + if ( p->desc != NULL && !(is_lpi(p->irq)) ) >> val |= GICH_LR_HW | (((uint64_t)p->desc->irq & GICH_LR_PHYSICAL_MASK) >> << GICH_LR_PHYSICAL_SHIFT); >> >> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c >> index cfc9c42..091f7e5 100644 >> --- a/xen/arch/arm/gic.c >> +++ b/xen/arch/arm/gic.c >> @@ -124,18 +124,31 @@ void gic_route_irq_to_xen(struct irq_desc *desc, const cpumask_t *cpu_mask, >> unsigned int priority) >> { >> ASSERT(priority <= 0xff); /* Only 8 bits of priority */ >> - ASSERT(desc->irq < gic_number_lines());/* Can't route interrupts that don't exist */ >> + /* Can't route interrupts that don't exist */ >> + ASSERT(desc->irq < gic_number_lines() && is_lpi(desc->irq)); > > ||, surely? Otherwise doesn't this hit for every possible irq? Aside that, the change in the ASSERT is wrong. The goal of the helper gic_number_lines is to return the number of IRQs (i.e PPIs, LPIs, SPIs) present in the hardware. We use in few places to ensure the validity of the IRQ. Although, this will require some extra care in places where an interrupt is assigned to a domain in order to only allow SPIs. Regards, -- Julien Grall