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

[Zenghui Yu]

On 2019/10/31 16:30, Marc Zyngier wrote:
> 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).

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.