From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH v4 05/17] xen/arm: ITS: implement hw_irq_controller for LPIs Date: Fri, 10 Jul 2015 14:46:38 +0100 Message-ID: <1436535998.10074.29.camel@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" 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 Cc: stefano.stabellini@eu.citrix.com, Prasun.Kapoor@caviumnetworks.com, vijaya.kumar@caviumnetworks.com, tim@xen.org, xen-devel@lists.xen.org, julien.grall@citrix.com, stefano.stabellini@citrix.com, manish.jaggi@caviumnetworks.com List-Id: xen-devel@lists.xenproject.org On Fri, 2015-07-10 at 13:12 +0530, vijay.kilari@gmail.com wrote: > From: Vijaya Kumar K > > Implements hw_irq_controller api's required > to handle LPI's > > Signed-off-by: Vijaya Kumar K > --- > v4: - Implement separate hw_irq_controller for LPIs > - Drop setting LPI affinity > - virq and vid are moved under union > - Introduced inv command handling > - its_device is stored in irq_desc > --- > xen/arch/arm/gic-v3-its.c | 132 +++++++++++++++++++++++++++++++++++++ > xen/arch/arm/gic-v3.c | 5 +- > xen/arch/arm/gic.c | 32 +++++++-- > xen/arch/arm/irq.c | 40 ++++++++++- > xen/include/asm-arm/gic-its.h | 4 ++ > xen/include/asm-arm/gic.h | 13 ++++ > xen/include/asm-arm/gic_v3_defs.h | 1 + > xen/include/asm-arm/irq.h | 8 ++- > 8 files changed, 227 insertions(+), 8 deletions(-) > > diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c > index b421a6f..b98d396 100644 > --- a/xen/arch/arm/gic-v3-its.c > +++ b/xen/arch/arm/gic-v3-its.c > @@ -295,6 +295,19 @@ post: > its_wait_for_range_completion(its, cmd, next_cmd); > } > > +static void its_send_inv(struct its_device *dev, struct its_collection *col, > + u32 event_id) > +{ > + its_cmd_block cmd; > + > + memset(&cmd, 0x0, sizeof(its_cmd_block)); > + cmd.inv.cmd = GITS_CMD_INV; > + cmd.inv.devid = dev->device_id; > + cmd.inv.event = event_id; > + > + its_send_single_command(dev->its, &cmd, col); > +} This ought to be in the prior patch doing such things I think. Oh I see, you didn't have struct its_device defined back then. I think you can just reorder patches #3 and #4 to solve that. > +static void its_host_irq_end(struct irq_desc *desc) > +{ > + /* Lower the priority */ > + gicv3_eoi_irq(desc); > + /* Deactivate */ > + gicv3_dir_irq(desc); > +} > + > +static void its_guest_irq_end(struct irq_desc *desc) > +{ > + gicv3_eoi_irq(desc); > +} Exposing those two gicv3 functions is a bit unfortunate, but I think it will do for now. Exposing gicv3_[host|guest]_irq_end might have been nicer, since you could just insert them into your its_[host|guest]_lpi_type instead of duplicating them. Eventually we may want to refactor such that register_its_ops gives back the ack/eoi hooks to use. > +static void its_irq_set_affinity(struct irq_desc *desc, const cpumask_t *mask) > +{ Please add /* Not yet supported */ > @@ -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() ) Should this be is_lpi as in other similar places? > + gic_hw_ops->set_irq_properties(desc, cpu_mask, priority); > } > > /* Program the GIC to route an interrupt to the host (i.e. Xen) > @@ -114,11 +137,12 @@ 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)); As discussed in <1436284206.25646.258.camel@citrix.com> please make some sort of is_valid_irq(irq) helper to encapsulate this logic. > ocessor), 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 */ "refers" in both cases And I'd say "to the ..." not just "to ..." and "in the case of..." too. > +unsigned int irq_to_vid(struct irq_desc *desc) > +{ > + return irq_get_guest_info(desc)->vid; > +} > + > +unsigned int irq_to_virq(struct irq_desc *desc) > +{ > + return irq_get_guest_info(desc)->virq; > +} Please assert that irq_desc->arch.its_device is (non-)NULL as appropriate in these two cases. BTW, while checking the field name I spotted "struct msi_desc *msi_desc" in the main struct irq_desc. Since MSIs are effectively the same as LPIs as a future cleanup I think we should s/its_device/msi_desc/g and use this field instead of adding a second redundant type and pointer to it. THis is not a blocker for 4.6 though. > +struct its_device *get_irq_device(struct irq_desc *desc) > +{ > + ASSERT(spin_is_locked(&desc->lock)); > + > + return desc->arch.dev; > +} > + > +void set_irq_device(struct irq_desc *desc, struct its_device *dev) > +{ > + ASSERT(spin_is_locked(&desc->lock)); > + desc->arch.dev = dev; > +} Please add _its to the names of both of these functions, ie..g set_irq_its_device. > 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; > /* Physical Device id */ > u32 device_id; > /* RB-tree entry */ > diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h > index e9d5f36..44c2317 100644 > --- a/xen/include/asm-arm/gic.h > +++ b/xen/include/asm-arm/gic.h > @@ -20,6 +20,9 @@ > > #define NR_GIC_LOCAL_IRQS NR_LOCAL_IRQS > #define NR_GIC_SGI 16 > +#define FIRST_GIC_LPI 8192 > +#define NR_GIC_LPI 4096 > +#define MAX_LPI (FIRST_GIC_LPI + NR_GIC_LPI) MAX_LPI and NR_GIC_LPI should be obtained from the hardware at init time and put somewhere, like a global nr_lpis perhaps, to be used throughout. > diff --git a/xen/include/asm-arm/gic_v3_defs.h b/xen/include/asm-arm/gic_v3_defs.h > index 051a95e..0443ae7 100644 > --- a/xen/include/asm-arm/gic_v3_defs.h > +++ b/xen/include/asm-arm/gic_v3_defs.h > @@ -169,6 +169,7 @@ > #define ICH_SGI_IRQ_MASK 0xf > #define ICH_SGI_TARGETLIST_MASK 0xffff > #define LPI_PROP_GROUP1 (1 << 1) > +#define LPI_PROP_ENABLED (1 << 0) Please order (1 << X) by the X. > 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; There are probably opportunities to make eoi_cpu or type smaller so as to allow col_id to be added without increasing the size of the struct. Not a blocker for 4.6 though. Ian.