[SeaBIOS] [PATCH] resume: Don't attempt to use generic reboot mechanisms on QEMU

Kevin O'Connor posted 1 patch 7 years ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/seabios tags/patchew/1488557585-916-1-git-send-email-kevin@koconnor.net
src/fw/shadow.c | 14 +++++++++++++-
src/hw/pci.c    |  1 -
src/hw/pci.h    |  2 ++
src/resume.c    |  4 ++--
src/util.h      |  2 +-
5 files changed, 18 insertions(+), 5 deletions(-)
[SeaBIOS] [PATCH] resume: Don't attempt to use generic reboot mechanisms on QEMU
Posted by Kevin O'Connor 7 years ago
On QEMU it's necessary to manually reset the BIOS memory region
between 0xc0000-0x100000 on a reboot.  After this manual memory reset
is completed, it's not valid to use the generic reset mechanisms.
Rename qemu_prep_reset() to qemu_reboot() and change the function to
immediately reboot after the code memcpy.

This fixes a bug that could cause code corruption on reboots - calling
the udelay() function (as invoked by i8042_reboot and/or pci_reboot)
was not valid after the BIOS was memcpy'd.

Reported-by: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
Signed-off-by: Kevin O'Connor <kevin@koconnor.net>
---

This patch is based on Paolo's recommendation of first attempting a
PCI style reboot on QEMU.  However, instead of next attempting a
keyboard reset, this patch attempts to signal an "INIT" via a triple
fault (as I think that's a bit simpler and less likely to fail).

---
 src/fw/shadow.c | 14 +++++++++++++-
 src/hw/pci.c    |  1 -
 src/hw/pci.h    |  2 ++
 src/resume.c    |  4 ++--
 src/util.h      |  2 +-
 5 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/src/fw/shadow.c b/src/fw/shadow.c
index cd02d3a..c80b266 100644
--- a/src/fw/shadow.c
+++ b/src/fw/shadow.c
@@ -167,7 +167,7 @@ make_bios_readonly(void)
 }
 
 void
-qemu_prep_reset(void)
+qemu_reboot(void)
 {
     if (!CONFIG_QEMU || runningOnXen())
         return;
@@ -187,4 +187,16 @@ qemu_prep_reset(void)
     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
+    // functionality may be available.
+
+    // Attempt PCI style reset
+    outb(0x02, PORT_PCI_REBOOT);
+    outb(0x06, PORT_PCI_REBOOT);
+
+    // Next try triple faulting the CPU to force a reset
+    asm volatile("int3");
 }
diff --git a/src/hw/pci.c b/src/hw/pci.c
index 506ee56..8e3d617 100644
--- a/src/hw/pci.c
+++ b/src/hw/pci.c
@@ -12,7 +12,6 @@
 #include "x86.h" // outl
 
 #define PORT_PCI_CMD           0x0cf8
-#define PORT_PCI_REBOOT        0x0cf9
 #define PORT_PCI_DATA          0x0cfc
 
 void pci_config_writel(u16 bdf, u32 addr, u32 val)
diff --git a/src/hw/pci.h b/src/hw/pci.h
index bf50430..ee6e196 100644
--- a/src/hw/pci.h
+++ b/src/hw/pci.h
@@ -3,6 +3,8 @@
 
 #include "types.h" // u32
 
+#define PORT_PCI_REBOOT        0x0cf9
+
 static inline u8 pci_bdf_to_bus(u16 bdf) {
     return bdf >> 8;
 }
