On 14/08/21 05:51, Lai Jiangshan wrote:
From: Lai Jiangshan <laijs@xxxxxxxxxxxxxxxxx>
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@xxxxxxx>
Cc: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>
Signed-off-by: Lai Jiangshan <laijs@xxxxxxxxxxxxxxxxx>
---
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
