Re: [Xen-devel] [RFC PATCH 17/19] xen/arm: its: Support ITS interrupt handling

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Mon, 2 Mar 2015, vijay.kilari@xxxxxxxxx wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxxxxxxxxxx>
> 
> Add support for handling ITS(LPI) interrupts.
> The LPI interrupts are handled by physical ITS
> driver.
> 
> nested LPI interrupt handling is not tested and
> enabled.
> 
> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxxxxxxxxxx>
> ---
>  xen/arch/arm/gic-v3-its.c  |   31 +++++++++++++++++++++++++++++++
>  xen/arch/arm/gic-v3.c      |    8 ++++++--
>  xen/arch/arm/gic.c         |   38 ++++++++++++++++++++++++++++++++++++--
>  xen/arch/arm/irq.c         |   10 +++++++---
>  xen/arch/arm/vgic-v3-its.c |   10 ++++++++++
>  xen/arch/arm/vgic.c        |   14 ++++++++++----
>  xen/include/asm-arm/gic.h  |    3 +++
>  xen/include/asm-arm/irq.h  |    1 +
>  8 files changed, 104 insertions(+), 11 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
> index b2c3320..7adbee4 100644
> --- a/xen/arch/arm/gic-v3-its.c
> +++ b/xen/arch/arm/gic-v3-its.c
> @@ -344,6 +344,37 @@ static const struct gic_its_hw_operations gic_its_ops = {
>      .gic_guest_lpi_type  = &gic_guest_its_type,
>  };
>  
> +void its_handle_lpi(uint32_t irqnr, struct cpu_user_regs *regs)
> +{
> +    struct domain *d;
> +    struct irq_desc *desc = irq_to_desc(irqnr);
> +
> +    irq_enter();
> +    spin_lock(&desc->lock);
> +
> +    if ( !desc->action )
> +    {
> +        printk("UNKNOWN LPI without handler\n");
> +        goto err;
> +    }
> +
> +    if ( desc->status & IRQ_GUEST )
> +    {
> +        d = irq_get_domain(desc);
> +
> +        desc->handler->end(desc);
> +
> +        desc->status |= IRQ_INPROGRESS;
> +        desc->arch.eoi_cpu = smp_processor_id();
> +
> +        /* XXX: inject irq into all guest vcpus */
> +        vgic_vcpu_inject_irq(d->vcpu[0], irqnr);
> +    }

Does it really need a separate handler? It seems to me that LPIs could
just be handled from do_IRQ like the rest.

Also the comment is wrong in this case.


> +err:
> +    spin_unlock(&desc->lock);
> +    irq_exit();
> +}
> +
>  static u64 its_cmd_ptr_to_offset(struct its_node *its,
>                                   struct its_cmd_block *ptr)
>  {
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 02e71dd..b654535 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -848,9 +848,13 @@ static void gicv3_update_lr(int lr, const struct 
> pending_irq *p,
>  
>      val =  (((uint64_t)state & 0x3) << GICH_LR_STATE_SHIFT) | grp;
>      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 ( is_lpi(p->irq) )
> +        val |= ((uint64_t)p->desc->virq & GICH_LR_VIRTUAL_MASK) << 
> GICH_LR_VIRTUAL_SHIFT;
> +    else
> +        val |= ((uint64_t)p->irq & GICH_LR_VIRTUAL_MASK) << 
> GICH_LR_VIRTUAL_SHIFT;

desc->virq should contain the right value for all interrupts, not just
lpis, so you should be able to do:

    val |= ((uint64_t)p->desc-virq & GICH_LR_VIRTUAL_MASK) << 
GICH_LR_VIRTUAL_SHIFT;

in all cases.


> +   if ( p->desc != NULL && !(is_lpi(p->irq)) )
>         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 fb77387..c4d352a 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -34,6 +34,7 @@
>  #include <asm/io.h>
>  #include <asm/gic.h>
>  #include <asm/vgic.h>
> +#include <asm/gic-its.h>
>  
>  static void gic_restore_pending_irqs(struct vcpu *v);
>  
> @@ -134,6 +135,21 @@ void gic_route_irq_to_xen(struct irq_desc *desc, const 
> cpumask_t *cpu_mask,
>      gic_set_irq_properties(desc, cpu_mask, priority);
>  }
>  
> +void gic_route_lpi_to_guest(struct domain *d, struct irq_desc *desc,
> +                            const cpumask_t *cpu_mask, unsigned int priority)
> +{
> +    struct pending_irq *p;
> +    ASSERT(spin_is_locked(&desc->lock));
> +
> +    desc->handler = gic_its_hw_ops->gic_guest_lpi_type;
> +    set_bit(_IRQ_GUEST, &desc->status);
> +
> +     
> +    /* TODO: do not assume delivery to vcpu0 */
> +    p = irq_to_pending(d->vcpu[0], desc->irq);
> +    p->desc = desc;
> +}
> +
>  /* Program the GIC to route an interrupt to a guest
>   *   - desc.lock must be held
>   */
> @@ -341,20 +357,33 @@ static void gic_update_one_lr(struct vcpu *v, int i)
>      struct pending_irq *p;
>      int irq;
>      struct gic_lr lr_val;
> +    uint32_t pirq;
>  
>      ASSERT(spin_is_locked(&v->arch.vgic.lock));
>      ASSERT(!local_irq_is_enabled());
>  
>      gic_hw_ops->read_lr(i, &lr_val);
>      irq = lr_val.virq;
> -    p = irq_to_pending(v, irq);
> +
> +    if ( is_lpi(irq) )
> +    {
> +        // Fetch corresponding plpi for vlpi
> +        if ( vgic_its_get_pid(v, irq, &pirq) )
> +            BUG();
> +        p = irq_to_pending(v, pirq);
> +        irq = pirq;
> +    }
> +    else
> +    {
> +        p = irq_to_pending(v, irq);

Shouldn't p->desc->irq return the pirq for LPIs too? If it doesn't, it
should :-)

I would like to see a more generic handling of virq != physical irq.
This is not specific to LPIs but to any scenario where the physical irq
differs from the virtual irq.


> +    }
>      if ( lr_val.state & GICH_LR_ACTIVE )
>      {
>          set_bit(GIC_IRQ_GUEST_ACTIVE, &p->status);
>          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) )

