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

From: Zenghui Yu <yuzenghui@huawei.com>
To: Marc Zyngier <maz@kernel.org>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
	Jason Cooper <jason@lakedaemon.net>,
	linux-kernel@vger.kernel.org,
	Robert Richter <rrichter@marvell.com>,
	Jayachandran C <jnair@marvell.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	kvmarm@lists.cs.columbia.edu
Subject: Re: [PATCH v2 06/36] irqchip/gic-v3-its: Kill its->device_ids and use TYPER copy instead
Date: Thu, 31 Oct 2019 17:08:03 +0800	[thread overview]
Message-ID: <7439499b-1626-51a5-2f67-e79e7fdbcdf9@huawei.com> (raw)
In-Reply-To: <86imo5xqoh.wl-maz@kernel.org>

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

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm