Patchwork [3/3] hw: correctly implement soft reset

login
register
mail settings
Submitter Paolo Bonzini
Date March 5, 2013, 3:04 p.m.
Message ID <1362495898-15352-4-git-send-email-pbonzini@redhat.com>
Download mbox | patch
Permalink /patch/225058/
State New
Headers show

Comments

Paolo Bonzini - March 5, 2013, 3:04 p.m.
Do not do a hard reset for port 92h, keyboard controller, or cf9h soft reset.
These only reset the CPU.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/lpc_ich9.c | 7 ++++++-
 hw/pc.c       | 3 ++-
 hw/pckbd.c    | 5 +++--
 hw/piix_pci.c | 8 ++++++--
 4 files changed, 17 insertions(+), 6 deletions(-)
Anthony Liguori - March 5, 2013, 5:18 p.m.
Paolo Bonzini <pbonzini@redhat.com> writes:

> Do not do a hard reset for port 92h, keyboard controller, or cf9h soft reset.
> These only reset the CPU.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

I'm quite confident these devices should trigger a soft reset but not
confident this is exhaustive.

Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>

Regards,

Anthony Liguori

> ---
>  hw/lpc_ich9.c | 7 ++++++-
>  hw/pc.c       | 3 ++-
>  hw/pckbd.c    | 5 +++--
>  hw/piix_pci.c | 8 ++++++--
>  4 files changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/hw/lpc_ich9.c b/hw/lpc_ich9.c
> index eceb052..fae31df 100644
> --- a/hw/lpc_ich9.c
> +++ b/hw/lpc_ich9.c
> @@ -45,6 +45,7 @@
>  #include "pci/pci_bus.h"
>  #include "exec/address-spaces.h"
>  #include "sysemu/sysemu.h"
> +#include "sysemu/cpus.h"
>  
>  static int ich9_lpc_sci_irq(ICH9LPCState *lpc);
>  
> @@ -506,7 +507,11 @@ static void ich9_rst_cnt_write(void *opaque, hwaddr addr, uint64_t val,
>      ICH9LPCState *lpc = opaque;
>  
>      if (val & 4) {
> -        qemu_system_reset_request();
> +        if (val & 0xA) {
> +            qemu_system_reset_request();
> +        } else {
> +            cpu_soft_reset();
> +        }
>          return;
>      }
>      lpc->rst_cnt = val & 0xA; /* keep FULL_RST (bit 3) and SYS_RST (bit 1) */
> diff --git a/hw/pc.c b/hw/pc.c
> index 523db1f..6080d62 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -45,6 +45,7 @@
>  #include "kvm_i386.h"
>  #include "xen.h"
>  #include "sysemu/blockdev.h"
> +#include "sysemu/cpus.h"
>  #include "hw/block-common.h"
>  #include "ui/qemu-spice.h"
>  #include "exec/memory.h"
> @@ -441,7 +442,7 @@ static void port92_write(void *opaque, hwaddr addr, uint64_t val,
>      s->outport = val;
>      qemu_set_irq(*s->a20_out, (val >> 1) & 1);
>      if ((val & 1) && !(oldval & 1)) {
> -        qemu_system_reset_request();
> +        cpu_soft_reset();
>      }
>  }
>  
> diff --git a/hw/pckbd.c b/hw/pckbd.c
> index 3bad09b..fd66788 100644
> --- a/hw/pckbd.c
> +++ b/hw/pckbd.c
> @@ -26,6 +26,7 @@
>  #include "pc.h"
>  #include "ps2.h"
>  #include "sysemu/sysemu.h"
> +#include "sysemu/cpus.h"
>  
>  /* debug PC keyboard */
>  //#define DEBUG_KBD
> @@ -220,7 +221,7 @@ static void outport_write(KBDState *s, uint32_t val)
>          qemu_set_irq(*s->a20_out, (val >> 1) & 1);
>      }
>      if (!(val & 1)) {
> -        qemu_system_reset_request();
> +        cpu_soft_reset();
>      }
>  }
>  
> @@ -299,7 +300,7 @@ static void kbd_write_command(void *opaque, hwaddr addr,
>          s->outport &= ~KBD_OUT_A20;
>          break;
>      case KBD_CCMD_RESET:
> -        qemu_system_reset_request();
> +        cpu_soft_reset();
>          break;
>      case KBD_CCMD_NO_OP:
>          /* ignore that */
> diff --git a/hw/piix_pci.c b/hw/piix_pci.c
> index 6c77e49..785e0a7 100644
> --- a/hw/piix_pci.c
> +++ b/hw/piix_pci.c
> @@ -32,6 +32,7 @@
>  #include "xen.h"
>  #include "pam.h"
>  #include "sysemu/sysemu.h"
> +#include "sysemu/cpus.h"
>  
>  /*
>   * I440FX chipset data sheet.
> @@ -521,8 +522,11 @@ static void rcr_write(void *opaque, hwaddr addr, uint64_t val, unsigned len)
>      PIIX3State *d = opaque;
>  
>      if (val & 4) {
> -        qemu_system_reset_request();
> -        return;
> +        if (val & 2) {
> +            qemu_system_reset_request();
> +        } else {
> +            cpu_soft_reset();
> +        }
>      }
>      d->rcr = val & 2; /* keep System Reset type only */
>  }
> -- 
> 1.8.1.4
David Woodhouse - March 5, 2013, 5:22 p.m.
On Tue, 2013-03-05 at 16:04 +0100, Paolo Bonzini wrote:
> Do not do a hard reset for port 92h, keyboard controller, or cf9h soft
> reset. These only reset the CPU.

