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

[Marc Zyngier]

Hi Zenghui,

On Thu, 31 Oct 2019 06:33:23 +0000,
Zenghui Yu <yuzenghui@xxxxxxxxxx> 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@xxxxxxxxxx>
> 
> Reviewed-by: Zenghui Yu <yuzenghui@xxxxxxxxxx>

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

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

Thanks,

	M.

-- 
Jazz is not dead, it just smells funny.