Re: [PATCH v4 05/17] xen/arm: ITS: implement hw_irq_controller for LPIs

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

On Fri, 2015-07-10 at 13:12 +0530, vijay.kilari@gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> 
> Implements hw_irq_controller api's required
> to handle LPI's
> 
> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> ---
> v4: - Implement separate hw_irq_controller for LPIs
>     - Drop setting LPI affinity
>     - virq and vid are moved under union
>     - Introduced inv command handling
>     - its_device is stored in irq_desc
> ---
>  xen/arch/arm/gic-v3-its.c         |  132 +++++++++++++++++++++++++++++++++++++
>  xen/arch/arm/gic-v3.c             |    5 +-
>  xen/arch/arm/gic.c                |   32 +++++++--
>  xen/arch/arm/irq.c                |   40 ++++++++++-
>  xen/include/asm-arm/gic-its.h     |    4 ++
>  xen/include/asm-arm/gic.h         |   13 ++++
>  xen/include/asm-arm/gic_v3_defs.h |    1 +
>  xen/include/asm-arm/irq.h         |    8 ++-
>  8 files changed, 227 insertions(+), 8 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
> index b421a6f..b98d396 100644
> --- a/xen/arch/arm/gic-v3-its.c
> +++ b/xen/arch/arm/gic-v3-its.c
> @@ -295,6 +295,19 @@ post:
>      its_wait_for_range_completion(its, cmd, next_cmd);
>  }
>  
> +static void its_send_inv(struct its_device *dev, struct its_collection *col,
> +                         u32 event_id)
> +{
> +    its_cmd_block cmd;
> +
> +    memset(&cmd, 0x0, sizeof(its_cmd_block));
> +    cmd.inv.cmd = GITS_CMD_INV;
> +    cmd.inv.devid = dev->device_id;
> +    cmd.inv.event = event_id;
> +
> +    its_send_single_command(dev->its, &cmd, col);
> +}

This ought to be in the prior patch doing such things I think.

Oh I see, you didn't have struct its_device defined back then. I think
you can just reorder patches #3 and #4 to solve that.

> +static void its_host_irq_end(struct irq_desc *desc)
> +{
> +    /* Lower the priority */
> +    gicv3_eoi_irq(desc);
> +    /* Deactivate */
> +    gicv3_dir_irq(desc);
> +}
> +
> +static void its_guest_irq_end(struct irq_desc *desc)
> +{
> +    gicv3_eoi_irq(desc);
> +}

Exposing those two gicv3 functions is a bit unfortunate, but I think it
will do for now.

Exposing gicv3_[host|guest]_irq_end might have been nicer, since you
could just insert them into your its_[host|guest]_lpi_type instead of
duplicating them.

Eventually we may want to refactor such that register_its_ops gives back
the ack/eoi hooks to use.

