Re: [RFC PATCH v3 13/18] xen/arm: ITS: Add irq descriptors for LPIs

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

On Mon, 2015-06-22 at 17:31 +0530, vijay.kilari@gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> 
> Add irq descriptors for LPIs and route

This seems to also do interrupt injection for LPIs and more. Please
check that your commit messages are accurately describing the contents
of the patch. If it turns into a long list of unrelated sounding things
then that might suggest the patch should be split up.

> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> ---
>  xen/arch/arm/gic-v3.c         |    8 +++-
>  xen/arch/arm/gic.c            |   17 +++++++-
>  xen/arch/arm/irq.c            |   38 +++++++++++++----
>  xen/arch/arm/vgic-v3-its.c    |    9 +++++
>  xen/arch/arm/vgic.c           |   90 ++++++++++++++++++++++++++++++++++++++---
>  xen/include/asm-arm/domain.h  |    2 +
>  xen/include/asm-arm/gic-its.h |    6 +++
>  xen/include/asm-arm/gic.h     |    3 ++
>  xen/include/asm-arm/vgic.h    |    1 +
>  9 files changed, 157 insertions(+), 17 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 737646c..793f2f0 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -899,9 +899,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->irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT;
> +    else
> +        val |= ((uint64_t)p->irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT;

Is there supposed to be something different between these two cases? (Or
am I missing it?)

> +
> +   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 cfc9c42..091f7e5 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -124,18 +124,31 @@ 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));

||, surely? Otherwise doesn't this hit for every possible irq?

>      ASSERT(test_bit(_IRQ_DISABLED, &desc->status));
>      ASSERT(spin_is_locked(&desc->lock));
>  
>      desc->handler = gic_hw_ops->gic_host_irq_type;
>  
> -    gic_set_irq_properties(desc, cpu_mask, priority);
> +    if ( !is_lpi(desc->irq) )
> +        gic_set_irq_properties(desc, cpu_mask, priority);

I think any lack of support for affinity or priority should be pushed
down into the lower level drivers, rather than encoding that lack of
current support in the pits driver into the generic code.

>  int gic_route_irq_to_guest(struct domain *d, unsigned int virq,
>                             struct irq_desc *desc, unsigned int priority)
>  {
> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> index 9dbdf7d..105ef85 100644
> --- a/xen/arch/arm/irq.c
> +++ b/xen/arch/arm/irq.c
> @@ -57,12 +57,22 @@ hw_irq_controller no_irq_type = {
>  };
>  
>  static irq_desc_t irq_desc[NR_IRQS];
> +static irq_desc_t irq_desc_lpi[MAX_NR_LPIS];
>  static DEFINE_PER_CPU(irq_desc_t[NR_LOCAL_IRQS], local_irq_desc);
>  
>  irq_desc_t *__irq_to_desc(int irq)
>  {
> +    struct irq_desc *desc = NULL;
>      if (irq < NR_LOCAL_IRQS) return &this_cpu(local_irq_desc)[irq];
> -    return &irq_desc[irq-NR_LOCAL_IRQS];
> +    else if ( irq >= NR_LOCAL_IRQS && irq < NR_IRQS)
> +        return &irq_desc[irq-NR_LOCAL_IRQS];
> +    else
> +    {
> +        if ( is_lpi(irq) )
> +            return &irq_desc_lpi[irq - NR_GIC_LPI];
> +    }
> +
> +    return desc;

I'd write this as just a chain of "if (...) return", no need for else or
{}s.

> +void vgic_vcpu_inject_lpi(struct domain *d, unsigned int irq)
> +{
> +    struct irq_desc *desc;
> +    struct pending_irq *p;
> +    struct its_device *dev;
> +    struct vitt vitt_entry;
> +    struct vdevice_table dt_entry;
> +    uint32_t devid, col_id;
> +    int event;
> +
> +    desc = irq_to_desc(irq);
> +    event =  irq_to_virq(desc);
> +
> +    dev = get_irq_device(desc);
> +    devid = dev->device_id;
> +    event = irq - dev->lpi_base;
> +    if ( event < 0  && event > dev->nr_lpis)
> +    {
> +        dprintk(XENLOG_WARNING, 
> +               "LPI %d received for dev 0x%x is not valid..dropping \n",
> +               irq, devid);
> +        return;
> +    }
> +
> +    /* validity of device is checheck on vitt entry request */    

Typo

Also trailing whitespace, please clean that up everywhere.

> +    vits_get_vdevice_entry(d, devid, &dt_entry);
> +    if ( dt_entry.vitt_ipa != INVALID_PADDR )
> +    {
> +        dprintk(XENLOG_WARNING,
> +                "LPI %d received for dev 0x%x which is disabled..dropping \n",
> +                irq, devid);

This drops if the entry is _not_ invalid, is that really right?


> +        return;
> +    }
> +
> +    if ( vits_get_vitt_entry(d, devid, event, &vitt_entry) )
> +    {
> +        dprintk(XENLOG_WARNING,
> +                "LPI %d received for dev 0x%x which is disabled..dropping \n",
> +                irq, devid);
> +        return;
> +    }
> +    if ( !vitt_entry.valid )
> +    {
> +        dprintk(XENLOG_WARNING,
> +                "LPI %d received for dev 0x%x which is not valid..dropping \n",
> +                irq, devid);
> +        return;
> +    }
> +    p = irq_to_pending(d->vcpu[0], vitt_entry.vlpi);

Need to bounds check vitt_entry.vlpi somewhere, since the guest might
have messed with it.

> +    col_id = vitt_entry.vcollection;
> +
> +    ASSERT(col_id < d->max_vcpus);

Likewise a guest which writes vitt_entry.vcollection can now crash the
host.

Very great care needs to be taken with both dt_entry and vitt_entry and
all of their fields.

> +int vits_get_vitt_entry(struct domain *d, uint32_t devid,
> +                        uint32_t event, struct vitt *entry);
> +int vits_get_vdevice_entry(struct domain *d, uint32_t devid,
> +                           struct vdevice_table *entry);

Please add all prototypes in the same patch as the implementation of the
function, not the addition of the first caller.

Ian.