Is a keyboard reset also supposed to reset the A20 gate?
Paolo Bonzini - March 5, 2013, 5:43 p.m.
Il 05/03/2013 18:22, David Woodhouse ha scritto:
> On Tue, 2013-03-05 at 16:04 +0100, Paolo Bonzini wrote:
>> > Do not do a hard reset for port 92h, keyboard controller, or cf9h soft
>> > reset. These only reset the CPU.
> Is a keyboard reset also supposed to reset the A20 gate?

Resetting with 0xFE is not supposed to reset the A20 gate.

Resetting with 0xD1 will set the A20 gate to whatever value you place in
the data byte.  (Also, resetting with 0xD1 for what I could find does
not have the same edge-triggered behavior as port 92).

Paolo
Laszlo Ersek - March 5, 2013, 6:32 p.m.
comments in-line

On 03/05/13 16:04, Paolo Bonzini wrote:
> Do not do a hard reset for port 92h, keyboard controller, or cf9h soft reset.
> These only reset the CPU.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/lpc_ich9.c | 7 ++++++-
>  hw/pc.c       | 3 ++-
>  hw/pckbd.c    | 5 +++--
>  hw/piix_pci.c | 8 ++++++--
>  4 files changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/lpc_ich9.c b/hw/lpc_ich9.c
> index eceb052..fae31df 100644
> --- a/hw/lpc_ich9.c
> +++ b/hw/lpc_ich9.c
> @@ -45,6 +45,7 @@
>  #include "pci/pci_bus.h"
>  #include "exec/address-spaces.h"
>  #include "sysemu/sysemu.h"
> +#include "sysemu/cpus.h"
>  
>  static int ich9_lpc_sci_irq(ICH9LPCState *lpc);
>  
> @@ -506,7 +507,11 @@ static void ich9_rst_cnt_write(void *opaque, hwaddr addr, uint64_t val,
>      ICH9LPCState *lpc = opaque;
>  
>      if (val & 4) {
> -        qemu_system_reset_request();
> +        if (val & 0xA) {
> +            qemu_system_reset_request();
> +        } else {
> +            cpu_soft_reset();
> +        }
>          return;
>      }
>      lpc->rst_cnt = val & 0xA; /* keep FULL_RST (bit 3) and SYS_RST (bit 1) */

So any of FULL_RST and SYS_RST render it hard.


> diff --git a/hw/pc.c b/hw/pc.c
> index 523db1f..6080d62 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -45,6 +45,7 @@
>  #include "kvm_i386.h"
>  #include "xen.h"
>  #include "sysemu/blockdev.h"
> +#include "sysemu/cpus.h"
>  #include "hw/block-common.h"
>  #include "ui/qemu-spice.h"
>  #include "exec/memory.h"
> @@ -441,7 +442,7 @@ static void port92_write(void *opaque, hwaddr addr, uint64_t val,
>      s->outport = val;
>      qemu_set_irq(*s->a20_out, (val >> 1) & 1);
>      if ((val & 1) && !(oldval & 1)) {
> -        qemu_system_reset_request();
> +        cpu_soft_reset();
>      }
>  }

I checked this against the data-sheet, OK.

>  
> diff --git a/hw/pckbd.c b/hw/pckbd.c
> index 3bad09b..fd66788 100644
> --- a/hw/pckbd.c
> +++ b/hw/pckbd.c
> @@ -26,6 +26,7 @@
>  #include "pc.h"
>  #include "ps2.h"
>  #include "sysemu/sysemu.h"
> +#include "sysemu/cpus.h"
>  
>  /* debug PC keyboard */
>  //#define DEBUG_KBD
> @@ -220,7 +221,7 @@ static void outport_write(KBDState *s, uint32_t val)
>          qemu_set_irq(*s->a20_out, (val >> 1) & 1);
>      }
>      if (!(val & 1)) {
> -        qemu_system_reset_request();
> +        cpu_soft_reset();
>      }
>  }
>  
> @@ -299,7 +300,7 @@ static void kbd_write_command(void *opaque, hwaddr addr,
>          s->outport &= ~KBD_OUT_A20;
>          break;
>      case KBD_CCMD_RESET:
> -        qemu_system_reset_request();
> +        cpu_soft_reset();
>          break;
>      case KBD_CCMD_NO_OP:
>          /* ignore that */

I couldn't find the datasheet for this, but
<http://wiki.osdev.org/%228042%22_PS/2_Controller> seems to confirm that
both of these should trigger a reset. Not sure about hard vs. soft.

> diff --git a/hw/piix_pci.c b/hw/piix_pci.c
> index 6c77e49..785e0a7 100644
> --- a/hw/piix_pci.c
> +++ b/hw/piix_pci.c
> @@ -32,6 +32,7 @@
>  #include "xen.h"
>  #include "pam.h"
>  #include "sysemu/sysemu.h"
> +#include "sysemu/cpus.h"
>  
>  /*
>   * I440FX chipset data sheet.
> @@ -521,8 +522,11 @@ static void rcr_write(void *opaque, hwaddr addr, uint64_t val, unsigned len)
>      PIIX3State *d = opaque;
>  
>      if (val & 4) {
> -        qemu_system_reset_request();
> -        return;
> +        if (val & 2) {
> +            qemu_system_reset_request();
> +        } else {
> +            cpu_soft_reset();
> +        }
>      }
>      d->rcr = val & 2; /* keep System Reset type only */
>  }
> 

