[PATCH 0/3] irqchip/gic-v3: Assorted fixes and improvements

[PATCH 0/3] irqchip/gic-v3: Assorted fixes and improvements

[Marc Zyngier]

This is a small series that happens on the back of [1], which
implements additional features in KVM's vgic.

The first patch fixes a pretty embarrassing bug, which is fortunately
unlikely to cause any issue in real life. The next two patches are
performance optimisations.

Thanks,

        M.

[1] https://lore.kernel.org/r/20220314164044.772709-1-maz@kernel.org

Marc Zyngier (3):
  irqchip/gic-v3: Fix GICR_CTLR.RWP polling
  irqchip/gic-v3: Detect LPI invalidation MMIO registers
  irqchip/gic-v3: Relax polling of GIC{R,D}_CTLR.RWP

 drivers/irqchip/irq-gic-v3.c       | 51 +++++++++++++++---------------
 include/linux/irqchip/arm-gic-v3.h |  2 ++
 2 files changed, 28 insertions(+), 25 deletions(-)

-- 
2.34.1

[PATCH 1/3] irqchip/gic-v3: Fix GICR_CTLR.RWP polling

[Marc Zyngier]

It turns out that our polling of RWP is totally wrong when checking
for it in the redistributors, as we test the *distributor* bit index,
whereas it is a different bit number in the RDs... Oopsie boo.

This is embarassing. Not only because it is wrong, but also because
it took *8 years* to notice the blunder...

Just fix the damn thing.

Fixes: 021f653791ad ("irqchip: gic-v3: Initial support for GICv3")
Signed-off-by: Marc Zyngier <maz@kernel.org>
Cc: stable@vger.kernel.org
---
 drivers/irqchip/irq-gic-v3.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index 5e935d97207d..736163d36b13 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -206,11 +206,11 @@ static inline void __iomem *gic_dist_base(struct irq_data *d)
 	}
 }
 
-static void gic_do_wait_for_rwp(void __iomem *base)
+static void gic_do_wait_for_rwp(void __iomem *base, u32 bit)
 {
 	u32 count = 1000000;	/* 1s! */
 
-	while (readl_relaxed(base + GICD_CTLR) & GICD_CTLR_RWP) {
+	while (readl_relaxed(base + GICD_CTLR) & bit) {
 		count--;
 		if (!count) {
 			pr_err_ratelimited("RWP timeout, gone fishing\n");
@@ -224,13 +224,13 @@ static void gic_do_wait_for_rwp(void __iomem *base)
 /* Wait for completion of a distributor change */
 static void gic_dist_wait_for_rwp(void)
 {
-	gic_do_wait_for_rwp(gic_data.dist_base);
+	gic_do_wait_for_rwp(gic_data.dist_base, GICD_CTLR_RWP);
 }
 
 /* Wait for completion of a redistributor change */
 static void gic_redist_wait_for_rwp(void)
 {
-	gic_do_wait_for_rwp(gic_data_rdist_rd_base());
+	gic_do_wait_for_rwp(gic_data_rdist_rd_base(), GICR_CTLR_RWP);
 }
 
 #ifdef CONFIG_ARM64
-- 
2.34.1

Re: [PATCH 1/3] irqchip/gic-v3: Fix GICR_CTLR.RWP polling

[Andre Przywara]

On Tue, 15 Mar 2022 16:50:32 +0000
Marc Zyngier <maz@kernel.org> wrote:

> It turns out that our polling of RWP is totally wrong when checking
> for it in the redistributors, as we test the *distributor* bit index,
> whereas it is a different bit number in the RDs... Oopsie boo.
> 
> This is embarassing. Not only because it is wrong, but also because
> it took *8 years* to notice the blunder...

Indeed, I wonder why we didn't see issues before. I guess it's either
the UWP bit at position GICR_CTLR[31] having a similar implementation,
or the MMIO access alone providing enough delay for the writes to
finish.

Anyway:

> Just fix the damn thing.
> 
> Fixes: 021f653791ad ("irqchip: gic-v3: Initial support for GICv3")
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> Cc: stable@vger.kernel.org

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre


> ---
>  drivers/irqchip/irq-gic-v3.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> index 5e935d97207d..736163d36b13 100644
> --- a/drivers/irqchip/irq-gic-v3.c
> +++ b/drivers/irqchip/irq-gic-v3.c
> @@ -206,11 +206,11 @@ static inline void __iomem *gic_dist_base(struct irq_data *d)
>  	}
>  }
>  
> -static void gic_do_wait_for_rwp(void __iomem *base)
> +static void gic_do_wait_for_rwp(void __iomem *base, u32 bit)
>  {
>  	u32 count = 1000000;	/* 1s! */
>  
> -	while (readl_relaxed(base + GICD_CTLR) & GICD_CTLR_RWP) {
> +	while (readl_relaxed(base + GICD_CTLR) & bit) {
>  		count--;
>  		if (!count) {
>  			pr_err_ratelimited("RWP timeout, gone fishing\n");
> @@ -224,13 +224,13 @@ static void gic_do_wait_for_rwp(void __iomem *base)
>  /* Wait for completion of a distributor change */
>  static void gic_dist_wait_for_rwp(void)
>  {
> -	gic_do_wait_for_rwp(gic_data.dist_base);
> +	gic_do_wait_for_rwp(gic_data.dist_base, GICD_CTLR_RWP);
>  }
>  
>  /* Wait for completion of a redistributor change */
>  static void gic_redist_wait_for_rwp(void)
>  {
> -	gic_do_wait_for_rwp(gic_data_rdist_rd_base());
> +	gic_do_wait_for_rwp(gic_data_rdist_rd_base(), GICR_CTLR_RWP);
>  }
>  
>  #ifdef CONFIG_ARM64

Re: [PATCH 1/3] irqchip/gic-v3: Fix GICR_CTLR.RWP polling

[Marc Zyngier]

On Wed, 16 Mar 2022 14:51:02 +0000,
Andre Przywara <andre.przywara@arm.com> wrote:
> 
> On Tue, 15 Mar 2022 16:50:32 +0000
> Marc Zyngier <maz@kernel.org> wrote:
> 
> > It turns out that our polling of RWP is totally wrong when checking
> > for it in the redistributors, as we test the *distributor* bit index,
> > whereas it is a different bit number in the RDs... Oopsie boo.
> > 
> > This is embarassing. Not only because it is wrong, but also because
> > it took *8 years* to notice the blunder...
> 
> Indeed, I wonder why we didn't see issues before. I guess it's either
> the UWP bit at position GICR_CTLR[31] having a similar implementation,
> or the MMIO access alone providing enough delay for the writes to
> finish.

Because we don't strictly need to wait. Most of the time, the
write will have taken place long before we can observe any effect of
it. And how often do we disable a SGI or a PPI? Almost never (the PMU
is the only one that I can think of).

> Anyway:
> 
> > Just fix the damn thing.
> > 
> > Fixes: 021f653791ad ("irqchip: gic-v3: Initial support for GICv3")
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > Cc: stable@vger.kernel.org
> 
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Thanks,

	M.

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

Re: [PATCH 1/3] irqchip/gic-v3: Fix GICR_CTLR.RWP polling

[Lorenzo Pieralisi]

On Tue, Mar 15, 2022 at 04:50:32PM +0000, Marc Zyngier wrote:
> It turns out that our polling of RWP is totally wrong when checking
> for it in the redistributors, as we test the *distributor* bit index,
> whereas it is a different bit number in the RDs... Oopsie boo.
> 
> This is embarassing. Not only because it is wrong, but also because
> it took *8 years* to notice the blunder...
> 
> Just fix the damn thing.
> 
> Fixes: 021f653791ad ("irqchip: gic-v3: Initial support for GICv3")
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> Cc: stable@vger.kernel.org
> ---
>  drivers/irqchip/irq-gic-v3.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

Reviewed-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>

> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> index 5e935d97207d..736163d36b13 100644
> --- a/drivers/irqchip/irq-gic-v3.c
> +++ b/drivers/irqchip/irq-gic-v3.c
> @@ -206,11 +206,11 @@ static inline void __iomem *gic_dist_base(struct irq_data *d)
>  	}
>  }
>  
> -static void gic_do_wait_for_rwp(void __iomem *base)
> +static void gic_do_wait_for_rwp(void __iomem *base, u32 bit)
>  {
>  	u32 count = 1000000;	/* 1s! */
>  
> -	while (readl_relaxed(base + GICD_CTLR) & GICD_CTLR_RWP) {
> +	while (readl_relaxed(base + GICD_CTLR) & bit) {
>  		count--;
>  		if (!count) {
>  			pr_err_ratelimited("RWP timeout, gone fishing\n");
> @@ -224,13 +224,13 @@ static void gic_do_wait_for_rwp(void __iomem *base)
>  /* Wait for completion of a distributor change */
>  static void gic_dist_wait_for_rwp(void)
>  {
> -	gic_do_wait_for_rwp(gic_data.dist_base);
> +	gic_do_wait_for_rwp(gic_data.dist_base, GICD_CTLR_RWP);
>  }
>  
>  /* Wait for completion of a redistributor change */
>  static void gic_redist_wait_for_rwp(void)
>  {
> -	gic_do_wait_for_rwp(gic_data_rdist_rd_base());
> +	gic_do_wait_for_rwp(gic_data_rdist_rd_base(), GICR_CTLR_RWP);
>  }
>  
>  #ifdef CONFIG_ARM64
> -- 
> 2.34.1
> 

[irqchip: irq/irqchip-next] irqchip/gic-v3: Fix GICR_CTLR.RWP polling

[irqchip-bot for Marc Zyngier]

The following commit has been merged into the irq/irqchip-next branch of irqchip:

Commit-ID:     c114741827436ad1f1d465f3719f77b996ea6eca
Gitweb:        https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms/c114741827436ad1f1d465f3719f77b996ea6eca
Author:        Marc Zyngier <maz@kernel.org>
AuthorDate:    Tue, 15 Mar 2022 16:50:32 
Committer:     Marc Zyngier <maz@kernel.org>
CommitterDate: Mon, 21 Mar 2022 09:17:13 

irqchip/gic-v3: Fix GICR_CTLR.RWP polling

It turns out that our polling of RWP is totally wrong when checking
for it in the redistributors, as we test the *distributor* bit index,
whereas it is a different bit number in the RDs... Oopsie boo.

This is embarassing. Not only because it is wrong, but also because
it took *8 years* to notice the blunder...

Just fix the damn thing.

Fixes: 021f653791ad ("irqchip: gic-v3: Initial support for GICv3")
Signed-off-by: Marc Zyngier <maz@kernel.org>
Cc: stable@vger.kernel.org
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
Reviewed-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Link: https://lore.kernel.org/r/20220315165034.794482-2-maz@kernel.org
---
 drivers/irqchip/irq-gic-v3.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index 0efe1a9..9b63165 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -206,11 +206,11 @@ static inline void __iomem *gic_dist_base(struct irq_data *d)
 	}
 }
 
-static void gic_do_wait_for_rwp(void __iomem *base)
+static void gic_do_wait_for_rwp(void __iomem *base, u32 bit)
 {
 	u32 count = 1000000;	/* 1s! */
 
-	while (readl_relaxed(base + GICD_CTLR) & GICD_CTLR_RWP) {
+	while (readl_relaxed(base + GICD_CTLR) & bit) {
 		count--;
 		if (!count) {
 			pr_err_ratelimited("RWP timeout, gone fishing\n");
@@ -224,13 +224,13 @@ static void gic_do_wait_for_rwp(void __iomem *base)
 /* Wait for completion of a distributor change */
 static void gic_dist_wait_for_rwp(void)
 {
-	gic_do_wait_for_rwp(gic_data.dist_base);
+	gic_do_wait_for_rwp(gic_data.dist_base, GICD_CTLR_RWP);
 }
 
 /* Wait for completion of a redistributor change */
 static void gic_redist_wait_for_rwp(void)
 {
-	gic_do_wait_for_rwp(gic_data_rdist_rd_base());
+	gic_do_wait_for_rwp(gic_data_rdist_rd_base(), GICR_CTLR_RWP);
 }
 
 #ifdef CONFIG_ARM64

[irqchip: irq/irqchip-next] irqchip/gic-v3: Fix GICR_CTLR.RWP polling

[irqchip-bot for Marc Zyngier]

The following commit has been merged into the irq/irqchip-next branch of irqchip:

Commit-ID:     49cdcea1b0772c29abb9468d82809933c718110e
Gitweb:        https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms/49cdcea1b0772c29abb9468d82809933c718110e
Author:        Marc Zyngier <maz@kernel.org>
AuthorDate:    Tue, 15 Mar 2022 16:50:32 
Committer:     Marc Zyngier <maz@kernel.org>
CommitterDate: Mon, 21 Mar 2022 14:02:44 

irqchip/gic-v3: Fix GICR_CTLR.RWP polling

It turns out that our polling of RWP is totally wrong when checking
for it in the redistributors, as we test the *distributor* bit index,
whereas it is a different bit number in the RDs... Oopsie boo.

This is embarassing. Not only because it is wrong, but also because
it took *8 years* to notice the blunder...

Just fix the damn thing.

Fixes: 021f653791ad ("irqchip: gic-v3: Initial support for GICv3")
Signed-off-by: Marc Zyngier <maz@kernel.org>
Cc: stable@vger.kernel.org
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
Reviewed-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Link: https://lore.kernel.org/r/20220315165034.794482-2-maz@kernel.org
---
 drivers/irqchip/irq-gic-v3.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index 0efe1a9..9b63165 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -206,11 +206,11 @@ static inline void __iomem *gic_dist_base(struct irq_data *d)
 	}
 }
 
-static void gic_do_wait_for_rwp(void __iomem *base)
+static void gic_do_wait_for_rwp(void __iomem *base, u32 bit)
 {
 	u32 count = 1000000;	/* 1s! */
 
-	while (readl_relaxed(base + GICD_CTLR) & GICD_CTLR_RWP) {
+	while (readl_relaxed(base + GICD_CTLR) & bit) {
 		count--;
 		if (!count) {
 			pr_err_ratelimited("RWP timeout, gone fishing\n");
@@ -224,13 +224,13 @@ static void gic_do_wait_for_rwp(void __iomem *base)
 /* Wait for completion of a distributor change */
 static void gic_dist_wait_for_rwp(void)
 {
-	gic_do_wait_for_rwp(gic_data.dist_base);
+	gic_do_wait_for_rwp(gic_data.dist_base, GICD_CTLR_RWP);
 }
 
 /* Wait for completion of a redistributor change */
 static void gic_redist_wait_for_rwp(void)
 {
-	gic_do_wait_for_rwp(gic_data_rdist_rd_base());
+	gic_do_wait_for_rwp(gic_data_rdist_rd_base(), GICR_CTLR_RWP);
 }
 
 #ifdef CONFIG_ARM64

[irqchip: irq/irqchip-fixes] irqchip/gic-v3: Fix GICR_CTLR.RWP polling

[irqchip-bot for Marc Zyngier]

The following commit has been merged into the irq/irqchip-fixes branch of irqchip:

Commit-ID:     0df6664531a12cdd8fc873f0cac0dcb40243d3e9
Gitweb:        https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms/0df6664531a12cdd8fc873f0cac0dcb40243d3e9
Author:        Marc Zyngier <maz@kernel.org>
AuthorDate:    Tue, 15 Mar 2022 16:50:32 
Committer:     Marc Zyngier <maz@kernel.org>
CommitterDate: Tue, 05 Apr 2022 16:33:13 +01:00

irqchip/gic-v3: Fix GICR_CTLR.RWP polling

It turns out that our polling of RWP is totally wrong when checking
for it in the redistributors, as we test the *distributor* bit index,
whereas it is a different bit number in the RDs... Oopsie boo.

This is embarassing. Not only because it is wrong, but also because
it took *8 years* to notice the blunder...

Just fix the damn thing.

Fixes: 021f653791ad ("irqchip: gic-v3: Initial support for GICv3")
Signed-off-by: Marc Zyngier <maz@kernel.org>
Cc: stable@vger.kernel.org
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
Reviewed-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Link: https://lore.kernel.org/r/20220315165034.794482-2-maz@kernel.org
---
 drivers/irqchip/irq-gic-v3.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index 0efe1a9..9b63165 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -206,11 +206,11 @@ static inline void __iomem *gic_dist_base(struct irq_data *d)
 	}
 }
 
-static void gic_do_wait_for_rwp(void __iomem *base)
+static void gic_do_wait_for_rwp(void __iomem *base, u32 bit)
 {
 	u32 count = 1000000;	/* 1s! */
 
-	while (readl_relaxed(base + GICD_CTLR) & GICD_CTLR_RWP) {
+	while (readl_relaxed(base + GICD_CTLR) & bit) {
 		count--;
 		if (!count) {
 			pr_err_ratelimited("RWP timeout, gone fishing\n");
@@ -224,13 +224,13 @@ static void gic_do_wait_for_rwp(void __iomem *base)
 /* Wait for completion of a distributor change */
 static void gic_dist_wait_for_rwp(void)
 {
-	gic_do_wait_for_rwp(gic_data.dist_base);
+	gic_do_wait_for_rwp(gic_data.dist_base, GICD_CTLR_RWP);
 }
 
 /* Wait for completion of a redistributor change */
 static void gic_redist_wait_for_rwp(void)
 {
-	gic_do_wait_for_rwp(gic_data_rdist_rd_base());
+	gic_do_wait_for_rwp(gic_data_rdist_rd_base(), GICR_CTLR_RWP);
 }
 
 #ifdef CONFIG_ARM64

[PATCH 2/3] irqchip/gic-v3: Detect LPI invalidation MMIO registers

[Marc Zyngier]

Since GICv4.1, an implementation can offer the same MMIO-based
implementation as DirectLPI, only with an ITS. Given that this
can be hugely beneficial for workloads that are very LPI masking
heavy (although these workloads are admitedly a bit odd).

Interestingly, this is independent of RVPEI, which only *implies*
the functionnality.

So let's detect whether the implementation has GICR_CTLR.IR set,
and propagate this as DirectLPI to the ITS driver.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 drivers/irqchip/irq-gic-v3.c       | 15 +++++++++++----
 include/linux/irqchip/arm-gic-v3.h |  2 ++
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index 736163d36b13..363bfe172033 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -918,7 +918,11 @@ static int gic_populate_rdist(void)
 static int __gic_update_rdist_properties(struct redist_region *region,
 					 void __iomem *ptr)
 {
-	u64 typer = gic_read_typer(ptr + GICR_TYPER);
+	u64 typer;
+	u32 ctlr;
+
+	typer = gic_read_typer(ptr + GICR_TYPER);
+	ctlr = readl_relaxed(ptr + GICR_CTLR);
 
 	/* Boot-time cleanip */
 	if ((typer & GICR_TYPER_VLPIS) && (typer & GICR_TYPER_RVPEID)) {
@@ -941,6 +945,7 @@ static int __gic_update_rdist_properties(struct redist_region *region,
 	/* RVPEID implies some form of DirectLPI, no matter what the doc says... :-/ */
 	gic_data.rdists.has_rvpeid &= !!(typer & GICR_TYPER_RVPEID);
 	gic_data.rdists.has_direct_lpi &= (!!(typer & GICR_TYPER_DirectLPIS) |
+					   !!(ctlr & GICR_CTLR_IR) |
 					   gic_data.rdists.has_rvpeid);
 	gic_data.rdists.has_vpend_valid_dirty &= !!(typer & GICR_TYPER_DIRTY);
 
@@ -962,7 +967,11 @@ static void gic_update_rdist_properties(void)
 	gic_iterate_rdists(__gic_update_rdist_properties);
 	if (WARN_ON(gic_data.ppi_nr == UINT_MAX))
 		gic_data.ppi_nr = 0;
-	pr_info("%d PPIs implemented\n", gic_data.ppi_nr);
+	pr_info("GICv3 features: %d PPIs, %s%s\n",
+		gic_data.ppi_nr,
+		gic_data.has_rss ? "RSS " : "",
+		gic_data.rdists.has_direct_lpi ? "DirectLPI " : "");
+	
 	if (gic_data.rdists.has_vlpis)
 		pr_info("GICv4 features: %s%s%s\n",
 			gic_data.rdists.has_direct_lpi ? "DirectLPI " : "",
@@ -1797,8 +1806,6 @@ static int __init gic_init_bases(void __iomem *dist_base,
 	irq_domain_update_bus_token(gic_data.domain, DOMAIN_BUS_WIRED);
 
 	gic_data.has_rss = !!(typer & GICD_TYPER_RSS);
-	pr_info("Distributor has %sRange Selector support\n",
-		gic_data.has_rss ? "" : "no ");
 
 	if (typer & GICD_TYPER_MBIS) {
 		err = mbi_init(handle, gic_data.domain);
diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
index 12d91f0dedf9..aeb8ced53880 100644
--- a/include/linux/irqchip/arm-gic-v3.h
+++ b/include/linux/irqchip/arm-gic-v3.h
@@ -127,6 +127,8 @@
 #define GICR_PIDR2			GICD_PIDR2
 
 #define GICR_CTLR_ENABLE_LPIS		(1UL << 0)
+#define GICR_CTLR_IR			(1UL << 1)
+#define GICR_CTLR_CES			(1UL << 2)
 #define GICR_CTLR_RWP			(1UL << 3)
 
 #define GICR_TYPER_CPU_NUMBER(r)	(((r) >> 8) & 0xffff)
-- 
2.34.1

Re: [PATCH 2/3] irqchip/gic-v3: Detect LPI invalidation MMIO registers

[Marc Zyngier]

On 2022-03-15 16:50, Marc Zyngier wrote:
> diff --git a/include/linux/irqchip/arm-gic-v3.h
> b/include/linux/irqchip/arm-gic-v3.h
> index 12d91f0dedf9..aeb8ced53880 100644
> --- a/include/linux/irqchip/arm-gic-v3.h
> +++ b/include/linux/irqchip/arm-gic-v3.h
> @@ -127,6 +127,8 @@
>  #define GICR_PIDR2			GICD_PIDR2
> 
>  #define GICR_CTLR_ENABLE_LPIS		(1UL << 0)
> +#define GICR_CTLR_IR			(1UL << 1)
> +#define GICR_CTLR_CES			(1UL << 2)
>  #define GICR_CTLR_RWP			(1UL << 3)
> 
>  #define GICR_TYPER_CPU_NUMBER(r)	(((r) >> 8) & 0xffff)

As Oliver pointed out in [1], this is bollocks, and the two
new bits are swapped. I've now fixed that locally.

Thanks,

         M.

[1] https://lore.kernel.org/r/YjEeNThfYFtTffWz@google.com
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH 2/3] irqchip/gic-v3: Detect LPI invalidation MMIO registers

[Andre Przywara]

On Tue, 15 Mar 2022 16:50:33 +0000
Marc Zyngier <maz@kernel.org> wrote:

Hi,

> Since GICv4.1, an implementation can offer the same MMIO-based
> implementation as DirectLPI, only with an ITS. Given that this
> can be hugely beneficial for workloads that are very LPI masking
> heavy (although these workloads are admitedly a bit odd).
> 
> Interestingly, this is independent of RVPEI, which only *implies*
> the functionnality.
> 
> So let's detect whether the implementation has GICR_CTLR.IR set,
> and propagate this as DirectLPI to the ITS driver.
> 
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  drivers/irqchip/irq-gic-v3.c       | 15 +++++++++++----
>  include/linux/irqchip/arm-gic-v3.h |  2 ++
>  2 files changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> index 736163d36b13..363bfe172033 100644
> --- a/drivers/irqchip/irq-gic-v3.c
> +++ b/drivers/irqchip/irq-gic-v3.c
> @@ -918,7 +918,11 @@ static int gic_populate_rdist(void)
>  static int __gic_update_rdist_properties(struct redist_region *region,
>  					 void __iomem *ptr)
>  {
> -	u64 typer = gic_read_typer(ptr + GICR_TYPER);
> +	u64 typer;
> +	u32 ctlr;
> +
> +	typer = gic_read_typer(ptr + GICR_TYPER);
> +	ctlr = readl_relaxed(ptr + GICR_CTLR);

Is there any reason you didn't keep this together? I thought this was
recommended, in general?

>  
>  	/* Boot-time cleanip */
>  	if ((typer & GICR_TYPER_VLPIS) && (typer & GICR_TYPER_RVPEID)) {
> @@ -941,6 +945,7 @@ static int __gic_update_rdist_properties(struct redist_region *region,
>  	/* RVPEID implies some form of DirectLPI, no matter what the doc says... :-/ */
>  	gic_data.rdists.has_rvpeid &= !!(typer & GICR_TYPER_RVPEID);
>  	gic_data.rdists.has_direct_lpi &= (!!(typer & GICR_TYPER_DirectLPIS) |
> +					   !!(ctlr & GICR_CTLR_IR) |

So this means that has_direct_lpi is not really correct anymore, as the
IR bit only covers the INVL and SYNCR registers, not the GICR_SETLPIR
and GICR_CLRLPIR registers, if I understand the spec correctly?

But I guess this is nitpicking, as we don't use direct LPIs at all in
Linux? And I guess the target is lpi_update_config(), which now doesn't
need the command queue anymore?

Maybe this could be clarified in the commit message?

>  					   gic_data.rdists.has_rvpeid);
>  	gic_data.rdists.has_vpend_valid_dirty &= !!(typer & GICR_TYPER_DIRTY);
>  
> @@ -962,7 +967,11 @@ static void gic_update_rdist_properties(void)
>  	gic_iterate_rdists(__gic_update_rdist_properties);
>  	if (WARN_ON(gic_data.ppi_nr == UINT_MAX))
>  		gic_data.ppi_nr = 0;
> -	pr_info("%d PPIs implemented\n", gic_data.ppi_nr);
> +	pr_info("GICv3 features: %d PPIs, %s%s\n",

I like having that on one line, but it looks a bit odd with the
trailing comma when we have neither RSS nor DirectLPI.
What about:
	pr_info("GICv3 features: %d PPIs%s%s\n",
	gic_data.ppi_nr,
	gic_data.has_rss ? ", RSS" : "",
	gic_data.rdists.has_direct_lpi ? ", DirectLPI" : "");

> +		gic_data.ppi_nr,
> +		gic_data.has_rss ? "RSS " : "",
> +		gic_data.rdists.has_direct_lpi ? "DirectLPI " : "");
> +	
>  	if (gic_data.rdists.has_vlpis)
>  		pr_info("GICv4 features: %s%s%s\n",
>  			gic_data.rdists.has_direct_lpi ? "DirectLPI " : "",
> @@ -1797,8 +1806,6 @@ static int __init gic_init_bases(void __iomem *dist_base,
>  	irq_domain_update_bus_token(gic_data.domain, DOMAIN_BUS_WIRED);
>  
>  	gic_data.has_rss = !!(typer & GICD_TYPER_RSS);
> -	pr_info("Distributor has %sRange Selector support\n",
> -		gic_data.has_rss ? "" : "no ");
>  
>  	if (typer & GICD_TYPER_MBIS) {
>  		err = mbi_init(handle, gic_data.domain);
> diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
> index 12d91f0dedf9..aeb8ced53880 100644
> --- a/include/linux/irqchip/arm-gic-v3.h
> +++ b/include/linux/irqchip/arm-gic-v3.h
> @@ -127,6 +127,8 @@
>  #define GICR_PIDR2			GICD_PIDR2
>  
>  #define GICR_CTLR_ENABLE_LPIS		(1UL << 0)
> +#define GICR_CTLR_IR			(1UL << 1)
> +#define GICR_CTLR_CES			(1UL << 2)

As Oliver (and you) already pointed out, this is swapped.

Cheers,
Andre

>  #define GICR_CTLR_RWP			(1UL << 3)
>  
>  #define GICR_TYPER_CPU_NUMBER(r)	(((r) >> 8) & 0xffff)

Re: [PATCH 2/3] irqchip/gic-v3: Detect LPI invalidation MMIO registers

[Marc Zyngier]

On Wed, 16 Mar 2022 14:51:58 +0000,
Andre Przywara <andre.przywara@arm.com> wrote:
> 
> On Tue, 15 Mar 2022 16:50:33 +0000
> Marc Zyngier <maz@kernel.org> wrote:
> 
> Hi,
> 
> > Since GICv4.1, an implementation can offer the same MMIO-based
> > implementation as DirectLPI, only with an ITS. Given that this
> > can be hugely beneficial for workloads that are very LPI masking
> > heavy (although these workloads are admitedly a bit odd).
> > 
> > Interestingly, this is independent of RVPEI, which only *implies*
> > the functionnality.
> > 
> > So let's detect whether the implementation has GICR_CTLR.IR set,
> > and propagate this as DirectLPI to the ITS driver.
> > 
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > ---
> >  drivers/irqchip/irq-gic-v3.c       | 15 +++++++++++----
> >  include/linux/irqchip/arm-gic-v3.h |  2 ++
> >  2 files changed, 13 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> > index 736163d36b13..363bfe172033 100644
> > --- a/drivers/irqchip/irq-gic-v3.c
> > +++ b/drivers/irqchip/irq-gic-v3.c
> > @@ -918,7 +918,11 @@ static int gic_populate_rdist(void)
> >  static int __gic_update_rdist_properties(struct redist_region *region,
> >  					 void __iomem *ptr)
> >  {
> > -	u64 typer = gic_read_typer(ptr + GICR_TYPER);
> > +	u64 typer;
> > +	u32 ctlr;
> > +
> > +	typer = gic_read_typer(ptr + GICR_TYPER);
> > +	ctlr = readl_relaxed(ptr + GICR_CTLR);
> 
> Is there any reason you didn't keep this together? I thought this was
> recommended, in general?

Sorry, keep what together with what?

> 
> >  
> >  	/* Boot-time cleanip */
> >  	if ((typer & GICR_TYPER_VLPIS) && (typer & GICR_TYPER_RVPEID)) {
> > @@ -941,6 +945,7 @@ static int __gic_update_rdist_properties(struct redist_region *region,
> >  	/* RVPEID implies some form of DirectLPI, no matter what the doc says... :-/ */
> >  	gic_data.rdists.has_rvpeid &= !!(typer & GICR_TYPER_RVPEID);
> >  	gic_data.rdists.has_direct_lpi &= (!!(typer & GICR_TYPER_DirectLPIS) |
> > +					   !!(ctlr & GICR_CTLR_IR) |
> 
> So this means that has_direct_lpi is not really correct anymore, as the
> IR bit only covers the INVL and SYNCR registers, not the GICR_SETLPIR
> and GICR_CLRLPIR registers, if I understand the spec correctly?
> 
> But I guess this is nitpicking, as we don't use direct LPIs at all in
> Linux? And I guess the target is lpi_update_config(), which now doesn't
> need the command queue anymore?

Exactly. The history of this crap is convoluted:

The canonical goal of DirectLPI was to support LPIs without an
ITS. Thankfully, this was never implemented. What was implemented by
our HiSi friends was DirectLPI *with* an ITS, which was illegal at the
time, but also the only way to make GICv4.0 work at a reasonable
speed. That's where the direct_lpi boolean comes from.

RVPEI added some more confusion by offering a subset of DirectLPI for
invalidation of vlpis. And then IR was introduced because there is
really no reason not to offer the same service on GICv3.

> 
> Maybe this could be clarified in the commit message?

Sure, can do.

> 
> >  					   gic_data.rdists.has_rvpeid);
> >  	gic_data.rdists.has_vpend_valid_dirty &= !!(typer & GICR_TYPER_DIRTY);
> >  
> > @@ -962,7 +967,11 @@ static void gic_update_rdist_properties(void)
> >  	gic_iterate_rdists(__gic_update_rdist_properties);
> >  	if (WARN_ON(gic_data.ppi_nr == UINT_MAX))
> >  		gic_data.ppi_nr = 0;
> > -	pr_info("%d PPIs implemented\n", gic_data.ppi_nr);
> > +	pr_info("GICv3 features: %d PPIs, %s%s\n",
> 
> I like having that on one line, but it looks a bit odd with the
> trailing comma when we have neither RSS nor DirectLPI.
> What about:
> 	pr_info("GICv3 features: %d PPIs%s%s\n",
> 	gic_data.ppi_nr,
> 	gic_data.has_rss ? ", RSS" : "",
> 	gic_data.rdists.has_direct_lpi ? ", DirectLPI" : "");

Yeah, looks better.

Thanks,

	M.

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

Re: [PATCH 2/3] irqchip/gic-v3: Detect LPI invalidation MMIO registers

[Andre Przywara]

On Wed, 16 Mar 2022 15:36:54 +0000
Marc Zyngier <maz@kernel.org> wrote:

Hi Marc,

> On Wed, 16 Mar 2022 14:51:58 +0000,
> Andre Przywara <andre.przywara@arm.com> wrote:
> > 
> > On Tue, 15 Mar 2022 16:50:33 +0000
> > Marc Zyngier <maz@kernel.org> wrote:
> > 
> > Hi,
> >   
> > > Since GICv4.1, an implementation can offer the same MMIO-based
> > > implementation as DirectLPI, only with an ITS. Given that this
> > > can be hugely beneficial for workloads that are very LPI masking
> > > heavy (although these workloads are admitedly a bit odd).
> > > 
> > > Interestingly, this is independent of RVPEI, which only *implies*
> > > the functionnality.
> > > 
> > > So let's detect whether the implementation has GICR_CTLR.IR set,
> > > and propagate this as DirectLPI to the ITS driver.
> > > 
> > > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > > ---
> > >  drivers/irqchip/irq-gic-v3.c       | 15 +++++++++++----
> > >  include/linux/irqchip/arm-gic-v3.h |  2 ++
> > >  2 files changed, 13 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> > > index 736163d36b13..363bfe172033 100644
> > > --- a/drivers/irqchip/irq-gic-v3.c
> > > +++ b/drivers/irqchip/irq-gic-v3.c
> > > @@ -918,7 +918,11 @@ static int gic_populate_rdist(void)
> > >  static int __gic_update_rdist_properties(struct redist_region *region,
> > >  					 void __iomem *ptr)
> > >  {
> > > -	u64 typer = gic_read_typer(ptr + GICR_TYPER);
> > > +	u64 typer;
> > > +	u32 ctlr;
> > > +
> > > +	typer = gic_read_typer(ptr + GICR_TYPER);
> > > +	ctlr = readl_relaxed(ptr + GICR_CTLR);  
> > 
> > Is there any reason you didn't keep this together? I thought this was
> > recommended, in general?  
> 
> Sorry, keep what together with what?

Sorry, I meant the variable declaration with the initialisation:

	u64 typer = gic_read_typer(ptr + GICR_TYPER);
	u32 ctlr = readl_relaxed(ptr + GICR_CTLR);

I see this a lot (especially in KVM code), so was just wondering if
this is not cool anymore.

> > >  
> > >  	/* Boot-time cleanip */
> > >  	if ((typer & GICR_TYPER_VLPIS) && (typer & GICR_TYPER_RVPEID)) {
> > > @@ -941,6 +945,7 @@ static int __gic_update_rdist_properties(struct redist_region *region,
> > >  	/* RVPEID implies some form of DirectLPI, no matter what the doc says... :-/ */
> > >  	gic_data.rdists.has_rvpeid &= !!(typer & GICR_TYPER_RVPEID);
> > >  	gic_data.rdists.has_direct_lpi &= (!!(typer & GICR_TYPER_DirectLPIS) |
> > > +					   !!(ctlr & GICR_CTLR_IR) |  
> > 
> > So this means that has_direct_lpi is not really correct anymore, as the
> > IR bit only covers the INVL and SYNCR registers, not the GICR_SETLPIR
> > and GICR_CLRLPIR registers, if I understand the spec correctly?
> > 
> > But I guess this is nitpicking, as we don't use direct LPIs at all in
> > Linux? And I guess the target is lpi_update_config(), which now doesn't
> > need the command queue anymore?  
> 
> Exactly. The history of this crap is convoluted:
> 
> The canonical goal of DirectLPI was to support LPIs without an
> ITS. Thankfully, this was never implemented. What was implemented by
> our HiSi friends was DirectLPI *with* an ITS, which was illegal at the
> time, but also the only way to make GICv4.0 work at a reasonable
> speed. That's where the direct_lpi boolean comes from.
> 
> RVPEI added some more confusion by offering a subset of DirectLPI for
> invalidation of vlpis. And then IR was introduced because there is
> really no reason not to offer the same service on GICv3.

Ah, I was hoping for this kind of answer ;-) , so many thanks!

Cheers,
Andre

> 
> > 
> > Maybe this could be clarified in the commit message?  
> 
> Sure, can do.
> 
> >   
> > >  					   gic_data.rdists.has_rvpeid);
> > >  	gic_data.rdists.has_vpend_valid_dirty &= !!(typer & GICR_TYPER_DIRTY);
> > >  
> > > @@ -962,7 +967,11 @@ static void gic_update_rdist_properties(void)
> > >  	gic_iterate_rdists(__gic_update_rdist_properties);
> > >  	if (WARN_ON(gic_data.ppi_nr == UINT_MAX))
> > >  		gic_data.ppi_nr = 0;
> > > -	pr_info("%d PPIs implemented\n", gic_data.ppi_nr);
> > > +	pr_info("GICv3 features: %d PPIs, %s%s\n",  
> > 
> > I like having that on one line, but it looks a bit odd with the
> > trailing comma when we have neither RSS nor DirectLPI.
> > What about:
> > 	pr_info("GICv3 features: %d PPIs%s%s\n",
> > 	gic_data.ppi_nr,
> > 	gic_data.has_rss ? ", RSS" : "",
> > 	gic_data.rdists.has_direct_lpi ? ", DirectLPI" : "");  
> 
> Yeah, looks better.
> 
> Thanks,
> 
> 	M.
> 

Re: [PATCH 2/3] irqchip/gic-v3: Detect LPI invalidation MMIO registers

[Lorenzo Pieralisi]

On Tue, Mar 15, 2022 at 04:50:33PM +0000, Marc Zyngier wrote:
> Since GICv4.1, an implementation can offer the same MMIO-based
> implementation as DirectLPI, only with an ITS. Given that this
> can be hugely beneficial for workloads that are very LPI masking
> heavy (although these workloads are admitedly a bit odd).
> 
> Interestingly, this is independent of RVPEI, which only *implies*
> the functionnality.
> 
> So let's detect whether the implementation has GICR_CTLR.IR set,
> and propagate this as DirectLPI to the ITS driver.
> 
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  drivers/irqchip/irq-gic-v3.c       | 15 +++++++++++----
>  include/linux/irqchip/arm-gic-v3.h |  2 ++
>  2 files changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> index 736163d36b13..363bfe172033 100644
> --- a/drivers/irqchip/irq-gic-v3.c
> +++ b/drivers/irqchip/irq-gic-v3.c
> @@ -918,7 +918,11 @@ static int gic_populate_rdist(void)
>  static int __gic_update_rdist_properties(struct redist_region *region,
>  					 void __iomem *ptr)
>  {
> -	u64 typer = gic_read_typer(ptr + GICR_TYPER);
> +	u64 typer;
> +	u32 ctlr;
> +
> +	typer = gic_read_typer(ptr + GICR_TYPER);
> +	ctlr = readl_relaxed(ptr + GICR_CTLR);
>  
>  	/* Boot-time cleanip */
>  	if ((typer & GICR_TYPER_VLPIS) && (typer & GICR_TYPER_RVPEID)) {
> @@ -941,6 +945,7 @@ static int __gic_update_rdist_properties(struct redist_region *region,
>  	/* RVPEID implies some form of DirectLPI, no matter what the doc says... :-/ */
>  	gic_data.rdists.has_rvpeid &= !!(typer & GICR_TYPER_RVPEID);
>  	gic_data.rdists.has_direct_lpi &= (!!(typer & GICR_TYPER_DirectLPIS) |
> +					   !!(ctlr & GICR_CTLR_IR) |
>  					   gic_data.rdists.has_rvpeid);
>  	gic_data.rdists.has_vpend_valid_dirty &= !!(typer & GICR_TYPER_DIRTY);
>  
> @@ -962,7 +967,11 @@ static void gic_update_rdist_properties(void)
>  	gic_iterate_rdists(__gic_update_rdist_properties);
>  	if (WARN_ON(gic_data.ppi_nr == UINT_MAX))
>  		gic_data.ppi_nr = 0;
> -	pr_info("%d PPIs implemented\n", gic_data.ppi_nr);
> +	pr_info("GICv3 features: %d PPIs, %s%s\n",
> +		gic_data.ppi_nr,
> +		gic_data.has_rss ? "RSS " : "",
> +		gic_data.rdists.has_direct_lpi ? "DirectLPI " : "");

I understand GICR_CTLR.IR detection (which is v4.1 feature) - I don't
get why in this patch we are adding a GICv3 DirectLPI info dump (hunk
above), it is probably nitpicking but the hunk above does not seem to
belong in this patch - it is a separate print info refactoring or I am
reading it wrongly.

Lorenzo

> +	
>  	if (gic_data.rdists.has_vlpis)
>  		pr_info("GICv4 features: %s%s%s\n",
>  			gic_data.rdists.has_direct_lpi ? "DirectLPI " : "",
> @@ -1797,8 +1806,6 @@ static int __init gic_init_bases(void __iomem *dist_base,
>  	irq_domain_update_bus_token(gic_data.domain, DOMAIN_BUS_WIRED);
>  
>  	gic_data.has_rss = !!(typer & GICD_TYPER_RSS);
> -	pr_info("Distributor has %sRange Selector support\n",
> -		gic_data.has_rss ? "" : "no ");
>  
>  	if (typer & GICD_TYPER_MBIS) {
>  		err = mbi_init(handle, gic_data.domain);
> diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
> index 12d91f0dedf9..aeb8ced53880 100644
> --- a/include/linux/irqchip/arm-gic-v3.h
> +++ b/include/linux/irqchip/arm-gic-v3.h
> @@ -127,6 +127,8 @@
>  #define GICR_PIDR2			GICD_PIDR2
>  
>  #define GICR_CTLR_ENABLE_LPIS		(1UL << 0)
> +#define GICR_CTLR_IR			(1UL << 1)
> +#define GICR_CTLR_CES			(1UL << 2)
>  #define GICR_CTLR_RWP			(1UL << 3)
>  
>  #define GICR_TYPER_CPU_NUMBER(r)	(((r) >> 8) & 0xffff)
> -- 
> 2.34.1
> 

Re: [PATCH 2/3] irqchip/gic-v3: Detect LPI invalidation MMIO registers

[Marc Zyngier]

On Thu, 17 Mar 2022 17:35:23 +0000,
Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> 
> On Tue, Mar 15, 2022 at 04:50:33PM +0000, Marc Zyngier wrote:
> > Since GICv4.1, an implementation can offer the same MMIO-based
> > implementation as DirectLPI, only with an ITS. Given that this
> > can be hugely beneficial for workloads that are very LPI masking
> > heavy (although these workloads are admitedly a bit odd).
> > 
> > Interestingly, this is independent of RVPEI, which only *implies*
> > the functionnality.
> > 
> > So let's detect whether the implementation has GICR_CTLR.IR set,
> > and propagate this as DirectLPI to the ITS driver.
> > 
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > ---
> >  drivers/irqchip/irq-gic-v3.c       | 15 +++++++++++----
> >  include/linux/irqchip/arm-gic-v3.h |  2 ++
> >  2 files changed, 13 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> > index 736163d36b13..363bfe172033 100644
> > --- a/drivers/irqchip/irq-gic-v3.c
> > +++ b/drivers/irqchip/irq-gic-v3.c
> > @@ -918,7 +918,11 @@ static int gic_populate_rdist(void)
> >  static int __gic_update_rdist_properties(struct redist_region *region,
> >  					 void __iomem *ptr)
> >  {
> > -	u64 typer = gic_read_typer(ptr + GICR_TYPER);
> > +	u64 typer;
> > +	u32 ctlr;
> > +
> > +	typer = gic_read_typer(ptr + GICR_TYPER);
> > +	ctlr = readl_relaxed(ptr + GICR_CTLR);
> >  
> >  	/* Boot-time cleanip */
> >  	if ((typer & GICR_TYPER_VLPIS) && (typer & GICR_TYPER_RVPEID)) {
> > @@ -941,6 +945,7 @@ static int __gic_update_rdist_properties(struct redist_region *region,
> >  	/* RVPEID implies some form of DirectLPI, no matter what the doc says... :-/ */
> >  	gic_data.rdists.has_rvpeid &= !!(typer & GICR_TYPER_RVPEID);
> >  	gic_data.rdists.has_direct_lpi &= (!!(typer & GICR_TYPER_DirectLPIS) |
> > +					   !!(ctlr & GICR_CTLR_IR) |
> >  					   gic_data.rdists.has_rvpeid);
> >  	gic_data.rdists.has_vpend_valid_dirty &= !!(typer & GICR_TYPER_DIRTY);
> >  
> > @@ -962,7 +967,11 @@ static void gic_update_rdist_properties(void)
> >  	gic_iterate_rdists(__gic_update_rdist_properties);
> >  	if (WARN_ON(gic_data.ppi_nr == UINT_MAX))
> >  		gic_data.ppi_nr = 0;
> > -	pr_info("%d PPIs implemented\n", gic_data.ppi_nr);
> > +	pr_info("GICv3 features: %d PPIs, %s%s\n",
> > +		gic_data.ppi_nr,
> > +		gic_data.has_rss ? "RSS " : "",
> > +		gic_data.rdists.has_direct_lpi ? "DirectLPI " : "");
> 
> I understand GICR_CTLR.IR detection (which is v4.1 feature) - I don't

No, it is *also* a GICv3 feature. RVPEI implies IR, but IR is a
feature on its own (see my reply to Andre on the same subject).
Nothing restrict IR to a GICv4.1+ implementation, and KVM is about to
expose these registers to the GICv*3* guest.

> get why in this patch we are adding a GICv3 DirectLPI info dump (hunk
> above), it is probably nitpicking but the hunk above does not seem to
> belong in this patch - it is a separate print info refactoring or I am
> reading it wrongly.

It is indeed just refactoring the kernel messages so that we can see
that we enable DirectLPI for GICv3 as well. I honestly don't think
this deserves a separate patch.

Thanks,

	M.

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

[PATCH 3/3] irqchip/gic-v3: Relax polling of GIC{R,D}_CTLR.RWP

[Marc Zyngier]

Recent work on the KVM GIC emulation has revealed that the GICv3
driver is a bit RWP-happy, as it polls this bit for each and
every write MMIO access involving a single interrupt.

As it turns out, polling RWP is only required when:
- Disabling an SGI, PPI or SPI
- Disabling LPIs at the redistributor level
- Disabling groups
- Enabling ARE
- Dealing with DPG*

Simplify the driver by removing all the other instances of RWP
polling, and add the one that was missing when enabling the distributor
(as that's where we set ARE).

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 drivers/irqchip/irq-gic-v3.c | 28 +++++++++++-----------------
 1 file changed, 11 insertions(+), 17 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index 363bfe172033..05ff7fef64cb 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -352,28 +352,27 @@ static int gic_peek_irq(struct irq_data *d, u32 offset)
 
 static void gic_poke_irq(struct irq_data *d, u32 offset)
 {
-	void (*rwp_wait)(void);
 	void __iomem *base;
 	u32 index, mask;
 
 	offset = convert_offset_index(d, offset, &index);
 	mask = 1 << (index % 32);
 
-	if (gic_irq_in_rdist(d)) {
+	if (gic_irq_in_rdist(d))
 		base = gic_data_rdist_sgi_base();
-		rwp_wait = gic_redist_wait_for_rwp;
-	} else {
+	else
 		base = gic_data.dist_base;
-		rwp_wait = gic_dist_wait_for_rwp;
-	}
 
 	writel_relaxed(mask, base + offset + (index / 32) * 4);
-	rwp_wait();
 }
 
 static void gic_mask_irq(struct irq_data *d)
 {
 	gic_poke_irq(d, GICD_ICENABLER);
+	if (gic_irq_in_rdist(d))
+		gic_redist_wait_for_rwp();
+	else
+		gic_dist_wait_for_rwp();
 }
 
 static void gic_eoimode1_mask_irq(struct irq_data *d)
@@ -574,7 +573,6 @@ static int gic_set_type(struct irq_data *d, unsigned int type)
 {
 	enum gic_intid_range range;
 	unsigned int irq = gic_irq(d);
-	void (*rwp_wait)(void);
 	void __iomem *base;
 	u32 offset, index;
 	int ret;
@@ -590,17 +588,14 @@ static int gic_set_type(struct irq_data *d, unsigned int type)
 	    type != IRQ_TYPE_LEVEL_HIGH && type != IRQ_TYPE_EDGE_RISING)
 		return -EINVAL;
 
-	if (gic_irq_in_rdist(d)) {
+	if (gic_irq_in_rdist(d))
 		base = gic_data_rdist_sgi_base();
-		rwp_wait = gic_redist_wait_for_rwp;
-	} else {
+	else
 		base = gic_data.dist_base;
-		rwp_wait = gic_dist_wait_for_rwp;
-	}
 
 	offset = convert_offset_index(d, GICD_ICFGR, &index);
 
-	ret = gic_configure_irq(index, type, base + offset, rwp_wait);
+	ret = gic_configure_irq(index, type, base + offset, NULL);
 	if (ret && (range == PPI_RANGE || range == EPPI_RANGE)) {
 		/* Misconfigured PPIs are usually not fatal */
 		pr_warn("GIC: PPI INTID%d is secure or misconfigured\n", irq);
@@ -808,7 +803,7 @@ static void __init gic_dist_init(void)
 		writel_relaxed(GICD_INT_DEF_PRI_X4, base + GICD_IPRIORITYRnE + i);
 
 	/* Now do the common stuff, and wait for the distributor to drain */
-	gic_dist_config(base, GIC_LINE_NR, gic_dist_wait_for_rwp);
+	gic_dist_config(base, GIC_LINE_NR, NULL);
 
 	val = GICD_CTLR_ARE_NS | GICD_CTLR_ENABLE_G1A | GICD_CTLR_ENABLE_G1;
 	if (gic_data.rdists.gicd_typer2 & GICD_TYPER2_nASSGIcap) {
@@ -818,6 +813,7 @@ static void __init gic_dist_init(void)
 
 	/* Enable distributor with ARE, Group1 */
 	writel_relaxed(val, base + GICD_CTLR);
+	gic_dist_wait_for_rwp();
 
 	/*
 	 * Set all global interrupts to the boot CPU only. ARE must be
@@ -1293,8 +1289,6 @@ static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val,
 	 */
 	if (enabled)
 		gic_unmask_irq(d);
-	else
-		gic_dist_wait_for_rwp();
 
 	irq_data_update_effective_affinity(d, cpumask_of(cpu));
 
-- 
2.34.1

Re: [PATCH 3/3] irqchip/gic-v3: Relax polling of GIC{R,D}_CTLR.RWP

[Andre Przywara]

On Tue, 15 Mar 2022 16:50:34 +0000
Marc Zyngier <maz@kernel.org> wrote:

Hi,

> Recent work on the KVM GIC emulation has revealed that the GICv3
> driver is a bit RWP-happy, as it polls this bit for each and
> every write MMIO access involving a single interrupt.
>
> As it turns out, polling RWP is only required when:
> - Disabling an SGI, PPI or SPI
> - Disabling LPIs at the redistributor level
> - Disabling groups
> - Enabling ARE
> - Dealing with DPG*
>
> Simplify the driver by removing all the other instances of RWP
> polling, and add the one that was missing when enabling the distributor
> (as that's where we set ARE).

Don't we need an explicit call to wait_for_rwp() now for:
gic_irq_set_irqchip_state(IRQCHIP_STATE_MASKED, true) ?

The rest looks fine to me.

Thanks,
Andre

>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  drivers/irqchip/irq-gic-v3.c | 28 +++++++++++-----------------
>  1 file changed, 11 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> index 363bfe172033..05ff7fef64cb 100644
> --- a/drivers/irqchip/irq-gic-v3.c
> +++ b/drivers/irqchip/irq-gic-v3.c
> @@ -352,28 +352,27 @@ static int gic_peek_irq(struct irq_data *d, u32 offset)
>
>  static void gic_poke_irq(struct irq_data *d, u32 offset)
>  {
> -     void (*rwp_wait)(void);
>       void __iomem *base;
>       u32 index, mask;
>
>       offset = convert_offset_index(d, offset, &index);
>       mask = 1 << (index % 32);
>
> -     if (gic_irq_in_rdist(d)) {
> +     if (gic_irq_in_rdist(d))
>               base = gic_data_rdist_sgi_base();
> -             rwp_wait = gic_redist_wait_for_rwp;
> -     } else {
> +     else
>               base = gic_data.dist_base;
> -             rwp_wait = gic_dist_wait_for_rwp;
> -     }
>
>       writel_relaxed(mask, base + offset + (index / 32) * 4);
> -     rwp_wait();
>  }
>
>  static void gic_mask_irq(struct irq_data *d)
>  {
>       gic_poke_irq(d, GICD_ICENABLER);
> +     if (gic_irq_in_rdist(d))
> +             gic_redist_wait_for_rwp();
> +     else
> +             gic_dist_wait_for_rwp();
>  }
>
>  static void gic_eoimode1_mask_irq(struct irq_data *d)
> @@ -574,7 +573,6 @@ static int gic_set_type(struct irq_data *d, unsigned int type)
>  {
>       enum gic_intid_range range;
>       unsigned int irq = gic_irq(d);
> -     void (*rwp_wait)(void);
>       void __iomem *base;
>       u32 offset, index;
>       int ret;
> @@ -590,17 +588,14 @@ static int gic_set_type(struct irq_data *d, unsigned int type)
>           type != IRQ_TYPE_LEVEL_HIGH && type != IRQ_TYPE_EDGE_RISING)
>               return -EINVAL;
>
> -     if (gic_irq_in_rdist(d)) {
> +     if (gic_irq_in_rdist(d))
>               base = gic_data_rdist_sgi_base();
> -             rwp_wait = gic_redist_wait_for_rwp;
> -     } else {
> +     else
>               base = gic_data.dist_base;
> -             rwp_wait = gic_dist_wait_for_rwp;
> -     }
>
>       offset = convert_offset_index(d, GICD_ICFGR, &index);
>
> -     ret = gic_configure_irq(index, type, base + offset, rwp_wait);
> +     ret = gic_configure_irq(index, type, base + offset, NULL);
>       if (ret && (range == PPI_RANGE || range == EPPI_RANGE)) {
>               /* Misconfigured PPIs are usually not fatal */
>               pr_warn("GIC: PPI INTID%d is secure or misconfigured\n", irq);
> @@ -808,7 +803,7 @@ static void __init gic_dist_init(void)
>               writel_relaxed(GICD_INT_DEF_PRI_X4, base + GICD_IPRIORITYRnE + i);
>
>       /* Now do the common stuff, and wait for the distributor to drain */
> -     gic_dist_config(base, GIC_LINE_NR, gic_dist_wait_for_rwp);
> +     gic_dist_config(base, GIC_LINE_NR, NULL);
>
>       val = GICD_CTLR_ARE_NS | GICD_CTLR_ENABLE_G1A | GICD_CTLR_ENABLE_G1;
>       if (gic_data.rdists.gicd_typer2 & GICD_TYPER2_nASSGIcap) {
> @@ -818,6 +813,7 @@ static void __init gic_dist_init(void)
>
>       /* Enable distributor with ARE, Group1 */
>       writel_relaxed(val, base + GICD_CTLR);
> +     gic_dist_wait_for_rwp();
>
>       /*
>        * Set all global interrupts to the boot CPU only. ARE must be
> @@ -1293,8 +1289,6 @@ static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val,
>        */
>       if (enabled)
>               gic_unmask_irq(d);
> -     else
> -             gic_dist_wait_for_rwp();
>
>       irq_data_update_effective_affinity(d, cpumask_of(cpu));
>

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Re: [PATCH 3/3] irqchip/gic-v3: Relax polling of GIC{R,D}_CTLR.RWP

[Marc Zyngier]

On Wed, 16 Mar 2022 14:54:03 +0000,
Andre Przywara <andre.przywara@arm.com> wrote:
> 
> On Tue, 15 Mar 2022 16:50:34 +0000
> Marc Zyngier <maz@kernel.org> wrote:
> 
> Hi,
> 
> > Recent work on the KVM GIC emulation has revealed that the GICv3
> > driver is a bit RWP-happy, as it polls this bit for each and
> > every write MMIO access involving a single interrupt.
> >
> > As it turns out, polling RWP is only required when:
> > - Disabling an SGI, PPI or SPI
> > - Disabling LPIs at the redistributor level
> > - Disabling groups
> > - Enabling ARE
> > - Dealing with DPG*
> >
> > Simplify the driver by removing all the other instances of RWP
> > polling, and add the one that was missing when enabling the distributor
> > (as that's where we set ARE).
> 
> Don't we need an explicit call to wait_for_rwp() now for:
> gic_irq_set_irqchip_state(IRQCHIP_STATE_MASKED, true) ?

Ah, yes, I missed that one. Thanks.

> IMPORTANT NOTICE: The contents of this email [...]

The ARM IT crap is firing again. Wrong SMTP server?

	M.

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