Re: [PATCH v5 15/22] xen/arm: ITS: implement hw_irq_controller for 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>
> 
> Implements hw_irq_controller api's required
> to handle LPI's

You need to explain in your commit message why you switched to callback.
Relevant maintainers should be CCed too (see scripts/get_maintainers.pl).

[..]

> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index 5cc2bca..5c580bc 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -620,6 +620,16 @@ static hw_irq_controller gicv2_guest_irq_type = {
>      .set_affinity = gicv2_irq_set_affinity,
>  };
>  
> +static hw_irq_controller *gicv2_get_host_irq_type(unsigned int irq)
> +{
> +    return &gicv2_host_irq_type;
> +}
> +
> +static hw_irq_controller *gicv2_get_guest_irq_type(unsigned int irq)
> +{
> +    return &gicv2_guest_irq_type;
> +}
> +
>  static int __init gicv2_init(void)
>  {
>      int res;
> @@ -702,8 +712,8 @@ const static struct gic_hw_operations gicv2_ops = {
>      .save_state          = gicv2_save_state,
>      .restore_state       = gicv2_restore_state,
>      .dump_state          = gicv2_dump_state,
> -    .gic_host_irq_type   = &gicv2_host_irq_type,
> -    .gic_guest_irq_type  = &gicv2_guest_irq_type,
> +    .gic_get_host_irq_type   = gicv2_get_host_irq_type,
> +    .gic_get_guest_irq_type  = gicv2_get_guest_irq_type,
>      .eoi_irq             = gicv2_eoi_irq,
>      .deactivate_irq      = gicv2_dir_irq,
>      .read_irq            = gicv2_read_irq,
> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
> index 0d17885..5ffd52f 100644
> --- a/xen/arch/arm/gic-v3-its.c
> +++ b/xen/arch/arm/gic-v3-its.c
> @@ -301,8 +301,8 @@ post:
>      its_wait_for_range_completion(its, cmd, next_cmd);
>  }
>  
> -void its_send_inv(struct its_device *dev, struct its_collection *col,
> -                  u32 event_id)
> +static void its_send_inv(struct its_device *dev, struct its_collection *col,
> +                         u32 event_id)

The static belongs to the patch where you introduced its_send_inv.

>  {
>      its_cmd_block cmd;
>  
> @@ -390,6 +390,126 @@ void its_send_discard(struct its_device *dev, struct its_collection *col,
>      its_send_single_command(dev->its, &cmd, col);
>  }
>  
> +static void its_flush_and_invalidate_prop(struct irq_desc *desc, u8 *cfg)
> +{
> +    struct its_collection *col;
> +    struct its_device *its_dev = get_irq_its_device(desc);
> +    u32 vid = irq_to_vid(desc);
> +    u16 col_id;
> +
> +    ASSERT(vid < its_dev->nr_lpis);
> +
> +    /*
> +     * Make the above write visible to the redistributors.
> +     * And yes, we're flushing exactly: One. Single. Byte.
> +     * Humpf...
> +     */
> +    if ( gic_rdists->flags & RDIST_FLAGS_PROPBASE_NEEDS_FLUSHING )
> +        clean_and_invalidate_dcache_va_range(cfg, sizeof(*cfg));
> +    else
> +        dsb(ishst);
> +
> +    /* Get collection id for this event id */
> +    col_id = irqdesc_get_collection(desc);
> +    col = &its_dev->its->collections[col_id];
> +    its_send_inv(its_dev, col, vid);

If you would have implemented the same solution as Linux for the
collection, you could have avoid those 2 lines to get the collection.

> +}
> +static void its_irq_shutdown(struct irq_desc *desc)
> +{
> +    its_irq_disable(desc);
> +}
> +
> +static void its_irq_ack(struct irq_desc *desc)
> +{
> +    /* No ACK -- reading IAR has done this for us */
> +}
> +
> +static void its_irq_set_affinity(struct irq_desc *desc, const cpumask_t *mask)
> +{
> +    /*TODO: Yet to support */

/* TODO: ... */

A print/BUG_ON would be helpful to detect if we ever go here.

> +    return;
> +}
> +
> +static const hw_irq_controller its_host_lpi_type = {
> +    .typename     = "gic-its",
> +    .startup      = its_irq_startup,
> +    .shutdown     = its_irq_shutdown,
> +    .enable       = its_irq_enable,
> +    .disable      = its_irq_disable,
> +    .ack          = its_irq_ack,
> +    .end          = gicv3_host_irq_end,
> +    .set_affinity = its_irq_set_affinity,
> +};
> +
> +static const hw_irq_controller its_guest_lpi_type = {
> +    .typename     = "gic-its",
> +    .startup      = its_irq_startup,
> +    .shutdown     = its_irq_shutdown,
> +    .enable       = its_irq_enable,
> +    .disable      = its_irq_disable,
> +    .ack          = its_irq_ack,
> +    .end          = gicv3_guest_irq_end,
> +    .set_affinity = its_irq_set_affinity,
> +};
> +
> +hw_irq_controller *its_get_host_lpi_type(void)
> +{
> +    return &its_host_lpi_type;
> +}
> +
> +hw_irq_controller *its_get_guest_lpi_type(void)
> +{
> +    return &its_guest_lpi_type;
> +}
> +

Please directly export the variables.

>  /*
>   * How we allocate LPIs:
>   *
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 98d45bc..58e878e 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -40,6 +40,7 @@
>  #include <asm/device.h>
>  #include <asm/gic.h>
>  #include <asm/gic_v3_defs.h>
> +#include <asm/gic-its.h>
>  #include <asm/cpufeature.h>
>  
>  /* Global state */
> @@ -1033,15 +1034,19 @@ static void gicv3_irq_ack(struct irq_desc *desc)
>      /* No ACK -- reading IAR has done this for us */
>  }
>  
> -static void gicv3_host_irq_end(struct irq_desc *desc)
> +void gicv3_host_irq_end(struct irq_desc *desc)
>  {
>      /* Lower the priority */
>      gicv3_eoi_irq(desc);
> -    /* Deactivate */
> -    gicv3_dir_irq(desc);
> +    /*
> +     * LPIs does not have active state. Do do not deactivate,
> +     *  when EOI mode is set to 1.
> +     */
> +    if ( !gic_is_lpi(desc->irq) )
> +        gicv3_dir_irq(desc);

I already told you that the goal of the hw_irq_controller is to avoid
checking the interrupt for every callback.

If the current function doesn't work for you, introduce a new one. But
don't put an if inside.

>  }
>  

[..]

> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> index 63feb43..ccbe088 100644
> --- a/xen/arch/arm/irq.c
> +++ b/xen/arch/arm/irq.c
> @@ -156,6 +156,52 @@ static inline struct domain *irq_get_domain(struct irq_desc *desc)
>      return irq_get_guest_info(desc)->d;
>  }
>  
> +unsigned int irq_to_virq(struct irq_desc *desc)
> +{
> +    return irq_get_guest_info(desc)->virq;
> +}

