Re: [PATCH v3 05/32] irqchip/gic-v4.1: VPE table (aka GICR_VPROPBASER) allocation

[Marc Zyngier]

On 2020-01-20 14:03, Zenghui Yu wrote:
> Hi Marc,
>
> On 2019/12/24 19:10, 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>
>
> Reviewed-by: Zenghui Yu <yuzenghui@xxxxxxxxxx>
>
> With two very minor concerns below.
>
> [...]
>
> > +static int allocate_vpe_l1_table(void)
> > +{
> > +	void __iomem *vlpi_base = gic_data_rdist_vlpi_base();
> > +	u64 val, gpsz, npg, pa;
> > +	unsigned int psz = SZ_64K;
> > +	unsigned int np, epp, esz;
> > +	struct page *page;
> > +
> > +	if (!gic_rdists->has_rvpeid)
> > +		return 0;
> > +
> > +	/*
> > +	 * if VPENDBASER.Valid is set, disable any previously programmed
> > +	 * VPE by setting PendingLast while clearing Valid. This has the
> > +	 * effect of making sure no doorbell will be generated and we can
> > +	 * then safely clear VPROPBASER.Valid.
> > +	 */
> > +	if (gits_read_vpendbaser(vlpi_base + GICR_VPENDBASER) & 
> > GICR_VPENDBASER_Valid)
> > +		gits_write_vpendbaser(GICR_VPENDBASER_PendingLast,
> > +				      vlpi_base + GICR_VPENDBASER);
>
> I'm confused here.  The Valid field resets to 0.  Under which scenario
> will the Valid==1 while we're doing initialization for this RD?

The cases I'm worried about are things like kexec or cpu hotplug,
where we could find that the RD programming is still valid, one way
or another. This is unlikely to hit in practice, but who knows...

> > +
> > +	/*
> > +	 * If we can inherit the configuration from another RD, let's do
> > +	 * so. Otherwise, we have to go through the allocation process. We
> > +	 * assume that all RDs have the exact same requirements, as
> > +	 * nothing will work otherwise.
> > +	 */
> > +	val = 
> > inherit_vpe_l1_table_from_rd(&gic_data_rdist()->vpe_table_mask);
> > +	if (val & GICR_VPROPBASER_4_1_VALID)
> > +		goto out;
> > +
> > +	gic_data_rdist()->vpe_table_mask = kzalloc(sizeof(cpumask_t), 
> > GFP_KERNEL);
> > +	if (!gic_data_rdist()->vpe_table_mask)
> > +		return -ENOMEM;
> > +
> > +	val = inherit_vpe_l1_table_from_its();
> > +	if (val & GICR_VPROPBASER_4_1_VALID)
> > +		goto out;
> > +
> > +	/* First probe the page size */
> > +	val = FIELD_PREP(GICR_VPROPBASER_4_1_PAGE_SIZE, GIC_PAGE_SIZE_64K);
> > +	gits_write_vpropbaser(val, vlpi_base + GICR_VPROPBASER);
> > +	val = gits_read_vpropbaser(vlpi_base + GICR_VPROPBASER);
> > +	gpsz = FIELD_GET(GICR_VPROPBASER_4_1_PAGE_SIZE, val);
> > +	esz = FIELD_GET(GICR_VPROPBASER_4_1_ENTRY_SIZE, val);
> > +
> > +	switch (gpsz) {
> > +	default:
> > +		gpsz = GIC_PAGE_SIZE_4K;
> > +		/* fall through */
> > +	case GIC_PAGE_SIZE_4K:
> > +		psz = SZ_4K;
> > +		break;
> > +	case GIC_PAGE_SIZE_16K:
> > +		psz = SZ_16K;
> > +		break;
> > +	case GIC_PAGE_SIZE_64K:
> > +		psz = SZ_64K;
> > +		break;
> > +	}
> > +
> > +	/*
> > +	 * Start populating the register from scratch, including RO fields
> > +	 * (which we want to print in debug cases...)
> > +	 */
> > +	val = 0;
> > +	val |= FIELD_PREP(GICR_VPROPBASER_4_1_PAGE_SIZE, gpsz);
> > +	val |= FIELD_PREP(GICR_VPROPBASER_4_1_ENTRY_SIZE, esz);
> > +
> > +	/* How many entries per GIC page? */
> > +	esz++;
> > +	epp = psz / (esz * SZ_8);
> > +
> > +	/*
> > +	 * If we need more than just a single L1 page, flag the table
> > +	 * as indirect and compute the number of required L1 pages.
> > +	 */
> > +	if (epp < ITS_MAX_VPEID) {
> > +		int nl2;
> > +
> > +		gic_rdists->flags |= RDIST_FLAGS_VPE_INDIRECT;
>
> This flag is set but not used, can we just drop it?

Yes, good point. I can't even remember what I had in mind with this
flag, so it can't be that important! ;-).

I'll clean that up.

Thanks,

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