Re: [Qemu-devel] [PATCH] SeaBios: Fix reset procedure reentrancy problem on qemu-kvm platform

[Kevin O'Connor <kevin@koconnor.net>]

On Mon, Nov 09, 2015 at 08:32:53AM -0500, Kevin O'Connor wrote:
> On Fri, Nov 06, 2015 at 09:12:34AM +0000, Xulei (Stone) wrote:
> > >On Wed, Nov 04, 2015 at 08:48:20AM +0800, Gonglei wrote:
> > >I'm surprised you would see the above on a recent qemu/kvm though - as
> > >on a newer KVM I think the second reset would have to happen after
> > >HaveAttemptedReboot is set and prior to the memcpy in
> > >qemu_prep_reset() completing.  Can you verify your KVM version?
> > 
> > I've tested on KVM-3.6 and KVM-4.1.3. On both of these versions, i can 
> > see this problem. 
> > I do like this: put a HA and a watchdog mechanism in a VM. Deliberately, 
> > let this VM lose heartbeat and don't feed dog. Then, after 2 minutes, 
> > a self-defined timeout, HA mechnism will issue a internal reboot command to
> > the VM and watchdog mechanism will issue a "virsh reset" from the host. Then, 
> > aforementioned problem will occurs in high probability. 
> 
> Ah, okay.  I'm not sure what the best solution to this problem is.

After thinking about this further, I think we can move the
HaveAttemptedReboot assignment after the memcpy.  Does the SeaBIOS
patch below fix things for you?

-Kevin


commit d0e9e2cca9fa6dacd2ad07081ef09c59be1ae945
Author: Kevin O'Connor <kevin@koconnor.net>
Date:   Mon Nov 9 15:00:19 2015 -0500

    resume: Don't set HaveAttemptedReboot until after internal bios memcpy
    
    Move the check for soft reboot loops from resume.c to shadow.c and
    only set the HaveAttemptedReboot flag after restoring the BIOS image.
    This prevents a hang if an external reboot request occurs during the
    BIOS memcpy.
    
    Signed-off-by: Kevin O'Connor <kevin@koconnor.net>

diff --git a/src/fw/shadow.c b/src/fw/shadow.c
index ee87d36..f2d0d65 100644
--- a/src/fw/shadow.c
+++ b/src/fw/shadow.c
@@ -156,6 +156,8 @@ make_bios_readonly(void)
         make_bios_readonly_intel(ShadowBDF, Q35_HOST_BRIDGE_PAM0);
 }
 
+u8 HaveAttemptedReboot VARLOW;
+
 void
 qemu_prep_reset(void)
 {
@@ -163,7 +165,13 @@ qemu_prep_reset(void)
         return;
     // QEMU doesn't map 0xc0000-0xfffff back to the original rom on a
     // reset, so do that manually before invoking a hard reset.
+    if (HaveAttemptedReboot) {
+        // Hard reboot has failed - try to shutdown machine.
+        dprintf(1, "Unable to hard-reboot machine - attempting shutdown.\n");
+        apm_shutdown();
+    }
     make_bios_writable();
     memcpy(VSYMBOL(code32flat_start), VSYMBOL(code32flat_start) + BIOS_SRC_OFFSET
            , SYMBOL(code32flat_end) - SYMBOL(code32flat_start));
+    HaveAttemptedReboot = 1;
 }
diff --git a/src/resume.c b/src/resume.c
index a5465d8..afeadcf 100644
--- a/src/resume.c
+++ b/src/resume.c
@@ -114,19 +114,10 @@ s3_resume(void)
     farcall16big(&br);
 }
 
-u8 HaveAttemptedReboot VARLOW;
-
 // Attempt to invoke a hard-reboot.
 static void
 tryReboot(void)
 {
-    if (HaveAttemptedReboot) {
-        // Hard reboot has failed - try to shutdown machine.
-        dprintf(1, "Unable to hard-reboot machine - attempting shutdown.\n");
-        apm_shutdown();
-    }
-    HaveAttemptedReboot = 1;
-
     dprintf(1, "Attempting a hard reboot\n");
 
     // Setup for reset on qemu.