Re: [RFC 3/8] xen/arm: gic: split set_irq_properties

[Stefano Stabellini <sstabellini@kernel.org>]

On Tue, 7 Jun 2016, Julien Grall wrote:
> The callback set_irq_properties will configure the GIC for a specific
> IRQ with the type and the priority.
> 
> In a follow-up patch, Xen will configure the type and the priority at
> different stage of the routing. So split it in 2 separate callbacks.
> 
> At the same time, move the ASSERT to check the validity of the type and
> if the desc->lock is locked in the common code (gic.c). This is because
> the constraint are the same between GICv2 and GICv3, however the driver
> of the latter did not contain any sanity check.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


>  xen/arch/arm/gic-v2.c     | 19 +++++++++++++------
>  xen/arch/arm/gic-v3.c     | 15 ++++++++++++---
>  xen/arch/arm/gic.c        | 23 ++++++++++++++---------
>  xen/include/asm-arm/gic.h |  7 ++++---
>  4 files changed, 43 insertions(+), 21 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index 90b07b3..fa2c5a5 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -200,16 +200,12 @@ static unsigned int gicv2_read_irq(void)
>      return (readl_gicc(GICC_IAR) & GICC_IA_IRQ);
>  }
>  
> -static void gicv2_set_irq_properties(struct irq_desc *desc,
> -                                     unsigned int priority)
> +static void gicv2_set_irq_type(struct irq_desc *desc)
>  {
>      uint32_t cfg, actual, edgebit;
>      unsigned int irq = desc->irq;
>      unsigned int type = desc->arch.type;
>  
> -    ASSERT(type != IRQ_TYPE_INVALID);
> -    ASSERT(spin_is_locked(&desc->lock));
> -
>      spin_lock(&gicv2.lock);
>      /* Set edge / level */
>      cfg = readl_gicd(GICD_ICFGR + (irq / 16) * 4);
> @@ -234,6 +230,16 @@ static void gicv2_set_irq_properties(struct irq_desc *desc,
>              IRQ_TYPE_LEVEL_HIGH;
>      }
>  
> +    spin_unlock(&gicv2.lock);
> +}
> +
> +static void gicv2_set_irq_priority(struct irq_desc *desc,
> +                                   unsigned int priority)
> +{
> +    unsigned int irq = desc->irq;
> +
> +    spin_lock(&gicv2.lock);
> +
>      /* Set priority */
>      writeb_gicd(priority, GICD_IPRIORITYR + irq);
>  
> @@ -916,7 +922,8 @@ const static struct gic_hw_operations gicv2_ops = {
>      .eoi_irq             = gicv2_eoi_irq,
>      .deactivate_irq      = gicv2_dir_irq,
>      .read_irq            = gicv2_read_irq,
> -    .set_irq_properties  = gicv2_set_irq_properties,
> +    .set_irq_type        = gicv2_set_irq_type,
> +    .set_irq_priority    = gicv2_set_irq_priority,
>      .send_SGI            = gicv2_send_SGI,
>      .disable_interface   = gicv2_disable_interface,
>      .update_lr           = gicv2_update_lr,
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index c936c8a..c25fe50 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -471,8 +471,7 @@ static inline uint64_t gicv3_mpidr_to_affinity(int cpu)
>               MPIDR_AFFINITY_LEVEL(mpidr, 0));
>  }
>  
> -static void gicv3_set_irq_properties(struct irq_desc *desc,
> -                                     unsigned int priority)
> +static void gicv3_set_irq_type(struct irq_desc *desc)
>  {
>      uint32_t cfg, actual, edgebit;
>      void __iomem *base;
> @@ -512,6 +511,15 @@ static void gicv3_set_irq_properties(struct irq_desc *desc,
>              IRQ_TYPE_EDGE_RISING :
>              IRQ_TYPE_LEVEL_HIGH;
>      }
> +    spin_unlock(&gicv3.lock);
> +}
> +
> +static void gicv3_set_irq_priority(struct irq_desc *desc,
> +                                   unsigned int priority)
> +{
> +    unsigned int irq = desc->irq;
> +
> +    spin_lock(&gicv3.lock);
>  
>      /* Set priority */
>      if ( irq < NR_GIC_LOCAL_IRQS )
> @@ -1547,7 +1555,8 @@ static const struct gic_hw_operations gicv3_ops = {
>      .eoi_irq             = gicv3_eoi_irq,
>      .deactivate_irq      = gicv3_dir_irq,
>      .read_irq            = gicv3_read_irq,
> -    .set_irq_properties  = gicv3_set_irq_properties,
> +    .set_irq_type        = gicv3_set_irq_type,
> +    .set_irq_priority    = gicv3_set_irq_priority,
>      .send_SGI            = gicv3_send_sgi,
>      .disable_interface   = gicv3_disable_interface,
>      .update_lr           = gicv3_update_lr,
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index f25381f..0fd7e8c 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -96,14 +96,17 @@ void gic_restore_state(struct vcpu *v)
>      gic_restore_pending_irqs(v);
>  }
>  
> -/*
> - * - desc.lock must be held
> - * - arch.type must be valid (i.e != IRQ_TYPE_INVALID)
> - */
> -static void gic_set_irq_properties(struct irq_desc *desc,
> -                                   unsigned int priority)
> +static void gic_set_irq_type(struct irq_desc *desc)
> +{
> +    ASSERT(spin_is_locked(&desc->lock));
> +    ASSERT(desc->arch.type != IRQ_TYPE_INVALID);
> +
> +    gic_hw_ops->set_irq_type(desc);
> +}
> +
> +static void gic_set_irq_priority(struct irq_desc *desc, unsigned int priority)
>  {
> -    gic_hw_ops->set_irq_properties(desc, priority);
> +    gic_hw_ops->set_irq_priority(desc, priority);
>  }
>  
>  /* Program the GIC to route an interrupt to the host (i.e. Xen)
> @@ -121,7 +124,8 @@ void gic_route_irq_to_xen(struct irq_desc *desc, const cpumask_t *cpu_mask,
>  
>      desc->handler->set_affinity(desc, cpu_mask);
>  
> -    gic_set_irq_properties(desc, priority);
> +    gic_set_irq_type(desc);
> +    gic_set_irq_priority(desc, priority);
>  }
>  
>  /* Program the GIC to route an interrupt to a guest
> @@ -153,7 +157,8 @@ int gic_route_irq_to_guest(struct domain *d, unsigned int virq,
>      desc->handler = gic_hw_ops->gic_guest_irq_type;
>      set_bit(_IRQ_GUEST, &desc->status);
>  
> -    gic_set_irq_properties(desc, priority);
> +    gic_set_irq_type(desc);
> +    gic_set_irq_priority(desc, priority);
>  
>      p->desc = desc;
>      res = 0;
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index b931f98..ffba469 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -329,9 +329,10 @@ struct gic_hw_operations {
>      void (*deactivate_irq)(struct irq_desc *irqd);
>      /* Read IRQ id and Ack */
>      unsigned int (*read_irq)(void);
> -    /* Set IRQ property */
> -    void (*set_irq_properties)(struct irq_desc *desc,
> -                               unsigned int priority);
> +    /* Set IRQ type - type is taken from desc->arch.type */
> +    void (*set_irq_type)(struct irq_desc *desc);
> +    /* Set IRQ priority */
> +    void (*set_irq_priority)(struct irq_desc *desc, unsigned int priority);
>      /* Send SGI */
>      void (*send_SGI)(enum gic_sgi sgi, enum gic_sgi_mode irqmode,
>                       const cpumask_t *online_mask);
> -- 
> 1.9.1
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel