From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH v4 12/17] xen/arm: ITS: Initialize LPI irq descriptors and route Date: Wed, 15 Jul 2015 20:13:39 +0200 Message-ID: <55A6A2D3.5060903@citrix.com> References: <1436514172-3263-1-git-send-email-vijay.kilari@gmail.com> <1436514172-3263-13-git-send-email-vijay.kilari@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1436514172-3263-13-git-send-email-vijay.kilari@gmail.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: vijay.kilari@gmail.com, Ian.Campbell@citrix.com, stefano.stabellini@eu.citrix.com, stefano.stabellini@citrix.com, tim@xen.org, xen-devel@lists.xen.org Cc: Prasun.Kapoor@caviumnetworks.com, Vijaya Kumar K , manish.jaggi@caviumnetworks.com List-Id: xen-devel@lists.xenproject.org Hi Vijay, On 10/07/2015 09:42, vijay.kilari@gmail.com wrote: > diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c > index e6004d2..53554e6 100644 > --- a/xen/arch/arm/gic-v3.c > +++ b/xen/arch/arm/gic-v3.c > @@ -895,7 +895,7 @@ static void gicv3_update_lr(int lr, const struct pending_irq *p, > 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 ( p->desc != NULL && !(is_lpi(p->irq)) ) It seems that you replaced all the p->desc != NULL by "p->desc != NULL && !is_lpi(p->irq). Why don't you avoid to set p->desc in this case? You may also want to put some explanation in the commit message to explain why you don't have to set the GICH_LR.HW bit for LPIs. > 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 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 */ This comment seems wrong. The function doesn't validate the IRQ but return the number of Lines (i.e PPIs/SGIs/SPIs). > unsigned int gic_number_lines(void) > { > return gic_hw_ops->info->nr_lines; > } [...] > int gic_route_irq_to_guest(struct domain *d, unsigned int virq, > struct irq_desc *desc, unsigned int priority) > { > @@ -454,7 +472,7 @@ static void gic_update_one_lr(struct vcpu *v, int i) > if ( test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) && > test_and_clear_bit(GIC_IRQ_GUEST_QUEUED, &p->status) ) > { > - if ( p->desc == NULL ) > + if ( p->desc == NULL || is_lpi(irq) ) > { > lr_val.state |= GICH_LR_PENDING; > gic_hw_ops->write_lr(i, &lr_val); > @@ -677,7 +695,7 @@ void gic_interrupt(struct cpu_user_regs *regs, int is_fiq) > /* Reading IRQ will ACK it */ > irq = gic_hw_ops->read_irq(); > > - if ( likely(irq >= 16 && irq < 1020) ) > + if ( (likely(irq >= 16 && irq < 1020)) || is_lpi(irq) ) Please move the is_lpi(irq) in likely. > { > local_irq_enable(); > do_IRQ(regs, irq, is_fiq); [...] > @@ -208,7 +226,7 @@ int request_irq(unsigned int irq, unsigned int irqflags, > * which interrupt is which (messes up the interrupt freeing > * logic etc). > */ > - if ( irq >= nr_irqs ) > + if ( irq >= nr_irqs && !is_lpi(irq) ) Technically nr_irqs should contain the total number of IRQ and not only the number of SPI/PPI/SGI. Either modify nr_irqs or use gic_is_valid_irq which would do the same here. > return -EINVAL; > if ( !handler ) > return -EINVAL; > @@ -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); > goto out_no_end; > } > > @@ -436,7 +459,8 @@ err: > bool_t is_assignable_irq(unsigned int irq) > { > /* For now, we can only route SPIs to the guest */ > - return ((irq >= NR_LOCAL_IRQS) && (irq < gic_number_lines())); > + return (((irq >= NR_LOCAL_IRQS) && (irq < gic_number_lines())) || > + is_lpi(irq)); If you modify the function, please also modify the comment which become invalid now. [...] > diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c > index bbcc7bb..4649b07 100644 > --- a/xen/arch/arm/vgic-v3-its.c > +++ b/xen/arch/arm/vgic-v3-its.c > @@ -625,6 +625,15 @@ err: > return 0; > } > > +uint8_t vgic_its_get_priority(struct vcpu *v, uint32_t pid) > +{ > + uint8_t priority; > + > + priority = readb_relaxed(v->domain->arch.vits->prop_page + pid); Why do you use readb_relaxed here? This should only be used for Device MMIO. Although, you need to ensure that the value will be correctly synchronize if another CPU is writing in prop_page which is protected by prop_lock. > + priority &= LPI_PRIORITY_MASK; > + > + return priority; > +} > static int vgic_v3_gits_lpi_mmio_read(struct vcpu *v, mmio_info_t *info) > { > uint32_t offset; > diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h > index 69bf1ff..537ed3d 100644 > --- a/xen/include/asm-arm/gic.h > +++ b/xen/include/asm-arm/gic.h > @@ -226,6 +226,9 @@ extern void gic_route_irq_to_xen(struct irq_desc *desc, const cpumask_t *cpu_mas > extern int gic_route_irq_to_guest(struct domain *, unsigned int virq, > struct irq_desc *desc, > unsigned int priority); > +extern int gic_route_lpi_to_guest(struct domain *d, unsigned int virq, > + struct irq_desc *desc, > + unsigned int priority); > > /* Remove an IRQ passthrough to a guest */ > int gic_remove_irq_from_guest(struct domain *d, unsigned int virq, > @@ -282,8 +285,10 @@ extern void send_SGI_allbutself(enum gic_sgi sgi); > /* print useful debug info */ > extern void gic_dump_info(struct vcpu *v); > > -/* Number of interrupt lines */ > +/* Number of interrupt lines (SPIs)*/ This is not really true. The interrupts lines is equals to PPIs + SGIs + SPIs. > extern unsigned int gic_number_lines(void); > +/* Check if irq is valid SPI or LPI */ This comment is not true. This function is used to check that an IRQ is valid in general, not only for SPI or LPI. > +bool_t gic_is_valid_irq(unsigned int irq); > /* Number of interrupt id bits supported */ > extern unsigned int gic_nr_id_bits(void); > /* LPI support info */ Regards, -- Julien Grall