[PATCHv3 0/3] arm64/irqentry: remove duplicate housekeeping of rcu

[PATCHv3 0/3] arm64/irqentry: remove duplicate housekeeping of rcu

[Pingfan Liu]

When an IRQ is taken, some accounting needs to be performed to enter and
exit IRQ context around the IRQ handler. Historically arch code would
leave this to the irqchip or core IRQ code, but these days we want this
to happen in exception entry code, and architectures such as arm64 do
this.

Currently handle_domain_irq() performs this entry/exit accounting, and
if used on an architecture where the entry code also does this, the
entry/exit accounting will be performed twice per IRQ. This is
problematic as core RCU code such as rcu_is_cpu_rrupt_from_idle()
depends on this happening once per IRQ, and will not detect quescent
periods correctly, leading to stall warnings.

As irqchip drivers which use handle_domain_irq() need to work on
architectures with or without their own entry/exit accounting, this
patch makes handle_domain_irq() conditionally perform the entry
accounting depending on a new HAVE_ARCH_IRQENTRY Kconfig symbol that
architectures can select if they perform this entry accounting
themselves.

V2 -> V3:
  Drop other patches and concentrate on the purpose of [3-4/5] of V2.
  And lift the level, where to add {irq_enter,exit}_rcu(), from the
  interrupt controler to exception entry  

History:
V1: https://lore.kernel.org/linux-arm-kernel/1607912752-12481-1-git-send-email-kernelfans@gmail.com
V2: https://lore.kernel.org/linux-arm-kernel/20210924132837.45994-1-kernelfans@gmail.com


Cc: "Paul E. McKenney" <paulmck@kernel.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Joey Gouly <joey.gouly@arm.com>
Cc: Sami Tolvanen <samitolvanen@google.com>
Cc: Julien Thierry <julien.thierry@arm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Yuichi Ito <ito-yuichi@fujitsu.com>
Cc: linux-kernel@vger.kernel.org
To: linux-arm-kernel@lists.infradead.org


Mark Rutland (1):
  arm64: entry: refactor EL1 interrupt entry logic

Pingfan Liu (2):
  kernel/irq: make irq_{enter,exit}() in handle_domain_irq() arch
    optional
  arm64/entry-common: supplement irq accounting

 arch/arm64/Kconfig               |  1 +
 arch/arm64/kernel/entry-common.c | 48 +++++++++++++++++---------------
 kernel/irq/Kconfig               |  3 ++
 kernel/irq/irqdesc.c             |  4 +++
 4 files changed, 34 insertions(+), 22 deletions(-)

-- 
2.31.1

[PATCHv3 0/3] arm64/irqentry: remove duplicate housekeeping of rcu

[Pingfan Liu]

When an IRQ is taken, some accounting needs to be performed to enter and
exit IRQ context around the IRQ handler. Historically arch code would
leave this to the irqchip or core IRQ code, but these days we want this
to happen in exception entry code, and architectures such as arm64 do
this.

Currently handle_domain_irq() performs this entry/exit accounting, and
if used on an architecture where the entry code also does this, the
entry/exit accounting will be performed twice per IRQ. This is
problematic as core RCU code such as rcu_is_cpu_rrupt_from_idle()
depends on this happening once per IRQ, and will not detect quescent
periods correctly, leading to stall warnings.

As irqchip drivers which use handle_domain_irq() need to work on
architectures with or without their own entry/exit accounting, this
patch makes handle_domain_irq() conditionally perform the entry
accounting depending on a new HAVE_ARCH_IRQENTRY Kconfig symbol that
architectures can select if they perform this entry accounting
themselves.

V2 -> V3:
  Drop other patches and concentrate on the purpose of [3-4/5] of V2.
  And lift the level, where to add {irq_enter,exit}_rcu(), from the
  interrupt controler to exception entry  

History:
V1: https://lore.kernel.org/linux-arm-kernel/1607912752-12481-1-git-send-email-kernelfans@gmail.com
V2: https://lore.kernel.org/linux-arm-kernel/20210924132837.45994-1-kernelfans@gmail.com


Cc: "Paul E. McKenney" <paulmck@kernel.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Joey Gouly <joey.gouly@arm.com>
Cc: Sami Tolvanen <samitolvanen@google.com>
Cc: Julien Thierry <julien.thierry@arm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Yuichi Ito <ito-yuichi@fujitsu.com>
Cc: linux-kernel@vger.kernel.org
To: linux-arm-kernel@lists.infradead.org


Mark Rutland (1):
  arm64: entry: refactor EL1 interrupt entry logic

Pingfan Liu (2):
  kernel/irq: make irq_{enter,exit}() in handle_domain_irq() arch
    optional
  arm64/entry-common: supplement irq accounting

 arch/arm64/Kconfig               |  1 +
 arch/arm64/kernel/entry-common.c | 48 +++++++++++++++++---------------
 kernel/irq/Kconfig               |  3 ++
 kernel/irq/irqdesc.c             |  4 +++
 4 files changed, 34 insertions(+), 22 deletions(-)

-- 
2.31.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCHv3 1/3] kernel/irq: make irq_{enter,exit}() in handle_domain_irq() arch optional

[Pingfan Liu]

When an IRQ is taken, some accounting needs to be performed to enter and
exit IRQ context around the IRQ handler. Historically arch code would
leave this to the irqchip or core IRQ code, but these days we want this
to happen in exception entry code, and architectures such as arm64 do
this.

Currently handle_domain_irq() performs this entry/exit accounting, and
if used on an architecture where the entry code also does this, the
entry/exit accounting will be performed twice per IRQ. This is
problematic as core RCU code such as rcu_is_cpu_rrupt_from_idle()
depends on this happening once per IRQ, and will not detect quescent
periods correctly, leading to stall warnings.

As irqchip drivers which use handle_domain_irq() need to work on
architectures with or without their own entry/exit accounting, this
patch makes handle_domain_irq() conditionally perform the entry
accounting depending on a new HAVE_ARCH_IRQENTRY Kconfig symbol that
architectures can select if they perform this entry accounting
themselves.

For architectures which do not select the symbol. there should be no
functional change as a result of this patch.

Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
Reviewed-by: Mark Rutland <mark.rutland@arm.com>
Cc: "Paul E. McKenney" <paulmck@kernel.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Joey Gouly <joey.gouly@arm.com>
Cc: Sami Tolvanen <samitolvanen@google.com>
Cc: Julien Thierry <julien.thierry@arm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Yuichi Ito <ito-yuichi@fujitsu.com>
Cc: linux-kernel@vger.kernel.org
To: linux-arm-kernel@lists.infradead.org
---
 kernel/irq/Kconfig   | 3 +++
 kernel/irq/irqdesc.c | 4 ++++
 2 files changed, 7 insertions(+)

diff --git a/kernel/irq/Kconfig b/kernel/irq/Kconfig
index fbc54c2a7f23..defa1db2d664 100644
--- a/kernel/irq/Kconfig
+++ b/kernel/irq/Kconfig
@@ -100,6 +100,9 @@ config IRQ_MSI_IOMMU
 config HANDLE_DOMAIN_IRQ
 	bool
 
+config HAVE_ARCH_IRQENTRY
+	bool
+
 config IRQ_TIMINGS
 	bool
 
diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
index 4e3c29bb603c..fd5dd9d278b5 100644
--- a/kernel/irq/irqdesc.c
+++ b/kernel/irq/irqdesc.c
@@ -693,7 +693,9 @@ int handle_domain_irq(struct irq_domain *domain,
 	struct irq_desc *desc;
 	int ret = 0;
 
+#ifndef CONFIG_HAVE_ARCH_IRQENTRY
 	irq_enter();
+#endif
 
 	/* The irqdomain code provides boundary checks */
 	desc = irq_resolve_mapping(domain, hwirq);
@@ -702,7 +704,9 @@ int handle_domain_irq(struct irq_domain *domain,
 	else
 		ret = -EINVAL;
 
+#ifndef CONFIG_HAVE_ARCH_IRQENTRY
 	irq_exit();
+#endif
 	set_irq_regs(old_regs);
 	return ret;
 }
-- 
2.31.1

[PATCHv3 1/3] kernel/irq: make irq_{enter, exit}() in handle_domain_irq() arch optional

[Pingfan Liu]

When an IRQ is taken, some accounting needs to be performed to enter and
exit IRQ context around the IRQ handler. Historically arch code would
leave this to the irqchip or core IRQ code, but these days we want this
to happen in exception entry code, and architectures such as arm64 do
this.

Currently handle_domain_irq() performs this entry/exit accounting, and
if used on an architecture where the entry code also does this, the
entry/exit accounting will be performed twice per IRQ. This is
problematic as core RCU code such as rcu_is_cpu_rrupt_from_idle()
depends on this happening once per IRQ, and will not detect quescent
periods correctly, leading to stall warnings.

As irqchip drivers which use handle_domain_irq() need to work on
architectures with or without their own entry/exit accounting, this
patch makes handle_domain_irq() conditionally perform the entry
accounting depending on a new HAVE_ARCH_IRQENTRY Kconfig symbol that
architectures can select if they perform this entry accounting
themselves.

For architectures which do not select the symbol. there should be no
functional change as a result of this patch.

Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
Reviewed-by: Mark Rutland <mark.rutland@arm.com>
Cc: "Paul E. McKenney" <paulmck@kernel.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Joey Gouly <joey.gouly@arm.com>
Cc: Sami Tolvanen <samitolvanen@google.com>
Cc: Julien Thierry <julien.thierry@arm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Yuichi Ito <ito-yuichi@fujitsu.com>
Cc: linux-kernel@vger.kernel.org
To: linux-arm-kernel@lists.infradead.org
---
 kernel/irq/Kconfig   | 3 +++
 kernel/irq/irqdesc.c | 4 ++++
 2 files changed, 7 insertions(+)

diff --git a/kernel/irq/Kconfig b/kernel/irq/Kconfig
index fbc54c2a7f23..defa1db2d664 100644
--- a/kernel/irq/Kconfig
+++ b/kernel/irq/Kconfig
@@ -100,6 +100,9 @@ config IRQ_MSI_IOMMU
 config HANDLE_DOMAIN_IRQ
 	bool
 
+config HAVE_ARCH_IRQENTRY
+	bool
+
 config IRQ_TIMINGS
 	bool
 
diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
index 4e3c29bb603c..fd5dd9d278b5 100644
--- a/kernel/irq/irqdesc.c
+++ b/kernel/irq/irqdesc.c
@@ -693,7 +693,9 @@ int handle_domain_irq(struct irq_domain *domain,
 	struct irq_desc *desc;
 	int ret = 0;
 
+#ifndef CONFIG_HAVE_ARCH_IRQENTRY
 	irq_enter();
+#endif
 
 	/* The irqdomain code provides boundary checks */
 	desc = irq_resolve_mapping(domain, hwirq);
@@ -702,7 +704,9 @@ int handle_domain_irq(struct irq_domain *domain,
 	else
 		ret = -EINVAL;
 
+#ifndef CONFIG_HAVE_ARCH_IRQENTRY
 	irq_exit();
+#endif
 	set_irq_regs(old_regs);
 	return ret;
 }
-- 
2.31.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCHv3 1/3] kernel/irq: make irq_{enter,exit}() in handle_domain_irq() arch optional

[Marc Zyngier]

On Thu, 30 Sep 2021 14:17:06 +0100,
Pingfan Liu <kernelfans@gmail.com> wrote:
> 
> When an IRQ is taken, some accounting needs to be performed to enter and
> exit IRQ context around the IRQ handler. Historically arch code would
> leave this to the irqchip or core IRQ code, but these days we want this
> to happen in exception entry code, and architectures such as arm64 do
> this.
> 
> Currently handle_domain_irq() performs this entry/exit accounting, and
> if used on an architecture where the entry code also does this, the
> entry/exit accounting will be performed twice per IRQ. This is
> problematic as core RCU code such as rcu_is_cpu_rrupt_from_idle()
> depends on this happening once per IRQ, and will not detect quescent
> periods correctly, leading to stall warnings.
> 
> As irqchip drivers which use handle_domain_irq() need to work on
> architectures with or without their own entry/exit accounting, this
> patch makes handle_domain_irq() conditionally perform the entry
> accounting depending on a new HAVE_ARCH_IRQENTRY Kconfig symbol that
> architectures can select if they perform this entry accounting
> themselves.
> 
> For architectures which do not select the symbol. there should be no
> functional change as a result of this patch.
> 
> Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
> Reviewed-by: Mark Rutland <mark.rutland@arm.com>
> Cc: "Paul E. McKenney" <paulmck@kernel.org>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Joey Gouly <joey.gouly@arm.com>
> Cc: Sami Tolvanen <samitolvanen@google.com>
> Cc: Julien Thierry <julien.thierry@arm.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Yuichi Ito <ito-yuichi@fujitsu.com>
> Cc: linux-kernel@vger.kernel.org
> To: linux-arm-kernel@lists.infradead.org
> ---
>  kernel/irq/Kconfig   | 3 +++
>  kernel/irq/irqdesc.c | 4 ++++
>  2 files changed, 7 insertions(+)
> 
> diff --git a/kernel/irq/Kconfig b/kernel/irq/Kconfig
> index fbc54c2a7f23..defa1db2d664 100644
> --- a/kernel/irq/Kconfig
> +++ b/kernel/irq/Kconfig
> @@ -100,6 +100,9 @@ config IRQ_MSI_IOMMU
>  config HANDLE_DOMAIN_IRQ
>  	bool
>  
> +config HAVE_ARCH_IRQENTRY
> +	bool
> +
>  config IRQ_TIMINGS
>  	bool
>  
> diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
> index 4e3c29bb603c..fd5dd9d278b5 100644
> --- a/kernel/irq/irqdesc.c
> +++ b/kernel/irq/irqdesc.c
> @@ -693,7 +693,9 @@ int handle_domain_irq(struct irq_domain *domain,
>  	struct irq_desc *desc;
>  	int ret = 0;
>  
> +#ifndef CONFIG_HAVE_ARCH_IRQENTRY
>  	irq_enter();
> +#endif

nit: I tend to prefer the 'if (!IS_ENABLED(CONFIG_*))' approach.

>  
>  	/* The irqdomain code provides boundary checks */
>  	desc = irq_resolve_mapping(domain, hwirq);
> @@ -702,7 +704,9 @@ int handle_domain_irq(struct irq_domain *domain,
>  	else
>  		ret = -EINVAL;
>  
> +#ifndef CONFIG_HAVE_ARCH_IRQENTRY
>  	irq_exit();
> +#endif
>  	set_irq_regs(old_regs);
>  	return ret;
>  }

Apart from that:

Reviewed-by: Marc Zyngier <maz@kernel.org>

Thanks,

	M.

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

Re: [PATCHv3 1/3] kernel/irq: make irq_{enter, exit}() in handle_domain_irq() arch optional

[Marc Zyngier]

On Thu, 30 Sep 2021 14:17:06 +0100,
Pingfan Liu <kernelfans@gmail.com> wrote:
> 
> When an IRQ is taken, some accounting needs to be performed to enter and
> exit IRQ context around the IRQ handler. Historically arch code would
> leave this to the irqchip or core IRQ code, but these days we want this
> to happen in exception entry code, and architectures such as arm64 do
> this.
> 
> Currently handle_domain_irq() performs this entry/exit accounting, and
> if used on an architecture where the entry code also does this, the
> entry/exit accounting will be performed twice per IRQ. This is
> problematic as core RCU code such as rcu_is_cpu_rrupt_from_idle()
> depends on this happening once per IRQ, and will not detect quescent
> periods correctly, leading to stall warnings.
> 
> As irqchip drivers which use handle_domain_irq() need to work on
> architectures with or without their own entry/exit accounting, this
> patch makes handle_domain_irq() conditionally perform the entry
> accounting depending on a new HAVE_ARCH_IRQENTRY Kconfig symbol that
> architectures can select if they perform this entry accounting
> themselves.
> 
> For architectures which do not select the symbol. there should be no
> functional change as a result of this patch.
> 
> Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
> Reviewed-by: Mark Rutland <mark.rutland@arm.com>
> Cc: "Paul E. McKenney" <paulmck@kernel.org>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Joey Gouly <joey.gouly@arm.com>
> Cc: Sami Tolvanen <samitolvanen@google.com>
> Cc: Julien Thierry <julien.thierry@arm.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Yuichi Ito <ito-yuichi@fujitsu.com>
> Cc: linux-kernel@vger.kernel.org
> To: linux-arm-kernel@lists.infradead.org
> ---
>  kernel/irq/Kconfig   | 3 +++
>  kernel/irq/irqdesc.c | 4 ++++
>  2 files changed, 7 insertions(+)
> 
> diff --git a/kernel/irq/Kconfig b/kernel/irq/Kconfig
> index fbc54c2a7f23..defa1db2d664 100644
> --- a/kernel/irq/Kconfig
> +++ b/kernel/irq/Kconfig
> @@ -100,6 +100,9 @@ config IRQ_MSI_IOMMU
>  config HANDLE_DOMAIN_IRQ
>  	bool
>  
> +config HAVE_ARCH_IRQENTRY
> +	bool
> +
>  config IRQ_TIMINGS
>  	bool
>  
> diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
> index 4e3c29bb603c..fd5dd9d278b5 100644
> --- a/kernel/irq/irqdesc.c
> +++ b/kernel/irq/irqdesc.c
> @@ -693,7 +693,9 @@ int handle_domain_irq(struct irq_domain *domain,
>  	struct irq_desc *desc;
>  	int ret = 0;
>  
> +#ifndef CONFIG_HAVE_ARCH_IRQENTRY
>  	irq_enter();
> +#endif

nit: I tend to prefer the 'if (!IS_ENABLED(CONFIG_*))' approach.

>  
>  	/* The irqdomain code provides boundary checks */
>  	desc = irq_resolve_mapping(domain, hwirq);
> @@ -702,7 +704,9 @@ int handle_domain_irq(struct irq_domain *domain,
>  	else
>  		ret = -EINVAL;
>  
> +#ifndef CONFIG_HAVE_ARCH_IRQENTRY
>  	irq_exit();
> +#endif
>  	set_irq_regs(old_regs);
>  	return ret;
>  }

Apart from that:

Reviewed-by: Marc Zyngier <maz@kernel.org>

Thanks,

	M.

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCHv3 1/3] kernel/irq: make irq_{enter, exit}() in handle_domain_irq() arch optional

[Pingfan Liu]

On Fri, Oct 01, 2021 at 10:15:16AM +0100, Marc Zyngier wrote:
> On Thu, 30 Sep 2021 14:17:06 +0100,
> Pingfan Liu <kernelfans@gmail.com> wrote:
> > 
> > When an IRQ is taken, some accounting needs to be performed to enter and
> > exit IRQ context around the IRQ handler. Historically arch code would
> > leave this to the irqchip or core IRQ code, but these days we want this
> > to happen in exception entry code, and architectures such as arm64 do
> > this.
> > 
> > Currently handle_domain_irq() performs this entry/exit accounting, and
> > if used on an architecture where the entry code also does this, the
> > entry/exit accounting will be performed twice per IRQ. This is
> > problematic as core RCU code such as rcu_is_cpu_rrupt_from_idle()
> > depends on this happening once per IRQ, and will not detect quescent
> > periods correctly, leading to stall warnings.
> > 
> > As irqchip drivers which use handle_domain_irq() need to work on
> > architectures with or without their own entry/exit accounting, this
> > patch makes handle_domain_irq() conditionally perform the entry
> > accounting depending on a new HAVE_ARCH_IRQENTRY Kconfig symbol that
> > architectures can select if they perform this entry accounting
> > themselves.
> > 
> > For architectures which do not select the symbol. there should be no
> > functional change as a result of this patch.
> > 
> > Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
> > Reviewed-by: Mark Rutland <mark.rutland@arm.com>
> > Cc: "Paul E. McKenney" <paulmck@kernel.org>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Cc: Will Deacon <will@kernel.org>
> > Cc: Marc Zyngier <maz@kernel.org>
> > Cc: Joey Gouly <joey.gouly@arm.com>
> > Cc: Sami Tolvanen <samitolvanen@google.com>
> > Cc: Julien Thierry <julien.thierry@arm.com>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Yuichi Ito <ito-yuichi@fujitsu.com>
> > Cc: linux-kernel@vger.kernel.org
> > To: linux-arm-kernel@lists.infradead.org
> > ---
> >  kernel/irq/Kconfig   | 3 +++
> >  kernel/irq/irqdesc.c | 4 ++++
> >  2 files changed, 7 insertions(+)
> > 
> > diff --git a/kernel/irq/Kconfig b/kernel/irq/Kconfig
> > index fbc54c2a7f23..defa1db2d664 100644
> > --- a/kernel/irq/Kconfig
> > +++ b/kernel/irq/Kconfig
> > @@ -100,6 +100,9 @@ config IRQ_MSI_IOMMU
> >  config HANDLE_DOMAIN_IRQ
> >  	bool
> >  
> > +config HAVE_ARCH_IRQENTRY
> > +	bool
> > +
> >  config IRQ_TIMINGS
> >  	bool
> >  
> > diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
> > index 4e3c29bb603c..fd5dd9d278b5 100644
> > --- a/kernel/irq/irqdesc.c
> > +++ b/kernel/irq/irqdesc.c
> > @@ -693,7 +693,9 @@ int handle_domain_irq(struct irq_domain *domain,
> >  	struct irq_desc *desc;
> >  	int ret = 0;
> >  
> > +#ifndef CONFIG_HAVE_ARCH_IRQENTRY
> >  	irq_enter();
> > +#endif
> 
> nit: I tend to prefer the 'if (!IS_ENABLED(CONFIG_*))' approach.
> 
I check the irqdesc.c, the other macro conditional statements have the style
"#ifdef". So if using 'if (!IS_ENABLED(CONFIG_*))', the file looks a
little disharmony.

> >  
> >  	/* The irqdomain code provides boundary checks */
> >  	desc = irq_resolve_mapping(domain, hwirq);
> > @@ -702,7 +704,9 @@ int handle_domain_irq(struct irq_domain *domain,
> >  	else
> >  		ret = -EINVAL;
> >  
> > +#ifndef CONFIG_HAVE_ARCH_IRQENTRY
> >  	irq_exit();
> > +#endif
> >  	set_irq_regs(old_regs);
> >  	return ret;
> >  }
> 
> Apart from that:
> 
> Reviewed-by: Marc Zyngier <maz@kernel.org>
> 
I am not sure whether you agree with my opinion about the style of macro
and hesitate to include your Reviewed-by.

Could you kindly give your Reviewed-by to my V4, if it is OK for you?


Thanks

	Pingfan

Re: [PATCHv3 1/3] kernel/irq: make irq_{enter, exit}() in handle_domain_irq() arch optional

[Pingfan Liu]

On Fri, Oct 01, 2021 at 10:15:16AM +0100, Marc Zyngier wrote:
> On Thu, 30 Sep 2021 14:17:06 +0100,
> Pingfan Liu <kernelfans@gmail.com> wrote:
> > 
> > When an IRQ is taken, some accounting needs to be performed to enter and
> > exit IRQ context around the IRQ handler. Historically arch code would
> > leave this to the irqchip or core IRQ code, but these days we want this
> > to happen in exception entry code, and architectures such as arm64 do
> > this.
> > 
> > Currently handle_domain_irq() performs this entry/exit accounting, and
> > if used on an architecture where the entry code also does this, the
> > entry/exit accounting will be performed twice per IRQ. This is
> > problematic as core RCU code such as rcu_is_cpu_rrupt_from_idle()
> > depends on this happening once per IRQ, and will not detect quescent
> > periods correctly, leading to stall warnings.
> > 
> > As irqchip drivers which use handle_domain_irq() need to work on
> > architectures with or without their own entry/exit accounting, this
> > patch makes handle_domain_irq() conditionally perform the entry
> > accounting depending on a new HAVE_ARCH_IRQENTRY Kconfig symbol that
> > architectures can select if they perform this entry accounting
> > themselves.
> > 
> > For architectures which do not select the symbol. there should be no
> > functional change as a result of this patch.
> > 
> > Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
> > Reviewed-by: Mark Rutland <mark.rutland@arm.com>
> > Cc: "Paul E. McKenney" <paulmck@kernel.org>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Cc: Will Deacon <will@kernel.org>
> > Cc: Marc Zyngier <maz@kernel.org>
> > Cc: Joey Gouly <joey.gouly@arm.com>
> > Cc: Sami Tolvanen <samitolvanen@google.com>
> > Cc: Julien Thierry <julien.thierry@arm.com>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Yuichi Ito <ito-yuichi@fujitsu.com>
> > Cc: linux-kernel@vger.kernel.org
> > To: linux-arm-kernel@lists.infradead.org
> > ---
> >  kernel/irq/Kconfig   | 3 +++
> >  kernel/irq/irqdesc.c | 4 ++++
> >  2 files changed, 7 insertions(+)
> > 
> > diff --git a/kernel/irq/Kconfig b/kernel/irq/Kconfig
> > index fbc54c2a7f23..defa1db2d664 100644
> > --- a/kernel/irq/Kconfig
> > +++ b/kernel/irq/Kconfig
> > @@ -100,6 +100,9 @@ config IRQ_MSI_IOMMU
> >  config HANDLE_DOMAIN_IRQ
> >  	bool
> >  
> > +config HAVE_ARCH_IRQENTRY
> > +	bool
> > +
> >  config IRQ_TIMINGS
> >  	bool
> >  
> > diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
> > index 4e3c29bb603c..fd5dd9d278b5 100644
> > --- a/kernel/irq/irqdesc.c
> > +++ b/kernel/irq/irqdesc.c
> > @@ -693,7 +693,9 @@ int handle_domain_irq(struct irq_domain *domain,
> >  	struct irq_desc *desc;
> >  	int ret = 0;
> >  
> > +#ifndef CONFIG_HAVE_ARCH_IRQENTRY
> >  	irq_enter();
> > +#endif
> 
> nit: I tend to prefer the 'if (!IS_ENABLED(CONFIG_*))' approach.
> 
I check the irqdesc.c, the other macro conditional statements have the style
"#ifdef". So if using 'if (!IS_ENABLED(CONFIG_*))', the file looks a
little disharmony.

> >  
> >  	/* The irqdomain code provides boundary checks */
> >  	desc = irq_resolve_mapping(domain, hwirq);
> > @@ -702,7 +704,9 @@ int handle_domain_irq(struct irq_domain *domain,
> >  	else
> >  		ret = -EINVAL;
> >  
> > +#ifndef CONFIG_HAVE_ARCH_IRQENTRY
> >  	irq_exit();
> > +#endif
> >  	set_irq_regs(old_regs);
> >  	return ret;
> >  }
> 
> Apart from that:
> 
> Reviewed-by: Marc Zyngier <maz@kernel.org>
> 
I am not sure whether you agree with my opinion about the style of macro
and hesitate to include your Reviewed-by.

Could you kindly give your Reviewed-by to my V4, if it is OK for you?


Thanks

	Pingfan

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCHv3 2/3] arm64: entry: refactor EL1 interrupt entry logic

[Pingfan Liu]

From: Mark Rutland <mark.rutland@arm.com>

Currently we distinguish IRQ and definitely-PNMI at entry/exit time
via the enter_el1_irq_or_nmi() and enter_el1_irq_or_nmi() helpers. In
subsequent patches we'll need to handle the two cases more distinctly
in the body of the exception handler.

To make this possible, this patch refactors el1_interrupt to be a
top-level dispatcher to separate handlers for the IRQ and PNMI cases,
removing the need for the enter_el1_irq_or_nmi() and
exit_el1_irq_or_nmi() helpers.

Note that since arm64_enter_nmi() calls __nmi_enter(), which
increments the preemt_count, we could never preempt when handling a
PNMI. We now only check for preemption in the IRQ case, which makes
this clearer.

There should be no functional change as a result of this patch.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: "Paul E. McKenney" <paulmck@kernel.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Joey Gouly <joey.gouly@arm.com>
Cc: Sami Tolvanen <samitolvanen@google.com>
Cc: Julien Thierry <julien.thierry@arm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Yuichi Ito <ito-yuichi@fujitsu.com>
Cc: Pingfan Liu <kernelfans@gmail.com>
Cc: linux-kernel@vger.kernel.org
To: linux-arm-kernel@lists.infradead.org
---
 arch/arm64/kernel/entry-common.c | 44 ++++++++++++++++----------------
 1 file changed, 22 insertions(+), 22 deletions(-)

diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c
index 32f9796c4ffe..5f1473319fb0 100644
--- a/arch/arm64/kernel/entry-common.c
+++ b/arch/arm64/kernel/entry-common.c
@@ -219,22 +219,6 @@ static void noinstr arm64_exit_el1_dbg(struct pt_regs *regs)
 		lockdep_hardirqs_on(CALLER_ADDR0);
 }
 
-static void noinstr enter_el1_irq_or_nmi(struct pt_regs *regs)
-{
-	if (IS_ENABLED(CONFIG_ARM64_PSEUDO_NMI) && !interrupts_enabled(regs))
-		arm64_enter_nmi(regs);
-	else
-		enter_from_kernel_mode(regs);
-}
-
-static void noinstr exit_el1_irq_or_nmi(struct pt_regs *regs)
-{
-	if (IS_ENABLED(CONFIG_ARM64_PSEUDO_NMI) && !interrupts_enabled(regs))
-		arm64_exit_nmi(regs);
-	else
-		exit_to_kernel_mode(regs);
-}
-
 static void __sched arm64_preempt_schedule_irq(void)
 {
 	lockdep_assert_irqs_disabled();
@@ -432,14 +416,19 @@ asmlinkage void noinstr el1h_64_sync_handler(struct pt_regs *regs)
 	}
 }
 
-static void noinstr el1_interrupt(struct pt_regs *regs,
-				  void (*handler)(struct pt_regs *))
+static __always_inline void
+__el1_pnmi(struct pt_regs *regs, void (*handler)(struct pt_regs *))
 {
-	write_sysreg(DAIF_PROCCTX_NOIRQ, daif);
-
-	enter_el1_irq_or_nmi(regs);
+	arm64_enter_nmi(regs);
 	do_interrupt_handler(regs, handler);
+	arm64_exit_nmi(regs);
+}
 
+static __always_inline void
+__el1_interrupt(struct pt_regs *regs, void (*handler)(struct pt_regs *))
+{
+	enter_from_kernel_mode(regs);
+	do_interrupt_handler(regs, handler);
 	/*
 	 * Note: thread_info::preempt_count includes both thread_info::count
 	 * and thread_info::need_resched, and is not equivalent to
@@ -448,8 +437,19 @@ static void noinstr el1_interrupt(struct pt_regs *regs,
 	if (IS_ENABLED(CONFIG_PREEMPTION) &&
 	    READ_ONCE(current_thread_info()->preempt_count) == 0)
 		arm64_preempt_schedule_irq();
+	exit_to_kernel_mode(regs);
+}
+
+static void noinstr el1_interrupt(struct pt_regs *regs,
+				  void (*handler)(struct pt_regs *))
+{
+	write_sysreg(DAIF_PROCCTX_NOIRQ, daif);
+
+	if (IS_ENABLED(CONFIG_ARM64_PSEUDO_NMI) && !interrupts_enabled(regs))
+		__el1_pnmi(regs, handler);
+	else
+		__el1_interrupt(regs, handler);
 
-	exit_el1_irq_or_nmi(regs);
 }
 
 asmlinkage void noinstr el1h_64_irq_handler(struct pt_regs *regs)
-- 
2.31.1

[PATCHv3 2/3] arm64: entry: refactor EL1 interrupt entry logic

[Pingfan Liu]

From: Mark Rutland <mark.rutland@arm.com>

Currently we distinguish IRQ and definitely-PNMI at entry/exit time
via the enter_el1_irq_or_nmi() and enter_el1_irq_or_nmi() helpers. In
subsequent patches we'll need to handle the two cases more distinctly
in the body of the exception handler.

To make this possible, this patch refactors el1_interrupt to be a
top-level dispatcher to separate handlers for the IRQ and PNMI cases,
removing the need for the enter_el1_irq_or_nmi() and
exit_el1_irq_or_nmi() helpers.

Note that since arm64_enter_nmi() calls __nmi_enter(), which
increments the preemt_count, we could never preempt when handling a
PNMI. We now only check for preemption in the IRQ case, which makes
this clearer.

There should be no functional change as a result of this patch.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: "Paul E. McKenney" <paulmck@kernel.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Joey Gouly <joey.gouly@arm.com>
Cc: Sami Tolvanen <samitolvanen@google.com>
Cc: Julien Thierry <julien.thierry@arm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Yuichi Ito <ito-yuichi@fujitsu.com>
Cc: Pingfan Liu <kernelfans@gmail.com>
Cc: linux-kernel@vger.kernel.org
To: linux-arm-kernel@lists.infradead.org
---
 arch/arm64/kernel/entry-common.c | 44 ++++++++++++++++----------------
 1 file changed, 22 insertions(+), 22 deletions(-)

diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c
index 32f9796c4ffe..5f1473319fb0 100644
--- a/arch/arm64/kernel/entry-common.c
+++ b/arch/arm64/kernel/entry-common.c
@@ -219,22 +219,6 @@ static void noinstr arm64_exit_el1_dbg(struct pt_regs *regs)
 		lockdep_hardirqs_on(CALLER_ADDR0);
 }
 
-static void noinstr enter_el1_irq_or_nmi(struct pt_regs *regs)
-{
-	if (IS_ENABLED(CONFIG_ARM64_PSEUDO_NMI) && !interrupts_enabled(regs))
-		arm64_enter_nmi(regs);
-	else
-		enter_from_kernel_mode(regs);
-}
-
-static void noinstr exit_el1_irq_or_nmi(struct pt_regs *regs)
-{
-	if (IS_ENABLED(CONFIG_ARM64_PSEUDO_NMI) && !interrupts_enabled(regs))
-		arm64_exit_nmi(regs);
-	else
-		exit_to_kernel_mode(regs);
-}
-
 static void __sched arm64_preempt_schedule_irq(void)
 {
 	lockdep_assert_irqs_disabled();
@@ -432,14 +416,19 @@ asmlinkage void noinstr el1h_64_sync_handler(struct pt_regs *regs)
 	}
 }
 
-static void noinstr el1_interrupt(struct pt_regs *regs,
-				  void (*handler)(struct pt_regs *))
+static __always_inline void
+__el1_pnmi(struct pt_regs *regs, void (*handler)(struct pt_regs *))
 {
-	write_sysreg(DAIF_PROCCTX_NOIRQ, daif);
-
-	enter_el1_irq_or_nmi(regs);
+	arm64_enter_nmi(regs);
 	do_interrupt_handler(regs, handler);
+	arm64_exit_nmi(regs);
+}
 
+static __always_inline void
+__el1_interrupt(struct pt_regs *regs, void (*handler)(struct pt_regs *))
+{
+	enter_from_kernel_mode(regs);
+	do_interrupt_handler(regs, handler);
 	/*
 	 * Note: thread_info::preempt_count includes both thread_info::count
 	 * and thread_info::need_resched, and is not equivalent to
@@ -448,8 +437,19 @@ static void noinstr el1_interrupt(struct pt_regs *regs,
 	if (IS_ENABLED(CONFIG_PREEMPTION) &&
 	    READ_ONCE(current_thread_info()->preempt_count) == 0)
 		arm64_preempt_schedule_irq();
+	exit_to_kernel_mode(regs);
+}
+
+static void noinstr el1_interrupt(struct pt_regs *regs,
+				  void (*handler)(struct pt_regs *))
+{
+	write_sysreg(DAIF_PROCCTX_NOIRQ, daif);
+
+	if (IS_ENABLED(CONFIG_ARM64_PSEUDO_NMI) && !interrupts_enabled(regs))
+		__el1_pnmi(regs, handler);
+	else
+		__el1_interrupt(regs, handler);
 
-	exit_el1_irq_or_nmi(regs);
 }
 
 asmlinkage void noinstr el1h_64_irq_handler(struct pt_regs *regs)
-- 
2.31.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCHv3 2/3] arm64: entry: refactor EL1 interrupt entry logic

[Mark Rutland]

On Thu, Sep 30, 2021 at 09:17:07PM +0800, Pingfan Liu wrote:
> From: Mark Rutland <mark.rutland@arm.com>
> 
> Currently we distinguish IRQ and definitely-PNMI at entry/exit time
> via the enter_el1_irq_or_nmi() and enter_el1_irq_or_nmi() helpers. In
> subsequent patches we'll need to handle the two cases more distinctly
> in the body of the exception handler.
> 
> To make this possible, this patch refactors el1_interrupt to be a
> top-level dispatcher to separate handlers for the IRQ and PNMI cases,
> removing the need for the enter_el1_irq_or_nmi() and
> exit_el1_irq_or_nmi() helpers.
> 
> Note that since arm64_enter_nmi() calls __nmi_enter(), which
> increments the preemt_count, we could never preempt when handling a
> PNMI. We now only check for preemption in the IRQ case, which makes
> this clearer.
> 
> There should be no functional change as a result of this patch.
> 
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: "Paul E. McKenney" <paulmck@kernel.org>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Joey Gouly <joey.gouly@arm.com>
> Cc: Sami Tolvanen <samitolvanen@google.com>
> Cc: Julien Thierry <julien.thierry@arm.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Yuichi Ito <ito-yuichi@fujitsu.com>
> Cc: Pingfan Liu <kernelfans@gmail.com>
> Cc: linux-kernel@vger.kernel.org
> To: linux-arm-kernel@lists.infradead.org

As a heads-up, you need to add your Signed-off-by tag when you post
patches from other people, even if you make no changes. See:

https://www.kernel.org/doc/html/v5.14/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin

Other than that, this looks fine to me.

Mark.

> ---
>  arch/arm64/kernel/entry-common.c | 44 ++++++++++++++++----------------
>  1 file changed, 22 insertions(+), 22 deletions(-)
> 
> diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c
> index 32f9796c4ffe..5f1473319fb0 100644
> --- a/arch/arm64/kernel/entry-common.c
> +++ b/arch/arm64/kernel/entry-common.c
> @@ -219,22 +219,6 @@ static void noinstr arm64_exit_el1_dbg(struct pt_regs *regs)
>  		lockdep_hardirqs_on(CALLER_ADDR0);
>  }
>  
> -static void noinstr enter_el1_irq_or_nmi(struct pt_regs *regs)
> -{
> -	if (IS_ENABLED(CONFIG_ARM64_PSEUDO_NMI) && !interrupts_enabled(regs))
> -		arm64_enter_nmi(regs);
> -	else
> -		enter_from_kernel_mode(regs);
> -}
> -
> -static void noinstr exit_el1_irq_or_nmi(struct pt_regs *regs)
> -{
> -	if (IS_ENABLED(CONFIG_ARM64_PSEUDO_NMI) && !interrupts_enabled(regs))
> -		arm64_exit_nmi(regs);
> -	else
> -		exit_to_kernel_mode(regs);
> -}
> -
>  static void __sched arm64_preempt_schedule_irq(void)
>  {
>  	lockdep_assert_irqs_disabled();
> @@ -432,14 +416,19 @@ asmlinkage void noinstr el1h_64_sync_handler(struct pt_regs *regs)
>  	}
>  }
>  
> -static void noinstr el1_interrupt(struct pt_regs *regs,
> -				  void (*handler)(struct pt_regs *))
> +static __always_inline void
> +__el1_pnmi(struct pt_regs *regs, void (*handler)(struct pt_regs *))
>  {
> -	write_sysreg(DAIF_PROCCTX_NOIRQ, daif);
> -
> -	enter_el1_irq_or_nmi(regs);
> +	arm64_enter_nmi(regs);
>  	do_interrupt_handler(regs, handler);
> +	arm64_exit_nmi(regs);
> +}
>  
> +static __always_inline void
> +__el1_interrupt(struct pt_regs *regs, void (*handler)(struct pt_regs *))
> +{
> +	enter_from_kernel_mode(regs);
> +	do_interrupt_handler(regs, handler);
>  	/*
>  	 * Note: thread_info::preempt_count includes both thread_info::count
>  	 * and thread_info::need_resched, and is not equivalent to
> @@ -448,8 +437,19 @@ static void noinstr el1_interrupt(struct pt_regs *regs,
>  	if (IS_ENABLED(CONFIG_PREEMPTION) &&
>  	    READ_ONCE(current_thread_info()->preempt_count) == 0)
>  		arm64_preempt_schedule_irq();
> +	exit_to_kernel_mode(regs);
> +}
> +
> +static void noinstr el1_interrupt(struct pt_regs *regs,
> +				  void (*handler)(struct pt_regs *))
> +{
> +	write_sysreg(DAIF_PROCCTX_NOIRQ, daif);
> +
> +	if (IS_ENABLED(CONFIG_ARM64_PSEUDO_NMI) && !interrupts_enabled(regs))
> +		__el1_pnmi(regs, handler);
> +	else
> +		__el1_interrupt(regs, handler);
>  
> -	exit_el1_irq_or_nmi(regs);
>  }
>  
>  asmlinkage void noinstr el1h_64_irq_handler(struct pt_regs *regs)
> -- 
> 2.31.1
> 

Re: [PATCHv3 2/3] arm64: entry: refactor EL1 interrupt entry logic

[Mark Rutland]

On Thu, Sep 30, 2021 at 09:17:07PM +0800, Pingfan Liu wrote:
> From: Mark Rutland <mark.rutland@arm.com>
> 
> Currently we distinguish IRQ and definitely-PNMI at entry/exit time
> via the enter_el1_irq_or_nmi() and enter_el1_irq_or_nmi() helpers. In
> subsequent patches we'll need to handle the two cases more distinctly
> in the body of the exception handler.
> 
> To make this possible, this patch refactors el1_interrupt to be a
> top-level dispatcher to separate handlers for the IRQ and PNMI cases,
> removing the need for the enter_el1_irq_or_nmi() and
> exit_el1_irq_or_nmi() helpers.
> 
> Note that since arm64_enter_nmi() calls __nmi_enter(), which
> increments the preemt_count, we could never preempt when handling a
> PNMI. We now only check for preemption in the IRQ case, which makes
> this clearer.
> 
> There should be no functional change as a result of this patch.
> 
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: "Paul E. McKenney" <paulmck@kernel.org>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Joey Gouly <joey.gouly@arm.com>
> Cc: Sami Tolvanen <samitolvanen@google.com>
> Cc: Julien Thierry <julien.thierry@arm.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Yuichi Ito <ito-yuichi@fujitsu.com>
> Cc: Pingfan Liu <kernelfans@gmail.com>
> Cc: linux-kernel@vger.kernel.org
> To: linux-arm-kernel@lists.infradead.org

As a heads-up, you need to add your Signed-off-by tag when you post
patches from other people, even if you make no changes. See:

https://www.kernel.org/doc/html/v5.14/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin

Other than that, this looks fine to me.

Mark.

> ---
>  arch/arm64/kernel/entry-common.c | 44 ++++++++++++++++----------------
>  1 file changed, 22 insertions(+), 22 deletions(-)
> 
> diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c
> index 32f9796c4ffe..5f1473319fb0 100644
> --- a/arch/arm64/kernel/entry-common.c
> +++ b/arch/arm64/kernel/entry-common.c
> @@ -219,22 +219,6 @@ static void noinstr arm64_exit_el1_dbg(struct pt_regs *regs)
>  		lockdep_hardirqs_on(CALLER_ADDR0);
>  }
>  
> -static void noinstr enter_el1_irq_or_nmi(struct pt_regs *regs)
> -{
> -	if (IS_ENABLED(CONFIG_ARM64_PSEUDO_NMI) && !interrupts_enabled(regs))
> -		arm64_enter_nmi(regs);
> -	else
> -		enter_from_kernel_mode(regs);
> -}
> -
> -static void noinstr exit_el1_irq_or_nmi(struct pt_regs *regs)
> -{
> -	if (IS_ENABLED(CONFIG_ARM64_PSEUDO_NMI) && !interrupts_enabled(regs))
> -		arm64_exit_nmi(regs);
> -	else
> -		exit_to_kernel_mode(regs);
> -}
> -
>  static void __sched arm64_preempt_schedule_irq(void)
>  {
>  	lockdep_assert_irqs_disabled();
> @@ -432,14 +416,19 @@ asmlinkage void noinstr el1h_64_sync_handler(struct pt_regs *regs)
>  	}
>  }
>  
> -static void noinstr el1_interrupt(struct pt_regs *regs,
> -				  void (*handler)(struct pt_regs *))
> +static __always_inline void
> +__el1_pnmi(struct pt_regs *regs, void (*handler)(struct pt_regs *))
>  {
> -	write_sysreg(DAIF_PROCCTX_NOIRQ, daif);
> -
> -	enter_el1_irq_or_nmi(regs);
> +	arm64_enter_nmi(regs);
>  	do_interrupt_handler(regs, handler);
> +	arm64_exit_nmi(regs);
> +}
>  
> +static __always_inline void
> +__el1_interrupt(struct pt_regs *regs, void (*handler)(struct pt_regs *))
> +{
> +	enter_from_kernel_mode(regs);
> +	do_interrupt_handler(regs, handler);
>  	/*
>  	 * Note: thread_info::preempt_count includes both thread_info::count
>  	 * and thread_info::need_resched, and is not equivalent to
> @@ -448,8 +437,19 @@ static void noinstr el1_interrupt(struct pt_regs *regs,
>  	if (IS_ENABLED(CONFIG_PREEMPTION) &&
>  	    READ_ONCE(current_thread_info()->preempt_count) == 0)
>  		arm64_preempt_schedule_irq();
> +	exit_to_kernel_mode(regs);
> +}
> +
> +static void noinstr el1_interrupt(struct pt_regs *regs,
> +				  void (*handler)(struct pt_regs *))
> +{
> +	write_sysreg(DAIF_PROCCTX_NOIRQ, daif);
> +
> +	if (IS_ENABLED(CONFIG_ARM64_PSEUDO_NMI) && !interrupts_enabled(regs))
> +		__el1_pnmi(regs, handler);
> +	else
> +		__el1_interrupt(regs, handler);
>  
> -	exit_el1_irq_or_nmi(regs);
>  }
>  
>  asmlinkage void noinstr el1h_64_irq_handler(struct pt_regs *regs)
> -- 
> 2.31.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCHv3 2/3] arm64: entry: refactor EL1 interrupt entry logic

[Pingfan Liu]

On Thu, Sep 30, 2021 at 03:00:59PM +0100, Mark Rutland wrote:
> On Thu, Sep 30, 2021 at 09:17:07PM +0800, Pingfan Liu wrote:
> > From: Mark Rutland <mark.rutland@arm.com>
> > 
> > Currently we distinguish IRQ and definitely-PNMI at entry/exit time
> > via the enter_el1_irq_or_nmi() and enter_el1_irq_or_nmi() helpers. In
> > subsequent patches we'll need to handle the two cases more distinctly
> > in the body of the exception handler.
> > 
> > To make this possible, this patch refactors el1_interrupt to be a
> > top-level dispatcher to separate handlers for the IRQ and PNMI cases,
> > removing the need for the enter_el1_irq_or_nmi() and
> > exit_el1_irq_or_nmi() helpers.
> > 
> > Note that since arm64_enter_nmi() calls __nmi_enter(), which
> > increments the preemt_count, we could never preempt when handling a
> > PNMI. We now only check for preemption in the IRQ case, which makes
> > this clearer.
> > 
> > There should be no functional change as a result of this patch.
> > 
> > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> > Cc: "Paul E. McKenney" <paulmck@kernel.org>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Cc: Will Deacon <will@kernel.org>
> > Cc: Marc Zyngier <maz@kernel.org>
> > Cc: Joey Gouly <joey.gouly@arm.com>
> > Cc: Sami Tolvanen <samitolvanen@google.com>
> > Cc: Julien Thierry <julien.thierry@arm.com>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Yuichi Ito <ito-yuichi@fujitsu.com>
> > Cc: Pingfan Liu <kernelfans@gmail.com>
> > Cc: linux-kernel@vger.kernel.org
> > To: linux-arm-kernel@lists.infradead.org
> 
> As a heads-up, you need to add your Signed-off-by tag when you post
> patches from other people, even if you make no changes. See:
> 
> https://www.kernel.org/doc/html/v5.14/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin
> 
Oh, thanks and I realize it is a serious license issue. 

> Other than that, this looks fine to me.
> 
Thank you very much.


Regards,

	Pingfan

Re: [PATCHv3 2/3] arm64: entry: refactor EL1 interrupt entry logic

[Pingfan Liu]

On Thu, Sep 30, 2021 at 03:00:59PM +0100, Mark Rutland wrote:
> On Thu, Sep 30, 2021 at 09:17:07PM +0800, Pingfan Liu wrote:
> > From: Mark Rutland <mark.rutland@arm.com>
> > 
> > Currently we distinguish IRQ and definitely-PNMI at entry/exit time
> > via the enter_el1_irq_or_nmi() and enter_el1_irq_or_nmi() helpers. In
> > subsequent patches we'll need to handle the two cases more distinctly
> > in the body of the exception handler.
> > 
> > To make this possible, this patch refactors el1_interrupt to be a
> > top-level dispatcher to separate handlers for the IRQ and PNMI cases,
> > removing the need for the enter_el1_irq_or_nmi() and
> > exit_el1_irq_or_nmi() helpers.
> > 
> > Note that since arm64_enter_nmi() calls __nmi_enter(), which
> > increments the preemt_count, we could never preempt when handling a
> > PNMI. We now only check for preemption in the IRQ case, which makes
> > this clearer.
> > 
> > There should be no functional change as a result of this patch.
> > 
> > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> > Cc: "Paul E. McKenney" <paulmck@kernel.org>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Cc: Will Deacon <will@kernel.org>
> > Cc: Marc Zyngier <maz@kernel.org>
> > Cc: Joey Gouly <joey.gouly@arm.com>
> > Cc: Sami Tolvanen <samitolvanen@google.com>
> > Cc: Julien Thierry <julien.thierry@arm.com>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Yuichi Ito <ito-yuichi@fujitsu.com>
> > Cc: Pingfan Liu <kernelfans@gmail.com>
> > Cc: linux-kernel@vger.kernel.org
> > To: linux-arm-kernel@lists.infradead.org
> 
> As a heads-up, you need to add your Signed-off-by tag when you post
> patches from other people, even if you make no changes. See:
> 
> https://www.kernel.org/doc/html/v5.14/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin
> 
Oh, thanks and I realize it is a serious license issue. 

> Other than that, this looks fine to me.
> 
Thank you very much.


Regards,

	Pingfan

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCHv3 2/3] arm64: entry: refactor EL1 interrupt entry logic

[Marc Zyngier]

On Thu, 30 Sep 2021 14:17:07 +0100,
Pingfan Liu <kernelfans@gmail.com> wrote:
> 
> From: Mark Rutland <mark.rutland@arm.com>
> 
> Currently we distinguish IRQ and definitely-PNMI at entry/exit time
> via the enter_el1_irq_or_nmi() and enter_el1_irq_or_nmi() helpers. In
> subsequent patches we'll need to handle the two cases more distinctly
> in the body of the exception handler.
> 
> To make this possible, this patch refactors el1_interrupt to be a
> top-level dispatcher to separate handlers for the IRQ and PNMI cases,
> removing the need for the enter_el1_irq_or_nmi() and
> exit_el1_irq_or_nmi() helpers.
> 
> Note that since arm64_enter_nmi() calls __nmi_enter(), which
> increments the preemt_count, we could never preempt when handling a
> PNMI. We now only check for preemption in the IRQ case, which makes
> this clearer.
> 
> There should be no functional change as a result of this patch.
> 
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: "Paul E. McKenney" <paulmck@kernel.org>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Joey Gouly <joey.gouly@arm.com>
> Cc: Sami Tolvanen <samitolvanen@google.com>
> Cc: Julien Thierry <julien.thierry@arm.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Yuichi Ito <ito-yuichi@fujitsu.com>
> Cc: Pingfan Liu <kernelfans@gmail.com>
> Cc: linux-kernel@vger.kernel.org
> To: linux-arm-kernel@lists.infradead.org
> ---
>  arch/arm64/kernel/entry-common.c | 44 ++++++++++++++++----------------
>  1 file changed, 22 insertions(+), 22 deletions(-)
> 
> diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c
> index 32f9796c4ffe..5f1473319fb0 100644
> --- a/arch/arm64/kernel/entry-common.c
> +++ b/arch/arm64/kernel/entry-common.c
> @@ -219,22 +219,6 @@ static void noinstr arm64_exit_el1_dbg(struct pt_regs *regs)
>  		lockdep_hardirqs_on(CALLER_ADDR0);
>  }
>  
> -static void noinstr enter_el1_irq_or_nmi(struct pt_regs *regs)
> -{
> -	if (IS_ENABLED(CONFIG_ARM64_PSEUDO_NMI) && !interrupts_enabled(regs))
> -		arm64_enter_nmi(regs);
> -	else
> -		enter_from_kernel_mode(regs);
> -}
> -
> -static void noinstr exit_el1_irq_or_nmi(struct pt_regs *regs)
> -{
> -	if (IS_ENABLED(CONFIG_ARM64_PSEUDO_NMI) && !interrupts_enabled(regs))
> -		arm64_exit_nmi(regs);
> -	else
> -		exit_to_kernel_mode(regs);
> -}
> -
>  static void __sched arm64_preempt_schedule_irq(void)
>  {
>  	lockdep_assert_irqs_disabled();
> @@ -432,14 +416,19 @@ asmlinkage void noinstr el1h_64_sync_handler(struct pt_regs *regs)
>  	}
>  }
>  
> -static void noinstr el1_interrupt(struct pt_regs *regs,
> -				  void (*handler)(struct pt_regs *))
> +static __always_inline void
> +__el1_pnmi(struct pt_regs *regs, void (*handler)(struct pt_regs *))
>  {
> -	write_sysreg(DAIF_PROCCTX_NOIRQ, daif);
> -
> -	enter_el1_irq_or_nmi(regs);
> +	arm64_enter_nmi(regs);
>  	do_interrupt_handler(regs, handler);
> +	arm64_exit_nmi(regs);
> +}
>  
> +static __always_inline void
> +__el1_interrupt(struct pt_regs *regs, void (*handler)(struct pt_regs *))
> +{
> +	enter_from_kernel_mode(regs);
> +	do_interrupt_handler(regs, handler);
>  	/*
>  	 * Note: thread_info::preempt_count includes both thread_info::count
>  	 * and thread_info::need_resched, and is not equivalent to
> @@ -448,8 +437,19 @@ static void noinstr el1_interrupt(struct pt_regs *regs,
>  	if (IS_ENABLED(CONFIG_PREEMPTION) &&
>  	    READ_ONCE(current_thread_info()->preempt_count) == 0)
>  		arm64_preempt_schedule_irq();
> +	exit_to_kernel_mode(regs);
> +}
> +
> +static void noinstr el1_interrupt(struct pt_regs *regs,
> +				  void (*handler)(struct pt_regs *))
> +{
> +	write_sysreg(DAIF_PROCCTX_NOIRQ, daif);
> +
> +	if (IS_ENABLED(CONFIG_ARM64_PSEUDO_NMI) && !interrupts_enabled(regs))
> +		__el1_pnmi(regs, handler);
> +	else
> +		__el1_interrupt(regs, handler);
>  
> -	exit_el1_irq_or_nmi(regs);

nit: spurious blank line.

>  }
>  
>  asmlinkage void noinstr el1h_64_irq_handler(struct pt_regs *regs)

With Mark's remark addressed,

Reviewed-by: Marc Zyngier <maz@kernel.org>

	M.

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

Re: [PATCHv3 2/3] arm64: entry: refactor EL1 interrupt entry logic

[Marc Zyngier]

On Thu, 30 Sep 2021 14:17:07 +0100,
Pingfan Liu <kernelfans@gmail.com> wrote:
> 
> From: Mark Rutland <mark.rutland@arm.com>
> 
> Currently we distinguish IRQ and definitely-PNMI at entry/exit time
> via the enter_el1_irq_or_nmi() and enter_el1_irq_or_nmi() helpers. In
> subsequent patches we'll need to handle the two cases more distinctly
> in the body of the exception handler.
> 
> To make this possible, this patch refactors el1_interrupt to be a
> top-level dispatcher to separate handlers for the IRQ and PNMI cases,
> removing the need for the enter_el1_irq_or_nmi() and
> exit_el1_irq_or_nmi() helpers.
> 
> Note that since arm64_enter_nmi() calls __nmi_enter(), which
> increments the preemt_count, we could never preempt when handling a
> PNMI. We now only check for preemption in the IRQ case, which makes
> this clearer.
> 
> There should be no functional change as a result of this patch.
> 
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: "Paul E. McKenney" <paulmck@kernel.org>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Joey Gouly <joey.gouly@arm.com>
> Cc: Sami Tolvanen <samitolvanen@google.com>
> Cc: Julien Thierry <julien.thierry@arm.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Yuichi Ito <ito-yuichi@fujitsu.com>
> Cc: Pingfan Liu <kernelfans@gmail.com>
> Cc: linux-kernel@vger.kernel.org
> To: linux-arm-kernel@lists.infradead.org
> ---
>  arch/arm64/kernel/entry-common.c | 44 ++++++++++++++++----------------
>  1 file changed, 22 insertions(+), 22 deletions(-)
> 
> diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c
> index 32f9796c4ffe..5f1473319fb0 100644
> --- a/arch/arm64/kernel/entry-common.c
> +++ b/arch/arm64/kernel/entry-common.c
> @@ -219,22 +219,6 @@ static void noinstr arm64_exit_el1_dbg(struct pt_regs *regs)
>  		lockdep_hardirqs_on(CALLER_ADDR0);
>  }
>  
> -static void noinstr enter_el1_irq_or_nmi(struct pt_regs *regs)
> -{
> -	if (IS_ENABLED(CONFIG_ARM64_PSEUDO_NMI) && !interrupts_enabled(regs))
> -		arm64_enter_nmi(regs);
> -	else
> -		enter_from_kernel_mode(regs);
> -}
> -
> -static void noinstr exit_el1_irq_or_nmi(struct pt_regs *regs)
> -{
> -	if (IS_ENABLED(CONFIG_ARM64_PSEUDO_NMI) && !interrupts_enabled(regs))
> -		arm64_exit_nmi(regs);
> -	else
> -		exit_to_kernel_mode(regs);
> -}
> -
>  static void __sched arm64_preempt_schedule_irq(void)
>  {
>  	lockdep_assert_irqs_disabled();
> @@ -432,14 +416,19 @@ asmlinkage void noinstr el1h_64_sync_handler(struct pt_regs *regs)
>  	}
>  }
>  
> -static void noinstr el1_interrupt(struct pt_regs *regs,
> -				  void (*handler)(struct pt_regs *))
> +static __always_inline void
> +__el1_pnmi(struct pt_regs *regs, void (*handler)(struct pt_regs *))
>  {
> -	write_sysreg(DAIF_PROCCTX_NOIRQ, daif);
> -
> -	enter_el1_irq_or_nmi(regs);
> +	arm64_enter_nmi(regs);
>  	do_interrupt_handler(regs, handler);
> +	arm64_exit_nmi(regs);
> +}
>  
> +static __always_inline void
> +__el1_interrupt(struct pt_regs *regs, void (*handler)(struct pt_regs *))
> +{
> +	enter_from_kernel_mode(regs);
> +	do_interrupt_handler(regs, handler);
>  	/*
>  	 * Note: thread_info::preempt_count includes both thread_info::count
>  	 * and thread_info::need_resched, and is not equivalent to
> @@ -448,8 +437,19 @@ static void noinstr el1_interrupt(struct pt_regs *regs,
>  	if (IS_ENABLED(CONFIG_PREEMPTION) &&
>  	    READ_ONCE(current_thread_info()->preempt_count) == 0)
>  		arm64_preempt_schedule_irq();
> +	exit_to_kernel_mode(regs);
> +}
> +
> +static void noinstr el1_interrupt(struct pt_regs *regs,
> +				  void (*handler)(struct pt_regs *))
> +{
> +	write_sysreg(DAIF_PROCCTX_NOIRQ, daif);
> +
> +	if (IS_ENABLED(CONFIG_ARM64_PSEUDO_NMI) && !interrupts_enabled(regs))
> +		__el1_pnmi(regs, handler);
> +	else
> +		__el1_interrupt(regs, handler);
>  
> -	exit_el1_irq_or_nmi(regs);

nit: spurious blank line.

>  }
>  
>  asmlinkage void noinstr el1h_64_irq_handler(struct pt_regs *regs)

With Mark's remark addressed,

Reviewed-by: Marc Zyngier <maz@kernel.org>

	M.

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCHv3 2/3] arm64: entry: refactor EL1 interrupt entry logic

[Pingfan Liu]

On Fri, Oct 01, 2021 at 10:21:06AM +0100, Marc Zyngier wrote:
> On Thu, 30 Sep 2021 14:17:07 +0100,
> Pingfan Liu <kernelfans@gmail.com> wrote:
> > 
> > From: Mark Rutland <mark.rutland@arm.com>
> > 
> > Currently we distinguish IRQ and definitely-PNMI at entry/exit time
> > via the enter_el1_irq_or_nmi() and enter_el1_irq_or_nmi() helpers. In
> > subsequent patches we'll need to handle the two cases more distinctly
> > in the body of the exception handler.
> > 
> > To make this possible, this patch refactors el1_interrupt to be a
> > top-level dispatcher to separate handlers for the IRQ and PNMI cases,
> > removing the need for the enter_el1_irq_or_nmi() and
> > exit_el1_irq_or_nmi() helpers.
> > 
> > Note that since arm64_enter_nmi() calls __nmi_enter(), which
> > increments the preemt_count, we could never preempt when handling a
> > PNMI. We now only check for preemption in the IRQ case, which makes
> > this clearer.
> > 
> > There should be no functional change as a result of this patch.
> > 
> > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> > Cc: "Paul E. McKenney" <paulmck@kernel.org>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Cc: Will Deacon <will@kernel.org>
> > Cc: Marc Zyngier <maz@kernel.org>
> > Cc: Joey Gouly <joey.gouly@arm.com>
> > Cc: Sami Tolvanen <samitolvanen@google.com>
> > Cc: Julien Thierry <julien.thierry@arm.com>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Yuichi Ito <ito-yuichi@fujitsu.com>
> > Cc: Pingfan Liu <kernelfans@gmail.com>
> > Cc: linux-kernel@vger.kernel.org
> > To: linux-arm-kernel@lists.infradead.org
> > ---
> >  arch/arm64/kernel/entry-common.c | 44 ++++++++++++++++----------------
> >  1 file changed, 22 insertions(+), 22 deletions(-)
> > 
> > diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c
> > index 32f9796c4ffe..5f1473319fb0 100644
> > --- a/arch/arm64/kernel/entry-common.c
> > +++ b/arch/arm64/kernel/entry-common.c
> > @@ -219,22 +219,6 @@ static void noinstr arm64_exit_el1_dbg(struct pt_regs *regs)
> >  		lockdep_hardirqs_on(CALLER_ADDR0);
> >  }
> >  
> > -static void noinstr enter_el1_irq_or_nmi(struct pt_regs *regs)
> > -{
> > -	if (IS_ENABLED(CONFIG_ARM64_PSEUDO_NMI) && !interrupts_enabled(regs))
> > -		arm64_enter_nmi(regs);
> > -	else
> > -		enter_from_kernel_mode(regs);
> > -}
> > -
> > -static void noinstr exit_el1_irq_or_nmi(struct pt_regs *regs)
> > -{
> > -	if (IS_ENABLED(CONFIG_ARM64_PSEUDO_NMI) && !interrupts_enabled(regs))
> > -		arm64_exit_nmi(regs);
> > -	else
> > -		exit_to_kernel_mode(regs);
> > -}
> > -
> >  static void __sched arm64_preempt_schedule_irq(void)
> >  {
> >  	lockdep_assert_irqs_disabled();
> > @@ -432,14 +416,19 @@ asmlinkage void noinstr el1h_64_sync_handler(struct pt_regs *regs)
> >  	}
> >  }
> >  
> > -static void noinstr el1_interrupt(struct pt_regs *regs,
> > -				  void (*handler)(struct pt_regs *))
> > +static __always_inline void
> > +__el1_pnmi(struct pt_regs *regs, void (*handler)(struct pt_regs *))
> >  {
> > -	write_sysreg(DAIF_PROCCTX_NOIRQ, daif);
> > -
> > -	enter_el1_irq_or_nmi(regs);
> > +	arm64_enter_nmi(regs);
> >  	do_interrupt_handler(regs, handler);
> > +	arm64_exit_nmi(regs);
> > +}
> >  
> > +static __always_inline void
> > +__el1_interrupt(struct pt_regs *regs, void (*handler)(struct pt_regs *))
> > +{
> > +	enter_from_kernel_mode(regs);
> > +	do_interrupt_handler(regs, handler);
> >  	/*
> >  	 * Note: thread_info::preempt_count includes both thread_info::count
> >  	 * and thread_info::need_resched, and is not equivalent to
> > @@ -448,8 +437,19 @@ static void noinstr el1_interrupt(struct pt_regs *regs,
> >  	if (IS_ENABLED(CONFIG_PREEMPTION) &&
> >  	    READ_ONCE(current_thread_info()->preempt_count) == 0)
> >  		arm64_preempt_schedule_irq();
> > +	exit_to_kernel_mode(regs);
> > +}
> > +
> > +static void noinstr el1_interrupt(struct pt_regs *regs,
> > +				  void (*handler)(struct pt_regs *))
> > +{
> > +	write_sysreg(DAIF_PROCCTX_NOIRQ, daif);
> > +
> > +	if (IS_ENABLED(CONFIG_ARM64_PSEUDO_NMI) && !interrupts_enabled(regs))
> > +		__el1_pnmi(regs, handler);
> > +	else
> > +		__el1_interrupt(regs, handler);
> >  
> > -	exit_el1_irq_or_nmi(regs);
> 
> nit: spurious blank line.
> 
OK, I will fix it.

> >  }
> >  
> >  asmlinkage void noinstr el1h_64_irq_handler(struct pt_regs *regs)
> 
> With Mark's remark addressed,
> 
> Reviewed-by: Marc Zyngier <maz@kernel.org>
> 
Thanks,

	Pingfan

Re: [PATCHv3 2/3] arm64: entry: refactor EL1 interrupt entry logic

[Pingfan Liu]

On Fri, Oct 01, 2021 at 10:21:06AM +0100, Marc Zyngier wrote:
> On Thu, 30 Sep 2021 14:17:07 +0100,
> Pingfan Liu <kernelfans@gmail.com> wrote:
> > 
> > From: Mark Rutland <mark.rutland@arm.com>
> > 
> > Currently we distinguish IRQ and definitely-PNMI at entry/exit time
> > via the enter_el1_irq_or_nmi() and enter_el1_irq_or_nmi() helpers. In
> > subsequent patches we'll need to handle the two cases more distinctly
> > in the body of the exception handler.
> > 
> > To make this possible, this patch refactors el1_interrupt to be a
> > top-level dispatcher to separate handlers for the IRQ and PNMI cases,
> > removing the need for the enter_el1_irq_or_nmi() and
> > exit_el1_irq_or_nmi() helpers.
> > 
> > Note that since arm64_enter_nmi() calls __nmi_enter(), which
> > increments the preemt_count, we could never preempt when handling a
> > PNMI. We now only check for preemption in the IRQ case, which makes
> > this clearer.
> > 
> > There should be no functional change as a result of this patch.
> > 
> > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> > Cc: "Paul E. McKenney" <paulmck@kernel.org>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Cc: Will Deacon <will@kernel.org>
> > Cc: Marc Zyngier <maz@kernel.org>
> > Cc: Joey Gouly <joey.gouly@arm.com>
> > Cc: Sami Tolvanen <samitolvanen@google.com>
> > Cc: Julien Thierry <julien.thierry@arm.com>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Yuichi Ito <ito-yuichi@fujitsu.com>
> > Cc: Pingfan Liu <kernelfans@gmail.com>
> > Cc: linux-kernel@vger.kernel.org
> > To: linux-arm-kernel@lists.infradead.org
> > ---
> >  arch/arm64/kernel/entry-common.c | 44 ++++++++++++++++----------------
> >  1 file changed, 22 insertions(+), 22 deletions(-)
> > 
> > diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c
> > index 32f9796c4ffe..5f1473319fb0 100644
> > --- a/arch/arm64/kernel/entry-common.c
> > +++ b/arch/arm64/kernel/entry-common.c
> > @@ -219,22 +219,6 @@ static void noinstr arm64_exit_el1_dbg(struct pt_regs *regs)
> >  		lockdep_hardirqs_on(CALLER_ADDR0);
> >  }
> >  
> > -static void noinstr enter_el1_irq_or_nmi(struct pt_regs *regs)
> > -{
> > -	if (IS_ENABLED(CONFIG_ARM64_PSEUDO_NMI) && !interrupts_enabled(regs))
> > -		arm64_enter_nmi(regs);
> > -	else
> > -		enter_from_kernel_mode(regs);
> > -}
> > -
> > -static void noinstr exit_el1_irq_or_nmi(struct pt_regs *regs)
> > -{
> > -	if (IS_ENABLED(CONFIG_ARM64_PSEUDO_NMI) && !interrupts_enabled(regs))
> > -		arm64_exit_nmi(regs);
> > -	else
> > -		exit_to_kernel_mode(regs);
> > -}
> > -
> >  static void __sched arm64_preempt_schedule_irq(void)
> >  {
> >  	lockdep_assert_irqs_disabled();
> > @@ -432,14 +416,19 @@ asmlinkage void noinstr el1h_64_sync_handler(struct pt_regs *regs)
> >  	}
> >  }
> >  
> > -static void noinstr el1_interrupt(struct pt_regs *regs,
> > -				  void (*handler)(struct pt_regs *))
> > +static __always_inline void
> > +__el1_pnmi(struct pt_regs *regs, void (*handler)(struct pt_regs *))
> >  {
> > -	write_sysreg(DAIF_PROCCTX_NOIRQ, daif);
> > -
> > -	enter_el1_irq_or_nmi(regs);
> > +	arm64_enter_nmi(regs);
> >  	do_interrupt_handler(regs, handler);
> > +	arm64_exit_nmi(regs);
> > +}
> >  
> > +static __always_inline void
> > +__el1_interrupt(struct pt_regs *regs, void (*handler)(struct pt_regs *))
> > +{
> > +	enter_from_kernel_mode(regs);
> > +	do_interrupt_handler(regs, handler);
> >  	/*
> >  	 * Note: thread_info::preempt_count includes both thread_info::count
> >  	 * and thread_info::need_resched, and is not equivalent to
> > @@ -448,8 +437,19 @@ static void noinstr el1_interrupt(struct pt_regs *regs,
> >  	if (IS_ENABLED(CONFIG_PREEMPTION) &&
> >  	    READ_ONCE(current_thread_info()->preempt_count) == 0)
> >  		arm64_preempt_schedule_irq();
> > +	exit_to_kernel_mode(regs);
> > +}
> > +
> > +static void noinstr el1_interrupt(struct pt_regs *regs,
> > +				  void (*handler)(struct pt_regs *))
> > +{
> > +	write_sysreg(DAIF_PROCCTX_NOIRQ, daif);
> > +
> > +	if (IS_ENABLED(CONFIG_ARM64_PSEUDO_NMI) && !interrupts_enabled(regs))
> > +		__el1_pnmi(regs, handler);
> > +	else
> > +		__el1_interrupt(regs, handler);
> >  
> > -	exit_el1_irq_or_nmi(regs);
> 
> nit: spurious blank line.
> 
OK, I will fix it.

> >  }
> >  
> >  asmlinkage void noinstr el1h_64_irq_handler(struct pt_regs *regs)
> 
> With Mark's remark addressed,
> 
> Reviewed-by: Marc Zyngier <maz@kernel.org>
> 
Thanks,

	Pingfan

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCHv3 3/3] arm64/entry-common: supplement irq accounting

[Pingfan Liu]

At present, the irq entry/exit accounting, which is performed by
handle_domain_irq(), overlaps with arm64 exception entry code somehow.

By supplementing irq accounting on arm64 exception entry code, the
accounting in handle_domain_irq() can be dropped totally by selecting
the macro HAVE_ARCH_IRQENTRY.

Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
Cc: "Paul E. McKenney" <paulmck@kernel.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Joey Gouly <joey.gouly@arm.com>
Cc: Sami Tolvanen <samitolvanen@google.com>
Cc: Julien Thierry <julien.thierry@arm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Yuichi Ito <ito-yuichi@fujitsu.com>
Cc: linux-kernel@vger.kernel.org
To: linux-arm-kernel@lists.infradead.org
---
 arch/arm64/Kconfig               | 1 +
 arch/arm64/kernel/entry-common.c | 4 ++++
 2 files changed, 5 insertions(+)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 5c7ae4c3954b..d29bae38a951 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -98,6 +98,7 @@ config ARM64
 	select ARCH_HAS_UBSAN_SANITIZE_ALL
 	select ARM_AMBA
 	select ARM_ARCH_TIMER
+	select HAVE_ARCH_IRQENTRY
 	select ARM_GIC
 	select AUDIT_ARCH_COMPAT_GENERIC
 	select ARM_GIC_V2M if PCI
diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c
index 5f1473319fb0..6d4dc3b3799f 100644
--- a/arch/arm64/kernel/entry-common.c
+++ b/arch/arm64/kernel/entry-common.c
@@ -428,7 +428,9 @@ static __always_inline void
 __el1_interrupt(struct pt_regs *regs, void (*handler)(struct pt_regs *))
 {
 	enter_from_kernel_mode(regs);
+	irq_enter_rcu();
 	do_interrupt_handler(regs, handler);
+	irq_exit_rcu();
 	/*
 	 * Note: thread_info::preempt_count includes both thread_info::count
 	 * and thread_info::need_resched, and is not equivalent to
@@ -667,7 +669,9 @@ static void noinstr el0_interrupt(struct pt_regs *regs,
 	if (regs->pc & BIT(55))
 		arm64_apply_bp_hardening();
 
+	irq_enter_rcu();
 	do_interrupt_handler(regs, handler);
+	irq_exit_rcu();
 
 	exit_to_user_mode(regs);
 }
-- 
2.31.1

[PATCHv3 3/3] arm64/entry-common: supplement irq accounting

[Pingfan Liu]

At present, the irq entry/exit accounting, which is performed by
handle_domain_irq(), overlaps with arm64 exception entry code somehow.

By supplementing irq accounting on arm64 exception entry code, the
accounting in handle_domain_irq() can be dropped totally by selecting
the macro HAVE_ARCH_IRQENTRY.

Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
Cc: "Paul E. McKenney" <paulmck@kernel.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Joey Gouly <joey.gouly@arm.com>
Cc: Sami Tolvanen <samitolvanen@google.com>
Cc: Julien Thierry <julien.thierry@arm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Yuichi Ito <ito-yuichi@fujitsu.com>
Cc: linux-kernel@vger.kernel.org
To: linux-arm-kernel@lists.infradead.org
---
 arch/arm64/Kconfig               | 1 +
 arch/arm64/kernel/entry-common.c | 4 ++++
 2 files changed, 5 insertions(+)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 5c7ae4c3954b..d29bae38a951 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -98,6 +98,7 @@ config ARM64
 	select ARCH_HAS_UBSAN_SANITIZE_ALL
 	select ARM_AMBA
 	select ARM_ARCH_TIMER
+	select HAVE_ARCH_IRQENTRY
 	select ARM_GIC
 	select AUDIT_ARCH_COMPAT_GENERIC
 	select ARM_GIC_V2M if PCI
diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c
index 5f1473319fb0..6d4dc3b3799f 100644
--- a/arch/arm64/kernel/entry-common.c
+++ b/arch/arm64/kernel/entry-common.c
@@ -428,7 +428,9 @@ static __always_inline void
 __el1_interrupt(struct pt_regs *regs, void (*handler)(struct pt_regs *))
 {
 	enter_from_kernel_mode(regs);
+	irq_enter_rcu();
 	do_interrupt_handler(regs, handler);
+	irq_exit_rcu();
 	/*
 	 * Note: thread_info::preempt_count includes both thread_info::count
 	 * and thread_info::need_resched, and is not equivalent to
@@ -667,7 +669,9 @@ static void noinstr el0_interrupt(struct pt_regs *regs,
 	if (regs->pc & BIT(55))
 		arm64_apply_bp_hardening();
 
+	irq_enter_rcu();
 	do_interrupt_handler(regs, handler);
+	irq_exit_rcu();
 
 	exit_to_user_mode(regs);
 }
-- 
2.31.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCHv3 3/3] arm64/entry-common: supplement irq accounting

[Mark Rutland]

On Thu, Sep 30, 2021 at 09:17:08PM +0800, Pingfan Liu wrote:
> At present, the irq entry/exit accounting, which is performed by
> handle_domain_irq(), overlaps with arm64 exception entry code somehow.
> 
> By supplementing irq accounting on arm64 exception entry code, the
> accounting in handle_domain_irq() can be dropped totally by selecting
> the macro HAVE_ARCH_IRQENTRY.

I think we need to be more thorough and explain the specific problem and
solution. How about we crib some wording from patch 1, and say:

  arm64: entry: avoid double-accounting IRQ RCU entry

  When an IRQ is taken, some accounting needs to be performed to enter
  and exit IRQ context around the IRQ handler. On arm64 some of this
  accounting is performed by both the architecture code and the IRQ
  domain code, resulting in calling rcu_irq_enter() twice per exception
  entry, violating the expectations of the core RCU code, and resulting
  in failing to identify quiescent periods correctly (e.g. in
  rcu_is_cpu_rrupt_from_idle()).

  To fix this, we must perform all the accounting from the architecture
  code. We prevent the IRQ domain code from performing any accounting by
  selecting HAVE_ARCH_IRQENTRY, and must call irq_enter_rcu() and
  irq_exit_rcu() around invoking the root IRQ handler.

  When we take a pNMI from a context with IRQs disabled, we'll perform
  the necessary accounting as part of arm64_enter_nmi() and
  arm64_exit_nmi(), and should only call irq_enter_rcu() and
  irq_exit_rcu() when we may have taken a regular interrupt.

That way it's clear what specifically the overlap is and the problem(s)
it results in. The bit at the end explains why we don't call
irq_{enter,exit}_rcu() when we're certain we've taken a pNMI.

> Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
> Cc: "Paul E. McKenney" <paulmck@kernel.org>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Joey Gouly <joey.gouly@arm.com>
> Cc: Sami Tolvanen <samitolvanen@google.com>
> Cc: Julien Thierry <julien.thierry@arm.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Yuichi Ito <ito-yuichi@fujitsu.com>
> Cc: linux-kernel@vger.kernel.org
> To: linux-arm-kernel@lists.infradead.org
> ---
>  arch/arm64/Kconfig               | 1 +
>  arch/arm64/kernel/entry-common.c | 4 ++++
>  2 files changed, 5 insertions(+)
> 
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 5c7ae4c3954b..d29bae38a951 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -98,6 +98,7 @@ config ARM64
>  	select ARCH_HAS_UBSAN_SANITIZE_ALL
>  	select ARM_AMBA
>  	select ARM_ARCH_TIMER
> +	select HAVE_ARCH_IRQENTRY

Please put this with the other HAVE_ARCH_* entries in
arch/arm64/Kconfig -- it should be between HAVE_ARCH_HUGE_VMAP and
HAVE_ARCH_JUMP_LABEL to keep that in alphabetical order.

With that and the title and commit message above:

Reviewed-by: Mark Rutland <mark.rutland@arm.com>

Thanks,
Mark.

>  	select ARM_GIC
>  	select AUDIT_ARCH_COMPAT_GENERIC
>  	select ARM_GIC_V2M if PCI
> diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c
> index 5f1473319fb0..6d4dc3b3799f 100644
> --- a/arch/arm64/kernel/entry-common.c
> +++ b/arch/arm64/kernel/entry-common.c
> @@ -428,7 +428,9 @@ static __always_inline void
>  __el1_interrupt(struct pt_regs *regs, void (*handler)(struct pt_regs *))
>  {
>  	enter_from_kernel_mode(regs);
> +	irq_enter_rcu();
>  	do_interrupt_handler(regs, handler);
> +	irq_exit_rcu();
>  	/*
>  	 * Note: thread_info::preempt_count includes both thread_info::count
>  	 * and thread_info::need_resched, and is not equivalent to
> @@ -667,7 +669,9 @@ static void noinstr el0_interrupt(struct pt_regs *regs,
>  	if (regs->pc & BIT(55))
>  		arm64_apply_bp_hardening();
>  
> +	irq_enter_rcu();
>  	do_interrupt_handler(regs, handler);
> +	irq_exit_rcu();
>  
>  	exit_to_user_mode(regs);
>  }
> -- 
> 2.31.1
> 

Re: [PATCHv3 3/3] arm64/entry-common: supplement irq accounting

[Mark Rutland]

On Thu, Sep 30, 2021 at 09:17:08PM +0800, Pingfan Liu wrote:
> At present, the irq entry/exit accounting, which is performed by
> handle_domain_irq(), overlaps with arm64 exception entry code somehow.
> 
> By supplementing irq accounting on arm64 exception entry code, the
> accounting in handle_domain_irq() can be dropped totally by selecting
> the macro HAVE_ARCH_IRQENTRY.

I think we need to be more thorough and explain the specific problem and
solution. How about we crib some wording from patch 1, and say:

  arm64: entry: avoid double-accounting IRQ RCU entry

  When an IRQ is taken, some accounting needs to be performed to enter
  and exit IRQ context around the IRQ handler. On arm64 some of this
  accounting is performed by both the architecture code and the IRQ
  domain code, resulting in calling rcu_irq_enter() twice per exception
  entry, violating the expectations of the core RCU code, and resulting
  in failing to identify quiescent periods correctly (e.g. in
  rcu_is_cpu_rrupt_from_idle()).

  To fix this, we must perform all the accounting from the architecture
  code. We prevent the IRQ domain code from performing any accounting by
  selecting HAVE_ARCH_IRQENTRY, and must call irq_enter_rcu() and
  irq_exit_rcu() around invoking the root IRQ handler.

  When we take a pNMI from a context with IRQs disabled, we'll perform
  the necessary accounting as part of arm64_enter_nmi() and
  arm64_exit_nmi(), and should only call irq_enter_rcu() and
  irq_exit_rcu() when we may have taken a regular interrupt.

That way it's clear what specifically the overlap is and the problem(s)
it results in. The bit at the end explains why we don't call
irq_{enter,exit}_rcu() when we're certain we've taken a pNMI.

> Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
> Cc: "Paul E. McKenney" <paulmck@kernel.org>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Joey Gouly <joey.gouly@arm.com>
> Cc: Sami Tolvanen <samitolvanen@google.com>
> Cc: Julien Thierry <julien.thierry@arm.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Yuichi Ito <ito-yuichi@fujitsu.com>
> Cc: linux-kernel@vger.kernel.org
> To: linux-arm-kernel@lists.infradead.org
> ---
>  arch/arm64/Kconfig               | 1 +
>  arch/arm64/kernel/entry-common.c | 4 ++++
>  2 files changed, 5 insertions(+)
> 
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 5c7ae4c3954b..d29bae38a951 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -98,6 +98,7 @@ config ARM64
>  	select ARCH_HAS_UBSAN_SANITIZE_ALL
>  	select ARM_AMBA
>  	select ARM_ARCH_TIMER
> +	select HAVE_ARCH_IRQENTRY

Please put this with the other HAVE_ARCH_* entries in
arch/arm64/Kconfig -- it should be between HAVE_ARCH_HUGE_VMAP and
HAVE_ARCH_JUMP_LABEL to keep that in alphabetical order.

With that and the title and commit message above:

Reviewed-by: Mark Rutland <mark.rutland@arm.com>

Thanks,
Mark.

>  	select ARM_GIC
>  	select AUDIT_ARCH_COMPAT_GENERIC
>  	select ARM_GIC_V2M if PCI
> diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c
> index 5f1473319fb0..6d4dc3b3799f 100644
> --- a/arch/arm64/kernel/entry-common.c
> +++ b/arch/arm64/kernel/entry-common.c
> @@ -428,7 +428,9 @@ static __always_inline void
>  __el1_interrupt(struct pt_regs *regs, void (*handler)(struct pt_regs *))
>  {
>  	enter_from_kernel_mode(regs);
> +	irq_enter_rcu();
>  	do_interrupt_handler(regs, handler);
> +	irq_exit_rcu();
>  	/*
>  	 * Note: thread_info::preempt_count includes both thread_info::count
>  	 * and thread_info::need_resched, and is not equivalent to
> @@ -667,7 +669,9 @@ static void noinstr el0_interrupt(struct pt_regs *regs,
>  	if (regs->pc & BIT(55))
>  		arm64_apply_bp_hardening();
>  
> +	irq_enter_rcu();
>  	do_interrupt_handler(regs, handler);
> +	irq_exit_rcu();
>  
>  	exit_to_user_mode(regs);
>  }
> -- 
> 2.31.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCHv3 3/3] arm64/entry-common: supplement irq accounting

[Marc Zyngier]

On Thu, 30 Sep 2021 14:53:14 +0100,
Mark Rutland <mark.rutland@arm.com> wrote:
> 
> On Thu, Sep 30, 2021 at 09:17:08PM +0800, Pingfan Liu wrote:
> > At present, the irq entry/exit accounting, which is performed by
> > handle_domain_irq(), overlaps with arm64 exception entry code somehow.
> > 
> > By supplementing irq accounting on arm64 exception entry code, the
> > accounting in handle_domain_irq() can be dropped totally by selecting
> > the macro HAVE_ARCH_IRQENTRY.
> 
> I think we need to be more thorough and explain the specific problem and
> solution. How about we crib some wording from patch 1, and say:
> 
>   arm64: entry: avoid double-accounting IRQ RCU entry
> 
>   When an IRQ is taken, some accounting needs to be performed to enter
>   and exit IRQ context around the IRQ handler. On arm64 some of this
>   accounting is performed by both the architecture code and the IRQ
>   domain code, resulting in calling rcu_irq_enter() twice per exception
>   entry, violating the expectations of the core RCU code, and resulting
>   in failing to identify quiescent periods correctly (e.g. in
>   rcu_is_cpu_rrupt_from_idle()).
> 
>   To fix this, we must perform all the accounting from the architecture
>   code. We prevent the IRQ domain code from performing any accounting by
>   selecting HAVE_ARCH_IRQENTRY, and must call irq_enter_rcu() and
>   irq_exit_rcu() around invoking the root IRQ handler.
> 
>   When we take a pNMI from a context with IRQs disabled, we'll perform
>   the necessary accounting as part of arm64_enter_nmi() and
>   arm64_exit_nmi(), and should only call irq_enter_rcu() and
>   irq_exit_rcu() when we may have taken a regular interrupt.
> 
> That way it's clear what specifically the overlap is and the problem(s)
> it results in. The bit at the end explains why we don't call
> irq_{enter,exit}_rcu() when we're certain we've taken a pNMI.
> 
> > Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
> > Cc: "Paul E. McKenney" <paulmck@kernel.org>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Cc: Will Deacon <will@kernel.org>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > Cc: Marc Zyngier <maz@kernel.org>
> > Cc: Joey Gouly <joey.gouly@arm.com>
> > Cc: Sami Tolvanen <samitolvanen@google.com>
> > Cc: Julien Thierry <julien.thierry@arm.com>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Yuichi Ito <ito-yuichi@fujitsu.com>
> > Cc: linux-kernel@vger.kernel.org
> > To: linux-arm-kernel@lists.infradead.org
> > ---
> >  arch/arm64/Kconfig               | 1 +
> >  arch/arm64/kernel/entry-common.c | 4 ++++
> >  2 files changed, 5 insertions(+)
> > 
> > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> > index 5c7ae4c3954b..d29bae38a951 100644
> > --- a/arch/arm64/Kconfig
> > +++ b/arch/arm64/Kconfig
> > @@ -98,6 +98,7 @@ config ARM64
> >  	select ARCH_HAS_UBSAN_SANITIZE_ALL
> >  	select ARM_AMBA
> >  	select ARM_ARCH_TIMER
> > +	select HAVE_ARCH_IRQENTRY
> 
> Please put this with the other HAVE_ARCH_* entries in
> arch/arm64/Kconfig -- it should be between HAVE_ARCH_HUGE_VMAP and
> HAVE_ARCH_JUMP_LABEL to keep that in alphabetical order.
> 
> With that and the title and commit message above:
> 
> Reviewed-by: Mark Rutland <mark.rutland@arm.com>

With the above changes,

Reviewed-by: Marc Zyngier <maz@kernel.org>

	M.

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

Re: [PATCHv3 3/3] arm64/entry-common: supplement irq accounting

[Marc Zyngier]

On Thu, 30 Sep 2021 14:53:14 +0100,
Mark Rutland <mark.rutland@arm.com> wrote:
> 
> On Thu, Sep 30, 2021 at 09:17:08PM +0800, Pingfan Liu wrote:
> > At present, the irq entry/exit accounting, which is performed by
> > handle_domain_irq(), overlaps with arm64 exception entry code somehow.
> > 
> > By supplementing irq accounting on arm64 exception entry code, the
> > accounting in handle_domain_irq() can be dropped totally by selecting
> > the macro HAVE_ARCH_IRQENTRY.
> 
> I think we need to be more thorough and explain the specific problem and
> solution. How about we crib some wording from patch 1, and say:
> 
>   arm64: entry: avoid double-accounting IRQ RCU entry
> 
>   When an IRQ is taken, some accounting needs to be performed to enter
>   and exit IRQ context around the IRQ handler. On arm64 some of this
>   accounting is performed by both the architecture code and the IRQ
>   domain code, resulting in calling rcu_irq_enter() twice per exception
>   entry, violating the expectations of the core RCU code, and resulting
>   in failing to identify quiescent periods correctly (e.g. in
>   rcu_is_cpu_rrupt_from_idle()).
> 
>   To fix this, we must perform all the accounting from the architecture
>   code. We prevent the IRQ domain code from performing any accounting by
>   selecting HAVE_ARCH_IRQENTRY, and must call irq_enter_rcu() and
>   irq_exit_rcu() around invoking the root IRQ handler.
> 
>   When we take a pNMI from a context with IRQs disabled, we'll perform
>   the necessary accounting as part of arm64_enter_nmi() and
>   arm64_exit_nmi(), and should only call irq_enter_rcu() and
>   irq_exit_rcu() when we may have taken a regular interrupt.
> 
> That way it's clear what specifically the overlap is and the problem(s)
> it results in. The bit at the end explains why we don't call
> irq_{enter,exit}_rcu() when we're certain we've taken a pNMI.
> 
> > Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
> > Cc: "Paul E. McKenney" <paulmck@kernel.org>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Cc: Will Deacon <will@kernel.org>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > Cc: Marc Zyngier <maz@kernel.org>
> > Cc: Joey Gouly <joey.gouly@arm.com>
> > Cc: Sami Tolvanen <samitolvanen@google.com>
> > Cc: Julien Thierry <julien.thierry@arm.com>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Yuichi Ito <ito-yuichi@fujitsu.com>
> > Cc: linux-kernel@vger.kernel.org
> > To: linux-arm-kernel@lists.infradead.org
> > ---
> >  arch/arm64/Kconfig               | 1 +
> >  arch/arm64/kernel/entry-common.c | 4 ++++
> >  2 files changed, 5 insertions(+)
> > 
> > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> > index 5c7ae4c3954b..d29bae38a951 100644
> > --- a/arch/arm64/Kconfig
> > +++ b/arch/arm64/Kconfig
> > @@ -98,6 +98,7 @@ config ARM64
> >  	select ARCH_HAS_UBSAN_SANITIZE_ALL
> >  	select ARM_AMBA
> >  	select ARM_ARCH_TIMER
> > +	select HAVE_ARCH_IRQENTRY
> 
> Please put this with the other HAVE_ARCH_* entries in
> arch/arm64/Kconfig -- it should be between HAVE_ARCH_HUGE_VMAP and
> HAVE_ARCH_JUMP_LABEL to keep that in alphabetical order.
> 
> With that and the title and commit message above:
> 
> Reviewed-by: Mark Rutland <mark.rutland@arm.com>

With the above changes,

Reviewed-by: Marc Zyngier <maz@kernel.org>

	M.

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCHv3 3/3] arm64/entry-common: supplement irq accounting

[Pingfan Liu]

On Thu, Sep 30, 2021 at 02:53:14PM +0100, Mark Rutland wrote:
> On Thu, Sep 30, 2021 at 09:17:08PM +0800, Pingfan Liu wrote:
> > At present, the irq entry/exit accounting, which is performed by
> > handle_domain_irq(), overlaps with arm64 exception entry code somehow.
> > 
> > By supplementing irq accounting on arm64 exception entry code, the
> > accounting in handle_domain_irq() can be dropped totally by selecting
> > the macro HAVE_ARCH_IRQENTRY.
> 
> I think we need to be more thorough and explain the specific problem and
> solution. How about we crib some wording from patch 1, and say:
> 
>   arm64: entry: avoid double-accounting IRQ RCU entry
> 
>   When an IRQ is taken, some accounting needs to be performed to enter
>   and exit IRQ context around the IRQ handler. On arm64 some of this
>   accounting is performed by both the architecture code and the IRQ
>   domain code, resulting in calling rcu_irq_enter() twice per exception
>   entry, violating the expectations of the core RCU code, and resulting
>   in failing to identify quiescent periods correctly (e.g. in
>   rcu_is_cpu_rrupt_from_idle()).
> 
>   To fix this, we must perform all the accounting from the architecture
>   code. We prevent the IRQ domain code from performing any accounting by
>   selecting HAVE_ARCH_IRQENTRY, and must call irq_enter_rcu() and
>   irq_exit_rcu() around invoking the root IRQ handler.
> 
>   When we take a pNMI from a context with IRQs disabled, we'll perform
>   the necessary accounting as part of arm64_enter_nmi() and
>   arm64_exit_nmi(), and should only call irq_enter_rcu() and
>   irq_exit_rcu() when we may have taken a regular interrupt.
> 
It is a wonderful and elaborated log.

> That way it's clear what specifically the overlap is and the problem(s)
> it results in. The bit at the end explains why we don't call
> irq_{enter,exit}_rcu() when we're certain we've taken a pNMI.
> 
I have learned much from the log you contribute to this series. I will keep
learning how to improve my ability of log. Thanks again!

> > Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
> > Cc: "Paul E. McKenney" <paulmck@kernel.org>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Cc: Will Deacon <will@kernel.org>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > Cc: Marc Zyngier <maz@kernel.org>
> > Cc: Joey Gouly <joey.gouly@arm.com>
> > Cc: Sami Tolvanen <samitolvanen@google.com>
> > Cc: Julien Thierry <julien.thierry@arm.com>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Yuichi Ito <ito-yuichi@fujitsu.com>
> > Cc: linux-kernel@vger.kernel.org
> > To: linux-arm-kernel@lists.infradead.org
> > ---
> >  arch/arm64/Kconfig               | 1 +
> >  arch/arm64/kernel/entry-common.c | 4 ++++
> >  2 files changed, 5 insertions(+)
> > 
> > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> > index 5c7ae4c3954b..d29bae38a951 100644
> > --- a/arch/arm64/Kconfig
> > +++ b/arch/arm64/Kconfig
> > @@ -98,6 +98,7 @@ config ARM64
> >  	select ARCH_HAS_UBSAN_SANITIZE_ALL
> >  	select ARM_AMBA
> >  	select ARM_ARCH_TIMER
> > +	select HAVE_ARCH_IRQENTRY
> 
> Please put this with the other HAVE_ARCH_* entries in
> arch/arm64/Kconfig -- it should be between HAVE_ARCH_HUGE_VMAP and
> HAVE_ARCH_JUMP_LABEL to keep that in alphabetical order.
> 
OK, I will fix it in V4.

> With that and the title and commit message above:
> 
> Reviewed-by: Mark Rutland <mark.rutland@arm.com>
> 
Thanks,

	Pingfan

Re: [PATCHv3 3/3] arm64/entry-common: supplement irq accounting

[Pingfan Liu]

On Thu, Sep 30, 2021 at 02:53:14PM +0100, Mark Rutland wrote:
> On Thu, Sep 30, 2021 at 09:17:08PM +0800, Pingfan Liu wrote:
> > At present, the irq entry/exit accounting, which is performed by
> > handle_domain_irq(), overlaps with arm64 exception entry code somehow.
> > 
> > By supplementing irq accounting on arm64 exception entry code, the
> > accounting in handle_domain_irq() can be dropped totally by selecting
> > the macro HAVE_ARCH_IRQENTRY.
> 
> I think we need to be more thorough and explain the specific problem and
> solution. How about we crib some wording from patch 1, and say:
> 
>   arm64: entry: avoid double-accounting IRQ RCU entry
> 
>   When an IRQ is taken, some accounting needs to be performed to enter
>   and exit IRQ context around the IRQ handler. On arm64 some of this
>   accounting is performed by both the architecture code and the IRQ
>   domain code, resulting in calling rcu_irq_enter() twice per exception
>   entry, violating the expectations of the core RCU code, and resulting
>   in failing to identify quiescent periods correctly (e.g. in
>   rcu_is_cpu_rrupt_from_idle()).
> 
>   To fix this, we must perform all the accounting from the architecture
>   code. We prevent the IRQ domain code from performing any accounting by
>   selecting HAVE_ARCH_IRQENTRY, and must call irq_enter_rcu() and
>   irq_exit_rcu() around invoking the root IRQ handler.
> 
>   When we take a pNMI from a context with IRQs disabled, we'll perform
>   the necessary accounting as part of arm64_enter_nmi() and
>   arm64_exit_nmi(), and should only call irq_enter_rcu() and
>   irq_exit_rcu() when we may have taken a regular interrupt.
> 
It is a wonderful and elaborated log.

> That way it's clear what specifically the overlap is and the problem(s)
> it results in. The bit at the end explains why we don't call
> irq_{enter,exit}_rcu() when we're certain we've taken a pNMI.
> 
I have learned much from the log you contribute to this series. I will keep
learning how to improve my ability of log. Thanks again!

> > Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
> > Cc: "Paul E. McKenney" <paulmck@kernel.org>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Cc: Will Deacon <will@kernel.org>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > Cc: Marc Zyngier <maz@kernel.org>
> > Cc: Joey Gouly <joey.gouly@arm.com>
> > Cc: Sami Tolvanen <samitolvanen@google.com>
> > Cc: Julien Thierry <julien.thierry@arm.com>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Yuichi Ito <ito-yuichi@fujitsu.com>
> > Cc: linux-kernel@vger.kernel.org
> > To: linux-arm-kernel@lists.infradead.org
> > ---
> >  arch/arm64/Kconfig               | 1 +
> >  arch/arm64/kernel/entry-common.c | 4 ++++
> >  2 files changed, 5 insertions(+)
> > 
> > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> > index 5c7ae4c3954b..d29bae38a951 100644
> > --- a/arch/arm64/Kconfig
> > +++ b/arch/arm64/Kconfig
> > @@ -98,6 +98,7 @@ config ARM64
> >  	select ARCH_HAS_UBSAN_SANITIZE_ALL
> >  	select ARM_AMBA
> >  	select ARM_ARCH_TIMER
> > +	select HAVE_ARCH_IRQENTRY
> 
> Please put this with the other HAVE_ARCH_* entries in
> arch/arm64/Kconfig -- it should be between HAVE_ARCH_HUGE_VMAP and
> HAVE_ARCH_JUMP_LABEL to keep that in alphabetical order.
> 
OK, I will fix it in V4.

> With that and the title and commit message above:
> 
> Reviewed-by: Mark Rutland <mark.rutland@arm.com>
> 
Thanks,

	Pingfan

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel