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

[Glauber Costa]

On Wed, Nov 04, 2009 at 02:51:08PM -0200, Marcelo Tosatti wrote:
> 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?

the logic behind it, is that since we are not touching any cpu state up to
that point, whatever we do in the cpu loop is fine, since we will not execute
it, just spin.

I do however, ackowledge this is a pretty complicated issue, and quite subtle.
I am not against removing it, or at least sending a separate patch for it,
so we can have better bisectability.