From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH v4 05/17] xen/arm: ITS: implement hw_irq_controller for LPIs Date: Mon, 13 Jul 2015 23:18:20 +0200 Message-ID: <55A42B1C.2030600@citrix.com> References: <1436514172-3263-1-git-send-email-vijay.kilari@gmail.com> <1436514172-3263-6-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-6-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@caviumnetworks.com, manish.jaggi@caviumnetworks.com List-Id: xen-devel@lists.xenproject.org Hi, On 10/07/2015 09:42, vijay.kilari@gmail.com wrote: > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c > index c41e82e..4f3801b 100644 > --- a/xen/arch/arm/gic.c > +++ b/xen/arch/arm/gic.c > +static inline hw_irq_controller *get_host_hw_irq_controller(unsigned int irq) > +{ > + if ( is_lpi(irq) ) > + return its_hw_ops->lpi_host_irq_type; > + else > + return gic_hw_ops->gic_host_irq_type; > +} This is not what I asked on v3 [1]. The ITS hardware controller shouldn't be exposed to the common GIC. We have to keep a clean and comprehensible interface. What I asked is to replace the gic_host_irq_type variable by a new callback which will return the correct hw_irq_controller. For GICv2, it will return the same hw_irq_controller as today. For GICv3, it will check is the IRQ is an LPI and return the correct controller. FWIW, it was "ack" by Ian [2]. > + > +static inline hw_irq_controller *get_guest_hw_irq_controller(unsigned int irq) > +{ > + if ( is_lpi(irq) ) > + return its_hw_ops->lpi_guest_irq_type; > + else > + return gic_hw_ops->gic_guest_irq_type; > +} > + > /* > * needs to be called with a valid cpu_mask, ie each cpu in the mask has > * already called gic_cpu_init > @@ -104,7 +126,8 @@ static void gic_set_irq_properties(struct irq_desc *desc, > const cpumask_t *cpu_mask, > unsigned int priority) > { > - gic_hw_ops->set_irq_properties(desc, cpu_mask, priority); > + if ( desc->irq < gic_number_lines() ) > + gic_hw_ops->set_irq_properties(desc, cpu_mask, priority); > } Why this function can't be called for LPI? The configuration should likely be the same... > @@ -149,7 +173,7 @@ int gic_route_irq_to_guest(struct domain *d, unsigned int virq, > test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) ) > goto out; > > - desc->handler = gic_hw_ops->gic_guest_irq_type; > + desc->handler = get_guest_hw_irq_controller(desc->irq); > set_bit(_IRQ_GUEST, &desc->status); > > gic_set_irq_properties(desc, cpumask_of(v_target->processor), priority); > diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c > index 2dd43ee..ba8528a 100644 > --- a/xen/arch/arm/irq.c > +++ b/xen/arch/arm/irq.c > @@ -35,7 +35,13 @@ static DEFINE_SPINLOCK(local_irqs_type_lock); > struct irq_guest > { > struct domain *d; > - unsigned int virq; > + union > + { > + /* virq refer to virtual irq in case of spi */ > + unsigned int virq; > + /* virq refer to event ID in case of lpi */ > + unsigned int vid; Why can't we store the event ID in the irq_guest? As said on v3, this is not domain specific [3]. Furthermore, you add support to route LPI in Xen (see gic_route_irq_to_xen) where you will clearly need the event ID. > void irq_set_affinity(struct irq_desc *desc, const cpumask_t *cpu_mask) > { > if ( desc != NULL ) > diff --git a/xen/include/asm-arm/gic-its.h b/xen/include/asm-arm/gic-its.h > index b5e09bd..e8d244f 100644 > --- a/xen/include/asm-arm/gic-its.h > +++ b/xen/include/asm-arm/gic-its.h > @@ -161,6 +161,10 @@ typedef union { > * The ITS view of a device. > */ > struct its_device { > + /* Physical ITS */ > + struct its_node *its; > + /* Number of Physical LPIs assigned */ > + int nr_lpis; Why didn't you add this field directly in the patch #4? It would be more logical. > /* > * ITS registers, offsets from ITS_base > diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h > index 34b492b..55e219f 100644 > --- a/xen/include/asm-arm/irq.h > +++ b/xen/include/asm-arm/irq.h > @@ -17,6 +17,8 @@ struct arch_pirq > struct arch_irq_desc { > int eoi_cpu; > unsigned int type; > + struct its_device *dev; > + u16 col_id; It has been suggested by Ian to move col_id in the its_device in the previous version [4]. Any reason to not doing it? Regards, [1] http://www.gossamer-threads.com/lists/xen/devel/386493#386493 [2] http://www.gossamer-threads.com/lists/xen/devel/386771#386771 [3] http://www.gossamer-threads.com/lists/xen/devel/385521#385521 [4] http://www.gossamer-threads.com/lists/xen/devel/387586#387586 -- Julien Grall