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

[Paolo Bonzini <pbonzini@redhat.com>]

Il 06/03/2013 03:02, li guang ha scritto:
> 在 2013-03-05二的 20:00 +0100，Paolo Bonzini写道：
>> Do not do a hard reset for port 92h, keyboard controller, or cf9h soft reset.
>> These only reset the CPU.
>>
>> Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>
>> 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 e473758..5540f61 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_reset_all_async();
>> +        }
>>          return;
> 
> in fact, soft reset is cpu reset, hard reset is platform reset,
> it is too harsh to require both bit 3 & 1 to do a system reset,
> they are independent, either of them can trigger that.

This is "full reset if (val & 0xA) != 0".  You interpreted it as (val &
0xA) = 0xA, I think.

>>      }
>>      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 3e1cf2e..54f5b72 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"
>> @@ -443,7 +444,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_reset_all_async();
>>      }
>>  }
>>  
>> 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_reset_all_async();
>>      }
>>  }
>>  
>> @@ -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_reset_all_async();
> 
> Oh, no, system reset is correct.

No, the keyboard controller is connected to the CPU reset signal.

> in the real world, system and cpu reset are quite different,
> cpu reset only reset processor power, while system reset
> will reset platform power.

Separating the two is exactly the purpose of this patch. :)

Paolo

>>          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_reset_all_async();
>> +        }
>>      }
>>      d->rcr = val & 2; /* keep System Reset type only */
>>  }
> 
> 
> 
>