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

[Marc Zyngier]

[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.

> 
> 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.

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);

-- 
Without deviation from the norm, progress is not possible.