This helper is never used.

> +
> +#ifdef HAS_GICV3
> +unsigned int irq_to_vid(struct irq_desc *desc)
> +{
> +    ASSERT(desc->msi_desc != NULL);
> +
> +    return desc->msi_desc->eventID;
> +}
> +
> +struct its_device *get_irq_its_device(struct irq_desc *desc)
> +{
> +    ASSERT(spin_is_locked(&desc->lock));
> +    ASSERT(desc->msi_desc != NULL);
> +
> +    return desc->msi_desc->dev;
> +}
> +
> +void set_irq_its_device(struct irq_desc *desc, struct its_device *dev)
> +{
> +    ASSERT(spin_is_locked(&desc->lock));
> +    ASSERT(desc->msi_desc != NULL);
> +
> +    desc->msi_desc->dev = dev;
> +}
> +
> +u16 irqdesc_get_collection(struct irq_desc *desc)
> +{
> +    ASSERT(spin_is_locked(&desc->lock));
> +    ASSERT(desc->msi_desc != NULL);
> + 
> +    return desc->msi_desc->col_id;
> +}
> +
> +void irqdesc_set_collection(struct irq_desc *desc, uint16_t col_id)
> +{
> +    ASSERT(spin_is_locked(&desc->lock));
> +    ASSERT(desc->msi_desc != NULL);
> +
> +    desc->msi_desc->col_id = col_id;
> +}
> +#endif

Your naming is not consistent at all. Please use irqdesc_{set/get}_
everywhere rather than trying to reinvent a new help for each pair.

But IHMO, those helpers are not necessary given that they should only be
used internally within the GICv3 ITS and in a couple if not only one
places. I would prefer to see helpers to get the msi_desc and the rest
done within the GICv3 ITS code.

