[PATCH v3 0/3] KVM: PPC: BookE: Fix boot hang on preemptible kernels

[PATCH v3 0/3] KVM: PPC: BookE: Fix boot hang on preemptible kernels

[Shrikanth Hegde]

Christian Zigotzky had reported that preemptible kernels don't boot on
his FSL Cyrus+ board. The board freezes on running either full/lazy
preemption and has been an issue for a while now (years!).

https://lore.kernel.org/all/b897b0fd-90f2-4215-bcd4-3714e497d773@xenosoft.de/#t
More details in cover-letter of v2[1]

v2->v3:
- Comment removal (Mukesh Kumar Chaurasiya)
- Add missing booke change (Sashiko)
- Collect tags (Thanks to Mukesh Kumar Chaurasiya, Christian Zigotzky)

v1->v2:
- Re-order to avoid preempt count dance (Narayana Murty N)
- Do only hard_irq_disable on PPC64 to avoid second warning. On PPC32
  use local_irq_disable to acheive the same. (Ritesh Harjani)

v2: https://lore.kernel.org/all/20261110053238.3794500-1-sshegde@linux.ibm.com/#t [1]

Apologies in case any nitpicks are missed.
I'm surviving on half a night's sleep these days.

As said in v2, remaining usecases of hard_irq_disable will be taken up
later.

Shrikanth Hegde (3):
  powerpc/irq: Add ppc_hard_irq_disable()
  KVM: PPC: BookE: Load FP and Altivec before enabling the interrupts
  KVM: PPC: Use ppc_hard_irq_disable before guest entry

 arch/powerpc/include/asm/hw_irq.h | 17 +++++++++++++++++
 arch/powerpc/kvm/booke.c          |  6 ++----
 arch/powerpc/kvm/powerpc.c        | 14 +++-----------
 3 files changed, 22 insertions(+), 15 deletions(-)

-- 
2.52.0

[PATCH v3 1/3] powerpc/irq: Add ppc_hard_irq_disable()

[Shrikanth Hegde]

Add a flavour of interrupt disable which could be used in
multiple places which also takes care of lockdep in both 32 and
64 bit archs.

On PPC64:
- Disable hard interrupts using hard_irq_disable. It takes care of
  lockdep and tracing. local_irq_disable only alters softmask's state.

On PPC32:
- Interrupts are hard disabled with local_irq_disable. It also takes
  care of lockdep and tacing.

Subsequent patch uses this API in KVM code to avoid the warnings.
Wider use of it is possible, but it will be taken subsequently.

Suggested-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
Tested-by: Christian Zigotzky <chzigotzky@xenosoft.de>
Reviewed-by: Mukesh Kumar Chaurasiya (IBM) <mkchauras@gmail.com>
Signed-off-by: Shrikanth Hegde <sshegde@linux.ibm.com>
---
 arch/powerpc/include/asm/hw_irq.h | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/arch/powerpc/include/asm/hw_irq.h b/arch/powerpc/include/asm/hw_irq.h
index b7eee6385ae5..075442366e1e 100644
--- a/arch/powerpc/include/asm/hw_irq.h
+++ b/arch/powerpc/include/asm/hw_irq.h
@@ -516,6 +516,23 @@ static inline unsigned long mtmsr_isync_irqsafe(unsigned long msr)
 
 #define ARCH_IRQ_INIT_FLAGS	IRQ_NOREQUEST
 
+/*
+ * Hard-disable interrupts while keeping the IRQ tracing state
+ * consistent.
+ *
+ * On PPC64, local_irq_disable() only soft-disables interrupts, leaving
+ * a window where an interrupt can be recorded in irq_happened before
+ * interrupts are hard-disabled.
+ *
+ * On PPC32, hard_irq_disable() does not update the IRQ tracing state, so
+ * use local_irq_disable().
+ */
+#ifdef CONFIG_PPC64
+#define ppc_hard_irq_disable()	hard_irq_disable()
+#else
+#define ppc_hard_irq_disable()	local_irq_disable()
+#endif
+
 #endif  /* __ASSEMBLER__ */
 #endif	/* __KERNEL__ */
 #endif	/* _ASM_POWERPC_HW_IRQ_H */
-- 
2.52.0

[PATCH v3 2/3] KVM: PPC: BookE: Load FP and Altivec before enabling the interrupts

[Shrikanth Hegde]

Christian reported that booting preemptible kernel on FSL Cyrus+ board
causes boot hang.

The logs pointed that system was busy in printing below warning.