This is slightly different from your ICH9 change at the top: you remove
the "return" statement only here. For a hard reset it doesn't matter,
but in case of a soft reset, PIIX3State.rcr will updated in a
guest-visible way, while ICH9LPCState.rst_cnt won't be updated at all.

I guess we should unify these by dropping the "return" from
ich9_rst_cnt_write() -- the other bits changed simultaneously when
kicking the Reset CPU bit should be visible after a soft reset, I think.

Laszlo

Patch

diff --git a/hw/lpc_ich9.c b/hw/lpc_ich9.c
index eceb052..fae31df 100644
--- a/hw/lpc_ich9.c
+++ b/hw/lpc_ich9.c
@@ -45,6 +45,7 @@ 
 #include "pci/pci_bus.h"
 #include "exec/address-spaces.h"
 #include "sysemu/sysemu.h"
+#include "sysemu/cpus.h"
 
 static int ich9_lpc_sci_irq(ICH9LPCState *lpc);
 
@@ -506,7 +507,11 @@  static void ich9_rst_cnt_write(void *opaque, hwaddr addr, uint64_t val,
     ICH9LPCState *lpc = opaque;
 
     if (val & 4) {
-        qemu_system_reset_request();
+        if (val & 0xA) {
+            qemu_system_reset_request();
+        } else {
+            cpu_soft_reset();
+        }
         return;
     }
     lpc->rst_cnt = val & 0xA; /* keep FULL_RST (bit 3) and SYS_RST (bit 1) */
diff --git a/hw/pc.c b/hw/pc.c
index 523db1f..6080d62 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -45,6 +45,7 @@ 
 #include "kvm_i386.h"
 #include "xen.h"
 #include "sysemu/blockdev.h"
+#include "sysemu/cpus.h"
 #include "hw/block-common.h"
 #include "ui/qemu-spice.h"
 #include "exec/memory.h"
@@ -441,7 +442,7 @@  static void port92_write(void *opaque, hwaddr addr, uint64_t val,
     s->outport = val;
     qemu_set_irq(*s->a20_out, (val >> 1) & 1);
     if ((val & 1) && !(oldval & 1)) {
-        qemu_system_reset_request();
+        cpu_soft_reset();
     }
 }
 
diff --git a/hw/pckbd.c b/hw/pckbd.c
index 3bad09b..fd66788 100644
--- a/hw/pckbd.c
+++ b/hw/pckbd.c
@@ -26,6 +26,7 @@ 
 #include "pc.h"
 #include "ps2.h"
 #include "sysemu/sysemu.h"
+#include "sysemu/cpus.h"
 
 /* debug PC keyboard */
 //#define DEBUG_KBD
@@ -220,7 +221,7 @@  static void outport_write(KBDState *s, uint32_t val)
         qemu_set_irq(*s->a20_out, (val >> 1) & 1);
     }
     if (!(val & 1)) {
-        qemu_system_reset_request();
+        cpu_soft_reset();
     }
 }
 
@@ -299,7 +300,7 @@  static void kbd_write_command(void *opaque, hwaddr addr,
         s->outport &= ~KBD_OUT_A20;
         break;
     case KBD_CCMD_RESET:
-        qemu_system_reset_request();
+        cpu_soft_reset();
         break;
     case KBD_CCMD_NO_OP:
         /* ignore that */
diff --git a/hw/piix_pci.c b/hw/piix_pci.c
index 6c77e49..785e0a7 100644
--- a/hw/piix_pci.c
+++ b/hw/piix_pci.c
@@ -32,6 +32,7 @@ 
 #include "xen.h"
 #include "pam.h"
 #include "sysemu/sysemu.h"
+#include "sysemu/cpus.h"
 
 /*
  * I440FX chipset data sheet.
@@ -521,8 +522,11 @@  static void rcr_write(void *opaque, hwaddr addr, uint64_t val, unsigned len)
     PIIX3State *d = opaque;
 
     if (val & 4) {
-        qemu_system_reset_request();
-        return;
+        if (val & 2) {
+            qemu_system_reset_request();
+        } else {
+            cpu_soft_reset();
+        }
     }
     d->rcr = val & 2; /* keep System Reset type only */
 }