Re: [RFC PATCH v3 07/18] xen/arm: ITS: implement hw_irq_controller for LPIs

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

Hi Vijay,

On 22/06/15 13:01, vijay.kilari@gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> 
> Implements hw_irq_controller api's required
> to handle LPI's

This patch doesn't hw_irq_controller for LPI but just hack around the
current GICv3 host hw_irq_controller.

As said on the previous version, the goal of hw_irq_controller is too
keep things simple (i.e few conditional code). Please introduce a
separate hw_irq_controller for LPIs.

> 
> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> ---
>  xen/arch/arm/gic-v3-its.c     |   39 +++++++++++++++++++++++++++++++++++++++
>  xen/arch/arm/gic-v3.c         |   26 +++++++++++++++++++-------
>  xen/arch/arm/irq.c            |   16 ++++++++++++++++
>  xen/include/asm-arm/gic-its.h |   10 ++++++++++
>  xen/include/asm-arm/gic.h     |    4 ++++
>  xen/include/asm-arm/irq.h     |    4 +++-
>  6 files changed, 91 insertions(+), 8 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
> index 349d0bb..535fc53 100644
> --- a/xen/arch/arm/gic-v3-its.c
> +++ b/xen/arch/arm/gic-v3-its.c
> @@ -508,6 +508,45 @@ static void its_send_invall(struct its_node *its, struct its_collection *col)
>      its_send_single_command(its, its_build_invall_cmd, &desc);
>  }
>  
> +void lpi_set_config(struct irq_desc *desc, int enable)

Any function exported should have their prototype defined within the
same patch...

> +{
> +    struct its_collection *col;
> +    struct its_device *its_dev = get_irq_device(desc);
> +    u8 *cfg;
> +    u32 virq = irq_to_virq(desc);
> +
> +    ASSERT(virq < its_dev->nr_lpis);
> +
> +    cfg = gic_rdists->prop_page + desc->irq - NR_GIC_LPI;
> +    if ( enable )
> +        *cfg |= LPI_PROP_ENABLED;
> +    else
> +        *cfg &= ~LPI_PROP_ENABLED;
> +
> +    /*
> +     * 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 = &its_dev->its->collections[virq % num_online_cpus()];

This is fragile, you are assuming that num_online_cpus() will never
change. Why don't you store the collection in every irq_desc?

> +    its_send_inv(its_dev, col, virq);
> +}
> +
> +void its_set_affinity(struct irq_desc *desc, int cpu)
> +{
> +    struct its_device *its_dev = get_irq_device(desc);
> +    struct its_collection *target_col;
> +
> +    /* Physical collection id */
> +    target_col = &its_dev->its->collections[cpu];
> +    its_send_movi(its_dev, target_col, irq_to_virq(desc));

The field "virq" in the structure irq_guest refers to the guest virtual
IRQ and not the event ID. As Ian suggested in the proposal [1], please
use an union to make this code clears.

Furthermore, when you set the LPI configuration (see lpi_set_config) you
are using a round robin to get the collection. This won't work anymore
if Xen decides to change the affinity... So you may want to drop
affinity support for now.

> +}

Missing newline.

[..]

> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> index 2dd43ee..9dbdf7d 100644
> --- a/xen/arch/arm/irq.c
> +++ b/xen/arch/arm/irq.c
> @@ -36,6 +36,7 @@ struct irq_guest
>  {
>      struct domain *d;
>      unsigned int virq;
> +    struct its_device *dev;

I know that this was suggested in the proposal [1]. But the goal of
irq_guest is to store anything specific to the guest. The event ID and
the its_device assigned are known when the device is added to Xen and
hence can be set in irq_desc (with a small memory impact, but we have
plenty of memory on ARM64).

This would avoid you to set dev and virq  every time the device is
passthrough to a guest.

>  };
>  
>  static void ack_none(struct irq_desc *irq)
> @@ -143,6 +144,21 @@ 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;
> +}
> +
> +struct its_device *get_irq_device(struct irq_desc *desc)
> +{
> +    return irq_get_guest_info(desc)->dev;
> +}
> +
> +void set_irq_device(struct irq_desc *desc, struct its_device *dev)
> +{
> +    irq_get_guest_info(desc)->dev = dev;
> +}

The goal of route_irq_guest is to setup correctly irq_guest. If the
current version doesn't fit your usage please update it or add a new
helper and no workaround the 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 59a6490..a47cf26 100644
> --- a/xen/include/asm-arm/gic-its.h
> +++ b/xen/include/asm-arm/gic-its.h
> @@ -205,6 +205,16 @@ static inline uint8_t its_decode_cmd(its_cmd_block *cmd)
>      return cmd->raw_cmd[0] & 0xff;
>  }
>  
> +static inline uint32_t its_decode_devid(struct domain *d, its_cmd_block *cmd)
> +{
> +    /* TODO: Use pci helper function to get physical id */
> +    return (cmd->raw_cmd[0] >> 32);  
> +}

This doesn't belong to this patch. And without more comment it makes
little sense why you are using cmd.

> +
> +void its_set_affinity(struct irq_desc *desc, int cpu);
> +void lpi_set_config(struct irq_desc *desc, int enable);
> +uint32_t its_get_nr_events(void);
> +
>  #endif /* __ASM_ARM_GIC_ITS_H__ */
>  
>  /*
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index e9d5f36..0209cc5 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 NR_GIC_LPI         8192
> +#define MAX_LPI            (8192 + 4096)
> +#define MAX_NR_LPIS        4096

For me NR_GIC_LPI, MAX_LPI and MAX_NR_LPIS means the exactly the same
things but you are using 3 different value.

Please name the correctly:
	- NR_GIC_LPI => FIRST_GIC_LPI
	- MAX_NR_LPIS => NR_GIC_LPI
	- Drop MAX_LPI

Although, why do you hardcode the number of LPIs? This should be
retrieved from the GIC hardware configuration.

>  #define MAX_RDIST_COUNT    4
>  
>  #define GICD_CTLR       (0x000)
> @@ -163,6 +166,7 @@
>  #define DT_MATCH_GIC_V3 DT_MATCH_COMPATIBLE("arm,gic-v3")
>  #define DT_MATCH_GIC_ITS DT_MATCH_COMPATIBLE("arm,gic-v3-its")
>  
> +#define is_lpi(lpi) (lpi >= NR_GIC_LPI && lpi < MAX_LPI)

Missing newline.

Regards,

[1]
http://xenbits.xensource.com/people/ianc/vits/draftF.html#virtual-lpi-interrupt-injection

-- 
Julien Grall