From mboxrd@z Thu Jan 1 00:00:00 1970 From: Vijay Kilari Subject: Re: [PATCH v4 12/17] xen/arm: ITS: Initialize LPI irq descriptors and route Date: Wed, 22 Jul 2015 19:01:50 +0530 Message-ID: References: <1436514172-3263-1-git-send-email-vijay.kilari@gmail.com> <1436514172-3263-13-git-send-email-vijay.kilari@gmail.com> <1436542215.10074.100.camel@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1436542215.10074.100.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 Cc: Stefano Stabellini , Prasun Kapoor , Vijaya Kumar K , Tim Deegan , "xen-devel@lists.xen.org" , Julien Grall , Stefano Stabellini , manish.jaggi@caviumnetworks.com List-Id: xen-devel@lists.xenproject.org On Fri, Jul 10, 2015 at 9:00 PM, Ian Campbell wrote: > On Fri, 2015-07-10 at 13:12 +0530, vijay.kilari@gmail.com wrote: >> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c >> index 3ebadcf..92d2be9 100644 >> --- a/xen/arch/arm/gic.c >> +++ b/xen/arch/arm/gic.c >> @@ -68,11 +68,18 @@ enum gic_version gic_hw_version(void) >> return gic_hw_ops->info->hw_version; >> } >> >> +/* Only validates PPIs/SGIs/SPIs supported */ >> unsigned int gic_number_lines(void) >> { >> return gic_hw_ops->info->nr_lines; >> } >> >> +/* Validates PPIs/SGIs/SPIs/LPIs supported */ >> +bool_t gic_is_valid_irq(unsigned int irq) >> +{ >> + return ((irq < gic_hw_ops->info->nr_lines) && is_lpi(irq)); >> +} >> + >> unsigned int gic_nr_id_bits(void) >> { >> return gic_hw_ops->info->nr_id_bits; >> @@ -148,7 +155,7 @@ void gic_route_irq_to_xen(struct irq_desc *desc, const cpumask_t *cpu_mask, >> { >> ASSERT(priority <= 0xff); /* Only 8 bits of priority */ >> /* Can't route interrupts that don't exist */ >> - ASSERT(desc->irq < gic_number_lines() || is_lpi(desc->irq)); >> + ASSERT(gic_is_valid_irq(desc->irq)); > > > The fact that I commented on this earlier is another artefact in the way > this series presents functionality in an essentially arbitrary order. > > I notice that you appear to have reintroduced the logic but when you > moved this code from here into gic_is_valid_irq. AFAICT that function > can never return true, but then I'm rather confused about how this > series can have been tested. > >> ASSERT(test_bit(_IRQ_DISABLED, &desc->status)); >> ASSERT(spin_is_locked(&desc->lock)); >> >> @@ -160,6 +167,17 @@ void gic_route_irq_to_xen(struct irq_desc *desc, const cpumask_t *cpu_mask, >> /* Program the GIC to route an interrupt to a guest >> * - desc.lock must be held >> */ >> +int gic_route_lpi_to_guest(struct domain *d, unsigned int virq, >> + struct irq_desc *desc, unsigned int priority) >> +{ >> + ASSERT(spin_is_locked(&desc->lock)); > > ASSERT(virq >= SOME_DEFINE_FOR_MIN_LPI); > >> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c >> index 3806d98..c8ea627 100644 >> --- a/xen/arch/arm/irq.c >> +++ b/xen/arch/arm/irq.c >> @@ -62,12 +62,21 @@ hw_irq_controller no_irq_type = { >> }; >> >> static irq_desc_t irq_desc[NR_IRQS]; >> +#ifdef CONFIG_ARM_64 >> +static irq_desc_t irq_desc_lpi[NR_GIC_LPI]; > > http://xenbits.xen.org/people/ianc/vits/draftG.html#irq-descriptors > contains: "Therefore a second dynamically allocated array will be added > to cover the range 8192..nr_lpis" > > IOW this should be dynamically allocated and therefore NR_GIC_LPI (which > I think I already commented on earlier) should go away, since the limit > should come from the h/w. >> @@ -267,9 +285,14 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int is_fiq) >> set_bit(_IRQ_INPROGRESS, &desc->status); >> desc->arch.eoi_cpu = smp_processor_id(); >> >> +#ifdef CONFIG_ARM_64 >> + if ( is_lpi(irq) ) >> + vgic_vcpu_inject_lpi(info->d, irq); >> + else >> +#endif >> /* the irq cannot be a PPI, we only support delivery of SPIs to >> * guests */ >> - vgic_vcpu_inject_spi(info->d, info->virq); >> + vgic_vcpu_inject_spi(info->d, info->virq); > > Comment should be reindented too. > >> + if ( !is_lpi(irq) ) >> + { >> + rank = vgic_rank_irq(v, irq); >> + >> + ASSERT(spin_is_locked(&rank->lock)); >> + priority = vgic_byte_read(rank->ipriority[REG_RANK_INDEX(8, >> irq, DABT_WORD)], 0, irq & 0x3); >> + } >> + if ( is_lpi(irq) && gic_lpi_supported() ) > > Should be "else if ( gic_lpi_supported() )" > >> + vdev = vits_find_device(&d->arch.vits->dev_root, devid); >> + if ( !vdev ) >> + { >> + dprintk(XENLOG_WARNING, >> + "LPI %d received for dev 0x%x not assigned..dropping\n", >> + irq, devid); >> + return; >> + } > > This isn't used again and the whole R-B tree should go away. > >> + p = irq_to_pending(d->vcpu[0], vitt_entry.vlpi); > > Perhaps given that we know this is a vlpi, we could skipping going via > vcpu[0] and just go right to the entry in the d->pending_lpi array (via > a suitable accessor of course) Why can't we use irq_to_pending(d->vcpu[target], vitt_entry.vlpi) ?. > >> + p->desc = desc; > > This should have happened during routing, not now. While routing we don't have vlpi to update p->desc. > > Oh. I see what is going on, your vgic_vcpu_inject_lpi appears to be > taking an IRQ, and not a vpli, or at least to be confused about what it > should do with the number which it calls irq. > > Therefore you find yourself needing to lookup the irqdesc, and look in > the vitt etc, none of which belongs here. > > For interrupts coming from a plpi all of that lookup should happen based > on the its_device which is contained in the irq_guest and other info > from there, before calling this function. You can get the irq_guest > which you can get because you have a PLPI in your hand and can look up > the irq_guest via the irq_desc which you get using the plpi number. > > For interrupts coming from the VITS INT command the command itself > contains a vDevice and vEvent, which can then be looked up in the ITT to > get a vpli which can be passed to vgic_vcpu_inject_lpi. > > Thus this function has no need to lookup an irq_desc, nor a dev id, nor > to do an rb tree lookup. > > I think both of these cases are adequately explained, with pseudocode, > in the draftG vits design doc. Please ask if you are unsure. > > Ian. >