| Message ID | 1362510056-3316-4-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
在 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. > } > 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. in the real world, system and cpu reset are quite different, cpu reset only reset processor power, while system reset will reset platform power. > 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 */ > }
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 */ >> } > > > >
在 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 <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. No, IMHO, full reset should be done only by bit 4. 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), 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". > > > 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 */ > >> } > > > > > > > > >
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 <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. > > 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
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; } 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(); 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 */ }