Re: [PATCH] irqchip/gic-v4.1:Check whether indirect table is supported in allocate_vpe_l1_table

[Tangnianyao]

On 1/22/2024 17:00, Marc Zyngier wrote:
> [Fixing the LKML address, which has bits of Stephan's address embedded
> in it...]
>
> On Mon, 22 Jan 2024 16:06:07 +0000,
> Nianyao Tang <tangnianyao@xxxxxxxxxx> wrote:
>> In allocate_vpe_l1_table, when we fail to inherit VPE table from other
>> redistributors or ITSs, and we allocate a new vpe table for current common 
>> affinity field without checking whether indirect table is supported.
>> Let's fix it.
> Is there an actual implementation that doesn't support the indirect
> property for the VPE table? I know this is allowed for consistency
> with the original revision of the architecture, but I never expected
> an actual GICv4.1 implementation to be *that* bad.
>
> If that's the case, I'm a bit puzzled/worried.

I met this problem in a developing implementation and find it's allowed by GIC spec.
In such environment,  in a common affinity field with only redistributors and without
any ITS in it, forcing its_vpe_id_alloc to allocate a large vpeid(like 65000), and there
comes an error message "VPE IRQ allocation failure". It originally comes from
allocate_vpe_l2_table, reading GICR_VPROPBASER with GICR_VPROPBASER_4_1_SIZE=1
and GICR_VPROPBASER_4_1_INDIRECT=0.

>
>> Signed-off-by: Nianyao Tang <tangnianyao@xxxxxxxxxx>
>> ---
>>  drivers/irqchip/irq-gic-v3-its.c   | 28 ++++++++++++++++++++++------
>>  include/linux/irqchip/arm-gic-v3.h |  1 +
>>  2 files changed, 23 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
>> index d097001c1e3e..4146d1e285ec 100644
>> --- a/drivers/irqchip/irq-gic-v3-its.c
>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>> @@ -2836,6 +2836,7 @@ static int allocate_vpe_l1_table(void)
>>  	unsigned int psz = SZ_64K;
>>  	unsigned int np, epp, esz;
>>  	struct page *page;
>> +	bool indirect = false;
> Why the upfront initialisation?
>
>>  
>>  	if (!gic_rdists->has_rvpeid)
>>  		return 0;
>> @@ -2890,6 +2891,12 @@ static int allocate_vpe_l1_table(void)
>>  		break;
>>  	}
>>  
>> +	/* probe the indirect */
>> +	val = GICR_VPROPBASER_4_1_INDIRECT;
>> +	gicr_write_vpropbaser(val, vlpi_base + GICR_VPROPBASER);
>> +	val = gicr_read_vpropbaser(vlpi_base + GICR_VPROPBASER);
>> +	indirect = !!(val & GICR_VPROPBASER_4_1_INDIRECT);
> You can probe the indirect bit as part of the page-size probe, no need
> for an extra R/W sequence.
>
>> +
>>  	/*
>>  	 * Start populating the register from scratch, including RO fields
>>  	 * (which we want to print in debug cases...)
>> @@ -2907,15 +2914,24 @@ static int allocate_vpe_l1_table(void)
>>  	 * as indirect and compute the number of required L1 pages.
>>  	 */
>>  	if (epp < ITS_MAX_VPEID) {
>> -		int nl2;
>> +		if (indirect) {
>> +			int nl2;
>>  
>> -		val |= GICR_VPROPBASER_4_1_INDIRECT;
>> +			val |= GICR_VPROPBASER_4_1_INDIRECT;
>>  
>> -		/* Number of L2 pages required to cover the VPEID space */
>> -		nl2 = DIV_ROUND_UP(ITS_MAX_VPEID, epp);
>> +			/* Number of L2 pages required to cover the VPEID space */
>> +			nl2 = DIV_ROUND_UP(ITS_MAX_VPEID, epp);
>>  
>> -		/* Number of L1 pages to point to the L2 pages */
>> -		npg = DIV_ROUND_UP(nl2 * SZ_8, psz);
>> +			/* Number of L1 pages to point to the L2 pages */
>> +			npg = DIV_ROUND_UP(nl2 * SZ_8, psz);
>> +		} else {
>> +			npg = DIV_ROUND_UP(ITS_MAX_VPEID, epp);
>> +			if (npg > GICR_VPROPBASER_PAGES_MAX) {
>> +				pr_warn("GICR_VPROPBASER pages too large, reduce %llu->%u\n",
>> +					npg, GICR_VPROPBASER_PAGES_MAX);
>> +				npg = GICR_VPROPBASER_PAGES_MAX;
>> +			}
>> +		}
>>  	} else {
>>  		npg = 1;
> Why don't you treat the two indirect cases at the same point? It
> really should read:
>
> 	if (epp < ITS_MAX_VPEID && indirect) {
> 		[unchanged]
> 	} else {
> 		[compute the number of L1 pages in the !indirect case]
> 	}
>
>>  	}
>> diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
>> index 728691365464..ace37dfbff20 100644
>> --- a/include/linux/irqchip/arm-gic-v3.h
>> +++ b/include/linux/irqchip/arm-gic-v3.h
>> @@ -303,6 +303,7 @@
>>  #define GICR_VPROPBASER_4_1_Z		(1ULL << 52)
>>  #define GICR_VPROPBASER_4_1_ADDR	GENMASK_ULL(51, 12)
>>  #define GICR_VPROPBASER_4_1_SIZE	GENMASK_ULL(6, 0)
>> +#define GICR_VPROPBASER_PAGES_MAX  128
> Don't hardcode numbers. Use the definition of the SIZE field
> instead. And if you must have a new #define, please use the 4_1
> indication so that it isn't confused with the v4.0 layout.