WARNING: at .enable_kernel_fp+0x30/0x78, CPU#3: qemu-system-ppc/4884
Modules linked in:
CPU: 3 UID: 1000 PID: 4884 Comm: qemu-system-ppc Not tainted 7.3.0-rc1-powerpc64-smp-preempt #1 PREEMPT
NIP [c000000000003338] .enable_kernel_fp+0x30/0x78
LR [c00000000005de84] .kvmppc_load_guest_fp+0x30/0x80
Call Trace:
[c000000085ca7700] [c00000000005de84] .kvmppc_load_guest_fp+0x30/0x80
[c000000085ca7780] [c00000000005f2a0] .kvmppc_handle_exit+0x5bc/0x5cc
[c000000085ca7830] [c00000000006204c] .kvmppc_resume_host+0xb8/0x10c

Which is...

void enable_kernel_fp(void)
{
        unsigned long cpumsr;
        WARN_ON(preemptible());

And...

Though irq's are hard disabled after kvmppc_prepare_to_enter, but
kvmppc_fix_ee_before_entry enables the softmask's IRQ state.
That causes the irqs_disabled to return false.
Hence leading to the warnings.

Fix it by re-ordering the code to load the FP and Altivec
before enabling the softmask IRQ state.

Suggested-by: Narayana Murty N <nnmlinux@linux.ibm.com>
Fixes: 3efc7da61f6c ("KVM: PPC: Book3E: Increase FPU laziness")
Reported-by: Christian Zigotzky <chzigotzky@xenosoft.de>
Closes: https://lore.kernel.org/all/33342fbf-eb7b-bde6-2c8c-254fe8bfb993@xenosoft.de/
Tested-by: Christian Zigotzky <chzigotzky@xenosoft.de>
Reviewed-by: Mukesh Kumar Chaurasiya (IBM) <mkchauras@gmail.com>
Signed-off-by: Shrikanth Hegde <sshegde@linux.ibm.com>
---
 arch/powerpc/kvm/booke.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
index 13ad4cf5fa71..aed7b476b7eb 100644
--- a/arch/powerpc/kvm/booke.c
+++ b/arch/powerpc/kvm/booke.c
@@ -1404,10 +1404,9 @@ int kvmppc_handle_exit(struct kvm_vcpu *vcpu, unsigned int exit_nr)
 		if (s <= 0)
 			r = (s << 2) | RESUME_HOST | (r & RESUME_FLAG_NV);
 		else {
-			/* interrupts now hard-disabled */
-			kvmppc_fix_ee_before_entry();
 			kvmppc_load_guest_fp(vcpu);
 			kvmppc_load_guest_altivec(vcpu);
+			kvmppc_fix_ee_before_entry();
 		}
 	}
 
-- 
2.52.0

[PATCH v3 3/3] KVM: PPC: Use ppc_hard_irq_disable before guest entry

[Shrikanth Hegde]

After applying preempt disable patch, i.e PATCH 2/3, Christian reported
a subsequent warning stopping his board to boot properly.

WARNING: at .kvmppc_fix_ee_before_entry+0x10/0x28, CPU#0: qemu-system-ppc/4667
CPU: 0 UID: 1000 PID: 4667 Comm: qemu-system-ppc Tainted: G        W           7.3.0-rc4-2-powerpc64-smp #1 PREEMPT
Tainted: [W]=WARN
Hardware name: varisys,CYRUS5040 e5500 0x80240012 CoreNet Generic
NIP [c00000000005dc3c] .kvmppc_fix_ee_before_entry+0x10/0x28
LR [c00000000005f298] .kvmppc_handle_exit+0x5b4/0x5e8
Call Trace:
[c000000086587780] [c00000000005ee10] .kvmppc_handle_exit+0x12c/0x5e8 (unreliable)
[c000000086587830] [c000000000062068] .kvmppc_resume_host+0xb8/0x10c

It triggers below warning...

