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:   Fri, 09 Jun 2023 14:10:34 +0100
Message-ID: <871qikok6t.wl-maz@kernel.org>
From:   Marc Zyngier <maz@kernel.org>
To:     wangwudi <wangwudi@hisilicon.com>
Cc:     <linux-kernel@vger.kernel.org>,
        "liaochang (A)" <liaochang1@huawei.com>,
        Thomas Gleixner <tglx@linutronix.de>
Subject: Re: [PATCH v2] irqchip: gic-v3: Extend collection table
In-Reply-To: <ec424c75-cce8-8828-18a7-1314d94e1875@hisilicon.com>
References: <1686131113-3611-1-git-send-email-wangwudi@hisilicon.com>
        <87cz26nzm7.wl-maz@kernel.org>
        <82ea3d910d104fbb8df9b27585085895@hisilicon.com>
        <ec424c75-cce8-8828-18a7-1314d94e1875@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=UTF-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Fri, 09 Jun 2023 11:02:04 +0100,
wangwudi <wangwudi@hisilicon.com> wrote:
>=20
> Hi Marc,
>=20
> =E5=9C=A8 2023/6/9 17:24, wangwudi =E5=86=99=E9=81=93:
> >=20
> >=20
> > -----=E9=82=AE=E4=BB=B6=E5=8E=9F=E4=BB=B6-----
> > =E5=8F=91=E4=BB=B6=E4=BA=BA: Marc Zyngier [mailto:maz@kernel.org]=20
> > =E5=8F=91=E9=80=81=E6=97=B6=E9=97=B4: 2023=E5=B9=B46=E6=9C=888=E6=97=A5=
 16:10
> > =E6=94=B6=E4=BB=B6=E4=BA=BA: wangwudi <wangwudi@hisilicon.com>
> > =E6=8A=84=E9=80=81: linux-kernel@vger.kernel.org; liaochang (A) <liaoch=
ang1@huawei.com>; Thomas Gleixner <tglx@linutronix.de>
> > =E4=B8=BB=E9=A2=98: Re: [PATCH v2] irqchip: gic-v3: Extend collection t=
able
> >=20
> > 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=
=20
> >> one page is allocated.
> >>
> >> Extend collection table to support more CPUs:
> >> 1. Recalculate the page number of collection table based on the number=
=20
> >> 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=20
> >> 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)->typ=
er) + 1)
> >> =20
> >>  #define ITS_ITT_ALIGN		SZ_256
> >> =20
> >> @@ -2626,6 +2627,10 @@ static int its_alloc_tables(struct its_node *it=
s)
> >>  			indirect =3D its_parse_indirect_baser(its, baser, &order,
> >>  							    ITS_MAX_VPEID_BITS);
> >>  			break;
> >> +		case GITS_BASER_TYPE_COLLECTION:
> >> +			indirect =3D its_parse_indirect_baser(its, baser, &order,
> >> +							    order_base_2(num_possible_cpus()));
> >> +			break;
> >=20
> > Nice try, but no. See below.
> >=20
> >>  		}
> >> =20
> >>  		err =3D its_setup_baser(its, baser, cache, shr, order, indirect); @=
@=20
> >> -3230,18 +3235,6 @@ static void its_cpu_init_collection(struct its_nod=
e *its)
> >>  	its_send_invall(its, &its->collections[cpu]);  }
> >> =20
> >> -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=20
> >> dev_id)  {
> >>  	struct its_device *its_dev =3D NULL, *tmp; @@ -3316,6 +3309,51 @@=20
> >> static bool its_alloc_table_entry(struct its_node *its,
> >>  	return true;
> >>  }
> >> =20
> >> +static bool its_alloc_collection_table(struct its_node *its, struct=20
> >> +its_baser *baser) {
> >> +	int cpu =3D smp_processor_id();
> >> +	int cpu_ids =3D 16;
> >> +
> >> +	if (its->typer & GITS_TYPER_CIL)
> >> +		cpu_ids =3D collection_ids(its);
> >> +
> >> +	if (!(ilog2(cpu) < cpu_ids)) {
> >> +		pr_warn("ITS: CPU%d out of Collection ID range for %dbits", cpu, cp=
u_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 =3D its_get_baser(its, GITS_BASER_TYPE_COLLECTION);
> >> +		if (!baser) {
> >> +			raw_spin_unlock(&its_lock);
> >> +			return false;
> >> +		}
> >=20
> > 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.
> >=20
> > So this patch more or less *guarantees* to break most
> > implementation that are more than 5 year old.
> >
>=20
> For the collection table, if the HCC field is not zero, neither
> l1-table nor l2-table table is allocated. How do you think?

What do I think? I've already said what I think, right under the 4
lines of code that break anything with a GIC500.

	M.

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