> +static void its_irq_set_affinity(struct irq_desc *desc, const cpumask_t *mask)
> +{

Please add

     /* Not yet supported */

> @@ -104,7 +126,8 @@ static void gic_set_irq_properties(struct irq_desc *desc,
>                                     const cpumask_t *cpu_mask,
>                                     unsigned int priority)
>  {
> -   gic_hw_ops->set_irq_properties(desc, cpu_mask, priority);
> +    if ( desc->irq < gic_number_lines() )

Should this be is_lpi as in other similar places?

> +        gic_hw_ops->set_irq_properties(desc, cpu_mask, priority);
>  }
>  
>  /* Program the GIC to route an interrupt to the host (i.e. Xen)
> @@ -114,11 +137,12 @@ 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));

As discussed in <1436284206.25646.258.camel@citrix.com> please make some
sort of is_valid_irq(irq) helper to encapsulate this logic.

> ocessor), priority);
> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> index 2dd43ee..ba8528a 100644
> --- a/xen/arch/arm/irq.c
> +++ b/xen/arch/arm/irq.c
> @@ -35,7 +35,13 @@ static DEFINE_SPINLOCK(local_irqs_type_lock);
>  struct irq_guest
>  {
>      struct domain *d;
> -    unsigned int virq;
> +    union
> +    {
> +        /* virq refer to virtual irq in case of spi */
> +        unsigned int virq;
> +        /* virq refer to event ID in case of lpi */

"refers" in both cases

And I'd say "to the ..." not just "to ..." and "in the case of..." too.

> +unsigned int irq_to_vid(struct irq_desc *desc)
> +{
> +    return irq_get_guest_info(desc)->vid;
> +}
> +
> +unsigned int irq_to_virq(struct irq_desc *desc)
> +{
> +    return irq_get_guest_info(desc)->virq;
> +}

Please assert that irq_desc->arch.its_device is (non-)NULL as
appropriate in these two cases.

BTW, while checking the field name I spotted "struct msi_desc
*msi_desc" in the main struct irq_desc.

Since MSIs are effectively the same as LPIs as a future cleanup I think
we should s/its_device/msi_desc/g and use this field instead of adding a
second redundant type and pointer to it. THis is not a blocker for 4.6
though.

> +struct its_device *get_irq_device(struct irq_desc *desc)
> +{
> +    ASSERT(spin_is_locked(&desc->lock));
> +
> +    return desc->arch.dev;
> +}
> +
> +void set_irq_device(struct irq_desc *desc, struct its_device *dev)
> +{
> +    ASSERT(spin_is_locked(&desc->lock));
> +    desc->arch.dev = dev;
> +}

Please add _its to the names of both of these functions, ie..g
set_irq_its_device.

> diff --git a/xen/include/asm-arm/gic-its.h b/xen/include/asm-arm/gic-its.h
> index b5e09bd..e8d244f 100644
> --- a/xen/include/asm-arm/gic-its.h
> +++ b/xen/include/asm-arm/gic-its.h
> @@ -161,6 +161,10 @@ typedef union {
>   * The ITS view of a device.
>   */
>  struct its_device {
> +    /* Physical ITS */
> +    struct its_node         *its;
> +    /* Number of Physical LPIs assigned */
> +    int                     nr_lpis;
>      /* Physical Device id */
>      u32                     device_id;
>      /* RB-tree entry */
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index e9d5f36..44c2317 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -20,6 +20,9 @@
>  
>  #define NR_GIC_LOCAL_IRQS  NR_LOCAL_IRQS
>  #define NR_GIC_SGI         16
> +#define FIRST_GIC_LPI      8192
> +#define NR_GIC_LPI         4096
> +#define MAX_LPI            (FIRST_GIC_LPI + NR_GIC_LPI)

MAX_LPI and NR_GIC_LPI should be obtained from the hardware at init time
and put somewhere, like a global nr_lpis perhaps, to be used throughout.

> diff --git a/xen/include/asm-arm/gic_v3_defs.h b/xen/include/asm-arm/gic_v3_defs.h
> index 051a95e..0443ae7 100644
> --- a/xen/include/asm-arm/gic_v3_defs.h
> +++ b/xen/include/asm-arm/gic_v3_defs.h
> @@ -169,6 +169,7 @@
>  #define ICH_SGI_IRQ_MASK             0xf
>  #define ICH_SGI_TARGETLIST_MASK      0xffff
>  #define LPI_PROP_GROUP1                 (1 << 1)
> +#define LPI_PROP_ENABLED                (1 << 0)

Please order (1 << X) by the X.

> diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h
> index 34b492b..55e219f 100644
> --- a/xen/include/asm-arm/irq.h
> +++ b/xen/include/asm-arm/irq.h
> @@ -17,6 +17,8 @@ struct arch_pirq
>  struct arch_irq_desc {
>      int eoi_cpu;
>      unsigned int type;
> +    struct its_device *dev;
> +    u16 col_id;

There are probably opportunities to make eoi_cpu or type smaller so as
to allow col_id to be added without increasing the size of the struct.
Not a blocker for 4.6 though.

Ian.