LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] x86/kvm: Don't enable IRQ when IRQ enabled in kvm_wait
@ 2021-08-14  3:51 Lai Jiangshan
  2021-08-18  1:36 ` Lai Jiangshan
  2021-09-06 10:32 ` Paolo Bonzini
  0 siblings, 2 replies; 3+ messages in thread
From: Lai Jiangshan @ 2021-08-14  3:51 UTC (permalink / raw)
  To: linux-kernel
  Cc: Lai Jiangshan, Mark Rutland, Peter Zijlstra, Paolo Bonzini,
	Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86,
	H. Peter Anvin, kvm

From: Lai Jiangshan <laijs@linux.alibaba.com>

Commit f4e61f0c9add3 ("x86/kvm: Fix broken irq restoration in kvm_wait")
replaced "local_irq_restore() when IRQ enabled" with "local_irq_enable()
when IRQ enabled" to suppress a warnning.

Although there is no similar debugging warnning for doing local_irq_enable()
when IRQ enabled as doing local_irq_restore() in the same IRQ situation.  But
doing local_irq_enable() when IRQ enabled is no less broken as doing
local_irq_restore() and we'd better avoid it.

Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Lai Jiangshan <laijs@linux.alibaba.com>
---

The original debugging warnning was introduced in commit 997acaf6b4b5
("lockdep: report broken irq restoration").  I think a similar debugging
check and warnning should also be added to "local_irq_enable() when IRQ
enabled" and even maybe "local_irq_disable() when IRQ disabled" to detect
something this:

    | local_irq_save(flags);
    | local_irq_disable();
    | local_irq_restore(flags);
    | local_irq_enable();

Or even we can do the check in lockdep+TRACE_IRQFLAGS:

In lockdep_hardirqs_on_prepare(), lockdep_hardirqs_enabled() was checked
(and exit) before checking DEBUG_LOCKS_WARN_ON(!irqs_disabled()), so lockdep
can't give any warning for these kind of situations.  If we did the check
in lockdep, we would have found the problem before, and we don't need
997acaf6b4b5.

Any thought? Mark? Peter?

 arch/x86/kernel/kvm.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
index a26643dc6bd6..b656456c3a94 100644
--- a/arch/x86/kernel/kvm.c
+++ b/arch/x86/kernel/kvm.c
@@ -884,10 +884,11 @@ static void kvm_wait(u8 *ptr, u8 val)
 	} else {
 		local_irq_disable();
 
+		/* safe_halt() will enable IRQ */
 		if (READ_ONCE(*ptr) == val)
 			safe_halt();
-
-		local_irq_enable();
+		else
+			local_irq_enable();
 	}
 }
 
-- 
2.19.1.6.gb485710b


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] x86/kvm: Don't enable IRQ when IRQ enabled in kvm_wait
  2021-08-14  3:51 [PATCH] x86/kvm: Don't enable IRQ when IRQ enabled in kvm_wait Lai Jiangshan
@ 2021-08-18  1:36 ` Lai Jiangshan
  2021-09-06 10:32 ` Paolo Bonzini
  1 sibling, 0 replies; 3+ messages in thread
From: Lai Jiangshan @ 2021-08-18  1:36 UTC (permalink / raw)
  To: LKML
  Cc: Lai Jiangshan, Mark Rutland, Peter Zijlstra, Paolo Bonzini,
	Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	X86 ML, H. Peter Anvin, kvm

Ping

On Sat, Aug 14, 2021 at 9:36 PM Lai Jiangshan <jiangshanlai@gmail.com> wrote:
>
> From: Lai Jiangshan <laijs@linux.alibaba.com>
>
> Commit f4e61f0c9add3 ("x86/kvm: Fix broken irq restoration in kvm_wait")
> replaced "local_irq_restore() when IRQ enabled" with "local_irq_enable()
> when IRQ enabled" to suppress a warnning.
>
> Although there is no similar debugging warnning for doing local_irq_enable()
> when IRQ enabled as doing local_irq_restore() in the same IRQ situation.  But
> doing local_irq_enable() when IRQ enabled is no less broken as doing
> local_irq_restore() and we'd better avoid it.
>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
> Signed-off-by: Lai Jiangshan <laijs@linux.alibaba.com>
> ---
>
> The original debugging warnning was introduced in commit 997acaf6b4b5
> ("lockdep: report broken irq restoration").  I think a similar debugging
> check and warnning should also be added to "local_irq_enable() when IRQ
> enabled" and even maybe "local_irq_disable() when IRQ disabled" to detect
> something this:
>
>     | local_irq_save(flags);
>     | local_irq_disable();
>     | local_irq_restore(flags);
>     | local_irq_enable();
>
> Or even we can do the check in lockdep+TRACE_IRQFLAGS:
>
> In lockdep_hardirqs_on_prepare(), lockdep_hardirqs_enabled() was checked
> (and exit) before checking DEBUG_LOCKS_WARN_ON(!irqs_disabled()), so lockdep
> can't give any warning for these kind of situations.  If we did the check
> in lockdep, we would have found the problem before, and we don't need
> 997acaf6b4b5.
>
> Any thought? Mark? Peter?
>
>  arch/x86/kernel/kvm.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
> index a26643dc6bd6..b656456c3a94 100644
> --- a/arch/x86/kernel/kvm.c
> +++ b/arch/x86/kernel/kvm.c
> @@ -884,10 +884,11 @@ static void kvm_wait(u8 *ptr, u8 val)
>         } else {
>                 local_irq_disable();
>
> +               /* safe_halt() will enable IRQ */
>                 if (READ_ONCE(*ptr) == val)
>                         safe_halt();
> -
> -               local_irq_enable();
> +               else
> +                       local_irq_enable();
>         }
>  }
>
> --
> 2.19.1.6.gb485710b
>

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] x86/kvm: Don't enable IRQ when IRQ enabled in kvm_wait
  2021-08-14  3:51 [PATCH] x86/kvm: Don't enable IRQ when IRQ enabled in kvm_wait Lai Jiangshan
  2021-08-18  1:36 ` Lai Jiangshan
@ 2021-09-06 10:32 ` Paolo Bonzini
  1 sibling, 0 replies; 3+ messages in thread
From: Paolo Bonzini @ 2021-09-06 10:32 UTC (permalink / raw)
  To: Lai Jiangshan, linux-kernel
  Cc: Lai Jiangshan, Mark Rutland, Peter Zijlstra, Sean Christopherson,
	Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel,
	Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86,
	H. Peter Anvin, kvm

On 14/08/21 05:51, Lai Jiangshan wrote:
> From: Lai Jiangshan <laijs@linux.alibaba.com>
> 
> Commit f4e61f0c9add3 ("x86/kvm: Fix broken irq restoration in kvm_wait")
> replaced "local_irq_restore() when IRQ enabled" with "local_irq_enable()
> when IRQ enabled" to suppress a warnning.
> 
> Although there is no similar debugging warnning for doing local_irq_enable()
> when IRQ enabled as doing local_irq_restore() in the same IRQ situation.  But
> doing local_irq_enable() when IRQ enabled is no less broken as doing
> local_irq_restore() and we'd better avoid it.
> 
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
> Signed-off-by: Lai Jiangshan <laijs@linux.alibaba.com>
> ---
> 
> The original debugging warnning was introduced in commit 997acaf6b4b5
> ("lockdep: report broken irq restoration").  I think a similar debugging
> check and warnning should also be added to "local_irq_enable() when IRQ
> enabled" and even maybe "local_irq_disable() when IRQ disabled" to detect
> something this:
> 
>      | local_irq_save(flags);
>      | local_irq_disable();
>      | local_irq_restore(flags);
>      | local_irq_enable();
> 
> Or even we can do the check in lockdep+TRACE_IRQFLAGS:
> 
> In lockdep_hardirqs_on_prepare(), lockdep_hardirqs_enabled() was checked
> (and exit) before checking DEBUG_LOCKS_WARN_ON(!irqs_disabled()), so lockdep
> can't give any warning for these kind of situations.  If we did the check
> in lockdep, we would have found the problem before, and we don't need
> 997acaf6b4b5.
> 
> Any thought? Mark? Peter?
> 
>   arch/x86/kernel/kvm.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
> index a26643dc6bd6..b656456c3a94 100644
> --- a/arch/x86/kernel/kvm.c
> +++ b/arch/x86/kernel/kvm.c
> @@ -884,10 +884,11 @@ static void kvm_wait(u8 *ptr, u8 val)
>   	} else {
>   		local_irq_disable();
>   
> +		/* safe_halt() will enable IRQ */
>   		if (READ_ONCE(*ptr) == val)
>   			safe_halt();
> -
> -		local_irq_enable();
> +		else
> +			local_irq_enable();
>   	}
>   }
>   
> 

Queued, thanks.

paolo


^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2021-09-06 10:33 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-14  3:51 [PATCH] x86/kvm: Don't enable IRQ when IRQ enabled in kvm_wait Lai Jiangshan
2021-08-18  1:36 ` Lai Jiangshan
2021-09-06 10:32 ` Paolo Bonzini

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).