Re: [PATCH 10/35] irqchip/gic-v4.1: VPE table (aka GICR_VPROPBASER) allocation

[Marc Zyngier]

On 2019-09-25 15:41, Marc Zyngier wrote:
> On 25/09/2019 14:04, Zenghui Yu wrote:
> > Hi Marc,
> >
> > Some questions about this patch, mostly to confirm that I would
> > understand things here correctly.
> >
> > On 2019/9/24 2:25, Marc Zyngier wrote:
> > > GICv4.1 defines a new VPE table that is potentially shared between
> > > both the ITSs and the redistributors, following complicated 
> > > affinity
> > > rules.
> > >
> > > To make things more confusing, the programming of this table at
> > > the redistributor level is reusing the GICv4.0 GICR_VPROPBASER 
> > > register
> > > for something completely different.
> > >
> > > The code flow is somewhat complexified by the need to respect the
> > > affinities required by the HW, meaning that tables can either be
> > > inherited from a previously discovered ITS or redistributor.
> > >
> > > Signed-off-by: Marc Zyngier <maz@xxxxxxxxxx>
> > > ---
> >
> > [...]
> >
> > > @@ -1962,6 +1965,65 @@ static bool its_parse_indirect_baser(struct 
> > > its_node *its,
> > >   	return indirect;
> > >   }
> > >
> > > +static u32 compute_common_aff(u64 val)
> > > +{
> > > +	u32 aff, clpiaff;
> > > +
> > > +	aff = FIELD_GET(GICR_TYPER_AFFINITY, val);
> > > +	clpiaff = FIELD_GET(GICR_TYPER_COMMON_LPI_AFF, val);
> > > +
> > > +	return aff & ~(GENMASK(31, 0) >> (clpiaff * 8));
> > > +}
> > > +
> > > +static u32 compute_its_aff(struct its_node *its)
> > > +{
> > > +	u64 val;
> > > +	u32 svpet;
> > > +
> > > +	/*
> > > +	 * Reencode the ITS SVPET and MPIDR as a GICR_TYPER, and compute
> > > +	 * the resulting affinity. We then use that to see if this match
> > > +	 * our own affinity.
> > > +	 */
> > > +	svpet = FIELD_GET(GITS_TYPER_SVPET, its->typer);
> > > +	val  = FIELD_PREP(GICR_TYPER_COMMON_LPI_AFF, svpet);
> > > +	val |= FIELD_PREP(GICR_TYPER_AFFINITY, its->mpidr);
> > > +	return compute_common_aff(val);
> > > +}
> > > +
> > > +static struct its_node *find_sibbling_its(struct its_node 
> > > *cur_its)
> > > +{
> > > +	struct its_node *its;
> > > +	u32 aff;
> > > +
> > > +	if (!FIELD_GET(GITS_TYPER_SVPET, cur_its->typer))
> > > +		return NULL;
> > > +
> > > +	aff = compute_its_aff(cur_its);
> > > +
> > > +	list_for_each_entry(its, &its_nodes, entry) {
> > > +		u64 baser;
> > > +
> > > +		if (!is_v4_1(its) || its == cur_its)
> > > +			continue;
> > > +
> > > +		if (!FIELD_GET(GITS_TYPER_SVPET, its->typer))
> > > +			continue;
> > > +
> > > +		if (aff != compute_its_aff(its))
> > > +			continue;
> > > +
> > > +		/* GICv4.1 guarantees that the vPE table is GITS_BASER2 */
> > > +		baser = its->tables[2].val;
> > > +		if (!(baser & GITS_BASER_VALID))
> > > +			continue;
> > > +
> > > +		return its;
> > > +	}
> > > +
> > > +	return NULL;
> > > +}
> > > +
> > >   static void its_free_tables(struct its_node *its)
> > >   {
> > >   	int i;
> > > @@ -2004,6 +2066,15 @@ static int its_alloc_tables(struct its_node 
> > > *its)
> > >   			break;
> > >
> > >   		case GITS_BASER_TYPE_VCPU:
> > > +			if (is_v4_1(its)) {
> > > +				struct its_node *sibbling;
> > > +
> > > +				if ((sibbling = find_sibbling_its(its))) {
> > > +					its->tables[2] = sibbling->tables[2];
> >
> > This means thst the vPE table is shared between ITSs which are under 
> > the
> > same affinity group?
>
> That's indeed what this is trying to do...
>
> > Don't we need to set GITS_BASER (by its_setup_baser()) here to tell 
> > this
> > ITS where the vPE table lacates?
>
> Absolutely. This is a bug. I didn't spot it as my model only has a
> single ITS.

FWIW, I've pushed out an updated branch with this fixed as well as the 
rest of the comments you had.

Thanks again,

        M.
--
Jazz is not dead. It just smells funny...