Re: [PATCH 2/3] irqchip/gic-v3-its: Spin off GICv4 init into a separate function

[Marc Zyngier]

On Mon, 19 Feb 2024 18:58:07 +0000,
Oliver Upton <oliver.upton@xxxxxxxxx> 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@xxxxxxxxx>
> ---
>  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.