Patchwork isapc: Fix segfault during initialization

login
register
mail settings
Submitter Jan Kiszka
Date Jan. 14, 2012, 1:12 p.m.
Message ID <4F117F49.1090208@web.de>
Download mbox | patch
Permalink /patch/136078/
State New
Headers show

Comments

Jan Kiszka - Jan. 14, 2012, 1:12 p.m.
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>
---
 hw/pc_piix.c |   22 +++++++++++-----------
 1 files changed, 11 insertions(+), 11 deletions(-)
Andreas Färber - Jan. 14, 2012, 2:37 p.m.
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,
Anthony Liguori - Jan. 15, 2012, 2:38 p.m.
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,
Jan Kiszka - Jan. 15, 2012, 2:40 p.m.
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
Anthony Liguori - Jan. 15, 2012, 4:12 p.m.
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
>
Jan Kiszka - Jan. 15, 2012, 4:16 p.m.
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
Anthony Liguori - Jan. 15, 2012, 4:20 p.m.
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
>

Patch

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,