yeah, I'm also confused when writing this hardcode number, and just follow
the marco GITS_BASER_PAGES_MAX.
I have another question here. The max number of pages  for GITS_BASER
and GICR_VPROPBASER is different here, while GITS_BASER.Size is
bit[7:0] with max 256, and GICR_4_1_VPROPBASER.Size is bit[6:0] with max 128.
Kernel usually probe ITS basers first and then probe GICR_4_1_VPROPBASER in
a common affinity group. Maybe we need to check this in "inherit_vpe_l1_table_from_its" ?

>
> I'd expect something like the following (untested) hack.
>
> 	M.
>
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index 9a7a74239eab..555b86f375e1 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -2836,6 +2836,7 @@ static int allocate_vpe_l1_table(void)
>  	unsigned int psz = SZ_64K;
>  	unsigned int np, epp, esz;
>  	struct page *page;
> +	bool indirect;
>  
>  	if (!gic_rdists->has_rvpeid)
>  		return 0;
> @@ -2870,10 +2871,12 @@ static int allocate_vpe_l1_table(void)
>  
>  	/* First probe the page size */
>  	val = FIELD_PREP(GICR_VPROPBASER_4_1_PAGE_SIZE, GIC_PAGE_SIZE_64K);
> +	val |= GICR_VPROPBASER_4_1_INDIRECT;
>  	gicr_write_vpropbaser(val, vlpi_base + GICR_VPROPBASER);
>  	val = gicr_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);
> +	indirect = !!(val & GICR_VPROPBASER_4_1_INDIRECT);
>  
>  	switch (gpsz) {
>  	default:
> @@ -2906,7 +2909,7 @@ static int allocate_vpe_l1_table(void)
>  	 * 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) {
> +	if (epp < ITS_MAX_VPEID && indirect) {
>  		int nl2;
>  
>  		val |= GICR_VPROPBASER_4_1_INDIRECT;
> @@ -2917,7 +2920,8 @@ static int allocate_vpe_l1_table(void)
>  		/* Number of L1 pages to point to the L2 pages */
>  		npg = DIV_ROUND_UP(nl2 * SZ_8, psz);
>  	} else {
> -		npg = 1;
> +		npg = DIV_ROUND_UP(ITS_MAX_VPEID, epp);
> +		npg = clamp_val(npg, 1, (GICR_VPROPBASER_4_1_SIZE + 1));
>  	}
>  
>  	val |= FIELD_PREP(GICR_VPROPBASER_4_1_SIZE, npg - 1);
>
I've tested your patch and it's ok and it can solve my problem.