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

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@huawei.com> 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@huawei.com>
> ---
>  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.

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@huawei.com> 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@huawei.com>
>> ---
>>  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.

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

[Marc Zyngier]

On Mon, 22 Jan 2024 13:13:09 +0000,
Tangnianyao <tangnianyao@huawei.com> wrote:
>
> 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@huawei.com> 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.

Really, you should get your HW engineers to fix their GIC
implementation.  I'm OK with working around this issue for
completeness, but shipping such an implementation would be a mistake.

[...]

> 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" ?

This is because GITS_BASER[] is generic (also works for devices and
collections), while GICR_VPROPBASER is tailored to the VPE table which
is usually smaller.

I would expect that GICD_TYPER2.VID reports something that cannot
result in something going wrong (in this case, the L1 allocation
cannot be more than 128 pages).

Overall, the kernel isn't a validation suite for the HW, and we expect
it to have some level of sanity. So if none of this is in shipping HW
but only in some model with crazy parameters, I don't think we should
go out of our way to support it.

Thanks,

	M.

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

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

[Tangnianyao]

On 1/22/2024 22:02, Marc Zyngier wrote:
> On Mon, 22 Jan 2024 13:13:09 +0000,
> Tangnianyao <tangnianyao@huawei.com> wrote:
>> 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@huawei.com> 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.
> Really, you should get your HW engineers to fix their GIC
> implementation.  I'm OK with working around this issue for
> completeness, but shipping such an implementation would be a mistake.
>
> [...]
>
>> 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" ?
> This is because GITS_BASER[] is generic (also works for devices and
> collections), while GICR_VPROPBASER is tailored to the VPE table which
> is usually smaller.
>
> I would expect that GICD_TYPER2.VID reports something that cannot
> result in something going wrong (in this case, the L1 allocation
> cannot be more than 128 pages).
>
> Overall, the kernel isn't a validation suite for the HW, and we expect
> it to have some level of sanity. So if none of this is in shipping HW
> but only in some model with crazy parameters, I don't think we should
> go out of our way to support it.
>
> Thanks,
>
> 	M.
>

Hi Marc,
Friendly ping. Do we have plan to fix this problem on kernel, or any other plan ?

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

[Marc Zyngier]

On Wed, 15 May 2024 09:56:10 +0100,
Tangnianyao <tangnianyao@huawei.com> wrote:
> 
> 
> 
> On 1/22/2024 22:02, Marc Zyngier wrote:
> > On Mon, 22 Jan 2024 13:13:09 +0000,
> > Tangnianyao <tangnianyao@huawei.com> wrote:
> >> 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@huawei.com> 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.
> > Really, you should get your HW engineers to fix their GIC
> > implementation.  I'm OK with working around this issue for
> > completeness, but shipping such an implementation would be a mistake.
> >
> > [...]
> >
> >> 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" ?
> > This is because GITS_BASER[] is generic (also works for devices and
> > collections), while GICR_VPROPBASER is tailored to the VPE table which
> > is usually smaller.
> >
> > I would expect that GICD_TYPER2.VID reports something that cannot
> > result in something going wrong (in this case, the L1 allocation
> > cannot be more than 128 pages).
> >
> > Overall, the kernel isn't a validation suite for the HW, and we expect
> > it to have some level of sanity. So if none of this is in shipping HW
> > but only in some model with crazy parameters, I don't think we should
> > go out of our way to support it.
> >
> > Thanks,
> >
> > 	M.
> >
> 
> Hi Marc,
> Friendly ping. Do we have plan to fix this problem on kernel, or any other plan ?

Hi Nianyao,

My earlier question still stand: is this something that affects a
shipping implementation? If not, then I don't think we should support
this upstream, as this doesn't seem like a realistic configuration.

If your employer has actually built this (which I still consider as a
mistake), then we can add the workaround I suggested.

Thanks,

	M.

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

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

[Tangnianyao]

On 5/15/2024 17:34, Marc Zyngier wrote:
> On Wed, 15 May 2024 09:56:10 +0100,
> Tangnianyao <tangnianyao@huawei.com> wrote:
>>
>>
>> On 1/22/2024 22:02, Marc Zyngier wrote:
>>> On Mon, 22 Jan 2024 13:13:09 +0000,
>>> Tangnianyao <tangnianyao@huawei.com> wrote:
>>>> 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@huawei.com> 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.
>>> Really, you should get your HW engineers to fix their GIC
>>> implementation.  I'm OK with working around this issue for
>>> completeness, but shipping such an implementation would be a mistake.
>>>
>>> [...]
>>>
>>>> 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" ?
>>> This is because GITS_BASER[] is generic (also works for devices and
>>> collections), while GICR_VPROPBASER is tailored to the VPE table which
>>> is usually smaller.
>>>
>>> I would expect that GICD_TYPER2.VID reports something that cannot
>>> result in something going wrong (in this case, the L1 allocation
>>> cannot be more than 128 pages).
>>>
>>> Overall, the kernel isn't a validation suite for the HW, and we expect
>>> it to have some level of sanity. So if none of this is in shipping HW
>>> but only in some model with crazy parameters, I don't think we should
>>> go out of our way to support it.
>>>
>>> Thanks,
>>>
>>> 	M.
>>>
>> Hi Marc,
>> Friendly ping. Do we have plan to fix this problem on kernel, or any other plan ?
> Hi Nianyao,
>
> My earlier question still stand: is this something that affects a
> shipping implementation? If not, then I don't think we should support
> this upstream, as this doesn't seem like a realistic configuration.
>
> If your employer has actually built this (which I still consider as a
> mistake), then we can add the workaround I suggested.
>
> Thanks,
>
> 	M.
>
Hi Marc,

For GIC4.1 of HIP09,HIP10,HIP10C, it only support one-level vcpu table and GITS_BASER<n>.Indirect
is RAZ/WI. It implements page size 16KB and entry size 32B,  each page support 512 vpe, and our
clients have requirement 1 or 2 pages at most, so it just supports flat page to simplify implementation.

Our designer has confirmed with ARM architect that we can waive this rule:
Quote from GIC spec 12.19.1 GITS_BASER<n> description, if the maximum width of the scaling factor
that is identified by GITS_BASER<n>.Type and the smallest page size that is supported result in a single
level table that requires multiple pages, then implementing this bit as RAZ/WI is DEPRECATED.

We have actually built this in HIP09,HIP10,HIP10C and would like to fix this in kernel.
Can we merge the above bug-fix patch to kernel?  Or we need to fix with errata for HIP09,HIP10,HIP10C?

thanks for your opinions

Nianyao Tang

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

[Marc Zyngier]

On Thu, 18 Sep 2025 03:56:04 +0100,
Tangnianyao <tangnianyao@huawei.com> wrote:
> 
> 
> 
> On 5/15/2024 17:34, Marc Zyngier wrote:
> > On Wed, 15 May 2024 09:56:10 +0100,
> > Tangnianyao <tangnianyao@huawei.com> wrote:
> >>
> >>
> >> On 1/22/2024 22:02, Marc Zyngier wrote:
> >>> On Mon, 22 Jan 2024 13:13:09 +0000,
> >>> Tangnianyao <tangnianyao@huawei.com> wrote:
> >>>> 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@huawei.com> 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.
> >>> Really, you should get your HW engineers to fix their GIC
> >>> implementation.  I'm OK with working around this issue for
> >>> completeness, but shipping such an implementation would be a mistake.
> >>>
> >>> [...]
> >>>
> >>>> 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" ?
> >>> This is because GITS_BASER[] is generic (also works for devices and
> >>> collections), while GICR_VPROPBASER is tailored to the VPE table which
> >>> is usually smaller.
> >>>
> >>> I would expect that GICD_TYPER2.VID reports something that cannot
> >>> result in something going wrong (in this case, the L1 allocation
> >>> cannot be more than 128 pages).
> >>>
> >>> Overall, the kernel isn't a validation suite for the HW, and we expect
> >>> it to have some level of sanity. So if none of this is in shipping HW
> >>> but only in some model with crazy parameters, I don't think we should
> >>> go out of our way to support it.
> >>>
> >>> Thanks,
> >>>
> >>> 	M.
> >>>
> >> Hi Marc,
> >> Friendly ping. Do we have plan to fix this problem on kernel, or any other plan ?
> > Hi Nianyao,
> >
> > My earlier question still stand: is this something that affects a
> > shipping implementation? If not, then I don't think we should support
> > this upstream, as this doesn't seem like a realistic configuration.
> >
> > If your employer has actually built this (which I still consider as a
> > mistake), then we can add the workaround I suggested.
> >
> > Thanks,
> >
> > 	M.
> >
> Hi Marc,
> 
> For GIC4.1 of HIP09,HIP10,HIP10C, it only support one-level vcpu table and GITS_BASER<n>.Indirect
> is RAZ/WI. It implements page size 16KB and entry size 32B,  each page support 512 vpe, and our
> clients have requirement 1 or 2 pages at most, so it just supports flat page to simplify implementation.
> 
> Our designer has confirmed with ARM architect that we can waive this rule:
> Quote from GIC spec 12.19.1 GITS_BASER<n> description, if the maximum width of the scaling factor
> that is identified by GITS_BASER<n>.Type and the smallest page size that is supported result in a single
> level table that requires multiple pages, then implementing this bit
> as RAZ/WI is DEPRECATED.