diff --git a/src/resume.c b/src/resume.c
index 99fa34f..fb0b8a8 100644
--- a/src/resume.c
+++ b/src/resume.c
@@ -125,8 +125,8 @@ tryReboot(void)
 {
     dprintf(1, "Attempting a hard reboot\n");
 
-    // Setup for reset on qemu.
-    qemu_prep_reset();
+    // Use a QEMU specific reboot on QEMU
+    qemu_reboot();
 
     // Reboot using ACPI RESET_REG
     acpi_reboot();
diff --git a/src/util.h b/src/util.h
index 336eaaf..8269057 100644
--- a/src/util.h
+++ b/src/util.h
@@ -123,7 +123,7 @@ void pirtable_setup(void);
 // fw/shadow.c
 void make_bios_writable(void);
 void make_bios_readonly(void);
-void qemu_prep_reset(void);
+void qemu_reboot(void);
 
 // fw/smbios.c
 void smbios_legacy_setup(void);
-- 
2.5.5


_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://www.coreboot.org/mailman/listinfo/seabios
Re: [SeaBIOS] [PATCH] resume: Don't attempt to use generic reboot mechanisms on QEMU
Posted by Dr. David Alan Gilbert 7 years ago
* Kevin O'Connor (kevin@koconnor.net) wrote:
> On QEMU it's necessary to manually reset the BIOS memory region
> between 0xc0000-0x100000 on a reboot.  After this manual memory reset
> is completed, it's not valid to use the generic reset mechanisms.
> Rename qemu_prep_reset() to qemu_reboot() and change the function to
> immediately reboot after the code memcpy.
> 
> This fixes a bug that could cause code corruption on reboots - calling
> the udelay() function (as invoked by i8042_reboot and/or pci_reboot)
> was not valid after the BIOS was memcpy'd.
> 
> Reported-by: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> Signed-off-by: Kevin O'Connor <kevin@koconnor.net>
> ---
> 
> This patch is based on Paolo's recommendation of first attempting a
> PCI style reboot on QEMU.  However, instead of next attempting a
> keyboard reset, this patch attempts to signal an "INIT" via a triple
> fault (as I think that's a bit simpler and less likely to fail).

Thanks, I've given this a few hours of testing and it seems to work
(this is testing on head rather than on our downstream).
Curiously I do occasionally get a KVM_EXIT_SHUTDOWN but I think that's
the triple fault you added rather than anything bad happening - it goes
away if I add a debug print before it; here's  a kvm trace
from it:

 qemu-system-x86-2998  [000] 347148.144515: kvm_exit:             reason EXIT_IOIO rip 0xf524a info cf90110 f524b
 qemu-system-x86-2998  [000] 347148.144517: kvm_pio:              pio_write at 0xcf9 size 1 count 1 val 0x2
 qemu-system-x86-2998  [000] 347148.144519: kvm_userspace_exit:   reason KVM_EXIT_IO (2)
 qemu-system-x86-2998  [000] 347148.144528: kvm_entry:            vcpu 0
 qemu-system-x86-2998  [000] 347148.144529: kvm_exit:             reason EXIT_IOIO rip 0xf524d info cf90110 f524e
 qemu-system-x86-2998  [000] 347148.144530: kvm_pio:              pio_write at 0xcf9 size 1 count 1 val 0x6
 qemu-system-x86-2998  [000] 347148.144531: kvm_userspace_exit:   reason KVM_EXIT_IO (2)
 qemu-system-x86-2998  [000] 347148.144715: kvm_entry:            vcpu 0
 qemu-system-x86-2998  [000] 347148.144718: kvm_exit:             reason EXIT_SHUTDOWN rip 0xf524e info 0 0
 qemu-system-x86-2998  [000] 347148.144727: kvm_userspace_exit:   reason KVM_EXIT_SHUTDOWN (8)
 qemu-system-x86-2993  [003] 347148.144881: kvm_set_irq:          gsi 8 level 0 source 0

so it does look like it's directly after that outb.

   f5243:       ba f9 0c 00 00          mov    $0xcf9,%edx
   f5248:       b0 02                   mov    $0x2,%al
   f524a:       ee                      out    %al,(%dx)
   f524b:       b0 06                   mov    $0x6,%al
   f524d:       ee                      out    %al,(%dx)
   f524e:       cc                      int3

So:
Tested-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

Dave
> ---
>  src/fw/shadow.c | 14 +++++++++++++-
>  src/hw/pci.c    |  1 -
>  src/hw/pci.h    |  2 ++
>  src/resume.c    |  4 ++--
>  src/util.h      |  2 +-
>  5 files changed, 18 insertions(+), 5 deletions(-)
> 
> diff --git a/src/fw/shadow.c b/src/fw/shadow.c
> index cd02d3a..c80b266 100644
> --- a/src/fw/shadow.c
> +++ b/src/fw/shadow.c
> @@ -167,7 +167,7 @@ make_bios_readonly(void)
>  }
>  
>  void
> -qemu_prep_reset(void)
> +qemu_reboot(void)
>  {
>      if (!CONFIG_QEMU || runningOnXen())
>          return;
> @@ -187,4 +187,16 @@ qemu_prep_reset(void)
>      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
> +    // functionality may be available.
> +
> +    // Attempt PCI style reset
> +    outb(0x02, PORT_PCI_REBOOT);
> +    outb(0x06, PORT_PCI_REBOOT);
> +
> +    // Next try triple faulting the CPU to force a reset
> +    asm volatile("int3");
>  }
> diff --git a/src/hw/pci.c b/src/hw/pci.c
> index 506ee56..8e3d617 100644
> --- a/src/hw/pci.c
> +++ b/src/hw/pci.c
> @@ -12,7 +12,6 @@
>  #include "x86.h" // outl
>  
>  #define PORT_PCI_CMD           0x0cf8
> -#define PORT_PCI_REBOOT        0x0cf9
>  #define PORT_PCI_DATA          0x0cfc
>  
>  void pci_config_writel(u16 bdf, u32 addr, u32 val)
> diff --git a/src/hw/pci.h b/src/hw/pci.h
> index bf50430..ee6e196 100644
> --- a/src/hw/pci.h
> +++ b/src/hw/pci.h
> @@ -3,6 +3,8 @@
>  
>  #include "types.h" // u32
>  
> +#define PORT_PCI_REBOOT        0x0cf9
> +
>  static inline u8 pci_bdf_to_bus(u16 bdf) {
>      return bdf >> 8;
>  }
> diff --git a/src/resume.c b/src/resume.c
> index 99fa34f..fb0b8a8 100644
> --- a/src/resume.c
> +++ b/src/resume.c
> @@ -125,8 +125,8 @@ tryReboot(void)
>  {
>      dprintf(1, "Attempting a hard reboot\n");
>  
> -    // Setup for reset on qemu.
> -    qemu_prep_reset();
> +    // Use a QEMU specific reboot on QEMU
> +    qemu_reboot();
>  
>      // Reboot using ACPI RESET_REG
>      acpi_reboot();
> diff --git a/src/util.h b/src/util.h
> index 336eaaf..8269057 100644
> --- a/src/util.h
> +++ b/src/util.h
> @@ -123,7 +123,7 @@ void pirtable_setup(void);
>  // fw/shadow.c
>  void make_bios_writable(void);
>  void make_bios_readonly(void);
> -void qemu_prep_reset(void);
> +void qemu_reboot(void);
>  
>  // fw/smbios.c
>  void smbios_legacy_setup(void);
> -- 
> 2.5.5
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://www.coreboot.org/mailman/listinfo/seabios
Re: [SeaBIOS] [PATCH] resume: Don't attempt to use generic reboot mechanisms on QEMU
Posted by Kevin O'Connor 7 years ago
On Mon, Mar 06, 2017 at 01:49:26PM +0000, Dr. David Alan Gilbert wrote:
> * Kevin O'Connor (kevin@koconnor.net) wrote:
> > On QEMU it's necessary to manually reset the BIOS memory region
> > between 0xc0000-0x100000 on a reboot.  After this manual memory reset
> > is completed, it's not valid to use the generic reset mechanisms.
> > Rename qemu_prep_reset() to qemu_reboot() and change the function to
> > immediately reboot after the code memcpy.
> > 
> > This fixes a bug that could cause code corruption on reboots - calling
> > the udelay() function (as invoked by i8042_reboot and/or pci_reboot)
> > was not valid after the BIOS was memcpy'd.
> > 
> > Reported-by: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > Signed-off-by: Kevin O'Connor <kevin@koconnor.net>
> > ---
> > 
> > This patch is based on Paolo's recommendation of first attempting a
> > PCI style reboot on QEMU.  However, instead of next attempting a
> > keyboard reset, this patch attempts to signal an "INIT" via a triple
> > fault (as I think that's a bit simpler and less likely to fail).
> 
> Thanks, I've given this a few hours of testing and it seems to work
> (this is testing on head rather than on our downstream).

Thanks.  I committed this patch.

-Kevin

_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://www.coreboot.org/mailman/listinfo/seabios