static inline void kvmppc_fix_ee_before_entry(void)
{
        trace_hardirqs_on();

        /*
         * To avoid races, the caller must have gone directly from having
         * interrupts fully-enabled to hard-disabled.
         */
        WARN_ON(local_paca->irq_happened != PACA_IRQ_HARD_DIS);

This happens since kvmppc_prepare_to_enter does first local_irq_disable
followed by hard_irq_disable. This leaves a small window where interrupt
may occur and it could set the irq pending bit in PACA.

That local_irq_disable was necessary for PPC32 to track the lockdep
state etc. On PPC64 hard_irq_disable is good enough. There is no need to
do both. Use the new irq api to do the necessary one.

Fixes: 12013e3d4695 ("KVM: powerpc: Use generic xfer to guest work function")
Reported-by: Christian Zigotzky <chzigotzky@xenosoft.de>
Closes: https://lore.kernel.org/all/b8f82519-9ae5-c247-e020-e79c627e57df@xenosoft.de/
Tested-by: Christian Zigotzky <chzigotzky@xenosoft.de>
Reviewed-by: Mukesh Kumar Chaurasiya (IBM) <mkchauras@gmail.com>
Signed-off-by: Shrikanth Hegde <sshegde@linux.ibm.com>
---
 arch/powerpc/kvm/booke.c   |  3 +--
 arch/powerpc/kvm/powerpc.c | 14 +++-----------
 2 files changed, 4 insertions(+), 13 deletions(-)

diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
index aed7b476b7eb..1ba28848e8bc 100644
--- a/arch/powerpc/kvm/booke.c
+++ b/arch/powerpc/kvm/booke.c
@@ -722,8 +722,7 @@ int kvmppc_core_prepare_to_enter(struct kvm_vcpu *vcpu)
 	if (vcpu->arch.shared->msr & MSR_WE) {
 		local_irq_enable();
 		kvm_vcpu_halt(vcpu);
-		local_irq_disable();
-		hard_irq_disable();
+		ppc_hard_irq_disable();
 
 		kvmppc_set_exit_type(vcpu, EMULATED_MTMSRWE_EXITS);
 		r = 1;
diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
index 9194cf492d1c..4ee3d7bfe917 100644
--- a/arch/powerpc/kvm/powerpc.c
+++ b/arch/powerpc/kvm/powerpc.c
@@ -81,13 +81,7 @@ int kvmppc_prepare_to_enter(struct kvm_vcpu *vcpu)
 	int r;
 
 	WARN_ON(irqs_disabled());
-	/*
-	 * local_irq_disable() first: on 32-bit, hard_irq_disable() alone is a
-	 * raw MSR[EE] clear that bypasses the lockdep/irq-tracing state, and
-	 * the xfer_to_guest_mode helpers assert IRQs are seen as disabled.
-	 */
-	local_irq_disable();
-	hard_irq_disable();
+	ppc_hard_irq_disable();
 
 	while (true) {
 		xfer_to_guest_mode_prepare();
@@ -101,8 +95,7 @@ int kvmppc_prepare_to_enter(struct kvm_vcpu *vcpu)
 			 */
 			local_irq_enable();
 			r = kvm_xfer_to_guest_mode_handle_work(vcpu);
-			local_irq_disable();
-			hard_irq_disable();
+			ppc_hard_irq_disable();
 			if (r) {
 				/*
 				 * The generic helper does not set the exit
@@ -134,8 +127,7 @@ int kvmppc_prepare_to_enter(struct kvm_vcpu *vcpu)
 			local_irq_enable();
 			trace_kvm_check_requests(vcpu);
 			r = kvmppc_core_check_requests(vcpu);
-			local_irq_disable();
-			hard_irq_disable();
+			ppc_hard_irq_disable();
 			if (r > 0)
 				continue;
 			break;
-- 
2.52.0

Re: [PATCH v3 2/3] KVM: PPC: BookE: Load FP and Altivec before enabling the interrupts

[Narayana Murty N]

Hi Shrikanth,

On 12/11/26 6:39 PM, Shrikanth Hegde wrote:
> Christian reported that booting preemptible kernel on FSL Cyrus+ board
> causes boot hang.
> 
> The logs pointed that system was busy in printing below warning.
> 
> WARNING: at .enable_kernel_fp+0x30/0x78, CPU#3: qemu-system-ppc/4884
> Modules linked in:
> CPU: 3 UID: 1000 PID: 4884 Comm: qemu-system-ppc Not tainted 7.3.0-rc1-powerpc64-smp-preempt #1 PREEMPT
> NIP [c000000000003338] .enable_kernel_fp+0x30/0x78
> LR [c00000000005de84] .kvmppc_load_guest_fp+0x30/0x80
> Call Trace:
> [c000000085ca7700] [c00000000005de84] .kvmppc_load_guest_fp+0x30/0x80
> [c000000085ca7780] [c00000000005f2a0] .kvmppc_handle_exit+0x5bc/0x5cc
> [c000000085ca7830] [c00000000006204c] .kvmppc_resume_host+0xb8/0x10c
> 
> Which is...
> 
> void enable_kernel_fp(void)
> {
>          unsigned long cpumsr;
>          WARN_ON(preemptible());
> 
> And...
> 
> Though irq's are hard disabled after kvmppc_prepare_to_enter, but
> kvmppc_fix_ee_before_entry enables the softmask's IRQ state.
> That causes the irqs_disabled to return false.
> Hence leading to the warnings.
> 
> Fix it by re-ordering the code to load the FP and Altivec
> before enabling the softmask IRQ state.
> 
> Suggested-by: Narayana Murty N <nnmlinux@linux.ibm.com>
> Fixes: 3efc7da61f6c ("KVM: PPC: Book3E: Increase FPU laziness")
> Reported-by: Christian Zigotzky <chzigotzky@xenosoft.de>
> Closes: https://lore.kernel.org/all/33342fbf-eb7b-bde6-2c8c-254fe8bfb993@xenosoft.de/
> Tested-by: Christian Zigotzky <chzigotzky@xenosoft.de>
> Reviewed-by: Mukesh Kumar Chaurasiya (IBM) <mkchauras@gmail.com>
> Signed-off-by: Shrikanth Hegde <sshegde@linux.ibm.com>
> ---
>   arch/powerpc/kvm/booke.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
> index 13ad4cf5fa71..aed7b476b7eb 100644
> --- a/arch/powerpc/kvm/booke.c
> +++ b/arch/powerpc/kvm/booke.c
> @@ -1404,10 +1404,9 @@ int kvmppc_handle_exit(struct kvm_vcpu *vcpu, unsigned int exit_nr)
>   		if (s <= 0)
>   			r = (s << 2) | RESUME_HOST | (r & RESUME_FLAG_NV);
>   		else {
> -			/* interrupts now hard-disabled */
> -			kvmppc_fix_ee_before_entry();
>   			kvmppc_load_guest_fp(vcpu);
>   			kvmppc_load_guest_altivec(vcpu);
> +			kvmppc_fix_ee_before_entry();
>   		}
>   	}
>   

This address my previous comment, feel free to add Reviewed-by Narayana 
Murty N <nnmlinux@linux.ibm.com>

thanks,
Narayana.

Re: [PATCH v3 3/3] KVM: PPC: Use ppc_hard_irq_disable before guest entry

[Narayana Murty N]

Hi,

On 12/11/26 6:39 PM, Shrikanth Hegde wrote:
> After applying preempt disable patch, i.e PATCH 2/3, Christian reported
> a subsequent warning stopping his board to boot properly.
> 
> WARNING: at .kvmppc_fix_ee_before_entry+0x10/0x28, CPU#0: qemu-system-ppc/4667
> CPU: 0 UID: 1000 PID: 4667 Comm: qemu-system-ppc Tainted: G        W           7.3.0-rc4-2-powerpc64-smp #1 PREEMPT
> Tainted: [W]=WARN
> Hardware name: varisys,CYRUS5040 e5500 0x80240012 CoreNet Generic
> NIP [c00000000005dc3c] .kvmppc_fix_ee_before_entry+0x10/0x28
> LR [c00000000005f298] .kvmppc_handle_exit+0x5b4/0x5e8
> Call Trace:
> [c000000086587780] [c00000000005ee10] .kvmppc_handle_exit+0x12c/0x5e8 (unreliable)
> [c000000086587830] [c000000000062068] .kvmppc_resume_host+0xb8/0x10c
> 
> It triggers below warning...
> 
> static inline void kvmppc_fix_ee_before_entry(void)
> {
>          trace_hardirqs_on();
> 
>          /*
>           * To avoid races, the caller must have gone directly from having
>           * interrupts fully-enabled to hard-disabled.
>           */
>          WARN_ON(local_paca->irq_happened != PACA_IRQ_HARD_DIS);
> 
> This happens since kvmppc_prepare_to_enter does first local_irq_disable
> followed by hard_irq_disable. This leaves a small window where interrupt
> may occur and it could set the irq pending bit in PACA.
> 
> That local_irq_disable was necessary for PPC32 to track the lockdep
> state etc. On PPC64 hard_irq_disable is good enough. There is no need to
> do both. Use the new irq api to do the necessary one.
> 
> Fixes: 12013e3d4695 ("KVM: powerpc: Use generic xfer to guest work function")
> Reported-by: Christian Zigotzky<chzigotzky@xenosoft.de>
> Closes:https://lore.kernel.org/all/b8f82519-9ae5-c247-e020- 
> e79c627e57df@xenosoft.de/
> Tested-by: Christian Zigotzky<chzigotzky@xenosoft.de>
> Reviewed-by: Mukesh Kumar Chaurasiya (IBM)<mkchauras@gmail.com>
> Signed-off-by: Shrikanth Hegde<sshegde@linux.ibm.com>
> ---
>   arch/powerpc/kvm/booke.c   |  3 +--
>   arch/powerpc/kvm/powerpc.c | 14 +++-----------
>   2 files changed, 4 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
> index aed7b476b7eb..1ba28848e8bc 100644
> --- a/arch/powerpc/kvm/booke.c
> +++ b/arch/powerpc/kvm/booke.c
> @@ -722,8 +722,7 @@ int kvmppc_core_prepare_to_enter(struct kvm_vcpu *vcpu)
>   	if (vcpu->arch.shared->msr & MSR_WE) {
>   		local_irq_enable();
>   		kvm_vcpu_halt(vcpu);
> -		local_irq_disable();
> -		hard_irq_disable();
> +		ppc_hard_irq_disable();
>   
>   		kvmppc_set_exit_type(vcpu, EMULATED_MTMSRWE_EXITS);
>   		r = 1;
> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> index 9194cf492d1c..4ee3d7bfe917 100644
> --- a/arch/powerpc/kvm/powerpc.c
> +++ b/arch/powerpc/kvm/powerpc.c
> @@ -81,13 +81,7 @@ int kvmppc_prepare_to_enter(struct kvm_vcpu *vcpu)
>   	int r;
>   
>   	WARN_ON(irqs_disabled());
> -	/*
> -	 * local_irq_disable() first: on 32-bit, hard_irq_disable() alone is a
> -	 * raw MSR[EE] clear that bypasses the lockdep/irq-tracing state, and
> -	 * the xfer_to_guest_mode helpers assert IRQs are seen as disabled.
> -	 */
> -	local_irq_disable();
> -	hard_irq_disable();
> +	ppc_hard_irq_disable();
>   
>   	while (true) {
>   		xfer_to_guest_mode_prepare();
> @@ -101,8 +95,7 @@ int kvmppc_prepare_to_enter(struct kvm_vcpu *vcpu)
>   			 */
>   			local_irq_enable();
>   			r = kvm_xfer_to_guest_mode_handle_work(vcpu);
> -			local_irq_disable();
> -			hard_irq_disable();
> +			ppc_hard_irq_disable();
>   			if (r) {
>   				/*
>   				 * The generic helper does not set the exit
> @@ -134,8 +127,7 @@ int kvmppc_prepare_to_enter(struct kvm_vcpu *vcpu)
>   			local_irq_enable();
>   			trace_kvm_check_requests(vcpu);
>   			r = kvmppc_core_check_requests(vcpu);
> -			local_irq_disable();
> -			hard_irq_disable();
> +			ppc_hard_irq_disable();
>   			if (r > 0)
>   				continue;
>   			break;
The changes look good to me.

Using ppc_hard_irq_disable() directly on PPC64 avoids the soft-disable 
to hard-disable window, while preserving the software IRQ mask and 
tracing state required by the guest-entry path. The BookE halt path and 
the guest-work retry paths are covered as well.

Reviewed-by: Narayana Murty N nnmlinux@linux.ibm.com

Re: [PATCH v3 2/3] KVM: PPC: BookE: Load FP and Altivec before enabling the interrupts

[Ritesh Harjani]

Shrikanth Hegde <sshegde@linux.ibm.com> writes:

> Christian reported that booting preemptible kernel on FSL Cyrus+ board
> causes boot hang.
>
> The logs pointed that system was busy in printing below warning.
>
> WARNING: at .enable_kernel_fp+0x30/0x78, CPU#3: qemu-system-ppc/4884
> Modules linked in:
> CPU: 3 UID: 1000 PID: 4884 Comm: qemu-system-ppc Not tainted 7.3.0-rc1-powerpc64-smp-preempt #1 PREEMPT
> NIP [c000000000003338] .enable_kernel_fp+0x30/0x78
> LR [c00000000005de84] .kvmppc_load_guest_fp+0x30/0x80
> Call Trace:
> [c000000085ca7700] [c00000000005de84] .kvmppc_load_guest_fp+0x30/0x80
> [c000000085ca7780] [c00000000005f2a0] .kvmppc_handle_exit+0x5bc/0x5cc
> [c000000085ca7830] [c00000000006204c] .kvmppc_resume_host+0xb8/0x10c
>
> Which is...
>
> void enable_kernel_fp(void)
> {
>         unsigned long cpumsr;
>         WARN_ON(preemptible());
>
> And...
>
> Though irq's are hard disabled after kvmppc_prepare_to_enter, but
> kvmppc_fix_ee_before_entry enables the softmask's IRQ state.
> That causes the irqs_disabled to return false.
> Hence leading to the warnings.
>
> Fix it by re-ordering the code to load the FP and Altivec
> before enabling the softmask IRQ state.
>
> Suggested-by: Narayana Murty N <nnmlinux@linux.ibm.com>
> Fixes: 3efc7da61f6c ("KVM: PPC: Book3E: Increase FPU laziness")
> Reported-by: Christian Zigotzky <chzigotzky@xenosoft.de>
> Closes: https://lore.kernel.org/all/33342fbf-eb7b-bde6-2c8c-254fe8bfb993@xenosoft.de/
> Tested-by: Christian Zigotzky <chzigotzky@xenosoft.de>
> Reviewed-by: Mukesh Kumar Chaurasiya (IBM) <mkchauras@gmail.com>
> Signed-off-by: Shrikanth Hegde <sshegde@linux.ibm.com>
> ---
>  arch/powerpc/kvm/booke.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

LGTM. So, please feel free to add:
Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>

Re: [PATCH v3 1/3] powerpc/irq: Add ppc_hard_irq_disable()

[Ritesh Harjani]

Shrikanth Hegde <sshegde@linux.ibm.com> writes:

> Add a flavour of interrupt disable which could be used in
> multiple places which also takes care of lockdep in both 32 and
> 64 bit archs.
>
> On PPC64:
> - Disable hard interrupts using hard_irq_disable. It takes care of
>   lockdep and tracing. local_irq_disable only alters softmask's state.
>
> On PPC32:
> - Interrupts are hard disabled with local_irq_disable. It also takes
>   care of lockdep and tacing.
>
> Subsequent patch uses this API in KVM code to avoid the warnings.
> Wider use of it is possible, but it will be taken subsequently.
>
> Suggested-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
> Tested-by: Christian Zigotzky <chzigotzky@xenosoft.de>
> Reviewed-by: Mukesh Kumar Chaurasiya (IBM) <mkchauras@gmail.com>
> Signed-off-by: Shrikanth Hegde <sshegde@linux.ibm.com>
> ---
>  arch/powerpc/include/asm/hw_irq.h | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
>
> diff --git a/arch/powerpc/include/asm/hw_irq.h b/arch/powerpc/include/asm/hw_irq.h
> index b7eee6385ae5..075442366e1e 100644
> --- a/arch/powerpc/include/asm/hw_irq.h
> +++ b/arch/powerpc/include/asm/hw_irq.h
> @@ -516,6 +516,23 @@ static inline unsigned long mtmsr_isync_irqsafe(unsigned long msr)
>  
>  #define ARCH_IRQ_INIT_FLAGS	IRQ_NOREQUEST
>  
> +/*
> + * Hard-disable interrupts while keeping the IRQ tracing state
> + * consistent.
> + *
> + * On PPC64, local_irq_disable() only soft-disables interrupts, leaving
> + * a window where an interrupt can be recorded in irq_happened before
> + * interrupts are hard-disabled.
> + *
> + * On PPC32, hard_irq_disable() does not update the IRQ tracing state, so
> + * use local_irq_disable().
> + */
> +#ifdef CONFIG_PPC64
> +#define ppc_hard_irq_disable()	hard_irq_disable()
> +#else
> +#define ppc_hard_irq_disable()	local_irq_disable()
> +#endif
> +

So I looked up the history of how did we end up here. And looks like we
will be papering over the problem, if we make the above change and add
yet another helper on PPC :)

Ideally, ppc32 shouldn't need a hard_irq_disable() definition at all.
As I understand it, the contract when this was all added was that, it is
meant for architectures which implement lazy IRQ disabling i.e.
    commit d04c56f73c30a5e593202ecfcf25ed43d42363a2
    Author: Paul Mackerras <paulus@ozlabs.org>
    Date:   Wed Oct 4 16:47:49 2006 +1000
        [POWERPC] Lazy interrupt disabling for 64-bit machines
 
...where local_irq_disable() only sets a soft mask. Code that really
wants interrupts disabled at the hardware level then calls
hard_irq_disable() as well.

This commit which defined a generic stub for hard_irq_disable() makes the
contract clear i.e.
    commit 2d3fbbb391e280724f7f7804ab00ff61cf1b6a4c
    Author: Benjamin Herrenschmidt <benh@kernel.crashing.org>
    Date:   Thu May 10 22:22:46 2007 -0700

        Add hard_irq_disable()
which says:
    This patch implements a generic hard_irq_disable() function which is meant to
    be called -after- local_irq_disable() and ensures that interrupts are fully
    disabled on that CPU.  The default implementation is a nop, though powerpc
    does already provide an appropriate one.

And then the following even implements such a contract in stop_machine() i.e.:
    commit a12bb44471b819c7f33d8a83044e7116c124e3d5
    Author: Benjamin Herrenschmidt <benh@kernel.crashing.org>
    Date:   Thu May 10 22:22:47 2007 -0700

        stop_machine() now uses hard_irq_disable

        Add a call to hard_irq_disable() to stop_machine so that we make sure IRQs are
        really disabled and not only lazy-disabled on archs like powerpc as some users
        of stop_machine() may rely on that.

        [akpm@linux-foundation.org: build fix]
        Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
        Acked-by: Rusty Russell <rusty@rustcorp.com.au>
        Cc: Paul Mackerras <paulus@samba.org>
        Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
        Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

    diff --git a/kernel/stop_machine.c b/kernel/stop_machine.c
    index daabb74ee0bc..fcee2a8e6da3 100644
    --- a/kernel/stop_machine.c
    +++ b/kernel/stop_machine.c
    @@ -8,6 +8,8 @@
    #include <linux/sched.h>
    #include <linux/stop_machine.h>
    #include <linux/syscalls.h>
    +#include <linux/interrupt.h>
    +
    #include <asm/atomic.h>
    #include <asm/semaphore.h>
    #include <asm/uaccess.h>
    @@ -45,6 +47,7 @@ static int stopmachine(void *cpu)
            if (stopmachine_state == STOPMACHINE_DISABLE_IRQ 
                && !irqs_disabled) {
                local_irq_disable();
    +			hard_irq_disable();
                irqs_disabled = 1;
                /* Ack: irqs disabled. */
                smp_mb(); /* Must read state first. */
    @@ -124,6 +127,7 @@ static int stop_machine(void)

        /* Make them disable irqs. */
        local_irq_disable();
    +	hard_irq_disable();
        stopmachine_set_state(STOPMACHINE_DISABLE_IRQ);

        return 0;


However, note that after introducing the lazy interrupt disabling in
2006, we saw build failures on ppc32, since hard_irq_disable()
definition didn't exist on that until then. This commit adds that
definition, which is not wrong, since the lazy interrupt disabling patch
really needed hard_irq_disable() to disable hardware interrupts. Hence
this commit defined it on PPC32 as well, as local_irq_disable() itself
i.e.

    commit 143db67af02cecab28032ec3a52e586b1433fe70
    Author: Michael Ellerman <michael@ellerman.id.au>
    Date:   Sun Nov 26 17:36:15 2006 +1100

        [POWERPC] Provide dummy hard_irq_enable/disable() for PPC32

        To allow arch/powerpc/kernel/crash.c to build on 32-bit we need a
        definition of hard_irq_disable(). 32-bit doesn't support the lazy
        interrupt disabling mechanism, so on 32-bit hard_irq_disable() is
        simply local_irq_disable(). Add a definition for hard_irq_enable()
        just for completeness.

        This allows (KEXEC=y && PPC32=y) to build again. Broken since
        d04c56f73c30a5e593202ecfcf25ed43d42363a2.

        Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
        Signed-off-by: Paul Mackerras <paulus@samba.org>

    diff --git a/include/asm-powerpc/hw_irq.h b/include/asm-powerpc/hw_irq.h
    index fd3f2a206271..d604863d72fb 100644
    --- a/include/asm-powerpc/hw_irq.h
    +++ b/include/asm-powerpc/hw_irq.h
    @@ -102,6 +102,9 @@ static inline void local_irq_save_ptr(unsigned long *flags)
    #define local_irq_save(flags)	local_irq_save_ptr(&flags)
    #define irqs_disabled()		((mfmsr() & MSR_EE) == 0)

    +#define hard_irq_enable()	local_irq_enable()
    +#define hard_irq_disable()	local_irq_disable()
    +
    #endif /* CONFIG_PPC64 */

    #define mask_irq(irq)						\


Note that the above is where we first defined hard_irq_disable() for
PPC32.
Ideally, IIUC, we don't really need this on PPC32 assuming the contract
is only for archs which uses lazy interrupt disabling (i.e. PPC64),
however, there were places where we ended up using hard_irq_disable()
assuming it disables hardware interrupts for both PPC32 and PPC64 code
and later maybe at a number of other places too.

Note that until this time, lockdep and IRQ tracing was not added. So it
wasn't a problem. However, later somewhere we ended up changing the
definition of hard_irq_disable(), to arch_local_irq_disable, maybe in
this...
   commit 5d38902c4838 ("powerpc: Add irqtrace support for 32-bit powerpc")


So IMO, in the longer term, I think we don't need hard_irq_disable() for
PPC32, and it should fallback to generic empty stub, after we fix all
those code paths and use appropriate helpers there. However, that might
need careful inspection of a lot of code paths.


Hence, meanwhile, I think instead of defining another helper like
ppc_hard_irq_disable(), (which was stupid on my part to even suggest w/o
looking at this history), we should change hard_irq_disable() definition
back to local_irq_disable() for PPC32, which anyways takes care of
trace_hardirqs_off().

Therefore, I think we need the following 2 diffs - which should fix both
of the reported issues as well. And this way, we don't need another hard
irq disabling helper on PPC.

DIFF(1)

    diff --git a/arch/powerpc/include/asm/hw_irq.h b/arch/powerpc/include/asm/hw_irq.h
    index b7eee6385ae5..ddb850e62abd 100644
    --- a/arch/powerpc/include/asm/hw_irq.h
    +++ b/arch/powerpc/include/asm/hw_irq.h
    @@ -464,7 +464,7 @@ static inline bool arch_irqs_disabled(void)
            return arch_irqs_disabled_flags(arch_local_save_flags());
     }
    
    -#define hard_irq_disable()             arch_local_irq_disable()
    +#define hard_irq_disable()             local_irq_disable()
    
     static inline bool regs_irqs_disabled(struct pt_regs *regs)
     {


and...

DIFF(2)

    diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
    index 13ad4cf5fa71..543479fc9630 100644
    --- a/arch/powerpc/kvm/booke.c
    +++ b/arch/powerpc/kvm/booke.c
    @@ -722,7 +722,6 @@ int kvmppc_core_prepare_to_enter(struct kvm_vcpu *vcpu)
            if (vcpu->arch.shared->msr & MSR_WE) {
                    local_irq_enable();
                    kvm_vcpu_halt(vcpu);
    -               local_irq_disable();
                    hard_irq_disable();

                    kvmppc_set_exit_type(vcpu, EMULATED_MTMSRWE_EXITS);
    diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
    index 9194cf492d1c..f4db7dea57b3 100644
    --- a/arch/powerpc/kvm/powerpc.c
    +++ b/arch/powerpc/kvm/powerpc.c
    @@ -81,12 +81,6 @@ int kvmppc_prepare_to_enter(struct kvm_vcpu *vcpu)
            int r;

            WARN_ON(irqs_disabled());
    -       /*
    -        * local_irq_disable() first: on 32-bit, hard_irq_disable() alone is a
    -        * raw MSR[EE] clear that bypasses the lockdep/irq-tracing state, and
    -        * the xfer_to_guest_mode helpers assert IRQs are seen as disabled.
    -        */
    -       local_irq_disable();
            hard_irq_disable();

            while (true) {
    @@ -101,7 +95,6 @@ int kvmppc_prepare_to_enter(struct kvm_vcpu *vcpu)
                            */
                            local_irq_enable();
                            r = kvm_xfer_to_guest_mode_handle_work(vcpu);
    -                       local_irq_disable();
                            hard_irq_disable();
                            if (r) {
                                    /*
    @@ -134,7 +127,6 @@ int kvmppc_prepare_to_enter(struct kvm_vcpu *vcpu)
                            local_irq_enable();
                            trace_kvm_check_requests(vcpu);
                            r = kvmppc_core_check_requests(vcpu);
    -                       local_irq_disable();
                            hard_irq_disable();
                            if (r > 0)
                                    continue;


@Shrikanth, @Mukesh, your thoughts?
If this looks good to you too, would you like to take this forward?

And, if I am not wrong, with the above 2 diffs, we don't need this patch
either [1].

[1]: [PATCH] powerpc/entry: Trace the IRQ disable before irqentry_exit() on 32-bit
[1]: https://lore.kernel.org/linuxppc-dev/a14571be1897955f43d9994f0e093de3ada6ecde.1793447880.git.ritesh.list@gmail.com/



-ritesh