I thought that LPIs cannot be ACTIVE. If so, it is probably a mistake to
set an LPI GICH_LR_ACTIVE in the LR register.


>              {
>                   lr_val.state |= GICH_LR_PENDING;
>                   gic_hw_ops->write_lr(i, &lr_val);
> @@ -580,6 +609,11 @@ void gic_interrupt(struct cpu_user_regs *regs, int 
> is_fiq)
>      do  {
>          /* Reading IRQ will ACK it */
>          irq = gic_hw_ops->read_irq();
> +        if ( is_lpi(irq) ) {
> +            // TODO: Enable irqs?
> +            its_handle_lpi(irq, regs);
> +            continue;
> +        }
>  
>          if ( likely(irq >= 16 && irq < 1021) )
>          {
> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> index 13583b4..52371be 100644
> --- a/xen/arch/arm/irq.c
> +++ b/xen/arch/arm/irq.c
> @@ -276,7 +276,7 @@ void __cpuinit init_secondary_IRQ(void)
>      BUG_ON(init_local_irq_data() < 0);
>  }
>  
> -static inline struct domain *irq_get_domain(struct irq_desc *desc)
> +struct domain *irq_get_domain(struct irq_desc *desc)
>  {
>      ASSERT(spin_is_locked(&desc->lock));
>  
> @@ -603,9 +603,13 @@ int route_irq_to_guest(struct domain *d, unsigned int 
> irq,
>      retval = __setup_irq(desc, 0, action);
>      if ( retval )
>          goto out;
> +    if ( irq >= NR_LOCAL_IRQS && irq < NR_IRQS)
> +        gic_route_irq_to_guest(d, desc, cpumask_of(smp_processor_id()),
> +                               GIC_PRI_IRQ);
> +    else
> +        gic_route_lpi_to_guest(d, desc, cpumask_of(smp_processor_id()),
> +                               GIC_PRI_IRQ);
>  
> -    gic_route_irq_to_guest(d, desc, cpumask_of(smp_processor_id()),
> -                           GIC_PRI_IRQ);
>      spin_unlock_irqrestore(&desc->lock, flags);
>      return 0;
>  
> diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
> index e7e587e..51b9614 100644
> --- a/xen/arch/arm/vgic-v3-its.c
> +++ b/xen/arch/arm/vgic-v3-its.c
> @@ -1022,6 +1022,16 @@ int vgic_its_get_pid(struct vcpu *v, uint32_t vid, 
> uint32_t *pid)
>      return 1;
>  }
>  
> +uint8_t vgic_its_get_priority(struct vcpu *v, uint32_t pid)
> +{
> +    uint8_t priority;
> +  
> +    priority =  readb_relaxed(v->domain->arch.vits->lpi_prop_page + pid);
> +    priority &= 0xfc;
> +
> +    return priority;
> +}

So it looks like we do handle priorities for LPIs, good! This doesn't
look like ITS specific, so we should move it vgic-v3.c


>  static int vgic_v3_gits_lpi_mmio_read(struct vcpu *v, mmio_info_t *info)
>  {
>      uint32_t offset;
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 3bf9ef3..5571856 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -389,14 +389,20 @@ void vgic_clear_pending_irqs(struct vcpu *v)
>  void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq)
>  {
>      uint8_t priority;
> -    struct vgic_irq_rank *rank = vgic_rank_irq(v, irq);
> +    struct vgic_irq_rank *rank;
>      struct pending_irq *iter, *n = irq_to_pending(v, irq);
>      unsigned long flags;
>      bool_t running;
>  
> -    vgic_lock_rank(v, rank, flags);
> -    priority = v->domain->arch.vgic.handler->get_irq_priority(v, irq);
> -    vgic_unlock_rank(v, rank, flags);
> +    if ( irq < NR_GIC_LPI )
> +    {
> +        rank = vgic_rank_irq(v, irq);
> +        vgic_lock_rank(v, rank, flags);
> +        priority = v->domain->arch.vgic.handler->get_irq_priority(v, irq);
> +        vgic_unlock_rank(v, rank, flags);
> +    }
> +    else
> +        priority = vgic_its_get_priority(v, irq);

I think that the handler should be set correctly for LPIs so that

priority = v->domain->arch.vgic.handler->get_irq_priority(v, irq);

works for them too without changes.


>      spin_lock_irqsave(&v->arch.vgic.lock, flags);
>  
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index 6f2237f..075f488 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -220,6 +220,7 @@ enum gic_version {
>  
>  extern enum gic_version gic_hw_version(void);
>  extern void gic_eoi_irq(struct irq_desc *desc);
> +extern void its_handle_lpi(uint32_t irqnr, struct cpu_user_regs *regs);
>  /* Program the GIC to route an interrupt */
>  extern void gic_route_irq_to_xen(struct irq_desc *desc, const cpumask_t 
> *cpu_mask,
>                                   unsigned int priority);
> @@ -356,6 +357,8 @@ void register_gic_ops(const struct gic_hw_operations 
> *ops);
>  void register_gic_its_ops(const struct gic_its_hw_operations *ops);
>  int gic_make_node(const struct domain *d,const struct dt_device_node *node,
>                    void *fdt);
> +void gic_route_lpi_to_guest(struct domain *d, struct irq_desc *desc,
> +                            const cpumask_t *cpu_mask, unsigned int 
> priority);
>  
>  #endif /* __ASSEMBLY__ */
>  #endif
> diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h
> index 1159a6d..19246f9 100644
> --- a/xen/include/asm-arm/irq.h
> +++ b/xen/include/asm-arm/irq.h
> @@ -51,6 +51,7 @@ int irq_set_spi_type(unsigned int spi, unsigned int type);
>  int irq_set_desc_data(unsigned int irq, void *d);
>  void *irq_get_desc_data(struct irq_desc *d);
>  int platform_get_irq(const struct dt_device_node *device, int index);
> +struct domain *irq_get_domain(struct irq_desc *desc);
>  
>  void irq_set_affinity(struct irq_desc *desc, const cpumask_t *cpu_mask);
>  
> -- 
> 1.7.9.5
> 

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