Re: [PATCH v2 06/36] irqchip/gic-v3-its: Kill its->device_ids and use TYPER copy instead

Zenghui Yu
2019-10-31T09:08:25Z

On 2019/10/31 16:30, Marc Zyngier wrote:
> Hi Zenghui,
>
> On Thu, 31 Oct 2019 06:33:23 +0000,
> Zenghui Yu <yuzenghui@huawei.com> wrote:
>>
>> Hi Marc,
>>
>> On 2019/10/27 22:42, Marc Zyngier wrote:
>>> Now that we have a copy of TYPER in the ITS structure, rely on this
>>> to provide the same service as its->device_ids, which gets axed.
>>> Errata workarounds are now updating the cached fields instead of
>>> requiring a separate field in the ITS structure.
>>>
>>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>>
>> Reviewed-by: Zenghui Yu <yuzenghui@huawei.com>
>
> Thanks for that.
>
>>
>>> ---
>>> drivers/irqchip/irq-gic-v3-its.c | 24 +++++++++++++-----------
>>> include/linux/irqchip/arm-gic-v3.h | 2 +-
>>> 2 files changed, 14 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
>>> index 3b046181ddfc..6c91c7feadf3 100644
>>> --- a/drivers/irqchip/irq-gic-v3-its.c
>>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>>> @@ -109,7 +109,6 @@ struct its_node {
>>> struct list_head its_device_list;
>>> u64 flags;
>>> unsigned long list_nr;
>>> - u32 device_ids;
>>> int numa_node;
>>> unsigned int msi_domain_flags;
>>> u32 pre_its_base; /* for Socionext Synquacer */
>>> @@ -117,6 +116,7 @@ struct its_node {
>>> };
>>> #define is_v4(its) (!!((its)->typer &
>>> GITS_TYPER_VLPIS))
>>> +#define device_ids(its) (FIELD_GET(GITS_TYPER_DEVBITS, (its)->typer) + 1)
>>> #define ITS_ITT_ALIGN SZ_256
>>> @@ -1938,9 +1938,9 @@ static bool its_parse_indirect_baser(struct
>>> its_node *its,
>>> if (new_order >= MAX_ORDER) {
>>> new_order = MAX_ORDER - 1;
>>> ids = ilog2(PAGE_ORDER_TO_SIZE(new_order) / (int)esz);
>>> - pr_warn("ITS@%pa: %s Table too large, reduce ids %u->%u\n",
>>> + pr_warn("ITS@%pa: %s Table too large, reduce ids %llu->%u\n",
>>> &its->phys_base, its_base_type_string[type],
>>> - its->device_ids, ids);
>>> + device_ids(its), ids);
>>
>> But this pr_warn() looks a bit odd. The table type is chosen from
>> its_base_type_string[], but ids is always Devbits (+1)?
>
> This is a bit of a shortcut, I agree. But the device table practically
> is the only one where we can run out of space if the ITS doesn't
> support two level tables. All the other tables are very small, being
> limited by the number of CPUs (collections) or a small ID space
> (vPEs).

Make sense. Thanks for the clarification.

Zenghui

>
> So while this is a bit ugly, I don't thing it is not too concerning.
>
> Thanks,
>
> M.
>