9e047b982452 "piix4: add acpi pci hotplug support" introduced a new property
'use_acpi_pci_hotplug' for pc-1.7 and older machines.
c24d5e0b91d1 "convert ACPI PCI hotplug to use hotplug-handler API" added the
qbus hotplug handlers but forgot to check for the 'use_acpi_pci_hotplug'
property.
Check for use_acpi_pci_hotplug before calling acpi_pcihp_device_[un]plug_cb().
If Xen is enabled, piix4_pm_init() disables use_acpi_pci_hotplug.
The following valgrind Trace equivs:
qdev_device_add( "ich9-ahci" )
-> device_set_realized()
-> hotplug_handler_plug()
-> piix4_device_plug_cb()
-> acpi_pcihp_device_plug_cb()
-> acpi_pcihp_get_bsel() "Property ACPI_PCIHP_PROP_BSEL not found"
-> object_unparent()
<- "Bus doesn't have property ACPI_PCIHP_PROP_BSEL set"
$ valgrind x86_64-softmmu/qemu-system-x86_64 -M pc-1.2 -nographic -S
(qemu) device_add ich9-ahci,id=ich9-ahci
==6604== Invalid read of size 8
==6604== at 0x609AB0: object_unparent (object.c:445)
==6604== by 0x4C4478: device_unparent (qdev.c:1095)
==6604== by 0x60A364: object_finalize_child_property (object.c:1396)
==6604== by 0x6092A6: object_property_del_child.isra.7 (object.c:427)
==6604== by 0x451728: qdev_device_add (qdev-monitor.c:634)
==6604== by 0x451C82: qmp_device_add (qdev-monitor.c:807)
==6604== by 0x46B689: hmp_device_add (hmp.c:1925)
==6604== by 0x364083: handle_hmp_command (monitor.c:3119)
==6604== by 0x365439: monitor_command_cb (monitor.c:3922)
==6604== by 0x6E5D27: readline_handle_byte (readline.c:393)
==6604== by 0x364311: monitor_read (monitor.c:3905)
==6604== by 0x67C573: mux_chr_read (char-mux.c:216)
==6604== Address 0x15fc5448 is 30,328 bytes inside a block of size 36,288 free'd
==6604== at 0x4C2ACDD: free (vg_replace_malloc.c:530)
==6604== by 0xA04EBCD: g_free (in /usr/lib64/libglib-2.0.so.0.5000.3)
==6604== by 0x50100E: pci_ich9_uninit (ich.c:161)
==6604== by 0x5428AB: pci_qdev_unrealize (pci.c:1083)
==6604== by 0x4C5EE9: device_set_realized (qdev.c:988)
==6604== by 0x608DCD: property_set_bool (object.c:1886)
==6604== by 0x60CEBE: object_property_set_qobject (qom-qobject.c:27)
==6604== by 0x60AB6F: object_property_set_bool (object.c:1162)
==6604== by 0x4516F3: qdev_device_add (qdev-monitor.c:630)
==6604== by 0x451C82: qmp_device_add (qdev-monitor.c:807)
==6604== by 0x46B689: hmp_device_add (hmp.c:1925)
==6604== by 0x364083: handle_hmp_command (monitor.c:3119)
==6604== Block was alloc'd at
==6604== at 0x4C2B975: calloc (vg_replace_malloc.c:711)
==6604== by 0xA04EB15: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.5000.3)
==6604== by 0x50094F: ahci_realize (ahci.c:1468)
==6604== by 0x501098: pci_ich9_ahci_realize (ich.c:115)
==6604== by 0x543E6D: pci_qdev_realize (pci.c:2002)
==6604== by 0x4C5E69: device_set_realized (qdev.c:914)
==6604== by 0x608DCD: property_set_bool (object.c:1886)
==6604== by 0x60CEBE: object_property_set_qobject (qom-qobject.c:27)
==6604== by 0x60AB6F: object_property_set_bool (object.c:1162)
==6604== by 0x4516F3: qdev_device_add (qdev-monitor.c:630)
==6604== by 0x451C82: qmp_device_add (qdev-monitor.c:807)
==6604== by 0x46B689: hmp_device_add (hmp.c:1925)
Reported-by: Thomas Huth <thuth@redhat.com>
Message-Id: <59a56959-ca12-ea75-33fa-ff07eba1b090@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
hw/acpi/piix4.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index f276967365..d4df209a2e 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -385,7 +385,7 @@ static void piix4_device_plug_cb(HotplugHandler *hotplug_dev,
dev, errp);
}
} else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
- if (!xen_enabled()) {
+ if (s->use_acpi_pci_hotplug) {
acpi_pcihp_device_plug_cb(hotplug_dev, &s->acpi_pci_hotplug, dev,
errp);
}
@@ -411,7 +411,7 @@ static void piix4_device_unplug_request_cb(HotplugHandler *hotplug_dev,
acpi_memory_unplug_request_cb(hotplug_dev, &s->acpi_memory_hotplug,
dev, errp);
} else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
- if (!xen_enabled()) {
+ if (s->use_acpi_pci_hotplug) {
acpi_pcihp_device_unplug_cb(hotplug_dev, &s->acpi_pci_hotplug, dev,
errp);
}
--
2.14.1
On Tue, Aug 22, 2017 at 06:43:43PM -0300, Philippe Mathieu-Daudé wrote:
> 9e047b982452 "piix4: add acpi pci hotplug support" introduced a new property
> 'use_acpi_pci_hotplug' for pc-1.7 and older machines.
> c24d5e0b91d1 "convert ACPI PCI hotplug to use hotplug-handler API" added the
> qbus hotplug handlers but forgot to check for the 'use_acpi_pci_hotplug'
> property.
>
> Check for use_acpi_pci_hotplug before calling acpi_pcihp_device_[un]plug_cb().
>
> If Xen is enabled, piix4_pm_init() disables use_acpi_pci_hotplug.
>
> The following valgrind Trace equivs:
>
> qdev_device_add( "ich9-ahci" )
> -> device_set_realized()
> -> hotplug_handler_plug()
> -> piix4_device_plug_cb()
> -> acpi_pcihp_device_plug_cb()
> -> acpi_pcihp_get_bsel() "Property ACPI_PCIHP_PROP_BSEL not found"
> -> object_unparent()
> <- "Bus doesn't have property ACPI_PCIHP_PROP_BSEL set"
>
> $ valgrind x86_64-softmmu/qemu-system-x86_64 -M pc-1.2 -nographic -S
> (qemu) device_add ich9-ahci,id=ich9-ahci
> ==6604== Invalid read of size 8
> ==6604== at 0x609AB0: object_unparent (object.c:445)
> ==6604== by 0x4C4478: device_unparent (qdev.c:1095)
> ==6604== by 0x60A364: object_finalize_child_property (object.c:1396)
> ==6604== by 0x6092A6: object_property_del_child.isra.7 (object.c:427)
> ==6604== by 0x451728: qdev_device_add (qdev-monitor.c:634)
> ==6604== by 0x451C82: qmp_device_add (qdev-monitor.c:807)
> ==6604== by 0x46B689: hmp_device_add (hmp.c:1925)
> ==6604== by 0x364083: handle_hmp_command (monitor.c:3119)
> ==6604== by 0x365439: monitor_command_cb (monitor.c:3922)
> ==6604== by 0x6E5D27: readline_handle_byte (readline.c:393)
> ==6604== by 0x364311: monitor_read (monitor.c:3905)
> ==6604== by 0x67C573: mux_chr_read (char-mux.c:216)
> ==6604== Address 0x15fc5448 is 30,328 bytes inside a block of size 36,288 free'd
> ==6604== at 0x4C2ACDD: free (vg_replace_malloc.c:530)
> ==6604== by 0xA04EBCD: g_free (in /usr/lib64/libglib-2.0.so.0.5000.3)
> ==6604== by 0x50100E: pci_ich9_uninit (ich.c:161)
> ==6604== by 0x5428AB: pci_qdev_unrealize (pci.c:1083)
> ==6604== by 0x4C5EE9: device_set_realized (qdev.c:988)
> ==6604== by 0x608DCD: property_set_bool (object.c:1886)
> ==6604== by 0x60CEBE: object_property_set_qobject (qom-qobject.c:27)
> ==6604== by 0x60AB6F: object_property_set_bool (object.c:1162)
> ==6604== by 0x4516F3: qdev_device_add (qdev-monitor.c:630)
> ==6604== by 0x451C82: qmp_device_add (qdev-monitor.c:807)
> ==6604== by 0x46B689: hmp_device_add (hmp.c:1925)
> ==6604== by 0x364083: handle_hmp_command (monitor.c:3119)
> ==6604== Block was alloc'd at
> ==6604== at 0x4C2B975: calloc (vg_replace_malloc.c:711)
> ==6604== by 0xA04EB15: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.5000.3)
> ==6604== by 0x50094F: ahci_realize (ahci.c:1468)
> ==6604== by 0x501098: pci_ich9_ahci_realize (ich.c:115)
> ==6604== by 0x543E6D: pci_qdev_realize (pci.c:2002)
> ==6604== by 0x4C5E69: device_set_realized (qdev.c:914)
> ==6604== by 0x608DCD: property_set_bool (object.c:1886)
> ==6604== by 0x60CEBE: object_property_set_qobject (qom-qobject.c:27)
> ==6604== by 0x60AB6F: object_property_set_bool (object.c:1162)
> ==6604== by 0x4516F3: qdev_device_add (qdev-monitor.c:630)
> ==6604== by 0x451C82: qmp_device_add (qdev-monitor.c:807)
> ==6604== by 0x46B689: hmp_device_add (hmp.c:1925)
>
> Reported-by: Thomas Huth <thuth@redhat.com>
> Message-Id: <59a56959-ca12-ea75-33fa-ff07eba1b090@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Looks like this is a very old bug, isn't it?
Objections to merging this after the release?
> ---
> hw/acpi/piix4.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index f276967365..d4df209a2e 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -385,7 +385,7 @@ static void piix4_device_plug_cb(HotplugHandler *hotplug_dev,
> dev, errp);
> }
> } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
> - if (!xen_enabled()) {
> + if (s->use_acpi_pci_hotplug) {
> acpi_pcihp_device_plug_cb(hotplug_dev, &s->acpi_pci_hotplug, dev,
> errp);
> }
> @@ -411,7 +411,7 @@ static void piix4_device_unplug_request_cb(HotplugHandler *hotplug_dev,
> acpi_memory_unplug_request_cb(hotplug_dev, &s->acpi_memory_hotplug,
> dev, errp);
> } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
> - if (!xen_enabled()) {
> + if (s->use_acpi_pci_hotplug) {
> acpi_pcihp_device_unplug_cb(hotplug_dev, &s->acpi_pci_hotplug, dev,
> errp);
> }
> --
> 2.14.1
On 08/22/2017 07:42 PM, Michael S. Tsirkin wrote:
> On Tue, Aug 22, 2017 at 06:43:43PM -0300, Philippe Mathieu-Daudé wrote:
>> 9e047b982452 "piix4: add acpi pci hotplug support" introduced a new property
>> 'use_acpi_pci_hotplug' for pc-1.7 and older machines.
>> c24d5e0b91d1 "convert ACPI PCI hotplug to use hotplug-handler API" added the
>> qbus hotplug handlers but forgot to check for the 'use_acpi_pci_hotplug'
>> property.
>>
>> Check for use_acpi_pci_hotplug before calling acpi_pcihp_device_[un]plug_cb().
>>
>> If Xen is enabled, piix4_pm_init() disables use_acpi_pci_hotplug.
>>
>> The following valgrind Trace equivs:
>>
>> qdev_device_add( "ich9-ahci" )
>> -> device_set_realized()
>> -> hotplug_handler_plug()
>> -> piix4_device_plug_cb()
>> -> acpi_pcihp_device_plug_cb()
>> -> acpi_pcihp_get_bsel() "Property ACPI_PCIHP_PROP_BSEL not found"
>> -> object_unparent()
>> <- "Bus doesn't have property ACPI_PCIHP_PROP_BSEL set"
>>
>> $ valgrind x86_64-softmmu/qemu-system-x86_64 -M pc-1.2 -nographic -S
>> (qemu) device_add ich9-ahci,id=ich9-ahci
>> ==6604== Invalid read of size 8
>> ==6604== at 0x609AB0: object_unparent (object.c:445)
>> ==6604== by 0x4C4478: device_unparent (qdev.c:1095)
>> ==6604== by 0x60A364: object_finalize_child_property (object.c:1396)
>> ==6604== by 0x6092A6: object_property_del_child.isra.7 (object.c:427)
>> ==6604== by 0x451728: qdev_device_add (qdev-monitor.c:634)
>> ==6604== by 0x451C82: qmp_device_add (qdev-monitor.c:807)
>> ==6604== by 0x46B689: hmp_device_add (hmp.c:1925)
>> ==6604== by 0x364083: handle_hmp_command (monitor.c:3119)
>> ==6604== by 0x365439: monitor_command_cb (monitor.c:3922)
>> ==6604== by 0x6E5D27: readline_handle_byte (readline.c:393)
>> ==6604== by 0x364311: monitor_read (monitor.c:3905)
>> ==6604== by 0x67C573: mux_chr_read (char-mux.c:216)
>> ==6604== Address 0x15fc5448 is 30,328 bytes inside a block of size 36,288 free'd
>> ==6604== at 0x4C2ACDD: free (vg_replace_malloc.c:530)
>> ==6604== by 0xA04EBCD: g_free (in /usr/lib64/libglib-2.0.so.0.5000.3)
>> ==6604== by 0x50100E: pci_ich9_uninit (ich.c:161)
>> ==6604== by 0x5428AB: pci_qdev_unrealize (pci.c:1083)
>> ==6604== by 0x4C5EE9: device_set_realized (qdev.c:988)
>> ==6604== by 0x608DCD: property_set_bool (object.c:1886)
>> ==6604== by 0x60CEBE: object_property_set_qobject (qom-qobject.c:27)
>> ==6604== by 0x60AB6F: object_property_set_bool (object.c:1162)
>> ==6604== by 0x4516F3: qdev_device_add (qdev-monitor.c:630)
>> ==6604== by 0x451C82: qmp_device_add (qdev-monitor.c:807)
>> ==6604== by 0x46B689: hmp_device_add (hmp.c:1925)
>> ==6604== by 0x364083: handle_hmp_command (monitor.c:3119)
>> ==6604== Block was alloc'd at
>> ==6604== at 0x4C2B975: calloc (vg_replace_malloc.c:711)
>> ==6604== by 0xA04EB15: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.5000.3)
>> ==6604== by 0x50094F: ahci_realize (ahci.c:1468)
>> ==6604== by 0x501098: pci_ich9_ahci_realize (ich.c:115)
>> ==6604== by 0x543E6D: pci_qdev_realize (pci.c:2002)
>> ==6604== by 0x4C5E69: device_set_realized (qdev.c:914)
>> ==6604== by 0x608DCD: property_set_bool (object.c:1886)
>> ==6604== by 0x60CEBE: object_property_set_qobject (qom-qobject.c:27)
>> ==6604== by 0x60AB6F: object_property_set_bool (object.c:1162)
>> ==6604== by 0x4516F3: qdev_device_add (qdev-monitor.c:630)
>> ==6604== by 0x451C82: qmp_device_add (qdev-monitor.c:807)
>> ==6604== by 0x46B689: hmp_device_add (hmp.c:1925)
>>
>> Reported-by: Thomas Huth <thuth@redhat.com>
>> Message-Id: <59a56959-ca12-ea75-33fa-ff07eba1b090@redhat.com>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>
> Looks like this is a very old bug, isn't it?
> Objections to merging this after the release?
Yes, I'm also inclined to delay it so we can release 2.10, I tagged
"2.10-rc4" since Thomas sent it as a bug within the 2.10 window so I'll
let him decide if it is worth crying wolf :) It's very likely no-one but
him used pre-pc-i440fx-1.7 the last 3 years, not even thinking about hot
plugging AHCI devices :D
>
>> ---
>> hw/acpi/piix4.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
>> index f276967365..d4df209a2e 100644
>> --- a/hw/acpi/piix4.c
>> +++ b/hw/acpi/piix4.c
>> @@ -385,7 +385,7 @@ static void piix4_device_plug_cb(HotplugHandler *hotplug_dev,
>> dev, errp);
>> }
>> } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
>> - if (!xen_enabled()) {
>> + if (s->use_acpi_pci_hotplug) {
>> acpi_pcihp_device_plug_cb(hotplug_dev, &s->acpi_pci_hotplug, dev,
>> errp);
>> }
>> @@ -411,7 +411,7 @@ static void piix4_device_unplug_request_cb(HotplugHandler *hotplug_dev,
>> acpi_memory_unplug_request_cb(hotplug_dev, &s->acpi_memory_hotplug,
>> dev, errp);
>> } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
>> - if (!xen_enabled()) {
>> + if (s->use_acpi_pci_hotplug) {
>> acpi_pcihp_device_unplug_cb(hotplug_dev, &s->acpi_pci_hotplug, dev,
>> errp);
>> }
>> --
>> 2.14.1
On 23.08.2017 02:10, Philippe Mathieu-Daudé wrote: > On 08/22/2017 07:42 PM, Michael S. Tsirkin wrote: >> On Tue, Aug 22, 2017 at 06:43:43PM -0300, Philippe Mathieu-Daudé wrote: >>> 9e047b982452 "piix4: add acpi pci hotplug support" introduced a new >>> property >>> 'use_acpi_pci_hotplug' for pc-1.7 and older machines. >>> c24d5e0b91d1 "convert ACPI PCI hotplug to use hotplug-handler API" >>> added the >>> qbus hotplug handlers but forgot to check for the 'use_acpi_pci_hotplug' >>> property. >>> >>> Check for use_acpi_pci_hotplug before calling >>> acpi_pcihp_device_[un]plug_cb(). [...] >>> Reported-by: Thomas Huth <thuth@redhat.com> >>> Message-Id: <59a56959-ca12-ea75-33fa-ff07eba1b090@redhat.com> >>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >> >> Looks like this is a very old bug, isn't it? >> Objections to merging this after the release? > > Yes, I'm also inclined to delay it so we can release 2.10, I tagged > "2.10-rc4" since Thomas sent it as a bug within the 2.10 window so I'll > let him decide if it is worth crying wolf :) It's very likely no-one but > him used pre-pc-i440fx-1.7 the last 3 years, not even thinking about hot > plugging AHCI devices :D I'm fine if this gets included in 2.11 - it's quite unlikely that a user tries hot-plug ahci on such an old machine type, I think. But we maybe should include this in the 2.10.1 stable release, so I'm putting qemu-stable on CC now. Anyway, your patch seems to fix the issue for me, thanks! Tested-by: Thomas Huth <thuth@redhat.com>
On 23.08.2017 07:40, Thomas Huth wrote: > On 23.08.2017 02:10, Philippe Mathieu-Daudé wrote: >> On 08/22/2017 07:42 PM, Michael S. Tsirkin wrote: >>> On Tue, Aug 22, 2017 at 06:43:43PM -0300, Philippe Mathieu-Daudé wrote: >>>> 9e047b982452 "piix4: add acpi pci hotplug support" introduced a new >>>> property >>>> 'use_acpi_pci_hotplug' for pc-1.7 and older machines. >>>> c24d5e0b91d1 "convert ACPI PCI hotplug to use hotplug-handler API" >>>> added the >>>> qbus hotplug handlers but forgot to check for the 'use_acpi_pci_hotplug' >>>> property. >>>> >>>> Check for use_acpi_pci_hotplug before calling >>>> acpi_pcihp_device_[un]plug_cb(). > [...] >>>> Reported-by: Thomas Huth <thuth@redhat.com> >>>> Message-Id: <59a56959-ca12-ea75-33fa-ff07eba1b090@redhat.com> >>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >>> >>> Looks like this is a very old bug, isn't it? >>> Objections to merging this after the release? >> >> Yes, I'm also inclined to delay it so we can release 2.10, I tagged >> "2.10-rc4" since Thomas sent it as a bug within the 2.10 window so I'll >> let him decide if it is worth crying wolf :) It's very likely no-one but >> him used pre-pc-i440fx-1.7 the last 3 years, not even thinking about hot >> plugging AHCI devices :D > > I'm fine if this gets included in 2.11 - it's quite unlikely that a user > tries hot-plug ahci on such an old machine type, I think. But we maybe > should include this in the 2.10.1 stable release, so I'm putting > qemu-stable on CC now. > > Anyway, your patch seems to fix the issue for me, thanks! > > Tested-by: Thomas Huth <thuth@redhat.com> ... No, I was too fast here. I'm afraid, it still crashes with mips64el: $ valgrind mips64el-softmmu/qemu-system-mips64el -S -nographic -M malta,accel=qtest ==17935== Memcheck, a memory error detector ==17935== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al. ==17935== Using Valgrind-3.12.0 and LibVEX; rerun with -h for copyright info ==17935== Command: mips64el-softmmu/qemu-system-mips64el -S -nographic -M malta,accel=qtest ==17935== QEMU 2.9.93 monitor - type 'help' for more information (qemu) device_add ich9-ahci ==17935== Invalid read of size 8 ==17935== at 0x5F6F10: object_unparent (object.c:445) ==17935== by 0x4BB2C8: device_unparent (qdev.c:1095) ==17935== by 0x5F77C4: object_finalize_child_property (object.c:1396) ==17935== by 0x5F6706: object_property_del_child.isra.7 (object.c:427) ==17935== by 0x448BC8: qdev_device_add (qdev-monitor.c:634) ==17935== by 0x449122: qmp_device_add (qdev-monitor.c:807) ==17935== by 0x462B29: hmp_device_add (hmp.c:1925) ==17935== by 0x370F83: handle_hmp_command (monitor.c:3119) ==17935== by 0x371E59: monitor_command_cb (monitor.c:3922) ==17935== by 0x6D3187: readline_handle_byte (readline.c:393) ==17935== by 0x371211: monitor_read (monitor.c:3905) ==17935== by 0x6699D3: mux_chr_read (char-mux.c:216) ==17935== Address 0x21c549d8 is 30,328 bytes inside a block of size 36,288 free'd ==17935== at 0x4C2ACDD: free (vg_replace_malloc.c:530) ==17935== by 0xA04EBCD: g_free (in /usr/lib64/libglib-2.0.so.0.5000.3) ==17935== by 0x4FB94E: pci_ich9_uninit (ich.c:161) ==17935== by 0x5350FB: pci_qdev_unrealize (pci.c:1083) ==17935== by 0x4BCD39: device_set_realized (qdev.c:988) ==17935== by 0x5F622D: property_set_bool (object.c:1886) ==17935== by 0x5FA31E: object_property_set_qobject (qom-qobject.c:27) ==17935== by 0x5F7FCF: object_property_set_bool (object.c:1162) ==17935== by 0x448B93: qdev_device_add (qdev-monitor.c:630) ==17935== by 0x449122: qmp_device_add (qdev-monitor.c:807) ==17935== by 0x462B29: hmp_device_add (hmp.c:1925) ==17935== by 0x370F83: handle_hmp_command (monitor.c:3119) ==17935== Block was alloc'd at ==17935== at 0x4C2B975: calloc (vg_replace_malloc.c:711) ==17935== by 0xA04EB15: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.5000.3) ==17935== by 0x4FB28F: ahci_realize (ahci.c:1468) ==17935== by 0x4FB9D8: pci_ich9_ahci_realize (ich.c:115) ==17935== by 0x5366BD: pci_qdev_realize (pci.c:2002) ==17935== by 0x4BCCB9: device_set_realized (qdev.c:914) ==17935== by 0x5F622D: property_set_bool (object.c:1886) ==17935== by 0x5FA31E: object_property_set_qobject (qom-qobject.c:27) ==17935== by 0x5F7FCF: object_property_set_bool (object.c:1162) ==17935== by 0x448B93: qdev_device_add (qdev-monitor.c:630) ==17935== by 0x449122: qmp_device_add (qdev-monitor.c:807) ==17935== by 0x462B29: hmp_device_add (hmp.c:1925) Do you've got an idea how to fix that, too? Thomas
On Wed, 23 Aug 2017 08:04:06 +0200 Thomas Huth <thuth@redhat.com> wrote: > On 23.08.2017 07:40, Thomas Huth wrote: > > On 23.08.2017 02:10, Philippe Mathieu-Daudé wrote: > >> On 08/22/2017 07:42 PM, Michael S. Tsirkin wrote: > >>> On Tue, Aug 22, 2017 at 06:43:43PM -0300, Philippe Mathieu-Daudé wrote: > >>>> 9e047b982452 "piix4: add acpi pci hotplug support" introduced a new > >>>> property > >>>> 'use_acpi_pci_hotplug' for pc-1.7 and older machines. > >>>> c24d5e0b91d1 "convert ACPI PCI hotplug to use hotplug-handler API" > >>>> added the > >>>> qbus hotplug handlers but forgot to check for the 'use_acpi_pci_hotplug' > >>>> property. > >>>> > >>>> Check for use_acpi_pci_hotplug before calling > >>>> acpi_pcihp_device_[un]plug_cb(). > > [...] > >>>> Reported-by: Thomas Huth <thuth@redhat.com> > >>>> Message-Id: <59a56959-ca12-ea75-33fa-ff07eba1b090@redhat.com> > >>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > >>> > >>> Looks like this is a very old bug, isn't it? > >>> Objections to merging this after the release? > >> > >> Yes, I'm also inclined to delay it so we can release 2.10, I tagged > >> "2.10-rc4" since Thomas sent it as a bug within the 2.10 window so I'll > >> let him decide if it is worth crying wolf :) It's very likely no-one but > >> him used pre-pc-i440fx-1.7 the last 3 years, not even thinking about hot > >> plugging AHCI devices :D > > > > I'm fine if this gets included in 2.11 - it's quite unlikely that a user > > tries hot-plug ahci on such an old machine type, I think. But we maybe question is should be ahci device by hotpluggable at all?
On Wed, Aug 23, 2017 at 07:40:39AM +0200, Thomas Huth wrote: > On 23.08.2017 02:10, Philippe Mathieu-Daudé wrote: > > On 08/22/2017 07:42 PM, Michael S. Tsirkin wrote: > >> On Tue, Aug 22, 2017 at 06:43:43PM -0300, Philippe Mathieu-Daudé wrote: > >>> 9e047b982452 "piix4: add acpi pci hotplug support" introduced a new > >>> property > >>> 'use_acpi_pci_hotplug' for pc-1.7 and older machines. > >>> c24d5e0b91d1 "convert ACPI PCI hotplug to use hotplug-handler API" > >>> added the > >>> qbus hotplug handlers but forgot to check for the 'use_acpi_pci_hotplug' > >>> property. > >>> > >>> Check for use_acpi_pci_hotplug before calling > >>> acpi_pcihp_device_[un]plug_cb(). > [...] > >>> Reported-by: Thomas Huth <thuth@redhat.com> > >>> Message-Id: <59a56959-ca12-ea75-33fa-ff07eba1b090@redhat.com> > >>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > >> > >> Looks like this is a very old bug, isn't it? > >> Objections to merging this after the release? > > > > Yes, I'm also inclined to delay it so we can release 2.10, I tagged > > "2.10-rc4" since Thomas sent it as a bug within the 2.10 window so I'll > > let him decide if it is worth crying wolf :) It's very likely no-one but > > him used pre-pc-i440fx-1.7 the last 3 years, not even thinking about hot > > plugging AHCI devices :D > > I'm fine if this gets included in 2.11 - it's quite unlikely that a user > tries hot-plug ahci on such an old machine type, I think. But we maybe > should include this in the 2.10.1 stable release, so I'm putting > qemu-stable on CC now. > > Anyway, your patch seems to fix the issue for me, thanks! > > Tested-by: Thomas Huth <thuth@redhat.com> ok, pls remember to repost or ping after the release.
On Tue, 22 Aug 2017 18:43:43 -0300
Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> 9e047b982452 "piix4: add acpi pci hotplug support" introduced a new property
> 'use_acpi_pci_hotplug' for pc-1.7 and older machines.
> c24d5e0b91d1 "convert ACPI PCI hotplug to use hotplug-handler API" added the
> qbus hotplug handlers but forgot to check for the 'use_acpi_pci_hotplug'
> property.
>
> Check for use_acpi_pci_hotplug before calling acpi_pcihp_device_[un]plug_cb().
>
> If Xen is enabled, piix4_pm_init() disables use_acpi_pci_hotplug.
>
> The following valgrind Trace equivs:
>
> qdev_device_add( "ich9-ahci" )
> -> device_set_realized()
> -> hotplug_handler_plug()
> -> piix4_device_plug_cb()
> -> acpi_pcihp_device_plug_cb()
> -> acpi_pcihp_get_bsel() "Property ACPI_PCIHP_PROP_BSEL not found"
> -> object_unparent()
> <- "Bus doesn't have property ACPI_PCIHP_PROP_BSEL set"
>
> $ valgrind x86_64-softmmu/qemu-system-x86_64 -M pc-1.2 -nographic -S
> (qemu) device_add ich9-ahci,id=ich9-ahci
> ==6604== Invalid read of size 8
> ==6604== at 0x609AB0: object_unparent (object.c:445)
> ==6604== by 0x4C4478: device_unparent (qdev.c:1095)
> ==6604== by 0x60A364: object_finalize_child_property (object.c:1396)
> ==6604== by 0x6092A6: object_property_del_child.isra.7 (object.c:427)
> ==6604== by 0x451728: qdev_device_add (qdev-monitor.c:634)
> ==6604== by 0x451C82: qmp_device_add (qdev-monitor.c:807)
> ==6604== by 0x46B689: hmp_device_add (hmp.c:1925)
> ==6604== by 0x364083: handle_hmp_command (monitor.c:3119)
> ==6604== by 0x365439: monitor_command_cb (monitor.c:3922)
> ==6604== by 0x6E5D27: readline_handle_byte (readline.c:393)
> ==6604== by 0x364311: monitor_read (monitor.c:3905)
> ==6604== by 0x67C573: mux_chr_read (char-mux.c:216)
> ==6604== Address 0x15fc5448 is 30,328 bytes inside a block of size 36,288 free'd
> ==6604== at 0x4C2ACDD: free (vg_replace_malloc.c:530)
> ==6604== by 0xA04EBCD: g_free (in /usr/lib64/libglib-2.0.so.0.5000.3)
> ==6604== by 0x50100E: pci_ich9_uninit (ich.c:161)
> ==6604== by 0x5428AB: pci_qdev_unrealize (pci.c:1083)
> ==6604== by 0x4C5EE9: device_set_realized (qdev.c:988)
> ==6604== by 0x608DCD: property_set_bool (object.c:1886)
> ==6604== by 0x60CEBE: object_property_set_qobject (qom-qobject.c:27)
> ==6604== by 0x60AB6F: object_property_set_bool (object.c:1162)
> ==6604== by 0x4516F3: qdev_device_add (qdev-monitor.c:630)
> ==6604== by 0x451C82: qmp_device_add (qdev-monitor.c:807)
> ==6604== by 0x46B689: hmp_device_add (hmp.c:1925)
> ==6604== by 0x364083: handle_hmp_command (monitor.c:3119)
> ==6604== Block was alloc'd at
> ==6604== at 0x4C2B975: calloc (vg_replace_malloc.c:711)
> ==6604== by 0xA04EB15: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.5000.3)
> ==6604== by 0x50094F: ahci_realize (ahci.c:1468)
> ==6604== by 0x501098: pci_ich9_ahci_realize (ich.c:115)
> ==6604== by 0x543E6D: pci_qdev_realize (pci.c:2002)
> ==6604== by 0x4C5E69: device_set_realized (qdev.c:914)
> ==6604== by 0x608DCD: property_set_bool (object.c:1886)
> ==6604== by 0x60CEBE: object_property_set_qobject (qom-qobject.c:27)
> ==6604== by 0x60AB6F: object_property_set_bool (object.c:1162)
> ==6604== by 0x4516F3: qdev_device_add (qdev-monitor.c:630)
> ==6604== by 0x451C82: qmp_device_add (qdev-monitor.c:807)
> ==6604== by 0x46B689: hmp_device_add (hmp.c:1925)
>
> Reported-by: Thomas Huth <thuth@redhat.com>
> Message-Id: <59a56959-ca12-ea75-33fa-ff07eba1b090@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> hw/acpi/piix4.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index f276967365..d4df209a2e 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -385,7 +385,7 @@ static void piix4_device_plug_cb(HotplugHandler *hotplug_dev,
> dev, errp);
> }
> } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
> - if (!xen_enabled()) {
> + if (s->use_acpi_pci_hotplug) {
> acpi_pcihp_device_plug_cb(hotplug_dev, &s->acpi_pci_hotplug, dev,
> errp);
> }
s->use_acpi_pci_hotplug is a switch between legacy and current modes
of PCI hotplug and acpi_pcihp_device_plug_cb() should be able to handle
both.
Looking at history train-wreck started since
(f0c9d64 pc: pcihp: avoid adding ACPI_PCIHP_PROP_BSEL twice)
when bsel became solely allocated by another component (acpi table builder)
with follow up fixups to make that split brain (hw(legacy|modern)/acpi(in seabios|in qemu))
combos work somehow.
I think Anthony's (CCed) patches is the right way to fix issues not only
for Xen but also for legacy SeaBIOS as well.
[PATCH for-2.10 v3 0/3] Fix hotplug of PCI passthrought dev
https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg03216.html
Series still needs to be fixed (broken reboot logic)
but it might work for your case.
> @@ -411,7 +411,7 @@ static void piix4_device_unplug_request_cb(HotplugHandler *hotplug_dev,
> acpi_memory_unplug_request_cb(hotplug_dev, &s->acpi_memory_hotplug,
> dev, errp);
> } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
> - if (!xen_enabled()) {
> + if (s->use_acpi_pci_hotplug) {
> acpi_pcihp_device_unplug_cb(hotplug_dev, &s->acpi_pci_hotplug, dev,
> errp);
> }
© 2016 - 2024 Red Hat, Inc.