Re: [Xen-devel] [PATCH v7 09/28] xen/arm: ITS: Introduce gic_is_lpi helper function

[Julien Grall <julien.grall@xxxxxxxxxx>]

Hi Vijay,

On 18/09/15 14:08, vijay.kilari@xxxxxxxxx wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxxxxxxxxxx>
> 
> Helper function gic_is_lpi() is used to find
> if irq is lpi or not. For GICv2 platforms this function
> returns number of irq ids which represents only number of line irqs.
> For GICv3 platform irq ids are calculated from nr_lpis if
> HW supports LPIs
> 
> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxxxxxxxxxx>
> CC: Zoltan Kiss <zoltan.kiss@xxxxxxxxxx>
> ---
> v7: - Rename gic_info.nr_id_bits as gic_info.nr_irq_ids
>     - gic_is_lpi() is common for both ARM64/32
>     - Introduce global variable nr_lpis from patch #17
>     - Reset nr_lpis to 0 when HW does not support LPIs
>     - pass nr_lpis as boot args to Xen. Set minimum to 8192
> v6: - Added gic_info.nr_id_bits to hold number of SPI/LPIs
>       supported
>     - Dropped is_lpi() callback
> ---
>  xen/arch/arm/gic-hip04.c  |    3 ++-
>  xen/arch/arm/gic-v2.c     |    3 ++-
>  xen/arch/arm/gic-v3.c     |   20 ++++++++++++++++++++
>  xen/arch/arm/gic.c        |   10 ++++++++++
>  xen/arch/arm/irq.c        |   15 +++++++++++++++
>  xen/include/asm-arm/gic.h |    5 +++++
>  xen/include/asm-arm/irq.h |    3 +++
>  7 files changed, 57 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-hip04.c b/xen/arch/arm/gic-hip04.c
> index c5ed545..398881b 100644
> --- a/xen/arch/arm/gic-hip04.c
> +++ b/xen/arch/arm/gic-hip04.c
> @@ -301,7 +301,8 @@ static void __init hip04gic_dist_init(void)
>  
>      /* Only 1020 interrupts are supported */
>      gicv2_info.nr_lines = min(1020U, nr_lines);
> -

This empty line was useful to separate internal initialization from
enabling the distributor. Please retain the empty line.

> +    /* Number of IRQ ids supported */
> +    gicv2_info.nr_irq_ids = gicv2_info.nr_lines;
>      /* Turn on the distributor */
>      writel_gicd(GICD_CTL_ENABLE, GICD_CTLR);
>  }
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index 596126d..6673f29 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -291,7 +291,8 @@ static void __init gicv2_dist_init(void)
>  
>      /* Only 1020 interrupts are supported */
>      gicv2_info.nr_lines = min(1020U, nr_lines);
> -

Ditto.

> +    /* Number of IRQ ids supported */
> +    gicv2_info.nr_irq_ids = nr_lines;
>      /* Turn on the distributor */
>      writel_gicd(GICD_CTL_ENABLE, GICD_CTLR);
>  }
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 5076706..6680bd2 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -529,6 +529,11 @@ static void gicv3_set_irq_properties(struct irq_desc 
> *desc,
>      spin_unlock(&gicv3.lock);
>  }
>  
> +static int gicv3_dist_supports_lpis(void)
> +{
> +    return readl_relaxed(GICD + GICD_TYPER) & GICD_TYPER_LPIS_SUPPORTED;
> +}
> +
>  static void __init gicv3_dist_init(void)
>  {
>      uint32_t type;
> @@ -578,6 +583,21 @@ static void __init gicv3_dist_init(void)
>  
>      /* Only 1020 interrupts are supported */
>      gicv3_info.nr_lines = min(1020U, nr_lines);
> +
> +    /*
> +     * Number of IRQ ids supported.
> +     * Here we override HW supported number of LPIs and
> +     * limit to to LPIs specified in nr_lpis.
> +     */

You still have to check that the number of LPIs requesting by the user
is supported by the hardware.

The user parameter can be seen as a restriction rather than a blind
overriding.

> +    if ( gicv3_dist_supports_lpis() )

I'm sure we already speak about it in a previous series. The GICv3 may
support LPIs even without an ITS. Here you would claim that Xen is
supporting LPIs which is clearly not true.

When the ITS is not present, this value should be the number of
interrupt line supported.

I haven't checked if you fixed it later, but all the patch should be
bisectable. I.e I shouldn't see any unwanted modification on supported
platform with GICv3 (for instance the foundation model).

> +        gicv3_info.nr_irq_ids = nr_lpis + FIRST_GIC_LPI;
> +    else
> +    {
> +        gicv3_info.nr_irq_ids = gicv3_info.nr_lines;
> +        /* LPIs are not supported by HW. Reset to 0 */
> +        nr_lpis = 0;

That's really ugly and you still let GICv2 hardware with nr_lpis != 0.
As said on v6 I don't want to see any usage of nr_lpis in code except
for letting the user restricting the number of LPIs supported.

That means that all the code should use gic_nr_irq_ids to know the
number of LPIs supported. Mixing the 2 usage will lead to big trouble
latter.

So please move nr_lpis in gic-v3 as a parameter and don't export it.

> +    }
> +
>  }
>  
>  static int gicv3_enable_redist(void)
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 1d94e5e..a0ed7df 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -62,6 +62,16 @@ enum gic_version gic_hw_version(void)
>     return gic_hw_ops->info->hw_version;
>  }
>  
> +unsigned int gic_nr_irq_ids(void)
> +{
> +    return gic_hw_ops->info->nr_irq_ids;
> +}
> +
> +bool_t gic_is_lpi(unsigned int irq)
> +{
> +    return (irq >= FIRST_GIC_LPI && irq < gic_nr_irq_ids());
> +}
> +
>  unsigned int gic_number_lines(void)
>  {
>      return gic_hw_ops->info->nr_lines;
> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> index d582f57..3a01f46 100644
> --- a/xen/arch/arm/irq.c
> +++ b/xen/arch/arm/irq.c
> @@ -31,6 +31,18 @@
>  static unsigned int local_irqs_type[NR_LOCAL_IRQS];
>  static DEFINE_SPINLOCK(local_irqs_type_lock);
>  
> +/* Number of LPIs supported by Xen.
> + *
> + * The LPI identifier starts from 8192. Given that the hardware is
> + * providing the number of identifier bits, supporting LPIs requires at
> + * least 14 bits. This will represent 16384 interrupt ID of which 8192
> + * LPIs. So the minimum of LPIs supported when the hardware supports LPIs
> + * is 8192.
> + */
> +#define DEFAULT_NR_LPIS 8192
> +unsigned int nr_lpis = DEFAULT_NR_LPIS;
> +integer_param("nr_lpis", nr_lpis);
> +

Any new parameter should be documented in
docs/misc/xen-command-line.markdown

Regards,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel