Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 08 Jun 2023 09:10:24 +0100
Message-ID: <87cz26nzm7.wl-maz@kernel.org>
From:   Marc Zyngier <maz@kernel.org>
To:     wangwudi <wangwudi@hisilicon.com>
Cc:     <linux-kernel@vger.kernel.org>, <liaochang1@huawei.com>,
        Thomas Gleixner <tglx@linutronix.de>
Subject: Re: [PATCH v2] irqchip: gic-v3: Extend collection table
In-Reply-To: <1686131113-3611-1-git-send-email-wangwudi@hisilicon.com>
References: <1686131113-3611-1-git-send-email-wangwudi@hisilicon.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue)
 FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/28.2
 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Wed, 07 Jun 2023 10:45:13 +0100,
wangwudi <wangwudi@hisilicon.com> wrote:
> 
> Only single level table is supported to the collection table, and only
> one page is allocated.
> 
> Extend collection table to support more CPUs:
> 1. Recalculate the page number of collection table based on the number of
> CPUs.
> 2. Add 2 level tables to collection table.
> 3. Add GITS_TYPER_CIDBITS macros.
> 
> It is noticed in an internal simulation research:
> - the page_size of collection table is 4 KB
> - the entry_size of collection table is	 16 Byte
> - with 512 CPUs
> 
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Marc Zyngier <maz@kernel.org>
> Signed-off-by: wangwudi <wangwudi@hisilicon.com>
> ---
> 
> ChangeLog:
> v1-->v2:
> 	1. Support 2 level table
> 	2. Rewrite the commit log
> 
>  drivers/irqchip/irq-gic-v3-its.c   | 62 ++++++++++++++++++++++++++++++--------
>  include/linux/irqchip/arm-gic-v3.h |  3 ++
>  2 files changed, 53 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index 0ec2b1e1df75..573ef26ad449 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -126,6 +126,7 @@ struct its_node {
>  #define is_v4(its)		(!!((its)->typer & GITS_TYPER_VLPIS))
>  #define is_v4_1(its)		(!!((its)->typer & GITS_TYPER_VMAPP))
>  #define device_ids(its)		(FIELD_GET(GITS_TYPER_DEVBITS, (its)->typer) + 1)
> +#define collection_ids(its)	(FIELD_GET(GITS_TYPER_CIDBITS, (its)->typer) + 1)
>  
>  #define ITS_ITT_ALIGN		SZ_256
>  
> @@ -2626,6 +2627,10 @@ static int its_alloc_tables(struct its_node *its)
>  			indirect = its_parse_indirect_baser(its, baser, &order,
>  							    ITS_MAX_VPEID_BITS);
>  			break;
> +		case GITS_BASER_TYPE_COLLECTION:
> +			indirect = its_parse_indirect_baser(its, baser, &order,
> +							    order_base_2(num_possible_cpus()));
> +			break;

Nice try, but no. See below.

>  		}
>  
>  		err = its_setup_baser(its, baser, cache, shr, order, indirect);
> @@ -3230,18 +3235,6 @@ static void its_cpu_init_collection(struct its_node *its)
>  	its_send_invall(its, &its->collections[cpu]);
>  }
>  
> -static void its_cpu_init_collections(void)
> -{
> -	struct its_node *its;
> -
> -	raw_spin_lock(&its_lock);
> -
> -	list_for_each_entry(its, &its_nodes, entry)
> -		its_cpu_init_collection(its);
> -
> -	raw_spin_unlock(&its_lock);
> -}
> -
>  static struct its_device *its_find_device(struct its_node *its, u32 dev_id)
>  {
>  	struct its_device *its_dev = NULL, *tmp;
> @@ -3316,6 +3309,51 @@ static bool its_alloc_table_entry(struct its_node *its,
>  	return true;
>  }
>  
> +static bool its_alloc_collection_table(struct its_node *its, struct its_baser *baser)
> +{
> +	int cpu = smp_processor_id();
> +	int cpu_ids = 16;
> +
> +	if (its->typer & GITS_TYPER_CIL)
> +		cpu_ids = collection_ids(its);
> +
> +	if (!(ilog2(cpu) < cpu_ids)) {
> +		pr_warn("ITS: CPU%d out of Collection ID range for %dbits", cpu, cpu_ids);
> +		return false;
> +	}
> +
> +	if (!its_alloc_table_entry(its, baser, cpu)) {
> +		pr_warn("ITS: CPU%d failed to allocate collection l2 table", cpu);
> +		return false;
> +	}
> +
> +	return true;
> +}
> +
> +static bool its_cpu_init_collections(void)
> +{
> +	struct its_node *its;
> +	struct its_baser *baser;
> +
> +	raw_spin_lock(&its_lock);
> +
> +	list_for_each_entry(its, &its_nodes, entry) {
> +		baser = its_get_baser(its, GITS_BASER_TYPE_COLLECTION);
> +		if (!baser) {
> +			raw_spin_unlock(&its_lock);
> +			return false;
> +		}

This looks wrong. ITSs that have a non-zero HCC field may not need
memory to back their collections at all, such as GIC500. There may not
even be a BASERn register holding the memory.

So this patch more or less *guarantees* to break most implementation
that are more than 5 year old.

	M.

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