Re: [Qemu-devel] [PATCH] don't call reset functions on cpu initialization\

[Marcelo Tosatti]

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 <address@hidden>
> ---
>  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?