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);
+	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;
 	}
 
 	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);
-	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();
 	}
 
-- 
2.44.0.rc0.258.g7320e95886-goog

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.

On Sat, Feb 24, 2024 at 10:30:04AM +0000, Marc Zyngier wrote:
> On Mon, 19 Feb 2024 18:58:07 +0000, Oliver Upton <oliver.upton@linux.dev> wrote:
> > @@ -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.

Yup, definitely not necessary. My aim was to have the control flow make
it a bit more obvious to the reader what's going on.

Having what reads as an allocation helper do a feature check isn't
entirely obvious.

I have no opinion either way though.

> >  	}
> >  
> >  	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.

LOL, and on the back of a bugfix no less. I'll fix this.

-- 
Thanks,
Oliver

On Sat, 24 Feb 2024 11:02:40 +0000,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> On Sat, Feb 24, 2024 at 10:30:04AM +0000, Marc Zyngier wrote:
> > On Mon, 19 Feb 2024 18:58:07 +0000, Oliver Upton <oliver.upton@linux.dev> wrote:
> > > @@ -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.
> 
> Yup, definitely not necessary. My aim was to have the control flow make
> it a bit more obvious to the reader what's going on.
> 
> Having what reads as an allocation helper do a feature check isn't
> entirely obvious.
> 
> I have no opinion either way though.

You could move the if (allocate_vpe_l1_table()) as an 'else' branch,
as the two are mutually exclusive.

Thanks,

	M.

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