I can read the spec just as well. Doesn't make it a good option.

> We have actually built this in HIP09,HIP10,HIP10C and would like to
> fix this in kernel.

Isn't that the broken systems that can't even do vSGIs properly?

> Can we merge the above bug-fix patch to kernel?  Or we need to fix with errata for HIP09,HIP10,HIP10C?

Frankly, I've completely lost track of what this patch is doing. This
has been going on for over 18 months, and you have been silent on the
subject for over a year. What do you expect?

If you want anything to be done on this front, please repost a patch,
and we'll review it again.

	M.

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

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

[Tangnianyao]

On 9/18/2025 17:50, Marc Zyngier wrote:
> On Thu, 18 Sep 2025 03:56:04 +0100,
> Tangnianyao <tangnianyao@huawei.com> wrote:
>>
>>
>> On 5/15/2024 17:34, Marc Zyngier wrote:
>>> On Wed, 15 May 2024 09:56:10 +0100,
>>> Tangnianyao <tangnianyao@huawei.com> wrote:
>>>>
>>>> On 1/22/2024 22:02, Marc Zyngier wrote:
>>>>> On Mon, 22 Jan 2024 13:13:09 +0000,
>>>>> Tangnianyao <tangnianyao@huawei.com> wrote:
>>>>>> 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@huawei.com> 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.
>>>>> Really, you should get your HW engineers to fix their GIC
>>>>> implementation.  I'm OK with working around this issue for
>>>>> completeness, but shipping such an implementation would be a mistake.
>>>>>
>>>>> [...]
>>>>>
>>>>>> 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" ?
>>>>> This is because GITS_BASER[] is generic (also works for devices and
>>>>> collections), while GICR_VPROPBASER is tailored to the VPE table which
>>>>> is usually smaller.
>>>>>
>>>>> I would expect that GICD_TYPER2.VID reports something that cannot
>>>>> result in something going wrong (in this case, the L1 allocation
>>>>> cannot be more than 128 pages).
>>>>>
>>>>> Overall, the kernel isn't a validation suite for the HW, and we expect
>>>>> it to have some level of sanity. So if none of this is in shipping HW
>>>>> but only in some model with crazy parameters, I don't think we should
>>>>> go out of our way to support it.
>>>>>
>>>>> Thanks,
>>>>>
>>>>> 	M.
>>>>>
>>>> Hi Marc,
>>>> Friendly ping. Do we have plan to fix this problem on kernel, or any other plan ?
>>> Hi Nianyao,
>>>
>>> My earlier question still stand: is this something that affects a
>>> shipping implementation? If not, then I don't think we should support
>>> this upstream, as this doesn't seem like a realistic configuration.
>>>
>>> If your employer has actually built this (which I still consider as a
>>> mistake), then we can add the workaround I suggested.
>>>
>>> Thanks,
>>>
>>> 	M.
>>>
>> Hi Marc,
>>
>> For GIC4.1 of HIP09,HIP10,HIP10C, it only support one-level vcpu table and GITS_BASER<n>.Indirect
>> is RAZ/WI. It implements page size 16KB and entry size 32B,  each page support 512 vpe, and our
>> clients have requirement 1 or 2 pages at most, so it just supports flat page to simplify implementation.
>>
>> Our designer has confirmed with ARM architect that we can waive this rule:
>> Quote from GIC spec 12.19.1 GITS_BASER<n> description, if the maximum width of the scaling factor
>> that is identified by GITS_BASER<n>.Type and the smallest page size that is supported result in a single
>> level table that requires multiple pages, then implementing this bit
>> as RAZ/WI is DEPRECATED.
> I can read the spec just as well. Doesn't make it a good option.
It maybe a good option for clients that do not use many vpe?
So we try to convinced ARM to waive this rule and received a positive response.

>> We have actually built this in HIP09,HIP10,HIP10C and would like to
>> fix this in kernel.
> Isn't that the broken systems that can't even do vSGIs properly?
>
>> Can we merge the above bug-fix patch to kernel?  Or we need to fix with errata for HIP09,HIP10,HIP10C?
> Frankly, I've completely lost track of what this patch is doing. This
> has been going on for over 18 months, and you have been silent on the
> subject for over a year. What do you expect?
>
> If you want anything to be done on this front, please repost a patch,
> and we'll review it again.
>
> 	M.
>
We have tested the last patch in our internal build and now aim to fix the open-source
kernel to support our SoC. We hope to address this either through a community patch or an errata fix.

I will repost the patch shortly. Thanks for your review.

Nianyao Tang