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

[Kevin O'Connor]

On Mon, Nov 09, 2015 at 03:06:18PM -0500, Kevin O'Connor wrote:
> 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.

The previous patch could cause corruption if the memcpy() failed.  I
think the new SeaBIOS patch below should be okay though.

-Kevin


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

    resume: Make KVM soft reboot loop detection more flexible
    
    Move the check for soft reboot loops from resume.c to shadow.c and
    directly check for the case where the memcpy fails.  This prevents a
    hang if an external reboot request occurs during the BIOS memcpy.
    
    Signed-off-by: Kevin O'Connor <kevin at koconnor.net>

diff --git a/src/fw/shadow.c b/src/fw/shadow.c
index ee87d36..b2f2dd8 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);
 }
 
+static u8 AttemptingReboot;
+
 void
 qemu_prep_reset(void)
 {
@@ -164,6 +166,19 @@ qemu_prep_reset(void)
     // QEMU doesn't map 0xc0000-0xfffff back to the original rom on a
     // reset, so do that manually before invoking a hard reset.
     make_bios_writable();
+    AttemptingReboot = 1;
+    barrier();
+    if (!AttemptingReboot)
+        goto fail;
+    barrier();
     memcpy(VSYMBOL(code32flat_start), VSYMBOL(code32flat_start) + BIOS_SRC_OFFSET
            , SYMBOL(code32flat_end) - SYMBOL(code32flat_start));
+    barrier();
+    if (AttemptingReboot)
+        goto fail;
+    return;
+fail:
+    // Attempt to restore code has failed - try to shutdown machine.
+    dprintf(1, "Unable to hard-reboot machine - attempting shutdown.\n");
+    apm_shutdown();
 }
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.