From mboxrd@z Thu Jan 1 00:00:00 1970 From: Vijay Kilari Subject: Re: [PATCH v4 05/17] xen/arm: ITS: implement hw_irq_controller for LPIs Date: Sat, 11 Jul 2015 20:10:42 +0530 Message-ID: References: <1436514172-3263-1-git-send-email-vijay.kilari@gmail.com> <1436514172-3263-6-git-send-email-vijay.kilari@gmail.com> <1436535998.10074.29.camel@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1436535998.10074.29.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 7:16 PM, Ian Campbell wrote: > 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. INV is used only in this patch in lpi_set_config(). So introduced in this patch > >> +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. I have added it patch#12. I remove this change from this patch > >> 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. These two functions are accessing irq_guest structure not arch.its_device > > 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. This MAX_LPI and NR_GIC_LPI is Xen limitation where in we are allocating irq_descriptors statically upto NR_GIC_LPI.