Re: [PATCH v5 16/22] xen/arm: ITS: Route LPIs

[Julien Grall <julien.grall@citrix.com>]

Hi Vijay,

On 27/07/15 12:11, vijay.kilari@gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> 
> Allocate and initialize irq descriptor for LPIs and
> route LPIs to guest

On v4, I've asked to see a commit message explaining how you route the
LPI and why you don't set GICH_LR.HW.

I don't see it know, so I expect to see it in v6...

In general your commit message should explain what you are doing in the
patch. It's helpful when you go back in the log to understand what was
done and for reviewers.

[..]

> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
> index 5ffd52f..0d8582a 100644
> --- a/xen/arch/arm/gic-v3-its.c
> +++ b/xen/arch/arm/gic-v3-its.c
> @@ -110,6 +110,11 @@ void dump_cmd(its_cmd_block *cmd)
>  void dump_cmd(its_cmd_block *cmd) { do {} while ( 0 ); }
>  #endif
>  
> +u32 its_get_nr_event_ids(void)
> +{
> +    return (1 << its_data.eventid_bits);
> +}
> +
>  /* RB-tree helpers for its_device */
>  struct its_device *its_find_device(u32 devid)
>  {
> @@ -415,6 +420,21 @@ static void its_flush_and_invalidate_prop(struct irq_desc *desc, u8 *cfg)
>      its_send_inv(its_dev, col, vid);
>  }
>  
> +void its_set_lpi_properties(struct irq_desc *desc,
> +                            const cpumask_t *cpu_mask,
> +                            unsigned int priority)
> +{
> +    unsigned long flags;
> +    u8 *cfg;
> +
> +    spin_lock_irqsave(&its_lock, flags);
> +    cfg = gic_rdists->prop_page + desc->irq - FIRST_GIC_LPI;
> +    *cfg = (*cfg & 3) | (priority & LPI_PRIORITY_MASK) ;
> +
> +    its_flush_and_invalidate_prop(desc, cfg);
> +    spin_unlock_irqrestore(&its_lock, flags);
> +}
> +
>  static void its_set_lpi_state(struct irq_desc *desc, int enable)
>  {
>      u8 *cfg;
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 58e878e..8c7c5cf 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -535,6 +535,16 @@ static void gicv3_set_irq_properties(struct irq_desc *desc,
>      spin_unlock(&gicv3.lock);
>  }
>  
> +static void gicv3_set_properties(struct irq_desc *desc,
> +                                 const cpumask_t *cpu_mask,
> +                                 unsigned int priority)
> +{
> +    if ( gic_is_lpi(desc->irq) )
> +        its_set_lpi_properties(desc, cpu_mask, priority);
> +    else
> +        gicv3_set_irq_properties(desc, cpu_mask, priority);

I'm sure I've said that on a previous version. The naming of the
function/variable/macro/define is not set. It was fitting our need and
may not after the LPIs series. The name should be updated rather kept
and making the code less readable to read.

In this case, your rename the callback to gicv3_set_properties. But
properties of what? You are setting the property of an IRQ (LPIs, SPIs,
PPIs,... are all IRQs).

IHMO, the old gicv3_set_irq_properties should have been renamed in
gicv3_set_line_properties.

> +}
> +
>  static void __init gicv3_dist_init(void)
>  {
>      uint32_t type;
> @@ -912,7 +922,7 @@ static void gicv3_update_lr(int lr, const struct pending_irq *p,
>      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 ( p->desc != NULL && !(gic_is_lpi(p->irq)) )

As said on v4, can you please explain why you need this? You never set
p->desc for LPI so it's not necessary.

>         val |= GICH_LR_HW | (((uint64_t)p->desc->irq & GICH_LR_PHYSICAL_MASK)
>                             << GICH_LR_PHYSICAL_SHIFT);
>  
> @@ -1312,7 +1322,10 @@ static int __init gicv3_init(void)
>      spin_lock(&gicv3.lock);
>  
>      if ( gicv3_dist_supports_lpis() )
> +    {
>          gicv3_info.lpi_supported = 1;
> +        gicv3_info.nr_event_ids = its_get_nr_event_ids();
> +    }
>      else
>          gicv3_info.lpi_supported = 0;
>  
> @@ -1336,7 +1349,7 @@ 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_properties  = gicv3_set_properties,
>      .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 092087d..f6be0e9 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -33,6 +33,7 @@
>  #include <asm/device.h>
>  #include <asm/io.h>
>  #include <asm/gic.h>
> +#include <asm/gic-its.h>

I don't avoid any include of gic-its.h in the common code. The common
code shouldn't contain any GIC hardware specific.

>  #include <asm/vgic.h>
>  
>  static void gic_restore_pending_irqs(struct vcpu *v);
> @@ -67,11 +68,23 @@ bool_t gic_is_lpi(unsigned int irq)
>      return gic_hw_ops->is_lpi(irq);
>  }
>  
> +/* Returns number of PPIs/SGIs/SPIs supported */
>  unsigned int gic_number_lines(void)
>  {
>      return gic_hw_ops->info->nr_lines;
>  }
>  
> +/* Validates PPIs/SGIs/SPIs/LPIs supported */
> +bool_t gic_is_valid_irq(unsigned int irq)
> +{
> +    return ((irq < gic_hw_ops->info->nr_lines) || gic_is_lpi(irq));
> +}
> +
> +unsigned int gic_nr_event_ids(void)
> +{
> +    return gic_hw_ops->info->nr_event_ids;
> +}
> +
>  bool_t gic_lpi_supported(void)
>  {
>      return gic_hw_ops->info->lpi_supported;
> @@ -134,7 +147,8 @@ 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(gic_is_valid_irq(desc->irq));
>      ASSERT(test_bit(_IRQ_DISABLED, &desc->status));
>      ASSERT(spin_is_locked(&desc->lock));
>  
> @@ -183,6 +197,24 @@ out:
>      return res;
>  }
>  
> +int gic_route_lpi_to_guest(struct domain *d, unsigned int virq,
> +                           struct irq_desc *desc, unsigned int priority)

I don't understand why you have a virq parameter. You don't have a vLPI
when you route the IRQ.

> +{
> +    ASSERT(spin_is_locked(&desc->lock));
> +    ASSERT(virq < gic_nr_event_ids());
> +
> +    desc->handler = get_guest_hw_irq_controller(desc->irq);
> +    set_bit(_IRQ_GUEST, &desc->status);
> +
> +    /* Set cpumask to current processor */
> +    gic_set_irq_properties(desc, cpumask_of(smp_processor_id()), priority);
> +
> +    /* Enable LPI by default? */

Please see the draft [1] and update the comment accordingly.

> +    desc->handler->enable(desc);
> +
> +    return 0;
> +}
> +
>  /* This function only works with SPIs for now */
>  int gic_remove_irq_from_guest(struct domain *d, unsigned int virq,
>                                struct irq_desc *desc)
> @@ -440,7 +472,7 @@ static void gic_update_one_lr(struct vcpu *v, int i)
>          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  || gic_is_lpi(irq) )

Same remark as gicv3_update_lr. I would add that the LPI as no pending
state so this change is technically invalid too. Am I wrong?

>              {
>                   lr_val.state |= GICH_LR_PENDING;
>                   gic_hw_ops->write_lr(i, &lr_val);
> @@ -663,7 +695,7 @@ void gic_interrupt(struct cpu_user_regs *regs, int is_fiq)
>          /* Reading IRQ will ACK it */
>          irq = gic_hw_ops->read_irq();
>  
> -        if ( likely(irq >= 16 && irq < 1020) )
> +        if ( (likely(irq >= 16 && irq < 1020)) || likely(gic_is_lpi(irq)) )

When I asked to put in likely, I meant

likely((irq >= 16 && irq < 1020) || gic_is_lpi(irq))

>          {
>              local_irq_enable();
>              do_IRQ(regs, irq, is_fiq);
> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> index ccbe088..ae301a4 100644
> --- a/xen/arch/arm/irq.c
> +++ b/xen/arch/arm/irq.c
> @@ -26,6 +26,7 @@
>  #include <xen/sched.h>
>  
>  #include <asm/gic.h>
> +#include <asm/gic-its.h>

Same remark as the include in gic.c

>  #include <asm/vgic.h>
>  
>  static unsigned int local_irqs_type[NR_LOCAL_IRQS];
> @@ -221,7 +222,7 @@ int request_irq(unsigned int irq, unsigned int irqflags,
>       * which interrupt is which (messes up the interrupt freeing
>       * logic etc).
>       */
> -    if ( irq >= nr_irqs )
> +    if ( !gic_is_valid_irq(irq) )
>          return -EINVAL;
>      if ( !handler )
>          return -EINVAL;
> @@ -279,11 +280,28 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int is_fiq)
>  
>          set_bit(_IRQ_INPROGRESS, &desc->status);
>  
> -        /*
> -         * The irq cannot be a PPI, we only support delivery of SPIs to
> -         * guests.
> -	 */
> -        vgic_vcpu_inject_spi(info->d, info->virq);
> +#ifdef HAS_GICV3
> +        if ( gic_is_lpi(irq) )
> +        {
> +            struct its_device *dev = get_irq_its_device(desc);
> +            unsigned int vdevid = dev->virt_device_id;
> +            unsigned int eventID = irq_to_vid(desc);
> +
> +            if ( info->d->domain_id != dev->domain_id)

Why this check? IHMO this could never happen if you implement correctly
the routing.

> +            {
> +                printk("LPI %"PRId32" not assigned to domain.. dropping \n",
> +                       irq);
> +                goto out_no_end;
> +            }
> +            vgic_vcpu_inject_lpi(info->d, vdevid, eventID);
> +        }
> +        else
> +#endif
> +            /*
> +             * The irq cannot be a PPI, we only support delivery of SPIs to
> +             * guests
> +             */
> +            vgic_vcpu_inject_spi(info->d, info->virq);
>          goto out_no_end;
>      }
>  
> @@ -449,10 +467,102 @@ err:
>  
>  bool_t is_assignable_irq(unsigned int irq)
>  {
> -    /* For now, we can only route SPIs to the guest */
> -    return ((irq >= NR_LOCAL_IRQS) && (irq < gic_number_lines()));
> +    /* For now, we can only route SPI/LPIs to the guest */
> +    return (((irq >= NR_LOCAL_IRQS) && (irq < gic_number_lines())) ||
> +              gic_is_lpi(irq));
>  }
>  
> +#ifdef HAS_GICV3
> +/*
> + * Route an LPI to a specific guest.
> + */
> +int route_lpi_to_guest(struct domain *d, unsigned int vid,

What does vid stand for? virtual eventID? If so, shouldn't it the same
as the physical eventID?

Overall, route_lpi_to_guest should only take an LPI in parameter.

> +                       unsigned int irq, const char *devname)
> +{
> +    struct irqaction *action;
> +    struct irq_guest *info;
> +    struct irq_desc *desc;
> +    unsigned long flags;
> +    int retval = 0;
> +
> +    if ( !gic_is_lpi(irq) )
> +    {
> +        printk(XENLOG_G_ERR "Only LPI can be routed \n");
> +        return -EINVAL;
> +    }
> +
> +    action = xmalloc(struct irqaction);
> +    if ( !action )
> +        return -ENOMEM;
> +
> +    info = xmalloc(struct irq_guest);
> +    if ( !info )
> +    {
> +        xfree(action);
> +        return -ENOMEM;
> +    }
> +    info->d = d;
> +
> +    action->dev_id = info;
> +    action->name = devname;
> +    action->free_on_release = 1;
> +
> +    desc = irq_to_desc(irq);
> +    spin_lock_irqsave(&desc->lock, flags);
> +
> +    ASSERT(desc->msi_desc != NULL);
> +    desc->msi_desc->eventID = vid;

Why?

> +    if ( desc->arch.type == DT_IRQ_TYPE_INVALID )
> +    {
> +        printk(XENLOG_G_ERR "LPI %u has not been configured\n", irq);
> +        retval = -EIO;
> +        goto out;
> +    }
> +
> +    /*
> +     * If the IRQ is already used by by same domain

I suspect that you forgot to finish this comment...

> +     */
> +    if ( desc->action != NULL )
> +    {
> +        struct domain *ad = irq_get_domain(desc);
> +
> +        if ( test_bit(_IRQ_GUEST, &desc->status) && d == ad )
> +        {
> +            printk(XENLOG_G_ERR
> +                   "d%u: vID %u is already assigned to domain %u\n",
> +                   d->domain_id, irq, d->domain_id);
> +            retval = -EBUSY;
> +            goto out;
> +        }
> +    }
> +
> +    retval = __setup_irq(desc, 0, action);
> +    if ( retval )
> +        goto out;
> +
> +    retval = gic_route_lpi_to_guest(d, vid, desc, GIC_PRI_IRQ);
> +
> +    spin_unlock_irqrestore(&desc->lock, flags);
> +
> +    if ( retval )
> +    {
> +        /* TODO: for LPI */

So if we merge this series and it's failing, you will just screw
knowingly Xen... You should just make sure that gic_route_lpi_to_guest
never return an error or support release_irq.

>  /*
>   * Route an IRQ to a specific guest.
>   * For now only SPIs are assignable to the guest.

[..]

> diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
> index b8f32ed..5323192 100644
> --- a/xen/arch/arm/vgic-v3-its.c
> +++ b/xen/arch/arm/vgic-v3-its.c
> @@ -75,6 +75,11 @@ bool_t is_domain_lpi(struct domain *d, unsigned int lpi)
>              (lpi < (d->arch.vgic.nr_lpis + FIRST_GIC_LPI)));
>  }
>  
> +bool_t is_valid_collection(struct domain *d, uint32_t col)
> +{
> +    return (col <= (d->max_vcpus + 1));
> +}
> +
>  static inline uint32_t vits_get_max_collections(struct domain *d)
>  {
>      /* Collection ID is only 16 bit */
> @@ -371,7 +376,7 @@ static int vits_process_int(struct vcpu *v, struct vgic_its *vits,
>      DPRINTK("%pv: vITS: INT: Device 0x%"PRIx32" id %"PRId32"\n",
>              v, dev_id, event);
>  
> -    /* TODO: Inject LPI */
> +    vgic_vcpu_inject_lpi(v->domain, dev_id, event);
>  
>      return 0;
>  }
> @@ -586,6 +591,21 @@ static inline uint32_t vits_get_word(uint32_t reg_offset, uint64_t val)
>          return (u32)(val >> 32);
>  }
>  
> +uint8_t vits_get_priority(struct vcpu *v, uint32_t vlpi)
> +{
> +    struct vgic_its *vits = v->domain->arch.vgic.vits;
> +    uint8_t priority;
> +
> +    ASSERT(vlpi < vits->prop_size);
> +
> +    spin_lock(&vits->prop_lock);

Please see my comment on #12 about the lock. I won't repeat here.

> +    priority = *((u8*)vits->prop_page + vlpi);

Looks like I forgot to mention it on v4. You should substract 8192 to
the vLPI in order to the offset in the property table.

> +    priority &= LPI_PRIORITY_MASK;

You need to explain why you just only mask and not shift.

> +    spin_unlock(&vits->prop_lock);
> +
> +    return priority;
> +}
> +
>  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-v3.c b/xen/arch/arm/vgic-v3.c
> index a466a8f..9e6e3ff 100644
> --- a/xen/arch/arm/vgic-v3.c
> +++ b/xen/arch/arm/vgic-v3.c
> @@ -1140,12 +1140,21 @@ static const struct mmio_handler_ops vgic_distr_mmio_handler = {
>  
>  static int vgic_v3_get_irq_priority(struct vcpu *v, unsigned int irq)
>  {
> -    int priority;
> -    struct vgic_irq_rank *rank = vgic_rank_irq(v, irq);
> +    int priority = 0;
> +    struct vgic_irq_rank *rank;
> +    unsigned long flags;
>  
> -    ASSERT(spin_is_locked(&rank->lock));
> -    priority = vgic_byte_read(rank->ipriority[REG_RANK_INDEX(8,
> +    if ( !gic_is_lpi(irq) )
> +    {
> +        rank = vgic_rank_irq(v, irq);
> +
> +        vgic_lock_rank(v, rank, flags);
> +        priority = vgic_byte_read(rank->ipriority[REG_RANK_INDEX(8,
>                                                irq, DABT_WORD)], 0, irq & 0x3);
> +        vgic_unlock_rank(v, rank, flags);
> +    }
> +    else if ( gic_lpi_supported() && is_domain_lpi(v->domain, irq) )

The gic_lpi_supported is pointless here. If the LPI is not supported,
the guest would have the nr_lpis fields equal to 0.

> +        priority = vits_get_priority(v, irq);
>  
>      return priority;
>  }
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index ab5e81b..57c0f52 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -33,6 +33,12 @@
>  #include <asm/gic-its.h>
>  #include <asm/vgic.h>
>  
> +static inline bool_t is_domains_lpi(struct domain *d, unsigned int irq)
> +{
> +    return ((irq > FIRST_GIC_LPI) &&
> +            (irq < (d->arch.vgic.nr_lpis + FIRST_GIC_LPI)));
> +}
> +

This not used.

>  static inline struct vgic_irq_rank *vgic_get_rank(struct vcpu *v, int rank)
>  {
>      if ( rank == 0 )
> @@ -414,14 +420,11 @@ void vgic_clear_pending_irqs(struct vcpu *v)
>  void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int virq)
>  {
>      uint8_t priority;
> -    struct vgic_irq_rank *rank = vgic_rank_irq(v, virq);
>      struct pending_irq *iter, *n = irq_to_pending(v, virq);
>      unsigned long flags;
>      bool_t running;
>  
> -    vgic_lock_rank(v, rank, flags);
>      priority = v->domain->arch.vgic.handler->get_irq_priority(v, virq);
> -    vgic_unlock_rank(v, rank, flags);
>  
>      spin_lock_irqsave(&v->arch.vgic.lock, flags);
>  
> @@ -478,6 +481,69 @@ void vgic_vcpu_inject_spi(struct domain *d, unsigned int virq)
>      vgic_vcpu_inject_irq(v, virq);
>  }
>  
> +#ifdef HAS_GICV3
> +void vgic_vcpu_inject_lpi(struct domain *d, unsigned int vdevid,
> +                          unsigned int eventID)

You will have to address the problem mentioned in [2] at some point
before it's merged. This is a real one, and as you said in a previous
mail the interrupt will never came back.

> +{
> +    struct vdevice_table dt_entry;
> +    struct vitt vitt_entry;
> +    uint32_t col_id, target;
> +
> +    if ( eventID > gic_nr_event_ids() )
> +    {
> +        dprintk(XENLOG_WARNING,
> +                "Invalid eventID %"PRId32" ..dropping\n", vdevid);
> +        return;
> +    }

This is not possible. Please drop this check.

> +
> +    if ( vits_get_vdevice_entry(d, vdevid, &dt_entry) )
> +    {
> +        dprintk(XENLOG_WARNING,
> +                "Failed to read dt entry for dev 0x%"PRIx32" ..dropping\n",
> +                vdevid);
> +        return;
> +    }
> +
> +    if ( dt_entry.vitt_ipa == INVALID_PADDR )
> +    {
> +        dprintk(XENLOG_WARNING,
> +                "Event %"PRId32" of dev 0x%"PRIx32" is invalid..dropping\n",
> +                eventID, vdevid);
> +        return;
> +    }
> +
> +    if ( vits_get_vitt_entry(d, vdevid, eventID, &vitt_entry) )
> +    {
> +        dprintk(XENLOG_WARNING,
> +                "Event %"PRId32" of dev 0x%"PRIx32" is invalid..dropping\n",
> +                eventID, vdevid);
> +        return;
> +    }
> +
> +    col_id = vitt_entry.vcollection;
> +
> +    if ( !vitt_entry.valid || !is_valid_collection(d, col_id) ||
> +         !is_domain_lpi(d, vitt_entry.vlpi) )
> +    {
> +        dprintk(XENLOG_WARNING,
> +                "vlpi %"PRId32" for dev 0x%"PRIx32" is not valid..dropping\n",
> +                vitt_entry.vlpi, vdevid);
> +        return;
> +    }
> +
> +    target = d->arch.vgic.vits->collections[col_id].target_address;
> +    if ( target > d->max_vcpus )
> +    {
> +        dprintk(XENLOG_WARNING,
> +                "vlpi %"PRId32" got invalid target %"PRId32" dev 0x%"PRIx32"\n",
> +                vitt_entry.vlpi, target, vdevid);
> +        return;
> +    }
> +
> +    vgic_vcpu_inject_irq(d->vcpu[target], vitt_entry.vlpi);
> +}
> +#endif
> +
>  void arch_evtchn_inject(struct vcpu *v)
>  {
>      vgic_vcpu_inject_irq(v, v->domain->arch.evtchn_irq);

Regards,

[1]
http://xenbits.xen.org/people/ianc/vits/draftF.html#handling-of-unroutedspurious-lpis
[2]
http://lists.xenproject.org/archives/html/xen-devel/2015-06/msg02591.html

-- 
Julien Grall