Re: [PATCH v4 12/17] xen/arm: ITS: Initialize LPI irq descriptors and route

[Ian Campbell <ian.campbell@citrix.com>]

On Fri, 2015-07-10 at 13:12 +0530, vijay.kilari@gmail.com wrote:
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 3ebadcf..92d2be9 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -68,11 +68,18 @@ enum gic_version gic_hw_version(void)
>     return gic_hw_ops->info->hw_version;
>  }
>  
> +/* Only validates 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) && is_lpi(irq));
> +}
> +
>  unsigned int gic_nr_id_bits(void)
>  {
>      return gic_hw_ops->info->nr_id_bits;
> @@ -148,7 +155,7 @@ void gic_route_irq_to_xen(struct irq_desc *desc, const cpumask_t *cpu_mask,
>  {
>      ASSERT(priority <= 0xff);     /* Only 8 bits of priority */
>      /* Can't route interrupts that don't exist */
> -    ASSERT(desc->irq < gic_number_lines() || is_lpi(desc->irq));
> +    ASSERT(gic_is_valid_irq(desc->irq));


The fact that I commented on this earlier is another artefact in the way
this series presents functionality in an essentially arbitrary order.

I notice that you appear to have reintroduced the logic but when you
moved this code from here into gic_is_valid_irq. AFAICT that function
can never return true, but then I'm rather confused about how this
series can have been tested.

>      ASSERT(test_bit(_IRQ_DISABLED, &desc->status));
>      ASSERT(spin_is_locked(&desc->lock));
>  
> @@ -160,6 +167,17 @@ void gic_route_irq_to_xen(struct irq_desc *desc, const cpumask_t *cpu_mask,
>  /* Program the GIC to route an interrupt to a guest
>   *   - desc.lock must be held
>   */
> +int gic_route_lpi_to_guest(struct domain *d, unsigned int virq,
> +                           struct irq_desc *desc, unsigned int priority)
> +{
> +    ASSERT(spin_is_locked(&desc->lock));

ASSERT(virq >= SOME_DEFINE_FOR_MIN_LPI);

> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> index 3806d98..c8ea627 100644
> --- a/xen/arch/arm/irq.c
> +++ b/xen/arch/arm/irq.c
> @@ -62,12 +62,21 @@ hw_irq_controller no_irq_type = {
>  };
>  
>  static irq_desc_t irq_desc[NR_IRQS];
> +#ifdef CONFIG_ARM_64
> +static irq_desc_t irq_desc_lpi[NR_GIC_LPI];

http://xenbits.xen.org/people/ianc/vits/draftG.html#irq-descriptors
contains: "Therefore a second dynamically allocated array will be added
to cover the range 8192..nr_lpis"

IOW this should be dynamically allocated and therefore NR_GIC_LPI (which
I think I already commented on earlier) should go away, since the limit
should come from the h/w.
> @@ -267,9 +285,14 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int is_fiq)
>          set_bit(_IRQ_INPROGRESS, &desc->status);
>          desc->arch.eoi_cpu = smp_processor_id();
>  
> +#ifdef CONFIG_ARM_64
> +        if ( is_lpi(irq) )
> +            vgic_vcpu_inject_lpi(info->d, irq);
> +        else
> +#endif
>          /* the irq cannot be a PPI, we only support delivery of SPIs to
>           * guests */
> -        vgic_vcpu_inject_spi(info->d, info->virq);
> +            vgic_vcpu_inject_spi(info->d, info->virq);

Comment should be reindented too.

> +    if ( !is_lpi(irq) )
> +    {
> +        rank = vgic_rank_irq(v, irq);
> +
> +        ASSERT(spin_is_locked(&rank->lock));
> +        priority = vgic_byte_read(rank->ipriority[REG_RANK_INDEX(8,
>                                                irq, DABT_WORD)], 0, irq & 0x3);
> +    }
> +    if ( is_lpi(irq) && gic_lpi_supported() )

Should be "else if ( gic_lpi_supported() )"

> +    vdev = vits_find_device(&d->arch.vits->dev_root, devid);
> +    if ( !vdev )
> +    {
> +        dprintk(XENLOG_WARNING,
> +                "LPI %d received for dev 0x%x not assigned..dropping\n",
> +                irq, devid);
> +        return;
> +    }

This isn't used again and the whole R-B tree should go away.

> +    p = irq_to_pending(d->vcpu[0], vitt_entry.vlpi);

Perhaps given that we know this is a vlpi, we could skipping going via
vcpu[0] and just go right to the entry in the d->pending_lpi array (via
a suitable accessor of course)

> +    p->desc = desc;

This should have happened during routing, not now.

Oh. I see what is going on, your vgic_vcpu_inject_lpi appears to be
taking an IRQ, and not a vpli, or at least to be confused about what it
should do with the number which it calls irq.

Therefore you find yourself needing to lookup the irqdesc, and look in
the vitt etc, none of which belongs here.

For interrupts coming from a plpi all of that lookup should happen based
on the its_device which is contained in the irq_guest and other info
from there, before calling this function. You can get the irq_guest
which you can get because you have a PLPI in your hand and can look up
the irq_guest via the irq_desc which you get using the plpi number.

For interrupts coming from the VITS INT command the command itself
contains a vDevice and vEvent, which can then be looked up in the ITT to
get a vpli which can be passed to vgic_vcpu_inject_lpi.

Thus this function has no need to lookup an irq_desc, nor a dev id, nor
to do an rb tree lookup.

I think both of these cases are adequately explained, with pseudocode,
in the draftG vits design doc. Please ask if you are unsure.

Ian.