Re: [PATCH RFC 3/5] irqchip/gic-v4.1: Ensure L2 vPE table is allocated at RD level

[Marc Zyngier <maz@kernel.org>]

Hi Zenghui,

Thanks for this. A few comments below.

On 2020-02-04 09:09, Zenghui Yu wrote:
> In GICv4, we will ensure that level2 vPE table memory is allocated
> for the specified vpe_id on all v4 ITS, in its_alloc_vpe_table().
> This still works well for the typical GICv4.1 implementation, where
> the new vPE table is shared between the ITSs and the RDs.
> 
> To make things explicit, introduce allocate_vpe_l1_table_entry() to
> make sure that the L2 tables are allocated on all v4.1 RDs. We're
> likely not need to allocate memory in it because the vPE table is
> shared and (L2 table is) already allocated at ITS level, except
> for the case where the ITS doesn't share anything (say SVPET == 0,
> practically unlikely but architecturally allowed).

Huh... Interesting. I definitely don't anticipate the case to pop up,
but you are right, this is architecturally allowed.

> The implementation of allocate_vpe_l1_table_entry() is mostly
> copied from its_alloc_table_entry().
> 
> Signed-off-by: Zenghui Yu <yuzenghui@huawei.com>
> ---
>  drivers/irqchip/irq-gic-v3-its.c | 68 ++++++++++++++++++++++++++++++++
>  1 file changed, 68 insertions(+)
> 
> diff --git a/drivers/irqchip/irq-gic-v3-its.c 
> b/drivers/irqchip/irq-gic-v3-its.c
> index 0f1fe56ce0af..c1d01454eec8 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -2443,6 +2443,64 @@ static u64 
> inherit_vpe_l1_table_from_rd(cpumask_t **mask)
>  	return 0;
>  }
> 
> +static int allocate_vpe_l1_table_entry(int cpu, u32 id)

Given that this actually allocates the L2 table, I'd rather have it 
called
something like allocate_vpe_l2_table() as the pendant of 
allocate_vpe_l1_table().

This should also properly return a bool rather then an int.

> +{
> +	void __iomem *base = gic_data_rdist_cpu(cpu)->rd_base;
> +	u64 val, gpsz, npg;
> +	unsigned int psz, esz, idx;
> +	struct page *page;
> +	__le64 *table;
> +
> +	if (!gic_rdists->has_rvpeid)
> +		return true;
> +
> +	val  = gits_read_vpropbaser(base + SZ_128K + GICR_VPROPBASER);
> +
> +	esz  = FIELD_GET(GICR_VPROPBASER_4_1_ENTRY_SIZE, val) + 1;
> +	gpsz = FIELD_GET(GICR_VPROPBASER_4_1_PAGE_SIZE, val);
> +	npg  = FIELD_GET(GICR_VPROPBASER_4_1_SIZE, val) + 1;
> +
> +	switch (gpsz) {
> +	default:
> +		WARN_ON(1);
> +		/* 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;
> +	}
> +
> +	/* Don't allow vpe_id that exceeds single, flat table limit */
> +	if (!(val & GICR_VPROPBASER_4_1_INDIRECT))
> +		return (id < (npg * psz / (esz * SZ_8)));
> +
> +	/* Compute 1st level table index & check if that exceeds table limit 
> */
> +	idx = id >> ilog2(psz / (esz * SZ_8));
> +	if (idx >= (npg * psz / GITS_LVL1_ENTRY_SIZE))
> +		return false;
> +
> +	table = gic_data_rdist_cpu(cpu)->vpe_l1_base;
> +
> +	/* Allocate memory for 2nd level table */
> +	if (!table[idx]) {
> +		page = alloc_pages(GFP_KERNEL | __GFP_ZERO, get_order(psz));
> +		if (!page)
> +			return false;
> +
> +		table[idx] = cpu_to_le64(page_to_phys(page) | GITS_BASER_VALID);

I think we may need a cache flushing here in some circumstances. We 
certainly
have it in its_alloc_table_entry.

> +
> +		/* Ensure updated table contents are visible to RD hardware */
> +		dsb(sy);
> +	}
> +
> +	return true;
> +}
> +
>  static int allocate_vpe_l1_table(void)
>  {
>  	void __iomem *vlpi_base = gic_data_rdist_vlpi_base();
> @@ -2957,6 +3015,7 @@ static bool its_alloc_device_table(struct
> its_node *its, u32 dev_id)
>  static bool its_alloc_vpe_table(u32 vpe_id)
>  {
>  	struct its_node *its;
> +	int cpu;
> 
>  	/*
>  	 * Make sure the L2 tables are allocated on *all* v4 ITSs. We
> @@ -2979,6 +3038,15 @@ static bool its_alloc_vpe_table(u32 vpe_id)
>  			return false;
>  	}
> 
> +	/*
> +	 * Make sure the L2 tables are allocated for all copies of
> +	 * the L1 table on *all* v4.1 RDs.
> +	 */
> +	for_each_possible_cpu(cpu) {

not: You could predicate this on gic_rdists->has_rvpeid so that you 
don't
iterate pointlessly on non v4.1 HW.

> +		if (!allocate_vpe_l1_table_entry(cpu, vpe_id))
> +			return false;
> +	}
> +
>  	return true;
>  }

Otherwise, looks good to me.

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