> +
>  void irq_set_affinity(struct irq_desc *desc, const cpumask_t *cpu_mask)
>  {
>      if ( desc != NULL )
> diff --git a/xen/include/asm-arm/gic-its.h b/xen/include/asm-arm/gic-its.h
> index 7bb645e..110516a 100644
> --- a/xen/include/asm-arm/gic-its.h
> +++ b/xen/include/asm-arm/gic-its.h
> @@ -354,6 +354,8 @@ struct gic_its_info {
>  };
>  
>  bool_t is_domain_lpi(struct domain *d, unsigned int lpi);
> +hw_irq_controller *its_get_host_lpi_type(void);
> +hw_irq_controller *its_get_guest_lpi_type(void);
>  int its_init(struct rdist_prop *rdists);
>  int its_cpu_init(void);
>  int vits_get_vitt_entry(struct domain *d, uint32_t devid,
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index 2f5d5e3..2edf9de 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -287,6 +287,8 @@ extern unsigned int gic_number_lines(void);
>  /* LPI support info */
>  bool_t gic_lpi_supported(void);
>  
> +void gicv3_host_irq_end(struct irq_desc *desc);
> +void gicv3_guest_irq_end(struct irq_desc *desc);

Nack. No GICv3 specific function in the common gic header.

>  /* IRQ translation function for the device tree */
>  int gic_irq_xlate(const u32 *intspec, unsigned int intsize,
>                    unsigned int *out_hwirq, unsigned int *out_type);
> @@ -322,10 +324,10 @@ struct gic_hw_operations {
>      void (*dump_state)(const struct vcpu *);
>  
>      /* hw_irq_controller to enable/disable/eoi host irq */
> -    hw_irq_controller *gic_host_irq_type;
> +    hw_irq_controller *(*gic_get_host_irq_type)(unsigned int irq);

const hw_irq_controller ...

>  
>      /* hw_irq_controller to enable/disable/eoi guest irq */
> -    hw_irq_controller *gic_guest_irq_type;
> +    hw_irq_controller *(*gic_get_guest_irq_type)(unsigned int irq);

Same here.

>  
>      /* End of Interrupt */
>      void (*eoi_irq)(struct irq_desc *irqd);
> @@ -364,6 +366,13 @@ struct gic_hw_operations {
>      bool_t (*is_lpi)(unsigned int irq);
>  };
>  
> +struct its_hw_operations {
> +    /* hw_irq_controller to enable/disable/eoi host lpi */
> +    hw_irq_controller *lpi_host_irq_type;
> +    /* hw_irq_controller to enable/disable/eoi guest lpi */
> +    hw_irq_controller *lpi_guest_irq_type;
> +};
> +

What's for? You never use it.

>  void register_gic_ops(const struct gic_hw_operations *ops);
>  int gic_make_hwdom_dt_node(const struct domain *d,
>                             const struct dt_device_node *node,
> diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h
> index f33c331..5923127 100644
> --- a/xen/include/asm-arm/irq.h
> +++ b/xen/include/asm-arm/irq.h
> @@ -18,6 +18,14 @@ struct arch_irq_desc {
>      unsigned int type;
>  };
>  
> +struct msi_desc {
> +#ifdef HAS_GICV3
> +    unsigned int eventID;
> +    struct its_device *dev;
> +    u16 col_id;
> +#endif
> +};
> +
>  #define NR_LOCAL_IRQS	32
>  #define NR_IRQS		1024
>  
> @@ -51,7 +59,12 @@ void arch_move_irqs(struct vcpu *v);
>  
>  /* Set IRQ type for an SPI */
>  int irq_set_spi_type(unsigned int spi, unsigned int type);
> -
> +unsigned int irq_to_virq(struct irq_desc *desc);
> +unsigned int irq_to_vid(struct irq_desc *desc);
> +struct its_device *get_irq_its_device(struct irq_desc *desc);
> +void set_irq_its_device(struct irq_desc *desc, struct its_device *dev);
> +u16 irqdesc_get_collection(struct irq_desc *desc);
> +void irqdesc_set_collection(struct irq_desc *desc, u16 col_id);

This will likely fail to build on ARM 32 bits. That another reason I'd
like to see only a msi_desc helpers.

Regards,

-- 
Julien Grall