Re: [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 
> >   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 <[e-mail]>
> >
> > 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 
> 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é <[e-mail]>
> Signed-off-by: Kevin O'Connor <[e-mail]>
>
> 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