[PATCH] x86/kvm: Don't enable IRQ when IRQ enabled in kvm_wait

[PATCH] x86/kvm: Don't enable IRQ when IRQ enabled in kvm_wait

[Lai Jiangshan]

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

Re: [PATCH] x86/kvm: Don't enable IRQ when IRQ enabled in kvm_wait

[Lai Jiangshan]

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
>

Re: [PATCH] x86/kvm: Don't enable IRQ when IRQ enabled in kvm_wait

[Paolo Bonzini]

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