Re: [PATCH] irqchip: gic-v3-its: ensure nr_ites >= nr_lpis

[Ard Biesheuvel]

On 6 March 2018 at 16:00, Marc Zyngier <marc.zyngier@xxxxxxx> wrote:
> On 06/03/18 15:51, Ard Biesheuvel wrote:
>> When struct its_device instances are created, the nr_ites member
>> will be set to a power of 2 that equals or exceeds the requested
>> number of MSIs passed to the msi_prepare() callback. At the same
>> time, the LPI map is allocated to be some multiple of 32 in size,
>> where the allocated size may be less than the requested size
>> depending on whether a contiguous range of sufficient size is
>> available in the global LPI bitmap.
>>
>> This may result in the situation where the nr_ites < nr_lpis, and
>> since nr_ites is what we program into the hardware when we map the
>> device, the additional LPIs will be non-functional.
>>
>> For bog standard hardware, this does not really matter. However,
>> in cases where ITS device IDs are shared between different PCIe
>> devices, we may end up allocating these additional LPIs without
>> taking into account that they don't actually work.
>>
>> So let's make nr_ites at least 32. This ensures that all allocated
>> LPIs are 'live', and that its_alloc_device_irq() will fail when
>> attempts are made to allocate MSIs beyond what was allocated in
>> the first place.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx>
>> ---
>>  drivers/irqchip/irq-gic-v3-its.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
>> index 1d3056f53747..ee488c468400 100644
>> --- a/drivers/irqchip/irq-gic-v3-its.c
>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>> @@ -1412,7 +1412,7 @@ static struct irq_chip its_irq_chip = {
>>   * This gives us (((1UL << id_bits) - 8192) >> 5) possible allocations.
>>   */
>>  #define IRQS_PER_CHUNK_SHIFT 5
>> -#define IRQS_PER_CHUNK               (1 << IRQS_PER_CHUNK_SHIFT)
>> +#define IRQS_PER_CHUNK               (1UL << IRQS_PER_CHUNK_SHIFT)
>>  #define ITS_MAX_LPI_NRBITS   16 /* 64K LPIs */
>>
>>  static unsigned long *lpi_bitmap;
>> @@ -2123,7 +2123,7 @@ static struct its_device *its_create_device(struct its_node *its, u32 dev_id,
>>        * of two entries. No, the architecture doesn't let you
>>        * express an ITT with a single entry.
>
> This comment feels slightly out of place now.
>

Indeed.

>>        */
>> -     nr_ites = max(2UL, roundup_pow_of_two(nvecs));
>> +     nr_ites = max(IRQS_PER_CHUNK, roundup_pow_of_two(nvecs));
>>       sz = nr_ites * its->ite_size;
>>       sz = max(sz, ITS_ITT_ALIGN) + ITS_ITT_ALIGN - 1;
>>       itt = kzalloc(sz, GFP_KERNEL);
>>
>
> Otherwise, this looks good to me. I'll amend the comment when applying
> the patch, no need to respin.
>

Excellent, thanks!