From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:55781) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UDAa3-00059T-3o for qemu-devel@nongnu.org; Wed, 06 Mar 2013 04:24:12 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UDAZv-00014B-P8 for qemu-devel@nongnu.org; Wed, 06 Mar 2013 04:24:10 -0500 Received: from mail-wi0-x229.google.com ([2a00:1450:400c:c05::229]:63548) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UDAZv-00013u-F6 for qemu-devel@nongnu.org; Wed, 06 Mar 2013 04:24:03 -0500 Received: by mail-wi0-f169.google.com with SMTP id l13so2936921wie.0 for ; Wed, 06 Mar 2013 01:24:02 -0800 (PST) Sender: Paolo Bonzini Message-ID: <51370B2F.5090705@redhat.com> Date: Wed, 06 Mar 2013 10:23:59 +0100 From: Paolo Bonzini MIME-Version: 1.0 References: <1362510056-3316-1-git-send-email-pbonzini@redhat.com> <1362510056-3316-4-git-send-email-pbonzini@redhat.com> <1362535347.21129.8.camel@liguang.fnst.cn.fujitsu.com> <5136FFF8.9040604@redhat.com> <1362560783.21129.28.camel@liguang.fnst.cn.fujitsu.com> In-Reply-To: <1362560783.21129.28.camel@liguang.fnst.cn.fujitsu.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH v2 3/3] hw: correctly implement soft reset List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: li guang Cc: lersek@redhat.com, aliguori@us.ibm.com, dwmw2@infradead.org, qemu-devel@nongnu.org, afaerber@suse.de Il 06/03/2013 10:06, li guang ha scritto: > 在 2013-03-06三的 09:36 +0100，Paolo Bonzini写道： >> > 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 >>>> > >> Signed-off-by: Paolo Bonzini >>>> > >> --- >>>> > >> 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. > > No, IMHO, full reset should be done only by bit 4. Ah, you mean by bit 1, as in bit 3 only being consulted if bit 1 is 1. Hence, writing 0xC should do an INIT# cycle, not a full reset. It would matter if you want a watchdog to do a full power cycle (asserting SLP_S3/4/5#), but you want to use cf9h to do a CPU reset. That could be the case. > that is bit 1 & 2 are combined to determined reset cpu/system. > and system reset does not always be the same with full reset( > mostly be different in hardware signal of power cycle), The ICH9 datasheet says exactly how the two differ. For a full reset, ICH9 will drive SLP_S3/4/5# low for 3-5 seconds. We do not emulate this in QEMU. > it's up to the board hardware designer. > >> > >>>> > >> } >>>> > >> 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. > Not always, AFAIK, > traditionally, this should do system reset, > though system reset may be triggered by INIT# which > I guess is you said "CPU reset signal". Some websites say it is a system reset, but I don't think it's ever been correct. But on PIIX and ICH9 the keyboard controller reset is connected to INIT#, which is definitely not a system reset. Paolo