plat/kvm/x86: Fix lost IRQs

There is a race condition in `ukplat_lcpu_halt_irq()`, where an IRQ
may fire in between the short window after which interrupts are
enabled and before the halt is done. This may halt the caller
forever (or much longer until the next interrupt).

As this issue is platform and architecture specific, this commit
moves `ukplat_lcpu_halt_irq()` from the common code to the
plat/arch-specific implementation. For x86 the race condition
is avoided by ensuring that there are no instructions between
`STI` and `HLT`.

Signed-off-by: Marc Rittinghaus <marc.rittinghaus@kit.edu>
Reviewed-by: Simon Kuenzer <simon.kuenzer@neclab.eu>
Tested-by: Unikraft CI <monkey@unikraft.io>
GitHub-Pull-Request: #257

diff --git a/plat/common/include/x86/irq.h b/plat/common/include/x86/irq.h
index f2bf2d9872be6a87cb5aa6b9daa4ebd1c009a2e8..2d2cd14a2739acdfd1bb67b9f3690b666ca807a6 100644 (file)
--- a/plat/common/include/x86/irq.h
+++ b/plat/common/include/x86/irq.h
@@ -49,6 +49,11 @@
        asm volatile("sti" : : : "memory"); \
 })
 
+#define __sti_hlt() \
+({ \
+       asm volatile("sti; hlt" : : : "memory"); \
+})
+
 #define __save_flags(x) \
        do { \
                unsigned long __f; \
@@ -83,6 +88,7 @@ static inline int irqs_disabled(void)
 #define local_save_flags(x)      __save_flags(x)
 #define local_irq_disable()      __cli()
 #define local_irq_enable()       __sti()
+#define local_irq_enable_halt()  __sti_hlt()
 
 #define __MAX_IRQ      16
 

diff --git a/plat/common/lcpu.c b/plat/common/lcpu.c
index 4c31231e65420a08b6aef64584116524d36c6d1f..a3a1e3995290fcdfea7338811d367c2082d17932 100644 (file)
--- a/plat/common/lcpu.c
+++ b/plat/common/lcpu.c
@@ -34,20 +34,12 @@
 #include <uk/plat/common/cpu.h>
 #include <uk/plat/common/_time.h>
 
-
 void ukplat_lcpu_halt(void)
 {
        ukplat_lcpu_disable_irq();
        halt();
 }
 
-void ukplat_lcpu_halt_irq(void)
-{
-       ukplat_lcpu_enable_irq();
-       halt();
-       ukplat_lcpu_disable_irq();
-}
-
 void ukplat_lcpu_halt_to(__snsec until)
 {
        unsigned long flags;

diff --git a/plat/kvm/arm/lcpu.c b/plat/kvm/arm/lcpu.c
index 2d322191e0e963b540722c091081c4dffa5e5a5b..548ca9afc75fbea6e986abb4bd301fb2ee251b8c 100644 (file)
--- a/plat/kvm/arm/lcpu.c
+++ b/plat/kvm/arm/lcpu.c
@@ -43,6 +43,13 @@ void ukplat_lcpu_disable_irq(void)
        local_irq_disable();
 }
 
+void ukplat_lcpu_halt_irq(void)
+{
+       ukplat_lcpu_enable_irq();
+       halt();
+       ukplat_lcpu_disable_irq();
+}
+
 unsigned long ukplat_lcpu_save_irqf(void)
 {
        unsigned long flags;

diff --git a/plat/kvm/x86/lcpu.c b/plat/kvm/x86/lcpu.c
index ed359e5c06a141cfd4b6cbd31947d9d7af5ad529..f3cbf55af7fa488aaa48e1045d11d122b441a2aa 100644 (file)
--- a/plat/kvm/x86/lcpu.c
+++ b/plat/kvm/x86/lcpu.c
@@ -35,7 +35,6 @@
 #include <uk/plat/lcpu.h>
 #include <x86/irq.h>
 
-
 void ukplat_lcpu_enable_irq(void)
 {
        local_irq_enable();
@@ -46,6 +45,21 @@ void ukplat_lcpu_disable_irq(void)
        local_irq_disable();
 }
 
+void ukplat_lcpu_halt_irq(void)
+{
+       /*
+        * We have to be careful when enabling interrupts before entering a
+        * halt state. If we want to wait for an interrupt (e.g., a timer)
+        * the interrupt may fire in the short window between sti and hlt and
+        * we are going to halt forever. As sti only enables interrupts after
+        * the following instruction, we can avoid the race condition by
+        * ensuring that hlt immediately follows sti. There must be no
+        * instruction in between.
+        */
+       local_irq_enable_halt();
+       local_irq_disable();
+}
+
 unsigned long ukplat_lcpu_save_irqf(void)
 {
        unsigned long flags;

diff --git a/plat/linuxu/irq.c b/plat/linuxu/irq.c
index f1dbd58cb42b8b3db4c0cb330a41982c54a76673..607f0d2fb15e1d33f01c58d643d06b3214d6ba07 100644 (file)
--- a/plat/linuxu/irq.c
+++ b/plat/linuxu/irq.c
@@ -79,6 +79,13 @@ void ukplat_lcpu_disable_irq(void)
        irq_enabled = 0;
 }
 
+void ukplat_lcpu_halt_irq(void)
+{
+       ukplat_lcpu_enable_irq();
+       halt();
+       ukplat_lcpu_disable_irq();
+}
+
 int ukplat_lcpu_irqs_disabled(void)
 {
        return (irq_enabled == 0);

diff --git a/plat/xen/lcpu.c b/plat/xen/lcpu.c
index d6b18f7a5523f2ffb6ec8f863c88a0f8285cdc85..5875dad7670ddbc302f658f50d87b3490eef241e 100644 (file)
--- a/plat/xen/lcpu.c
+++ b/plat/xen/lcpu.c
@@ -51,6 +51,13 @@ void ukplat_lcpu_disable_irq(void)
        local_irq_disable();
 }
 
+void ukplat_lcpu_halt_irq(void)
+{
+       ukplat_lcpu_enable_irq();
+       halt();
+       ukplat_lcpu_disable_irq();
+}
+
 unsigned long ukplat_lcpu_save_irqf(void)
 {
        unsigned long flags;