| Message ID | 1257437115-22725-1-git-send-email-glommer@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Nov 5, 2009 at 6:05 PM, Glauber Costa <glommer@redhat.com> 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 Nice idea. But shouldn't you remove all calls to reset functions, not just some random x86 ones? > --- a/target-i386/helper.c > +++ b/target-i386/helper.c > @@ -1885,7 +1885,9 @@ CPUX86State *cpu_x86_init(const char *cpu_model) > return NULL; > } > mce_init(env); > +#ifdef CONFIG_USER_ONLY > cpu_reset(env); > +#endif Please push the call to *-user/main.c, just after call to cpu_init().
On Fri, Nov 06, 2009 at 08:05:40PM +0200, Blue Swirl wrote: > On Thu, Nov 5, 2009 at 6:05 PM, Glauber Costa <glommer@redhat.com> 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 > > Nice idea. But shouldn't you remove all calls to reset functions, not > just some random x86 ones? Yes, but I can't test it, so I don't feel comfortable. After the patch is merged, others can remove their own too. > > > --- a/target-i386/helper.c > > +++ b/target-i386/helper.c > > @@ -1885,7 +1885,9 @@ CPUX86State *cpu_x86_init(const char *cpu_model) > > return NULL; > > } > > mce_init(env); > > +#ifdef CONFIG_USER_ONLY > > cpu_reset(env); > > +#endif > > Please push the call to *-user/main.c, just after call to cpu_init(). I'd prefer it that way too. But cpu_reset is also called in some other places, and Laurent suggested me to to this way. I don't really know much about -user, so I'm fine with whatever you guys agree on.
On Fri, Nov 6, 2009 at 7:11 PM, Glauber Costa <glommer@redhat.com> wrote: > On Fri, Nov 06, 2009 at 08:05:40PM +0200, Blue Swirl wrote: >> On Thu, Nov 5, 2009 at 6:05 PM, Glauber Costa <glommer@redhat.com> wrote: [...] >> > --- a/target-i386/helper.c >> > +++ b/target-i386/helper.c >> > @@ -1885,7 +1885,9 @@ CPUX86State *cpu_x86_init(const char *cpu_model) >> > return NULL; >> > } >> > mce_init(env); >> > +#ifdef CONFIG_USER_ONLY >> > cpu_reset(env); >> > +#endif >> >> Please push the call to *-user/main.c, just after call to cpu_init(). > I'd prefer it that way too. But cpu_reset is also called in some other places, > and Laurent suggested me to to this way. Yes, you'd have to remove all calls to cpu_reset from all CPUs. And also add a call to cpu_reset to cpu_copy. > I don't really know much about -user, so I'm fine with whatever you guys agree on. I honestly don't care that much as long as all targets still work in user mode :-) The aim was to make Glauber's patch less intrusive. Laurent
On Fri, Nov 6, 2009 at 8:43 PM, Laurent Desnogues <laurent.desnogues@gmail.com> wrote: > On Fri, Nov 6, 2009 at 7:11 PM, Glauber Costa <glommer@redhat.com> wrote: >> On Fri, Nov 06, 2009 at 08:05:40PM +0200, Blue Swirl wrote: >>> On Thu, Nov 5, 2009 at 6:05 PM, Glauber Costa <glommer@redhat.com> wrote: > [...] >>> > --- a/target-i386/helper.c >>> > +++ b/target-i386/helper.c >>> > @@ -1885,7 +1885,9 @@ CPUX86State *cpu_x86_init(const char *cpu_model) >>> > return NULL; >>> > } >>> > mce_init(env); >>> > +#ifdef CONFIG_USER_ONLY >>> > cpu_reset(env); >>> > +#endif >>> >>> Please push the call to *-user/main.c, just after call to cpu_init(). >> I'd prefer it that way too. But cpu_reset is also called in some other places, >> and Laurent suggested me to to this way. > > Yes, you'd have to remove all calls to cpu_reset from all CPUs. > And also add a call to cpu_reset to cpu_copy. > >> I don't really know much about -user, so I'm fine with whatever you guys agree on. > > I honestly don't care that much as long as all targets still work > in user mode :-) > > The aim was to make Glauber's patch less intrusive. Given that only the new calls to cpu_reset are important and the removals are much less so (double reset shouldn't be a problem), the least intrusive version would be to just add the new calls and do the clean up later.
On Fri, Nov 6, 2009 at 8:01 PM, Blue Swirl <blauwirbel@gmail.com> wrote: > On Fri, Nov 6, 2009 at 8:43 PM, Laurent Desnogues > <laurent.desnogues@gmail.com> wrote: [...] >> I honestly don't care that much as long as all targets still work >> in user mode :-) >> >> The aim was to make Glauber's patch less intrusive. > > Given that only the new calls to cpu_reset are important and the > removals are much less so (double reset shouldn't be a problem), the > least intrusive version would be to just add the new calls and do the > clean up later. Multiple resets would result in memory leaks for MIPS for instance. So I don't think they are that innocuous. Though that probably is more a problem of the MIPS reset than anything else :-) Laurent
On Fri, Nov 6, 2009 at 9:13 PM, Laurent Desnogues <laurent.desnogues@gmail.com> wrote: > On Fri, Nov 6, 2009 at 8:01 PM, Blue Swirl <blauwirbel@gmail.com> wrote: >> On Fri, Nov 6, 2009 at 8:43 PM, Laurent Desnogues >> <laurent.desnogues@gmail.com> wrote: > [...] >>> I honestly don't care that much as long as all targets still work >>> in user mode :-) >>> >>> The aim was to make Glauber's patch less intrusive. >> >> Given that only the new calls to cpu_reset are important and the >> removals are much less so (double reset shouldn't be a problem), the >> least intrusive version would be to just add the new calls and do the >> clean up later. > > Multiple resets would result in memory leaks for MIPS for instance. > So I don't think they are that innocuous. Though that probably is > more a problem of the MIPS reset than anything else :-) You are hinting at target-mips/translate_init.c: mmu_init()? Right, it shouldn't do the allocation.
On Fri, Nov 6, 2009 at 8:36 PM, Blue Swirl <blauwirbel@gmail.com> wrote: > On Fri, Nov 6, 2009 at 9:13 PM, Laurent Desnogues > <laurent.desnogues@gmail.com> wrote: >> On Fri, Nov 6, 2009 at 8:01 PM, Blue Swirl <blauwirbel@gmail.com> wrote: >>> On Fri, Nov 6, 2009 at 8:43 PM, Laurent Desnogues >>> <laurent.desnogues@gmail.com> wrote: >> [...] >>>> I honestly don't care that much as long as all targets still work >>>> in user mode :-) >>>> >>>> The aim was to make Glauber's patch less intrusive. >>> >>> Given that only the new calls to cpu_reset are important and the >>> removals are much less so (double reset shouldn't be a problem), the >>> least intrusive version would be to just add the new calls and do the >>> clean up later. >> >> Multiple resets would result in memory leaks for MIPS for instance. >> So I don't think they are that innocuous. Though that probably is >> more a problem of the MIPS reset than anything else :-) > > You are hinting at target-mips/translate_init.c: mmu_init()? Right, it > shouldn't do the allocation. No, I was talking about mvp_init. Laurent
On Fri, Nov 6, 2009 at 9:50 PM, Laurent Desnogues <laurent.desnogues@gmail.com> wrote: > On Fri, Nov 6, 2009 at 8:36 PM, Blue Swirl <blauwirbel@gmail.com> wrote: >> On Fri, Nov 6, 2009 at 9:13 PM, Laurent Desnogues >> <laurent.desnogues@gmail.com> wrote: >>> On Fri, Nov 6, 2009 at 8:01 PM, Blue Swirl <blauwirbel@gmail.com> wrote: >>>> On Fri, Nov 6, 2009 at 8:43 PM, Laurent Desnogues >>>> <laurent.desnogues@gmail.com> wrote: >>> [...] >>>>> I honestly don't care that much as long as all targets still work >>>>> in user mode :-) >>>>> >>>>> The aim was to make Glauber's patch less intrusive. >>>> >>>> Given that only the new calls to cpu_reset are important and the >>>> removals are much less so (double reset shouldn't be a problem), the >>>> least intrusive version would be to just add the new calls and do the >>>> clean up later. >>> >>> Multiple resets would result in memory leaks for MIPS for instance. >>> So I don't think they are that innocuous. Though that probably is >>> more a problem of the MIPS reset than anything else :-) >> >> You are hinting at target-mips/translate_init.c: mmu_init()? Right, it >> shouldn't do the allocation. > > No, I was talking about mvp_init. Both are then buggy. Why are these even allocated instead being members of CPUState?
On Fri, Nov 06, 2009 at 09:01:45PM +0200, Blue Swirl wrote: > On Fri, Nov 6, 2009 at 8:43 PM, Laurent Desnogues > <laurent.desnogues@gmail.com> wrote: > > On Fri, Nov 6, 2009 at 7:11 PM, Glauber Costa <glommer@redhat.com> wrote: > >> On Fri, Nov 06, 2009 at 08:05:40PM +0200, Blue Swirl wrote: > >>> On Thu, Nov 5, 2009 at 6:05 PM, Glauber Costa <glommer@redhat.com> wrote: > > [...] > >>> > --- a/target-i386/helper.c > >>> > +++ b/target-i386/helper.c > >>> > @@ -1885,7 +1885,9 @@ CPUX86State *cpu_x86_init(const char *cpu_model) > >>> > return NULL; > >>> > } > >>> > mce_init(env); > >>> > +#ifdef CONFIG_USER_ONLY > >>> > cpu_reset(env); > >>> > +#endif > >>> > >>> Please push the call to *-user/main.c, just after call to cpu_init(). > >> I'd prefer it that way too. But cpu_reset is also called in some other places, > >> and Laurent suggested me to to this way. > > > > Yes, you'd have to remove all calls to cpu_reset from all CPUs. > > And also add a call to cpu_reset to cpu_copy. > > > >> I don't really know much about -user, so I'm fine with whatever you guys agree on. > > > > I honestly don't care that much as long as all targets still work > > in user mode :-) > > > > The aim was to make Glauber's patch less intrusive. > > Given that only the new calls to cpu_reset are important and the > removals are much less so (double reset shouldn't be a problem), the > least intrusive version would be to just add the new calls and do the > clean up later. Which IMHO, pretty much means apply it this way, and then later on move the reset functions elsewhere for -user, if one wants to.
On Fri, Nov 6, 2009 at 10:41 PM, Glauber Costa <glommer@redhat.com> wrote: > On Fri, Nov 06, 2009 at 09:01:45PM +0200, Blue Swirl wrote: >> On Fri, Nov 6, 2009 at 8:43 PM, Laurent Desnogues >> <laurent.desnogues@gmail.com> wrote: >> > On Fri, Nov 6, 2009 at 7:11 PM, Glauber Costa <glommer@redhat.com> wrote: >> >> On Fri, Nov 06, 2009 at 08:05:40PM +0200, Blue Swirl wrote: >> >>> On Thu, Nov 5, 2009 at 6:05 PM, Glauber Costa <glommer@redhat.com> wrote: >> > [...] >> >>> > --- a/target-i386/helper.c >> >>> > +++ b/target-i386/helper.c >> >>> > @@ -1885,7 +1885,9 @@ CPUX86State *cpu_x86_init(const char *cpu_model) >> >>> > return NULL; >> >>> > } >> >>> > mce_init(env); >> >>> > +#ifdef CONFIG_USER_ONLY >> >>> > cpu_reset(env); >> >>> > +#endif >> >>> >> >>> Please push the call to *-user/main.c, just after call to cpu_init(). >> >> I'd prefer it that way too. But cpu_reset is also called in some other places, >> >> and Laurent suggested me to to this way. >> > >> > Yes, you'd have to remove all calls to cpu_reset from all CPUs. >> > And also add a call to cpu_reset to cpu_copy. >> > >> >> I don't really know much about -user, so I'm fine with whatever you guys agree on. >> > >> > I honestly don't care that much as long as all targets still work >> > in user mode :-) >> > >> > The aim was to make Glauber's patch less intrusive. >> >> Given that only the new calls to cpu_reset are important and the >> removals are much less so (double reset shouldn't be a problem), the >> least intrusive version would be to just add the new calls and do the >> clean up later. > Which IMHO, pretty much means apply it this way, and then later on move > the reset functions elsewhere for -user, if one wants to. Thanks, applied. While testing I noticed that the poor x86 emulator does not survive even five system_resets until BIOS refuses to boot. This happens without your patch. In comparison, I see no problems resetting Sparc32, Sparc64 or PPC.
On Sat, Nov 7, 2009 at 9:40 AM, Blue Swirl <blauwirbel@gmail.com> wrote: [...] > Thanks, applied. While testing I noticed that the poor x86 emulator > does not survive even five system_resets until BIOS refuses to boot. > This happens without your patch. In comparison, I see no problems > resetting Sparc32, Sparc64 or PPC. Unless I missed something you didn't commit Glauber's patch. So now in user mode cpu_reset differs for i386, SPARC and PPC, and all other targets. On top of that you didn't add a call to cpu_reset from cpu_copy which also calls cpu_init. Are you sure it's OK? Laurent
On Sat, Nov 7, 2009 at 6:02 PM, Laurent Desnogues <laurent.desnogues@gmail.com> wrote: > On Sat, Nov 7, 2009 at 9:40 AM, Blue Swirl <blauwirbel@gmail.com> wrote: > [...] >> Thanks, applied. While testing I noticed that the poor x86 emulator >> does not survive even five system_resets until BIOS refuses to boot. >> This happens without your patch. In comparison, I see no problems >> resetting Sparc32, Sparc64 or PPC. > > Unless I missed something you didn't commit Glauber's patch. What's c169998802505c244b8bcad562633f29de7d74a4 then? > So now in user mode cpu_reset differs for i386, SPARC > and PPC, and all other targets. No, for example MIPS cpu_reset is still called only once from cpu_mips_init. > On top of that you didn't add a call to cpu_reset from cpu_copy > which also calls cpu_init. Are you sure it's OK? It's not, I forgot to add that, sorry.
On Sat, Nov 7, 2009 at 5:11 PM, Blue Swirl <blauwirbel@gmail.com> wrote: [...] >> Unless I missed something you didn't commit Glauber's patch. > > What's c169998802505c244b8bcad562633f29de7d74a4 then? My bad, I updated and didn't notice you had applied further modifications. I seriously lack the commit mail notifications :-) Laurent
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..3fff1bb 100644 --- a/target-i386/helper.c +++ b/target-i386/helper.c @@ -1885,7 +1885,9 @@ CPUX86State *cpu_x86_init(const char *cpu_model) return NULL; } mce_init(env); +#ifdef CONFIG_USER_ONLY cpu_reset(env); +#endif qemu_init_vcpu(env); diff --git a/vl.c b/vl.c index e57f58f..9031911 100644 --- a/vl.c +++ b/vl.c @@ -4034,6 +4034,7 @@ static void main_loop(void) qemu_system_ready = 1; qemu_cond_broadcast(&qemu_system_cond); #endif + qemu_system_reset(); for (;;) { do {
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. [ v2: user mode still needs cpu_reset, so put it in ifdef. ] [ v3: leave qemu_system_cond for now. ] Signed-off-by: Glauber Costa <glommer@redhat.com> --- 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 | 2 ++ vl.c | 1 + 14 files changed, 4 insertions(+), 13 deletions(-)