[SeaBIOS] reboot fails with QEMU "isapc" machine type

[Daniel P. Berrangé]

On Thu, Feb 22, 2018 at 08:40:09PM -0500, Kevin O'Connor wrote:
> On Mon, Feb 05, 2018 at 12:46:41PM +0000, Daniel P. Berrangé wrote:
> > I was experimenting with old MS-DOS 6.22 under QEMU with the 'isapc' machine
> > type, and found it is not able to reboot the guest. Bisecting QEMU blamed
> > the QEMU change that rebased from "rel-1.9.3", to "rel-1.10.0". Further
> > bisecting SeaBIOS blames this change:
> > 
> >   commit b837e68d5a6c1a5945513f1995875445a1594c8a (refs/bisect/bad)
> >   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 copy of the BIOS in flash
> >     appears to be a memory alias instead.  This prevents a hang if an
> >     external reboot request occurs during the BIOS memcpy.
> >     
> >     Signed-off-by: Kevin O'Connor <kevin at koconnor.net>
> > 
> > I was testing as follows:
> > 
> >   $ qemu-system-x86_64 -machine isapc -monitor stdio demo.img
> 
> I finally got this to work locally.  Apparently the isapc machine type
> requires a rom that is 128K in size.
> 
> This does appear to be a regression.  I think the patch below should
> fix it.

Thanks, I've tested this patch and confirm it fixes the behaviour
I've been seeing.


> commit 42812e062a77b27b0544c8e0d46d206afc3b2fae (HEAD -> master)
> Author: Kevin O'Connor <kevin at koconnor.net>
> Date:   Thu Feb 22 20:29:27 2018 -0500
> 
>     shadow: Don't invoke a shutdown on reboot unless in a reboot loop
>     
>     Old versions of KVM would map the same writable copy of the BIOS at
>     both 0x000f0000 and 0xffff0000.  As a result, a reboot on these
>     machines would result in a reboot loop.  So, the code attempts to
>     check for that situation and invoke a shutdown instead.
>     
>     Commit b837e68d changed the check to run prior to the first reboot.
>     However, this broke reboots on the QEMU isapc machine type.  Change
>     the reboot loop check to only be invoked after at least one reboot has
>     been attempted.
>     
>     Reported-by: Daniel P. Berrangé <berrange at redhat.com>
>     Signed-off-by: Kevin O'Connor <kevin at koconnor.net>
> 
> diff --git a/src/fw/shadow.c b/src/fw/shadow.c
> index c80b266..987eaf4 100644
> --- a/src/fw/shadow.c
> +++ b/src/fw/shadow.c
> @@ -176,18 +176,23 @@ qemu_reboot(void)
>      void *cstart = VSYMBOL(code32flat_start), *cend = VSYMBOL(code32flat_end);
>      void *hrp = &HaveRunPost;
>      if (readl(hrp + BIOS_SRC_OFFSET)) {
> -        // Some old versions of KVM don't store a pristine copy of the
> -        // BIOS in high memory.  Try to shutdown the machine instead.
> -        dprintf(1, "Unable to hard-reboot machine - attempting shutdown.\n");
> -        apm_shutdown();
> +        // There isn't a pristine copy of the BIOS at 0xffff0000 to copy
> +        if (HaveRunPost == 3) {
> +            // In a reboot loop.  Try to shutdown the machine instead.
> +            dprintf(1, "Unable to hard-reboot machine - attempting shutdown.\n");
> +            apm_shutdown();
> +        }
> +        make_bios_writable();
> +        HaveRunPost = 3;
> +    } else {
> +        // Copy the BIOS making sure to only reset HaveRunPost at end
> +        make_bios_writable();
> +        memcpy(cstart, cstart + BIOS_SRC_OFFSET, hrp - cstart);
> +        memcpy(hrp + 4, hrp + 4 + BIOS_SRC_OFFSET, cend - (hrp + 4));
> +        barrier();
> +        HaveRunPost = 0;
> +        barrier();
>      }
> -    // Copy the BIOS making sure to only reset HaveRunPost at end
> -    make_bios_writable();
> -    memcpy(cstart, cstart + BIOS_SRC_OFFSET, hrp - cstart);
> -    memcpy(hrp + 4, hrp + 4 + BIOS_SRC_OFFSET, cend - (hrp + 4));
> -    barrier();
> -    HaveRunPost = 0;
> -    barrier();
>  
>      // Request a QEMU system reset.  Do the reset in this function as
>      // the BIOS code was overwritten above and not all BIOS

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|