Re: [Xen-devel] [PATCH v6 17/31] xen/arm: ITS: Store LPIs allocated and IRQ ID bits per domain

[Vijay Kilari <vijay.kilari@xxxxxxxxx>]

On Thu, Sep 3, 2015 at 9:55 PM, Julien Grall <julien.grall@xxxxxxxxxx> wrote:
> Hi Vijay,
>
> On 31/08/15 12:06, vijay.kilari@xxxxxxxxx wrote:
>> From: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxxxxxxxxxx>
>>
>> Store number of lpis and number of id bits
>> in vgic structure
>>
>> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxxxxxxxxxx>
>> ---
>>  xen/arch/arm/irq.c           |    9 +++++++++
>>  xen/arch/arm/vgic-v3-its.c   |    2 ++
>>  xen/arch/arm/vgic.c          |   12 ++++++++++++
>>  xen/include/asm-arm/domain.h |    3 +++
>>  xen/include/asm-arm/irq.h    |    3 +++
>>  5 files changed, 29 insertions(+)
>>
>> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
>> index 24c4f24..93e9411 100644
>> --- a/xen/arch/arm/irq.c
>> +++ b/xen/arch/arm/irq.c
>> @@ -31,6 +31,15 @@
>>  static unsigned int local_irqs_type[NR_LOCAL_IRQS];
>>  static DEFINE_SPINLOCK(local_irqs_type_lock);
>>
>> +/* Number of LPI supported in XEN */
>> +/*
>
> Number of LPI supported by Xen.
>
> Also, it's not necessary to have a separate comment for this. It could
> be included in the next one. I.e:
>
> /*
>  * Number of LPI supported by Xen
>  *
>  * LPI ....
>
>> + * LPI number start from 8192. Minimum number of bits
>> + * required to represent 8192 is 13 bits. So to Support LPIs minimum
>> + * 14 bits are required which can represent maximum LPI 16384.
>> + * 16384 - 8192 = 8192. Minimum number of LPIs supported is 8192
>> + */
>
> The explanation is hard to understand. I would rewrite like:
>
> "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 ".
>
>> +unsigned int nr_lpis = 8192;
>> +
>
> I still don't think that it makes sense to introduce the number of LPIs
> variable in a patch for vITS. As you describe it, it should be part of
> an ITS/GICv3 patch.
>
> Although, I think you should use the field nr_irq_ids in the gic
> structure (see patch #10) to avoid problem you currently have with this
> solution.
>
> For instance, gic_is_lpi is relying on the number of ID bits exposed by
> the hardware but you allocate the IRQ desc for LPIs based on nr_lpis.
>

You mean to restrict nr_irq_ids to FIRST_GIC_LPI + 8192 (nr_lpis)
instead of HW supported value?
and let gic_is_lpi() always check for Xen supported lpi number instead of hw
supported value?

> It's fine to restrict the value in the GIC compare to hardware.
>
> Furthermore this value should be 0 on platform where LPIs is not
> supported in order to make confusion and introduce possible problem with
> the code later. I could add that, AFAICT, this new variable (nr_lpis) is
> not that often within the code.

 nr_lpis is global variable that we define to tell number of LPIs supported
by Xen. If platform does not support LPIs, then vgic.nr_lpis is set to 0.

>
> Lastly, on a previous version (don't remember which one) we said that
> the user should be able to change the value on the command line. What's
> your plan for that?

Yes, it was part of our discussion on chat. I will fix it.

>
>>  /* Describe an IRQ assigned to a guest */
>>  struct irq_guest
>>  {
>> diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
>> index fabbad0..cef6139 100644
>> --- a/xen/arch/arm/vgic-v3-its.c
>> +++ b/xen/arch/arm/vgic-v3-its.c
>> @@ -547,6 +547,8 @@ int vits_domain_init(struct domain *d)
>>
>>      ASSERT(is_hardware_domain(d));
>>
>> +    d->arch.vgic.nr_lpis = nr_lpis;
>> +
>
> With  my suggestion, this would turn into gic_nr_irq_ids() - 8192;

As suggested above, if we restrict nr_irq_ids to Xen supported
value (FIRST_GIC_LPI + 8192(nr_lpis) then you suggestion is right.

Regards
Vijay

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