| Message ID | 4F117F49.1090208@web.de |
|---|---|
| State | New |
| Headers | show |
Am 14.01.2012 14:12, schrieb Jan Kiszka: > From: Jan Kiszka <jan.kiszka@siemens.com> > > Linking the RTC device state to the PIIX does not belong into the > common path that is shared with the isapc. QEMU crashes otherwise. Doesn't that indicate a missing NULL-check or something in qdev_property_add_child() that should be fixed, too? Andreas > > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> > --- > hw/pc_piix.c | 22 +++++++++++----------- > 1 files changed, 11 insertions(+), 11 deletions(-) > > diff --git a/hw/pc_piix.c b/hw/pc_piix.c > index b70431f..3aea3cc 100644 > --- a/hw/pc_piix.c > +++ b/hw/pc_piix.c > @@ -201,6 +201,17 @@ static void pc_init1(MemoryRegion *system_memory, > } > idebus[0] = qdev_get_child_bus(&dev->qdev, "ide.0"); > idebus[1] = qdev_get_child_bus(&dev->qdev, "ide.1"); > + > + /* FIXME there's some major spaghetti here. Somehow we create the > + * devices on the PIIX before we actually create it. We create the > + * PIIX3 deep in the recess of the i440fx creation too and then lose > + * the DeviceState. > + * > + * For now, let's "fix" this by making judicious use of paths. This > + * is not generally the right way to do this. > + */ > + qdev_property_add_child(qdev_resolve_path("/i440fx/piix3", NULL), > + "rtc", (DeviceState *)rtc_state, NULL); > } else { > for(i = 0; i < MAX_IDE_BUS; i++) { > ISADevice *dev; > @@ -211,17 +222,6 @@ static void pc_init1(MemoryRegion *system_memory, > } > } > > - /* FIXME there's some major spaghetti here. Somehow we create the devices > - * on the PIIX before we actually create it. We create the PIIX3 deep in > - * the recess of the i440fx creation too and then lose the DeviceState. > - * > - * For now, let's "fix" this by making judicious use of paths. This is not > - * generally the right way to do this. > - */ > - > - qdev_property_add_child(qdev_resolve_path("/i440fx/piix3", NULL), > - "rtc", (DeviceState *)rtc_state, NULL); > - > audio_init(isa_bus, pci_enabled ? pci_bus : NULL); > > pc_cmos_init(below_4g_mem_size, above_4g_mem_size, boot_device,
On 01/14/2012 07:12 AM, Jan Kiszka wrote: > From: Jan Kiszka<jan.kiszka@siemens.com> > > Linking the RTC device state to the PIIX does not belong into the > common path that is shared with the isapc. QEMU crashes otherwise. > > Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com> Does isapc actually work for you? I tried to write a qemu-test test case (attached below) to help prevent future regressions. I can reproduce your SEGV but with your patch applied, I get no output (not even the BIOS runs). Here's the command line. Even a simple 'qemu-system-x86_64 -M isapc' reproduces it: /home/anthony/build/qemu/x86_64-softmmu/qemu-system-x86_64 -kernel bin/vmlinuz-3.0 -initrd .tmp-3510/initramfs-3510.img.gz -append console=ttyS0 seed=24689 -M isapc -pidfile .tmp-3510/pidfile-3510.pid -qmp unix:.tmp-3510/qmpsock-3510.sock,server,nowait Regards, Anthony Liguori > --- > hw/pc_piix.c | 22 +++++++++++----------- > 1 files changed, 11 insertions(+), 11 deletions(-) > > diff --git a/hw/pc_piix.c b/hw/pc_piix.c > index b70431f..3aea3cc 100644 > --- a/hw/pc_piix.c > +++ b/hw/pc_piix.c > @@ -201,6 +201,17 @@ static void pc_init1(MemoryRegion *system_memory, > } > idebus[0] = qdev_get_child_bus(&dev->qdev, "ide.0"); > idebus[1] = qdev_get_child_bus(&dev->qdev, "ide.1"); > + > + /* FIXME there's some major spaghetti here. Somehow we create the > + * devices on the PIIX before we actually create it. We create the > + * PIIX3 deep in the recess of the i440fx creation too and then lose > + * the DeviceState. > + * > + * For now, let's "fix" this by making judicious use of paths. This > + * is not generally the right way to do this. > + */ > + qdev_property_add_child(qdev_resolve_path("/i440fx/piix3", NULL), > + "rtc", (DeviceState *)rtc_state, NULL); > } else { > for(i = 0; i< MAX_IDE_BUS; i++) { > ISADevice *dev; > @@ -211,17 +222,6 @@ static void pc_init1(MemoryRegion *system_memory, > } > } > > - /* FIXME there's some major spaghetti here. Somehow we create the devices > - * on the PIIX before we actually create it. We create the PIIX3 deep in > - * the recess of the i440fx creation too and then lose the DeviceState. > - * > - * For now, let's "fix" this by making judicious use of paths. This is not > - * generally the right way to do this. > - */ > - > - qdev_property_add_child(qdev_resolve_path("/i440fx/piix3", NULL), > - "rtc", (DeviceState *)rtc_state, NULL); > - > audio_init(isa_bus, pci_enabled ? pci_bus : NULL); > > pc_cmos_init(below_4g_mem_size, above_4g_mem_size, boot_device,
On 2012-01-15 15:38, Anthony Liguori wrote: > On 01/14/2012 07:12 AM, Jan Kiszka wrote: >> From: Jan Kiszka<jan.kiszka@siemens.com> >> >> Linking the RTC device state to the PIIX does not belong into the >> common path that is shared with the isapc. QEMU crashes otherwise. >> >> Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com> > > Does isapc actually work for you? I tried to write a qemu-test test > case (attached below) to help prevent future regressions. I can > reproduce your SEGV but with your patch applied, I get no output (not > even the BIOS runs). > > Here's the command line. Even a simple 'qemu-system-x86_64 -M isapc' > reproduces it: > > /home/anthony/build/qemu/x86_64-softmmu/qemu-system-x86_64 -kernel > bin/vmlinuz-3.0 -initrd .tmp-3510/initramfs-3510.img.gz -append > console=ttyS0 seed=24689 -M isapc -pidfile .tmp-3510/pidfile-3510.pid > -qmp unix:.tmp-3510/qmpsock-3510.sock,server,nowait You need to update seabios to the last release at least (should have been done much earlier), and it only works for KVM (as that mode ignores some ROM write protections where seabios obviously has some troubles with). Jan
On 01/15/2012 08:40 AM, Jan Kiszka wrote: > On 2012-01-15 15:38, Anthony Liguori wrote: >> On 01/14/2012 07:12 AM, Jan Kiszka wrote: >>> From: Jan Kiszka<jan.kiszka@siemens.com> >>> >>> Linking the RTC device state to the PIIX does not belong into the >>> common path that is shared with the isapc. QEMU crashes otherwise. >>> >>> Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com> >> >> Does isapc actually work for you? I tried to write a qemu-test test >> case (attached below) to help prevent future regressions. I can >> reproduce your SEGV but with your patch applied, I get no output (not >> even the BIOS runs). >> >> Here's the command line. Even a simple 'qemu-system-x86_64 -M isapc' >> reproduces it: >> >> /home/anthony/build/qemu/x86_64-softmmu/qemu-system-x86_64 -kernel >> bin/vmlinuz-3.0 -initrd .tmp-3510/initramfs-3510.img.gz -append >> console=ttyS0 seed=24689 -M isapc -pidfile .tmp-3510/pidfile-3510.pid >> -qmp unix:.tmp-3510/qmpsock-3510.sock,server,nowait > > You need to update seabios to the last release at least (should have > been done much earlier), and it only works for KVM (as that mode ignores > some ROM write protections where seabios obviously has some troubles with). Can you send a pull request? SeaBIOS no longer builds on my laptop... Regards, Anthony Liguori > > Jan >
On 2012-01-15 17:12, Anthony Liguori wrote: > On 01/15/2012 08:40 AM, Jan Kiszka wrote: >> On 2012-01-15 15:38, Anthony Liguori wrote: >>> On 01/14/2012 07:12 AM, Jan Kiszka wrote: >>>> From: Jan Kiszka<jan.kiszka@siemens.com> >>>> >>>> Linking the RTC device state to the PIIX does not belong into the >>>> common path that is shared with the isapc. QEMU crashes otherwise. >>>> >>>> Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com> >>> >>> Does isapc actually work for you? I tried to write a qemu-test test >>> case (attached below) to help prevent future regressions. I can >>> reproduce your SEGV but with your patch applied, I get no output (not >>> even the BIOS runs). >>> >>> Here's the command line. Even a simple 'qemu-system-x86_64 -M isapc' >>> reproduces it: >>> >>> /home/anthony/build/qemu/x86_64-softmmu/qemu-system-x86_64 -kernel >>> bin/vmlinuz-3.0 -initrd .tmp-3510/initramfs-3510.img.gz -append >>> console=ttyS0 seed=24689 -M isapc -pidfile .tmp-3510/pidfile-3510.pid >>> -qmp unix:.tmp-3510/qmpsock-3510.sock,server,nowait >> >> You need to update seabios to the last release at least (should have >> been done much earlier), and it only works for KVM (as that mode ignores >> some ROM write protections where seabios obviously has some troubles >> with). > > Can you send a pull request? SeaBIOS no longer builds on my laptop... Will do. Last release of current master preferred? Jan
On 01/15/2012 10:16 AM, Jan Kiszka wrote: > On 2012-01-15 17:12, Anthony Liguori wrote: >> On 01/15/2012 08:40 AM, Jan Kiszka wrote: >>> On 2012-01-15 15:38, Anthony Liguori wrote: >>>> On 01/14/2012 07:12 AM, Jan Kiszka wrote: >>>>> From: Jan Kiszka<jan.kiszka@siemens.com> >>>>> >>>>> Linking the RTC device state to the PIIX does not belong into the >>>>> common path that is shared with the isapc. QEMU crashes otherwise. >>>>> >>>>> Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com> >>>> >>>> Does isapc actually work for you? I tried to write a qemu-test test >>>> case (attached below) to help prevent future regressions. I can >>>> reproduce your SEGV but with your patch applied, I get no output (not >>>> even the BIOS runs). >>>> >>>> Here's the command line. Even a simple 'qemu-system-x86_64 -M isapc' >>>> reproduces it: >>>> >>>> /home/anthony/build/qemu/x86_64-softmmu/qemu-system-x86_64 -kernel >>>> bin/vmlinuz-3.0 -initrd .tmp-3510/initramfs-3510.img.gz -append >>>> console=ttyS0 seed=24689 -M isapc -pidfile .tmp-3510/pidfile-3510.pid >>>> -qmp unix:.tmp-3510/qmpsock-3510.sock,server,nowait >>> >>> You need to update seabios to the last release at least (should have >>> been done much earlier), and it only works for KVM (as that mode ignores >>> some ROM write protections where seabios obviously has some troubles >>> with). >> >> Can you send a pull request? SeaBIOS no longer builds on my laptop... > > Will do. Last release of current master preferred? Yes. Regards, Anthony Liguori > > Jan >
diff --git a/hw/pc_piix.c b/hw/pc_piix.c index b70431f..3aea3cc 100644 --- a/hw/pc_piix.c +++ b/hw/pc_piix.c @@ -201,6 +201,17 @@ static void pc_init1(MemoryRegion *system_memory, } idebus[0] = qdev_get_child_bus(&dev->qdev, "ide.0"); idebus[1] = qdev_get_child_bus(&dev->qdev, "ide.1"); + + /* FIXME there's some major spaghetti here. Somehow we create the + * devices on the PIIX before we actually create it. We create the + * PIIX3 deep in the recess of the i440fx creation too and then lose + * the DeviceState. + * + * For now, let's "fix" this by making judicious use of paths. This + * is not generally the right way to do this. + */ + qdev_property_add_child(qdev_resolve_path("/i440fx/piix3", NULL), + "rtc", (DeviceState *)rtc_state, NULL); } else { for(i = 0; i < MAX_IDE_BUS; i++) { ISADevice *dev; @@ -211,17 +222,6 @@ static void pc_init1(MemoryRegion *system_memory, } } - /* FIXME there's some major spaghetti here. Somehow we create the devices - * on the PIIX before we actually create it. We create the PIIX3 deep in - * the recess of the i440fx creation too and then lose the DeviceState. - * - * For now, let's "fix" this by making judicious use of paths. This is not - * generally the right way to do this. - */ - - qdev_property_add_child(qdev_resolve_path("/i440fx/piix3", NULL), - "rtc", (DeviceState *)rtc_state, NULL); - audio_init(isa_bus, pci_enabled ? pci_bus : NULL); pc_cmos_init(below_4g_mem_size, above_4g_mem_size, boot_device,