From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1N5j53-0005RQ-Cw for qemu-devel@nongnu.org; Wed, 04 Nov 2009 11:51:33 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1N5j50-0005Q4-2E for qemu-devel@nongnu.org; Wed, 04 Nov 2009 11:51:33 -0500 Received: from [199.232.76.173] (port=59936 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1N5j4z-0005Q0-Cq for qemu-devel@nongnu.org; Wed, 04 Nov 2009 11:51:29 -0500 Received: from mx1.redhat.com ([209.132.183.28]:63572) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1N5j4y-00078h-Pq for qemu-devel@nongnu.org; Wed, 04 Nov 2009 11:51:29 -0500 Date: Wed, 4 Nov 2009 14:51:08 -0200 From: Marcelo Tosatti Subject: Re: [Qemu-devel] [PATCH] don't call reset functions on cpu initialization\ Message-ID: <20091104165108.GA20353@amt.cnet> References: <1257277805-637-1-git-send-email-glommer@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1257277805-637-1-git-send-email-glommer@redhat.com> List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Glauber Costa Cc: aliguori@us.ibm.com, qemu-devel@nongnu.org On Tue, Nov 03, 2009 at 05:50:05PM -0200, Glauber Costa wrote: > There is absolutely no need to call reset functions when initializing > devices. Since we are already registering them, calling qemu_system_reset() > should suffice. Actually, it is what happens when we reboot the machine, > and using the same process instead of a special case semantics will even > allow us to find bugs easier. > > Furthermore, the fact that we initialize things like the cpu quite early, > leads to the need to introduce synchronization stuff like qemu_system_cond. > This patch removes it entirely. All we need to do is call qemu_system_reset() > only when we're already sure the system is up and running > > I tested it with qemu (with and without io-thread) and qemu-kvm, and it > seems to be doing okay - although qemu-kvm uses a slightly different patch. > > Signed-off-by: Glauber Costa > --- > hw/apic.c | 1 - > hw/e1000.c | 1 - > hw/hpet.c | 1 - > hw/i8254.c | 2 -- > hw/ide/piix.c | 1 - > hw/piix4.c | 1 - > hw/piix_pci.c | 1 - > hw/rtl8139.c | 2 +- > hw/serial.c | 1 - > hw/usb-ohci.c | 1 - > hw/usb-uhci.c | 1 - > hw/vga.c | 1 - > target-i386/helper.c | 1 - > vl.c | 15 +-------------- > 14 files changed, 2 insertions(+), 28 deletions(-) > > diff --git a/hw/apic.c b/hw/apic.c > index c89008e..87e7dc0 100644 > --- a/hw/apic.c > +++ b/hw/apic.c > @@ -981,7 +981,6 @@ int apic_init(CPUState *env) > s->id = env->cpuid_apic_id; > s->cpu_env = env; > > - apic_reset(s); > msix_supported = 1; > > /* XXX: mapping more APICs at the same memory location */ > diff --git a/hw/e1000.c b/hw/e1000.c > index 028afd1..8fb299d 100644 > --- a/hw/e1000.c > +++ b/hw/e1000.c > @@ -1113,7 +1113,6 @@ static int pci_e1000_init(PCIDevice *pci_dev) > qemu_format_nic_info_str(d->vc, macaddr); > > vmstate_register(-1, &vmstate_e1000, d); > - e1000_reset(d); > > if (!pci_dev->qdev.hotplugged) { > static int loaded = 0; > diff --git a/hw/hpet.c b/hw/hpet.c > index 64163bd..6f39711 100644 > --- a/hw/hpet.c > +++ b/hw/hpet.c > @@ -577,7 +577,6 @@ void hpet_init(qemu_irq *irq) { > HPETTimer *timer = &s->timer[i]; > timer->qemu_timer = qemu_new_timer(vm_clock, hpet_timer, timer); > } > - hpet_reset(s); > vmstate_register(-1, &vmstate_hpet, s); > qemu_register_reset(hpet_reset, s); > /* HPET Area */ > diff --git a/hw/i8254.c b/hw/i8254.c > index 5c49e6e..faaa884 100644 > --- a/hw/i8254.c > +++ b/hw/i8254.c > @@ -513,7 +513,5 @@ PITState *pit_init(int base, qemu_irq irq) > register_ioport_write(base, 4, 1, pit_ioport_write, pit); > register_ioport_read(base, 3, 1, pit_ioport_read, pit); > > - pit_reset(pit); > - > return pit; > } > diff --git a/hw/ide/piix.c b/hw/ide/piix.c > index a17bf59..60b37a3 100644 > --- a/hw/ide/piix.c > +++ b/hw/ide/piix.c > @@ -120,7 +120,6 @@ static int pci_piix_ide_initfn(PCIIDEState *d) > pci_conf[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL; // header_type > > qemu_register_reset(piix3_reset, d); > - piix3_reset(d); > > pci_register_bar(&d->dev, 4, 0x10, PCI_ADDRESS_SPACE_IO, bmdma_map); > > diff --git a/hw/piix4.c b/hw/piix4.c > index a6aea15..f75951b 100644 > --- a/hw/piix4.c > +++ b/hw/piix4.c > @@ -97,7 +97,6 @@ static int piix4_initfn(PCIDevice *d) > PCI_HEADER_TYPE_NORMAL | PCI_HEADER_TYPE_MULTI_FUNCTION; // header_type = PCI_multifunction, generic > > piix4_dev = d; > - piix4_reset(d); > qemu_register_reset(piix4_reset, d); > return 0; > } > diff --git a/hw/piix_pci.c b/hw/piix_pci.c > index ed036fe..20d834f 100644 > --- a/hw/piix_pci.c > +++ b/hw/piix_pci.c > @@ -341,7 +341,6 @@ static int piix3_initfn(PCIDevice *dev) > pci_conf[PCI_HEADER_TYPE] = > PCI_HEADER_TYPE_NORMAL | PCI_HEADER_TYPE_MULTI_FUNCTION; // header_type = PCI_multifunction, generic > > - piix3_reset(d); > qemu_register_reset(piix3_reset, d); > return 0; > } > diff --git a/hw/rtl8139.c b/hw/rtl8139.c > index d26df48..27cc618 100644 > --- a/hw/rtl8139.c > +++ b/hw/rtl8139.c > @@ -3331,7 +3331,7 @@ static int pci_rtl8139_init(PCIDevice *dev) > PCI_ADDRESS_SPACE_MEM, rtl8139_mmio_map); > > qemu_macaddr_default_if_unset(&s->conf.macaddr); > - rtl8139_reset(&s->dev.qdev); > + > s->vc = qemu_new_vlan_client(NET_CLIENT_TYPE_NIC, > s->conf.vlan, s->conf.peer, > dev->qdev.info->name, dev->qdev.id, > diff --git a/hw/serial.c b/hw/serial.c > index 9353201..fa12dcc 100644 > --- a/hw/serial.c > +++ b/hw/serial.c > @@ -725,7 +725,6 @@ static void serial_init_core(SerialState *s) > s->transmit_timer = qemu_new_timer(vm_clock, (QEMUTimerCB *) serial_xmit, s); > > qemu_register_reset(serial_reset, s); > - serial_reset(s); > > qemu_chr_add_handlers(s->chr, serial_can_receive1, serial_receive1, > serial_event, s); > diff --git a/hw/usb-ohci.c b/hw/usb-ohci.c > index 48ccd49..f71d6b8 100644 > --- a/hw/usb-ohci.c > +++ b/hw/usb-ohci.c > @@ -1698,7 +1698,6 @@ static void usb_ohci_init(OHCIState *ohci, DeviceState *dev, > > ohci->async_td = 0; > qemu_register_reset(ohci_reset, ohci); > - ohci_reset(ohci); > } > > typedef struct { > diff --git a/hw/usb-uhci.c b/hw/usb-uhci.c > index 67a9a23..1580a50 100644 > --- a/hw/usb-uhci.c > +++ b/hw/usb-uhci.c > @@ -1079,7 +1079,6 @@ static int usb_uhci_common_initfn(UHCIState *s) > s->num_ports_vmstate = NB_PORTS; > > qemu_register_reset(uhci_reset, s); > - uhci_reset(s); > > /* Use region 4 for consistency with real hardware. BSD guests seem > to rely on this. */ > diff --git a/hw/vga.c b/hw/vga.c > index 95a7650..55e9b85 100644 > --- a/hw/vga.c > +++ b/hw/vga.c > @@ -2235,7 +2235,6 @@ void vga_common_init(VGACommonState *s, int vga_ram_size) > s->update_retrace_info = vga_precise_update_retrace_info; > break; > } > - vga_reset(s); > } > > /* used by both ISA and PCI */ > diff --git a/target-i386/helper.c b/target-i386/helper.c > index c961544..957b3fc 100644 > --- a/target-i386/helper.c > +++ b/target-i386/helper.c > @@ -1885,7 +1885,6 @@ CPUX86State *cpu_x86_init(const char *cpu_model) > return NULL; > } > mce_init(env); > - cpu_reset(env); > > qemu_init_vcpu(env); > > diff --git a/vl.c b/vl.c > index e57f58f..9f63da3 100644 > --- a/vl.c > +++ b/vl.c > @@ -3415,11 +3415,9 @@ static QemuThread io_thread; > static QemuThread *tcg_cpu_thread; > static QemuCond *tcg_halt_cond; > > -static int qemu_system_ready; > /* cpu creation */ > static QemuCond qemu_cpu_cond; > /* system init */ > -static QemuCond qemu_system_cond; > static QemuCond qemu_pause_cond; > > static void block_io_signals(void); > @@ -3484,10 +3482,6 @@ static void *kvm_cpu_thread_fn(void *arg) > env->created = 1; > qemu_cond_signal(&qemu_cpu_cond); > > - /* and wait for machine initialization */ > - while (!qemu_system_ready) > - qemu_cond_timedwait(&qemu_system_cond, &qemu_global_mutex, 100); > - I don't understand why you remove this (and what it has to do with system reset). What prevents cpus from executing their main loop between cpu creation and full machine initialization?