Received-SPF: pass (google.com: domain of linux-kernel+bounces-79565-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Sat, 24 Feb 2024 10:30:04 +0000
Message-ID: <87cysm9mtv.wl-maz@kernel.org>
From: Marc Zyngier <maz@kernel.org>
To: Oliver Upton <oliver.upton@linux.dev>
Cc: Thomas Gleixner <tglx@linutronix.de>,
	Zenghui Yu <yuzenghui@huawei.com>,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org,
	kvmarm@lists.linux.dev,
	Jing Zhang <jingzhangos@google.com>
Subject: Re: [PATCH 2/3] irqchip/gic-v3-its: Spin off GICv4 init into a separate function
In-Reply-To: <20240219185809.286724-3-oliver.upton@linux.dev>
References: <20240219185809.286724-1-oliver.upton@linux.dev>
	<20240219185809.286724-3-oliver.upton@linux.dev>
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)
Precedence: bulk
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII

On Mon, 19 Feb 2024 18:58:07 +0000,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> Burying the GICv4 redistributor initialization into the routine for LPIs
> is a bit confusing, and can lead to sillies where unexpected codepaths
> may not fully initialize the RD.
> 
> Hoist it out of its_cpu_init_lpis() into a dedicated function.
> 
> Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> ---
>  drivers/irqchip/irq-gic-v3-its.c | 32 +++++++++++++++++++++-----------
>  1 file changed, 21 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index 0022852ce494..63d1743f08cc 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -3173,8 +3173,25 @@ static void its_cpu_init_lpis(void)
>  	writel_relaxed(val, rbase + GICR_CTLR);
>  
>  out:
> -	if (gic_rdists->has_vlpis && !gic_rdists->has_rvpeid) {
> +	/* Make sure the GIC has seen the above */
> +	dsb(sy);

So having hoisted the dsb() here...

> +	gic_data_rdist()->flags |= RD_LOCAL_LPI_ENABLED;
> +	pr_info("GICv3: CPU%d: using %s LPI pending table @%pa\n",
> +		smp_processor_id(),
> +		gic_data_rdist()->flags & RD_LOCAL_PENDTABLE_PREALLOCATED ?
> +		"reserved" : "allocated",
> +		&paddr);
> +}
> +
> +static void its_cpu_init_vlpis(void)
> +{
> +	/* No vLPIs? No problem. */
> +	if (!gic_rdists->has_vlpis)
> +		return;
> +
> +	if (!gic_rdists->has_rvpeid) {
>  		void __iomem *vlpi_base = gic_data_rdist_vlpi_base();
> +		u64 val;
>  
>  		/*
>  		 * It's possible for CPU to receive VLPIs before it is
> @@ -3193,7 +3210,8 @@ static void its_cpu_init_lpis(void)
>  		 * ancient programming gets left in and has possibility of
>  		 * corrupting memory.
>  		 */
> -		val = its_clear_vpend_valid(vlpi_base, 0, 0);
> +		its_clear_vpend_valid(vlpi_base, 0, 0);
> +		return;

I'm not sure about the necessity of this return statement.
allocate_vpe_l1_table() checks for rvpeid already, so it should be
fine to carry on.

>  	}
>  
>  	if (allocate_vpe_l1_table()) {
> @@ -3205,15 +3223,6 @@ static void its_cpu_init_lpis(void)
>  		gic_rdists->has_rvpeid = false;
>  		gic_rdists->has_vlpis = false;
>  	}
> -
> -	/* Make sure the GIC has seen the above */
> -	dsb(sy);

.. we're now missing a dsb affecting the VPE table programming, as we
expect things to take effect immediately.

> -	gic_data_rdist()->flags |= RD_LOCAL_LPI_ENABLED;
> -	pr_info("GICv3: CPU%d: using %s LPI pending table @%pa\n",
> -		smp_processor_id(),
> -		gic_data_rdist()->flags & RD_LOCAL_PENDTABLE_PREALLOCATED ?
> -		"reserved" : "allocated",
> -		&paddr);
>  }
>  
>  static void its_cpu_init_collection(struct its_node *its)
> @@ -5265,6 +5274,7 @@ int its_cpu_init(void)
>  			return ret;
>  
>  		its_cpu_init_lpis();
> +		its_cpu_init_vlpis();
>  		its_cpu_init_collections();
>  	}
>  

I'm otherwise OK with the idea of splitting things up.

Thanks,

	M.

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