Re: [Qemu-devel] [PATCH 3/3] hw: correctly implement soft reset

[Laszlo Ersek <lersek@redhat.com>]

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