From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 74B2239AC3 for ; Mon, 22 Jan 2024 09:00:27 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1705914027; cv=none; b=NiK03DKGyYDQ3FdMA4uU2UK7NLgdPFgS5EPS14SqaOrL2WEZqjrIRHMYe0CEsF1uT08ycCD0AyZo2P5bLMv7oTQuvUxcLUgK37QJGe1fkhd6Tpuc6fq7n5mCp/fjk4zi0DrfauFmGaUegrnuW4DbxTapJjzAB4IKcVNdvB/PNX4= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1705914027; c=relaxed/simple; bh=M1TqWzuYGMAOlVXyny/8l/psSIQooyQf8WaHwuG26Yw=; h=Date:Message-ID:From:To:Cc:Subject:In-Reply-To:References: MIME-Version:Content-Type; b=unBaJ6j4C9xVgtJwXhap6Qk2XxsBvHJrcPDgVH2fneNKvhEL2iiOWmsmj6oCioX2b4D4pYaD2W9fSC47GM+zOXJ94qexgYCM6CqPuzL9QTuUZZLi/h9lAKr+5+fIefgi3q5wqKJadH5HzsrwhvKyYbdaYnF+16MytzW44H+w4tQ= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=d+PZDNlF; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="d+PZDNlF" Received: by smtp.kernel.org (Postfix) with ESMTPSA id E017CC433F1; Mon, 22 Jan 2024 09:00:26 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1705914026; bh=M1TqWzuYGMAOlVXyny/8l/psSIQooyQf8WaHwuG26Yw=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=d+PZDNlFc1l85hemBexvjACkEmOojaPjpK70bZ9KHaY0roQa2SNNsbsk9Ei5fbSRG ARqZbVCWV+qkFrzrF3uTL3AgqDI1ZSZa69SSYiRQBmfc6A8t3MYGrVOckGebuRICUj PgoYzwqqDijSEw+I4E1FWlNIO/QZL0QPMMihga5q94vQAp3sykPkPFNYRKcrPPp5hB H+tH8CqhSfICT3mKSgevAlgIsToncFISumUQw2lXt81+U1jzstB5+55ZmBHTOBttNQ WP/bnDayS467ZXY4kdiIfxQjV0jp4/2PpX4Jdlj835/b/yc8bUKajhLQ7GTRlssNhe b7Te9qVKi3B6g== Received: from sofa.misterjones.org ([185.219.108.64] helo=goblin-girl.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.95) (envelope-from ) id 1rRqAJ-00DVPJ-58; Mon, 22 Jan 2024 09:00:24 +0000 Date: Mon, 22 Jan 2024 09:00:22 +0000 Message-ID: <86sf2p91zt.wl-maz@kernel.org> From: Marc Zyngier To: Nianyao Tang Cc: , , , , Subject: Re: [PATCH] irqchip/gic-v4.1:Check whether indirect table is supported in allocate_vpe_l1_table In-Reply-To: <20240122160607.1078960-1-tangnianyao@huawei.com> References: <20240122160607.1078960-1-tangnianyao@huawei.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/29.1 (aarch64-unknown-linux-gnu) MULE/6.0 (HANACHIRUSATO) Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=US-ASCII X-SA-Exim-Connect-IP: 185.219.108.64 X-SA-Exim-Rcpt-To: tangnianyao@huawei.com, tglx@linutronix.de, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, guoyang2@huawei.com, wangwudi@hisilicon.com X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false [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 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 > --- > 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.