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

2015-09-22 Thread Vijay Kilari 
On Mon, Sep 21, 2015 at 7:50 PM, Julien Grall  wrote:
> Hi Vijay,
>
> On 18/09/15 14:08, vijay.kil...@gmail.com wrote:
>> From: Vijaya Kumar K 
>>
>> 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
>>
[...]

>>  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.

Here nr_lpis is used to update nr_irq_ids which is used by gic_nr_irq_ids().
>
> So please move nr_lpis in gic-v3 as a parameter and don't export it.

nr_lpis is user defined/initialized in irq.c . How can we pass this to gic-v3 as
parameter?. You mean to have helper function to read/update nr_lpis?

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

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

2015-09-22 Thread Julien Grall 
On 22/09/15 10:10, Vijay Kilari wrote:
> On Mon, Sep 21, 2015 at 7:50 PM, Julien Grall  wrote:
>> 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.
> 
> Here nr_lpis is used to update nr_irq_ids which is used by gic_nr_irq_ids().

That not the only usage of nr_lpis within this series. You are using it
to create the IRQ desc for LPIs.

We should have only one way to get the number of LPIs (i.e
gic_nr_irq_ids()). Any attempt to have 2 different to retrieve the
number of LPIs will be a problem later.

>>
>> So please move nr_lpis in gic-v3 as a parameter and don't export it.
> 
> nr_lpis is user defined/initialized in irq.c . How can we pass this to gic-v3 
> as
> parameter?. You mean to have helper function to read/update nr_lpis?

I meant moving the parameter in gic-v3 (i.e moving integer_param(...)).

We can speak about moving the parameter in another place when we will
have the need to.

Regards,

-- 
Julien Grall

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

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

2015-09-21 Thread Julien Grall 
Hi Vijay,

On 18/09/15 14:08, vijay.kil...@gmail.com wrote:
> From: Vijaya Kumar K 
> 
> 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 
> CC: Zoltan Kiss 
> ---
> 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();
>  }
>  
> +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

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

2015-09-18 Thread vijay . kilari 
From: Vijaya Kumar K 

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 
CC: Zoltan Kiss 
---
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);
-
+/* 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);
-
+/* 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();
 }
 
+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.
+ */
+if ( gicv3_dist_supports_lpis() )
+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;
+}
+
 }
 
 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);
+
 /* Describe an IRQ assigned to a guest */
 struct irq_guest
 {
@@ -115,6 +127,9 @@ void __init init_IRQ(void)
 {
 int irq;
 
+if ( nr_lpis < DEFAULT_NR_LPIS )
+nr_lpis = DEFAULT_NR_LPIS;
+
 spin_lock(_irqs_type_lock);
 for ( irq = 0; irq < NR_LOCAL_IRQS; irq++ )
 local_irqs_type[irq] = DT_IRQ_TYPE_INVALID;
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index e330fe3..ce2279e 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -20,6 +20,7 @@
 
 #define NR_GIC_LOCAL_IRQS  NR_LOCAL_IRQS
 #define NR_GIC_SGI 16
+#define