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

[Vijay Kilari <vijay.kilari@xxxxxxxxx>]

On Mon, Sep 21, 2015 at 7:50 PM, Julien Grall <julien.grall@xxxxxxxxxx> wrote:
> 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
>>
[...]

>>  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@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel